On 2025/4/23 02:50, Christophe JAILLET wrote:
Le 22/04/2025 à 20:23, SeongJae Park a écrit :
On Tue, 22 Apr 2025 13:44:39 +0300 Dan Carpenter
<dan.carpenter@xxxxxxxxxx> wrote:
On Tue, Apr 22, 2025 at 01:38:05PM +0300, Dan Carpenter wrote:
On Mon, Apr 21, 2025 at 02:24:24PM +0800, Su Hui wrote:
It's safer to using kmalloc_array() and size_add() because it can
prevent possible overflow problem.
Signed-off-by: Su Hui <suhui@xxxxxxxxxxxx>
[...]
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -465,7 +465,8 @@ static ssize_t memcg_path_store(struct kobject
*kobj,
{
struct damon_sysfs_scheme_filter *filter = container_of(kobj,
struct damon_sysfs_scheme_filter, kobj);
- char *path = kmalloc(sizeof(*path) * (count + 1), GFP_KERNEL);
+ char *path = kmalloc_array(size_add(count, 1), sizeof(*path),
+ GFP_KERNEL);
Count is clamped in rw_verify_area().
Smatch does a kind of ugly hack to handle rw_verify_area() which is
that
it says neither the count nor the pos can be more than 1G. And
obviously
files which are larger than 2GB exist but pretending they don't
silences
all these integer overflow warnings.
Actually rw_verify_area() ensures that "pos + count" can't
overflow. But
here we are multiplying. Fortunately, we are multiplying by 1 so
that's
safe and also count can't be larger than PAGE_SIZE here which is
safe as
well.
Thank you for adding these details, Dan. I understand the size_add()
change
can make warnings slience, though it is not really fixing a real
bug. So I
believe there is no action item to make a change to this patch. Maybe
making
the commit message more clarified can be helpful, though?
Please let me know if I'm misunderstanding your point and/or you want
some
changes.
As sizeof(*path) = 1, maybe, just change it to:
char *path = kmalloc(count + 1, GFP_KERNEL);
Maybe nothing should change?
Thanks for Dan's explanation again.
I send this patch because it is mentioned in
Documentation/process/deprecated.rst that:
"
Dynamic size calculations (especially multiplication) should not be
performed in memory allocator (or similar) function arguments due to the
risk of them overflowing.
"
Although in this case, this dynamic size calculations is safe.
But it's also fine for me to change this patch or discard this patch
because there is no
bug really fixed.
Su Hui
