[v1] locktorture: Fix memory leak in param_set_cpumask()

[PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Wang Liang 2 weeks, 6 days ago

When setting the locktorture module parameter 'bind_writers', the variable
'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
is not freed, when removing module or setting the parameter again.

Below kmemleak trace is seen for this issue:

unreferenced object 0xffff888100aabff8 (size 8):
  comm "bash", pid 323, jiffies 4295059233
  hex dump (first 8 bytes):
    07 00 00 00 00 00 00 00                          ........
  backtrace (crc ac50919):
    __kmalloc_node_noprof+0x2e5/0x420
    alloc_cpumask_var_node+0x1f/0x30
    param_set_cpumask+0x26/0xb0 [locktorture]
    param_attr_store+0x93/0x100
    module_attr_store+0x1b/0x30
    kernfs_fop_write_iter+0x114/0x1b0
    vfs_write+0x300/0x410
    ksys_write+0x60/0xd0
    do_syscall_64+0xa4/0x260
    entry_SYSCALL_64_after_hwframe+0x77/0x7f

This issue can be reproduced by:
  insmod locktorture.ko
  echo 0-2 > /sys/module/locktorture/parameters/bind_writers
  rmmod locktorture

or:
  insmod locktorture.ko
  echo 0-2 > /sys/module/locktorture/parameters/bind_writers
  echo 0-2 > /sys/module/locktorture/parameters/bind_writers

The parameter 'bind_readers' also has the same problem. Free the memory
when removing module or setting the parameter.

Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
Signed-off-by: Wang Liang <wangliang74@huawei.com>
---
 kernel/locking/locktorture.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index ce0362f0a871..cad80c050502 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
 	int ret;
 	char *s;
 
+	free_cpumask_var(*cm_bind);
+	*cm_bind = NULL;
+
 	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
 		s = "Out of memory";
 		ret = -ENOMEM;
@@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
 			cxt.cur_ops->exit();
 		cxt.init_called = false;
 	}
+
+	free_cpumask_var(bind_readers);
+	free_cpumask_var(bind_writers);
+	bind_readers = NULL;
+	bind_writers = NULL;
+
 	torture_cleanup_end();
 }
 
-- 
2.34.1

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Zhang Changzhong 2 weeks, 6 days ago


在 2025/9/12 9:57, Wang Liang 写道:
> When setting the locktorture module parameter 'bind_writers', the variable
> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
> is not freed, when removing module or setting the parameter again.
> 
> Below kmemleak trace is seen for this issue:
> 
> unreferenced object 0xffff888100aabff8 (size 8):
>   comm "bash", pid 323, jiffies 4295059233
>   hex dump (first 8 bytes):
>     07 00 00 00 00 00 00 00                          ........
>   backtrace (crc ac50919):
>     __kmalloc_node_noprof+0x2e5/0x420
>     alloc_cpumask_var_node+0x1f/0x30
>     param_set_cpumask+0x26/0xb0 [locktorture]
>     param_attr_store+0x93/0x100
>     module_attr_store+0x1b/0x30
>     kernfs_fop_write_iter+0x114/0x1b0
>     vfs_write+0x300/0x410
>     ksys_write+0x60/0xd0
>     do_syscall_64+0xa4/0x260
>     entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> This issue can be reproduced by:
>   insmod locktorture.ko
>   echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>   rmmod locktorture
> 
> or:
>   insmod locktorture.ko
>   echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>   echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> 
> The parameter 'bind_readers' also has the same problem. Free the memory
> when removing module or setting the parameter.
> 
> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> ---
>  kernel/locking/locktorture.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index ce0362f0a871..cad80c050502 100644
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
>  	int ret;
>  	char *s;
>  
> +	free_cpumask_var(*cm_bind);
> +	*cm_bind = NULL;

这个NULL没必要吧

> +
>  	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
>  		s = "Out of memory";
>  		ret = -ENOMEM;
> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
>  			cxt.cur_ops->exit();
>  		cxt.init_called = false;
>  	}
> +
> +	free_cpumask_var(bind_readers);
> +	free_cpumask_var(bind_writers);
> +	bind_readers = NULL;
> +	bind_writers = NULL;

同上

> +
>  	torture_cleanup_end();
>  }
>

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Wang Liang 2 weeks, 3 days ago

在 2025/9/12 10:16, Zhang Changzhong 写道:
>
> 在 2025/9/12 9:57, Wang Liang 写道:
>> When setting the locktorture module parameter 'bind_writers', the variable
>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
>> is not freed, when removing module or setting the parameter again.
>>
>> Below kmemleak trace is seen for this issue:
>>
>> unreferenced object 0xffff888100aabff8 (size 8):
>>    comm "bash", pid 323, jiffies 4295059233
>>    hex dump (first 8 bytes):
>>      07 00 00 00 00 00 00 00                          ........
>>    backtrace (crc ac50919):
>>      __kmalloc_node_noprof+0x2e5/0x420
>>      alloc_cpumask_var_node+0x1f/0x30
>>      param_set_cpumask+0x26/0xb0 [locktorture]
>>      param_attr_store+0x93/0x100
>>      module_attr_store+0x1b/0x30
>>      kernfs_fop_write_iter+0x114/0x1b0
>>      vfs_write+0x300/0x410
>>      ksys_write+0x60/0xd0
>>      do_syscall_64+0xa4/0x260
>>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>
>> This issue can be reproduced by:
>>    insmod locktorture.ko
>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>    rmmod locktorture
>>
>> or:
>>    insmod locktorture.ko
>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>
>> The parameter 'bind_readers' also has the same problem. Free the memory
>> when removing module or setting the parameter.
>>
>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>> ---
>>   kernel/locking/locktorture.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>> index ce0362f0a871..cad80c050502 100644
>> --- a/kernel/locking/locktorture.c
>> +++ b/kernel/locking/locktorture.c
>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
>>   	int ret;
>>   	char *s;
>>   
>> +	free_cpumask_var(*cm_bind);
>> +	*cm_bind = NULL;
> 这个NULL没必要吧


Setting global pointer to NULL after free may be more safe. ^-^

>> +
>>   	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
>>   		s = "Out of memory";
>>   		ret = -ENOMEM;
>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
>>   			cxt.cur_ops->exit();
>>   		cxt.init_called = false;
>>   	}
>> +
>> +	free_cpumask_var(bind_readers);
>> +	free_cpumask_var(bind_writers);
>> +	bind_readers = NULL;
>> +	bind_writers = NULL;
> 同上
>
>> +
>>   	torture_cleanup_end();
>>   }
>>

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Paul E. McKenney 2 weeks ago

On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
> 
> 在 2025/9/12 10:16, Zhang Changzhong 写道:
> > 
> > 在 2025/9/12 9:57, Wang Liang 写道:
> > > When setting the locktorture module parameter 'bind_writers', the variable
> > > 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
> > > is not freed, when removing module or setting the parameter again.
> > > 
> > > Below kmemleak trace is seen for this issue:
> > > 
> > > unreferenced object 0xffff888100aabff8 (size 8):
> > >    comm "bash", pid 323, jiffies 4295059233
> > >    hex dump (first 8 bytes):
> > >      07 00 00 00 00 00 00 00                          ........
> > >    backtrace (crc ac50919):
> > >      __kmalloc_node_noprof+0x2e5/0x420
> > >      alloc_cpumask_var_node+0x1f/0x30
> > >      param_set_cpumask+0x26/0xb0 [locktorture]
> > >      param_attr_store+0x93/0x100
> > >      module_attr_store+0x1b/0x30
> > >      kernfs_fop_write_iter+0x114/0x1b0
> > >      vfs_write+0x300/0x410
> > >      ksys_write+0x60/0xd0
> > >      do_syscall_64+0xa4/0x260
> > >      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > 
> > > This issue can be reproduced by:
> > >    insmod locktorture.ko
> > >    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> > >    rmmod locktorture
> > > 
> > > or:
> > >    insmod locktorture.ko
> > >    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> > >    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> > > 
> > > The parameter 'bind_readers' also has the same problem. Free the memory
> > > when removing module or setting the parameter.
> > > 
> > > Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
> > > Signed-off-by: Wang Liang <wangliang74@huawei.com>
> > > ---
> > >   kernel/locking/locktorture.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> > > index ce0362f0a871..cad80c050502 100644
> > > --- a/kernel/locking/locktorture.c
> > > +++ b/kernel/locking/locktorture.c
> > > @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
> > >   	int ret;
> > >   	char *s;
> > > +	free_cpumask_var(*cm_bind);
> > > +	*cm_bind = NULL;
> > 这个NULL没必要吧

Assuming this translates to "This NULL is unnecessary", I have to
agree with Zhang Changzhong.  I would go further and argue that the
free_cpumask_var() is also unnecessary here.

> Setting global pointer to NULL after free may be more safe. ^-^

In lock_torture_cleanup(), you mean?  I would agree with that.

> > > +
> > >   	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
> > >   		s = "Out of memory";
> > >   		ret = -ENOMEM;
> > > @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
> > >   			cxt.cur_ops->exit();
> > >   		cxt.init_called = false;
> > >   	}
> > > +
> > > +	free_cpumask_var(bind_readers);
> > > +	free_cpumask_var(bind_writers);
> > > +	bind_readers = NULL;
> > > +	bind_writers = NULL;
> > 同上

But here I agree with Wang Liang, as it helps people running debuggers
on the kernel.  Instead of a dangling pointer, they see a NULL pointer.

Except...  Is this NULLing really the right thing to do for
CONFIG_CPUMASK_OFFSTACK=n kernels?

							Thanx, Paul

> > > +
> > >   	torture_cleanup_end();
> > >   }

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Zhang Changzhong 2 weeks ago


在 2025/9/18 17:03, Paul E. McKenney 写道:
> On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
>>
>> 在 2025/9/12 10:16, Zhang Changzhong 写道:
>>>
>>> 在 2025/9/12 9:57, Wang Liang 写道:
>>>> When setting the locktorture module parameter 'bind_writers', the variable
>>>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
>>>> is not freed, when removing module or setting the parameter again.
>>>>
>>>> Below kmemleak trace is seen for this issue:
>>>>
>>>> unreferenced object 0xffff888100aabff8 (size 8):
>>>>    comm "bash", pid 323, jiffies 4295059233
>>>>    hex dump (first 8 bytes):
>>>>      07 00 00 00 00 00 00 00                          ........
>>>>    backtrace (crc ac50919):
>>>>      __kmalloc_node_noprof+0x2e5/0x420
>>>>      alloc_cpumask_var_node+0x1f/0x30
>>>>      param_set_cpumask+0x26/0xb0 [locktorture]
>>>>      param_attr_store+0x93/0x100
>>>>      module_attr_store+0x1b/0x30
>>>>      kernfs_fop_write_iter+0x114/0x1b0
>>>>      vfs_write+0x300/0x410
>>>>      ksys_write+0x60/0xd0
>>>>      do_syscall_64+0xa4/0x260
>>>>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>>
>>>> This issue can be reproduced by:
>>>>    insmod locktorture.ko
>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>    rmmod locktorture
>>>>
>>>> or:
>>>>    insmod locktorture.ko
>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>
>>>> The parameter 'bind_readers' also has the same problem. Free the memory
>>>> when removing module or setting the parameter.
>>>>
>>>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
>>>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>>>> ---
>>>>   kernel/locking/locktorture.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>>>> index ce0362f0a871..cad80c050502 100644
>>>> --- a/kernel/locking/locktorture.c
>>>> +++ b/kernel/locking/locktorture.c
>>>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
>>>>   	int ret;
>>>>   	char *s;
>>>> +	free_cpumask_var(*cm_bind);
>>>> +	*cm_bind = NULL;
>>> 这个NULL没必要吧
> 
> Assuming this translates to "This NULL is unnecessary", I have to
> agree with Zhang Changzhong.  I would go further and argue that the
> free_cpumask_var() is also unnecessary here.
> 

Sorry, I used Chinese by mistake—I didn't notice this was a public thread.

With CONFIG_CPUMASK_OFFSTACK=y, the free_cpumask_var() here seems necessary,
when param_set_cpumask() called multiple times, 'cm_bind' gets overwritten,
and the free_cpumask_var() in lock_torture_cleanup() cannot free the old memory.

>> Setting global pointer to NULL after free may be more safe. ^-^
> 
> In lock_torture_cleanup(), you mean?  I would agree with that.
> 
>>>> +
>>>>   	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
>>>>   		s = "Out of memory";
>>>>   		ret = -ENOMEM;
>>>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
>>>>   			cxt.cur_ops->exit();
>>>>   		cxt.init_called = false;
>>>>   	}
>>>> +
>>>> +	free_cpumask_var(bind_readers);
>>>> +	free_cpumask_var(bind_writers);
>>>> +	bind_readers = NULL;
>>>> +	bind_writers = NULL;
>>> 同上
> 
> But here I agree with Wang Liang, as it helps people running debuggers
> on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
> 
> Except...  Is this NULLing really the right thing to do for
> CONFIG_CPUMASK_OFFSTACK=n kernels?
> 
> 							Thanx, Paul
> 
>>>> +
>>>>   	torture_cleanup_end();
>>>>   }
>

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Paul E. McKenney 2 weeks ago

On Thu, Sep 18, 2025 at 11:06:45PM +0800, Zhang Changzhong wrote:
> 在 2025/9/18 17:03, Paul E. McKenney 写道:
> > On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
> >> 在 2025/9/12 10:16, Zhang Changzhong 写道:
> >>> 在 2025/9/12 9:57, Wang Liang 写道:
> >>>> When setting the locktorture module parameter 'bind_writers', the variable
> >>>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
> >>>> is not freed, when removing module or setting the parameter again.
> >>>>
> >>>> Below kmemleak trace is seen for this issue:
> >>>>
> >>>> unreferenced object 0xffff888100aabff8 (size 8):
> >>>>    comm "bash", pid 323, jiffies 4295059233
> >>>>    hex dump (first 8 bytes):
> >>>>      07 00 00 00 00 00 00 00                          ........
> >>>>    backtrace (crc ac50919):
> >>>>      __kmalloc_node_noprof+0x2e5/0x420
> >>>>      alloc_cpumask_var_node+0x1f/0x30
> >>>>      param_set_cpumask+0x26/0xb0 [locktorture]
> >>>>      param_attr_store+0x93/0x100
> >>>>      module_attr_store+0x1b/0x30
> >>>>      kernfs_fop_write_iter+0x114/0x1b0
> >>>>      vfs_write+0x300/0x410
> >>>>      ksys_write+0x60/0xd0
> >>>>      do_syscall_64+0xa4/0x260
> >>>>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >>>>
> >>>> This issue can be reproduced by:
> >>>>    insmod locktorture.ko
> >>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> >>>>    rmmod locktorture
> >>>>
> >>>> or:
> >>>>    insmod locktorture.ko
> >>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> >>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> >>>>
> >>>> The parameter 'bind_readers' also has the same problem. Free the memory
> >>>> when removing module or setting the parameter.
> >>>>
> >>>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
> >>>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> >>>> ---
> >>>>   kernel/locking/locktorture.c | 9 +++++++++
> >>>>   1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> >>>> index ce0362f0a871..cad80c050502 100644
> >>>> --- a/kernel/locking/locktorture.c
> >>>> +++ b/kernel/locking/locktorture.c
> >>>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
> >>>>   	int ret;
> >>>>   	char *s;
> >>>> +	free_cpumask_var(*cm_bind);
> >>>> +	*cm_bind = NULL;
> >>> 这个NULL没必要吧
> > 
> > Assuming this translates to "This NULL is unnecessary", I have to
> > agree with Zhang Changzhong.  I would go further and argue that the
> > free_cpumask_var() is also unnecessary here.
> 
> Sorry, I used Chinese by mistake—I didn't notice this was a public thread.

Not a problem!  There is always translation software, not that I ever
will completely trust it.  ;-)

> With CONFIG_CPUMASK_OFFSTACK=y, the free_cpumask_var() here seems necessary,
> when param_set_cpumask() called multiple times, 'cm_bind' gets overwritten,
> and the free_cpumask_var() in lock_torture_cleanup() cannot free the old memory.

So the situation you are worried about is when the user mistakenly puts
multiple copies of one of the locktorture.bind_{readers,writers} module
parameters on the kernel boot command line or as a modprobe parameter?

If so, what do we really want to happen in that case?  Do we want the
last (say) locktorture.bind_readers value to win?  Or do we want to OR
together all such values?

							Thanx, Paul

> >> Setting global pointer to NULL after free may be more safe. ^-^
> > 
> > In lock_torture_cleanup(), you mean?  I would agree with that.
> > 
> >>>> +
> >>>>   	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
> >>>>   		s = "Out of memory";
> >>>>   		ret = -ENOMEM;
> >>>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
> >>>>   			cxt.cur_ops->exit();
> >>>>   		cxt.init_called = false;
> >>>>   	}
> >>>> +
> >>>> +	free_cpumask_var(bind_readers);
> >>>> +	free_cpumask_var(bind_writers);
> >>>> +	bind_readers = NULL;
> >>>> +	bind_writers = NULL;
> >>> 同上
> > 
> > But here I agree with Wang Liang, as it helps people running debuggers
> > on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
> > 
> > Except...  Is this NULLing really the right thing to do for
> > CONFIG_CPUMASK_OFFSTACK=n kernels?
> > 
> > 							Thanx, Paul
> > 
> >>>> +
> >>>>   	torture_cleanup_end();
> >>>>   }
> > 
>

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Zhang Changzhong 1 week, 6 days ago

在 2025/9/18 23:20, Paul E. McKenney 写道:
> On Thu, Sep 18, 2025 at 11:06:45PM +0800, Zhang Changzhong wrote:
>> 在 2025/9/18 17:03, Paul E. McKenney 写道:
>>> On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
>>>> 在 2025/9/12 10:16, Zhang Changzhong 写道:
>>>>> 在 2025/9/12 9:57, Wang Liang 写道:
>>>>>> When setting the locktorture module parameter 'bind_writers', the variable
>>>>>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
>>>>>> is not freed, when removing module or setting the parameter again.
>>>>>>
>>>>>> Below kmemleak trace is seen for this issue:
>>>>>>
>>>>>> unreferenced object 0xffff888100aabff8 (size 8):
>>>>>>    comm "bash", pid 323, jiffies 4295059233
>>>>>>    hex dump (first 8 bytes):
>>>>>>      07 00 00 00 00 00 00 00                          ........
>>>>>>    backtrace (crc ac50919):
>>>>>>      __kmalloc_node_noprof+0x2e5/0x420
>>>>>>      alloc_cpumask_var_node+0x1f/0x30
>>>>>>      param_set_cpumask+0x26/0xb0 [locktorture]
>>>>>>      param_attr_store+0x93/0x100
>>>>>>      module_attr_store+0x1b/0x30
>>>>>>      kernfs_fop_write_iter+0x114/0x1b0
>>>>>>      vfs_write+0x300/0x410
>>>>>>      ksys_write+0x60/0xd0
>>>>>>      do_syscall_64+0xa4/0x260
>>>>>>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>>>>
>>>>>> This issue can be reproduced by:
>>>>>>    insmod locktorture.ko
>>>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>>>    rmmod locktorture
>>>>>>
>>>>>> or:
>>>>>>    insmod locktorture.ko
>>>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>>>
>>>>>> The parameter 'bind_readers' also has the same problem. Free the memory
>>>>>> when removing module or setting the parameter.
>>>>>>
>>>>>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
>>>>>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>>>>>> ---
>>>>>>   kernel/locking/locktorture.c | 9 +++++++++
>>>>>>   1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>>>>>> index ce0362f0a871..cad80c050502 100644
>>>>>> --- a/kernel/locking/locktorture.c
>>>>>> +++ b/kernel/locking/locktorture.c
>>>>>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
>>>>>>   	int ret;
>>>>>>   	char *s;
>>>>>> +	free_cpumask_var(*cm_bind);
>>>>>> +	*cm_bind = NULL;
>>>>> 这个NULL没必要吧
>>>
>>> Assuming this translates to "This NULL is unnecessary", I have to
>>> agree with Zhang Changzhong.  I would go further and argue that the
>>> free_cpumask_var() is also unnecessary here.
>>
>> Sorry, I used Chinese by mistake—I didn't notice this was a public thread.
> 
> Not a problem!  There is always translation software, not that I ever
> will completely trust it.  ;-)
> 
>> With CONFIG_CPUMASK_OFFSTACK=y, the free_cpumask_var() here seems necessary,
>> when param_set_cpumask() called multiple times, 'cm_bind' gets overwritten,
>> and the free_cpumask_var() in lock_torture_cleanup() cannot free the old memory.
> 
> So the situation you are worried about is when the user mistakenly puts
> multiple copies of one of the locktorture.bind_{readers,writers} module
> parameters on the kernel boot command line or as a modprobe parameter?
> 

I didn't consider this situation. What I noticed is that bind_{readers,writers}
are writable interface, and fuzz testing tools like syzkaller can easily write
to /sys/module/locktorture/parameters/bind_{readers,writers} and trigger memory
leak.

In this case, the patch fixes the memory leak issue, but the old parameters
remain in effect instead of the newly written ones. Considering that writing
to this interface after modprobe has no real effect, how about set the
permissions to 0444?

> If so, what do we really want to happen in that case?  Do we want the
> last (say) locktorture.bind_readers value to win?  Or do we want to OR
> together all such values?

In the case you mentioned, it seems more reasonable that the last
locktorture.bind_readers wins, which is also the current behavior.
 >
> 							Thanx, Paul
> 
>>>> Setting global pointer to NULL after free may be more safe. ^-^
>>>
>>> In lock_torture_cleanup(), you mean?  I would agree with that.
>>>
>>>>>> +
>>>>>>   	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
>>>>>>   		s = "Out of memory";
>>>>>>   		ret = -ENOMEM;
>>>>>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
>>>>>>   			cxt.cur_ops->exit();
>>>>>>   		cxt.init_called = false;
>>>>>>   	}
>>>>>> +
>>>>>> +	free_cpumask_var(bind_readers);
>>>>>> +	free_cpumask_var(bind_writers);
>>>>>> +	bind_readers = NULL;
>>>>>> +	bind_writers = NULL;
>>>>> 同上
>>>
>>> But here I agree with Wang Liang, as it helps people running debuggers
>>> on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
>>>
>>> Except...  Is this NULLing really the right thing to do for
>>> CONFIG_CPUMASK_OFFSTACK=n kernels?
>>>
>>> 							Thanx, Paul
>>>
>>>>>> +
>>>>>>   	torture_cleanup_end();
>>>>>>   }
>>>
>>
>

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Paul E. McKenney 1 week, 6 days ago

On Fri, Sep 19, 2025 at 06:22:40PM +0800, Zhang Changzhong wrote:
> 在 2025/9/18 23:20, Paul E. McKenney 写道:
> > On Thu, Sep 18, 2025 at 11:06:45PM +0800, Zhang Changzhong wrote:
> >> 在 2025/9/18 17:03, Paul E. McKenney 写道:
> >>> On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
> >>>> 在 2025/9/12 10:16, Zhang Changzhong 写道:
> >>>>> 在 2025/9/12 9:57, Wang Liang 写道:
> >>>>>> When setting the locktorture module parameter 'bind_writers', the variable
> >>>>>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
> >>>>>> is not freed, when removing module or setting the parameter again.
> >>>>>>
> >>>>>> Below kmemleak trace is seen for this issue:
> >>>>>>
> >>>>>> unreferenced object 0xffff888100aabff8 (size 8):
> >>>>>>    comm "bash", pid 323, jiffies 4295059233
> >>>>>>    hex dump (first 8 bytes):
> >>>>>>      07 00 00 00 00 00 00 00                          ........
> >>>>>>    backtrace (crc ac50919):
> >>>>>>      __kmalloc_node_noprof+0x2e5/0x420
> >>>>>>      alloc_cpumask_var_node+0x1f/0x30
> >>>>>>      param_set_cpumask+0x26/0xb0 [locktorture]
> >>>>>>      param_attr_store+0x93/0x100
> >>>>>>      module_attr_store+0x1b/0x30
> >>>>>>      kernfs_fop_write_iter+0x114/0x1b0
> >>>>>>      vfs_write+0x300/0x410
> >>>>>>      ksys_write+0x60/0xd0
> >>>>>>      do_syscall_64+0xa4/0x260
> >>>>>>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >>>>>>
> >>>>>> This issue can be reproduced by:
> >>>>>>    insmod locktorture.ko
> >>>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> >>>>>>    rmmod locktorture
> >>>>>>
> >>>>>> or:
> >>>>>>    insmod locktorture.ko
> >>>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> >>>>>>    echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> >>>>>>
> >>>>>> The parameter 'bind_readers' also has the same problem. Free the memory
> >>>>>> when removing module or setting the parameter.
> >>>>>>
> >>>>>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
> >>>>>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
> >>>>>> ---
> >>>>>>   kernel/locking/locktorture.c | 9 +++++++++
> >>>>>>   1 file changed, 9 insertions(+)
> >>>>>>
> >>>>>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> >>>>>> index ce0362f0a871..cad80c050502 100644
> >>>>>> --- a/kernel/locking/locktorture.c
> >>>>>> +++ b/kernel/locking/locktorture.c
> >>>>>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
> >>>>>>   	int ret;
> >>>>>>   	char *s;
> >>>>>> +	free_cpumask_var(*cm_bind);
> >>>>>> +	*cm_bind = NULL;
> >>>>> 这个NULL没必要吧
> >>>
> >>> Assuming this translates to "This NULL is unnecessary", I have to
> >>> agree with Zhang Changzhong.  I would go further and argue that the
> >>> free_cpumask_var() is also unnecessary here.
> >>
> >> Sorry, I used Chinese by mistake—I didn't notice this was a public thread.
> > 
> > Not a problem!  There is always translation software, not that I ever
> > will completely trust it.  ;-)
> > 
> >> With CONFIG_CPUMASK_OFFSTACK=y, the free_cpumask_var() here seems necessary,
> >> when param_set_cpumask() called multiple times, 'cm_bind' gets overwritten,
> >> and the free_cpumask_var() in lock_torture_cleanup() cannot free the old memory.
> > 
> > So the situation you are worried about is when the user mistakenly puts
> > multiple copies of one of the locktorture.bind_{readers,writers} module
> > parameters on the kernel boot command line or as a modprobe parameter?
> > 
> 
> I didn't consider this situation. What I noticed is that bind_{readers,writers}
> are writable interface, and fuzz testing tools like syzkaller can easily write
> to /sys/module/locktorture/parameters/bind_{readers,writers} and trigger memory
> leak.
> 
> In this case, the patch fixes the memory leak issue, but the old parameters
> remain in effect instead of the newly written ones. Considering that writing
> to this interface after modprobe has no real effect, how about set the
> permissions to 0444?

That sounds like a most excellent approach.

> > If so, what do we really want to happen in that case?  Do we want the
> > last (say) locktorture.bind_readers value to win?  Or do we want to OR
> > together all such values?
> 
> In the case you mentioned, it seems more reasonable that the last
> locktorture.bind_readers wins, which is also the current behavior.

In which case, what has to happen to handle something like this?

	modprobe locktorture bind_readers=8
	rmmod locktorture
	modprobe locktorture bind_readers=5

Doesn't this require careful handling of the bind_readers variable in
lock_torture_cleanup(), for both possible settings of the CPUMASK_OFFSTACK
Kconfig option?

 							Thanx, Paul

> >>>> Setting global pointer to NULL after free may be more safe. ^-^
> >>>
> >>> In lock_torture_cleanup(), you mean?  I would agree with that.
> >>>
> >>>>>> +
> >>>>>>   	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
> >>>>>>   		s = "Out of memory";
> >>>>>>   		ret = -ENOMEM;
> >>>>>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
> >>>>>>   			cxt.cur_ops->exit();
> >>>>>>   		cxt.init_called = false;
> >>>>>>   	}
> >>>>>> +
> >>>>>> +	free_cpumask_var(bind_readers);
> >>>>>> +	free_cpumask_var(bind_writers);
> >>>>>> +	bind_readers = NULL;
> >>>>>> +	bind_writers = NULL;
> >>>>> 同上
> >>>
> >>> But here I agree with Wang Liang, as it helps people running debuggers
> >>> on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
> >>>
> >>> Except...  Is this NULLing really the right thing to do for
> >>> CONFIG_CPUMASK_OFFSTACK=n kernels?
> >>>
> >>> 							Thanx, Paul
> >>>
> >>>>>> +
> >>>>>>   	torture_cleanup_end();
> >>>>>>   }
> >>>
> >>
> > 
>

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Wang Liang 1 week, 2 days ago

在 2025/9/19 18:41, Paul E. McKenney 写道:
> On Fri, Sep 19, 2025 at 06:22:40PM +0800, Zhang Changzhong wrote:
>> 在 2025/9/18 23:20, Paul E. McKenney 写道:
>>> On Thu, Sep 18, 2025 at 11:06:45PM +0800, Zhang Changzhong wrote:
>>>> 在 2025/9/18 17:03, Paul E. McKenney 写道:
>>>>> On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
>>>>>> 在 2025/9/12 10:16, Zhang Changzhong 写道:
>>>>>>> 在 2025/9/12 9:57, Wang Liang 写道:
>>>>>>>> When setting the locktorture module parameter 'bind_writers', the variable
>>>>>>>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
>>>>>>>> is not freed, when removing module or setting the parameter again.
>>>>>>>>
>>>>>>>> Below kmemleak trace is seen for this issue:
>>>>>>>>
>>>>>>>> unreferenced object 0xffff888100aabff8 (size 8):
>>>>>>>>     comm "bash", pid 323, jiffies 4295059233
>>>>>>>>     hex dump (first 8 bytes):
>>>>>>>>       07 00 00 00 00 00 00 00                          ........
>>>>>>>>     backtrace (crc ac50919):
>>>>>>>>       __kmalloc_node_noprof+0x2e5/0x420
>>>>>>>>       alloc_cpumask_var_node+0x1f/0x30
>>>>>>>>       param_set_cpumask+0x26/0xb0 [locktorture]
>>>>>>>>       param_attr_store+0x93/0x100
>>>>>>>>       module_attr_store+0x1b/0x30
>>>>>>>>       kernfs_fop_write_iter+0x114/0x1b0
>>>>>>>>       vfs_write+0x300/0x410
>>>>>>>>       ksys_write+0x60/0xd0
>>>>>>>>       do_syscall_64+0xa4/0x260
>>>>>>>>       entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>>>>>>
>>>>>>>> This issue can be reproduced by:
>>>>>>>>     insmod locktorture.ko
>>>>>>>>     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>>>>>     rmmod locktorture
>>>>>>>>
>>>>>>>> or:
>>>>>>>>     insmod locktorture.ko
>>>>>>>>     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>>>>>     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>>>>>
>>>>>>>> The parameter 'bind_readers' also has the same problem. Free the memory
>>>>>>>> when removing module or setting the parameter.
>>>>>>>>
>>>>>>>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
>>>>>>>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>>>>>>>> ---
>>>>>>>>    kernel/locking/locktorture.c | 9 +++++++++
>>>>>>>>    1 file changed, 9 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>>>>>>>> index ce0362f0a871..cad80c050502 100644
>>>>>>>> --- a/kernel/locking/locktorture.c
>>>>>>>> +++ b/kernel/locking/locktorture.c
>>>>>>>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
>>>>>>>>    	int ret;
>>>>>>>>    	char *s;
>>>>>>>> +	free_cpumask_var(*cm_bind);
>>>>>>>> +	*cm_bind = NULL;
>>>>>>> 这个NULL没必要吧
>>>>> Assuming this translates to "This NULL is unnecessary", I have to
>>>>> agree with Zhang Changzhong.  I would go further and argue that the
>>>>> free_cpumask_var() is also unnecessary here.
>>>> Sorry, I used Chinese by mistake—I didn't notice this was a public thread.
>>> Not a problem!  There is always translation software, not that I ever
>>> will completely trust it.  ;-)
>>>
>>>> With CONFIG_CPUMASK_OFFSTACK=y, the free_cpumask_var() here seems necessary,
>>>> when param_set_cpumask() called multiple times, 'cm_bind' gets overwritten,
>>>> and the free_cpumask_var() in lock_torture_cleanup() cannot free the old memory.
>>> So the situation you are worried about is when the user mistakenly puts
>>> multiple copies of one of the locktorture.bind_{readers,writers} module
>>> parameters on the kernel boot command line or as a modprobe parameter?
>>>
>> I didn't consider this situation. What I noticed is that bind_{readers,writers}
>> are writable interface, and fuzz testing tools like syzkaller can easily write
>> to /sys/module/locktorture/parameters/bind_{readers,writers} and trigger memory
>> leak.
>>
>> In this case, the patch fixes the memory leak issue, but the old parameters
>> remain in effect instead of the newly written ones. Considering that writing
>> to this interface after modprobe has no real effect, how about set the
>> permissions to 0444?
> That sounds like a most excellent approach.
>
>>> If so, what do we really want to happen in that case?  Do we want the
>>> last (say) locktorture.bind_readers value to win?  Or do we want to OR
>>> together all such values?
>> In the case you mentioned, it seems more reasonable that the last
>> locktorture.bind_readers wins, which is also the current behavior.
> In which case, what has to happen to handle something like this?
>
> 	modprobe locktorture bind_readers=8
> 	rmmod locktorture
> 	modprobe locktorture bind_readers=5
>
> Doesn't this require careful handling of the bind_readers variable in
> lock_torture_cleanup(), for both possible settings of the CPUMASK_OFFSTACK
> Kconfig option?
>
>   							Thanx, Paul


@Paul @Zhang thanks for all your suggestions!

Setting the parameter permissions to 0444 is a good approach, so user can't
change the parameters and not need to call free_cpumask_var() in
param_set_cpumask().

The free_cpumask_var() is still need to free bind_readers/bind_writers.

In below case, the 'bind_readers' is freed by rmmod. When run modprobe at
the second time, the 'bind_readers' is reinitialized to NULL before
allocated in param_set_cpumask().

     modprobe locktorture bind_readers=8
     rmmod locktorture
     modprobe locktorture bind_readers=5

So no need to NULLing bind_readers, the final patch like this:

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index ce0362f0a871..6567e5eeacc0 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -103,8 +103,8 @@ static const struct kernel_param_ops lt_bind_ops = {
         .get = param_get_cpumask,
  };

-module_param_cb(bind_readers, &lt_bind_ops, &bind_readers, 0644);
-module_param_cb(bind_writers, &lt_bind_ops, &bind_writers, 0644);
+module_param_cb(bind_readers, &lt_bind_ops, &bind_readers, 0444);
+module_param_cb(bind_writers, &lt_bind_ops, &bind_writers, 0444);

  long torture_sched_setaffinity(pid_t pid, const struct cpumask 
*in_mask, bool dowarn);

@@ -1211,6 +1211,10 @@ static void lock_torture_cleanup(void)
                         cxt.cur_ops->exit();
                 cxt.init_called = false;
         }
+
+       free_cpumask_var(bind_readers);
+       free_cpumask_var(bind_writers);
+
         torture_cleanup_end();
  }

------
Best regards
Wang Liang

>>>>>> Setting global pointer to NULL after free may be more safe. ^-^
>>>>> In lock_torture_cleanup(), you mean?  I would agree with that.
>>>>>
>>>>>>>> +
>>>>>>>>    	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
>>>>>>>>    		s = "Out of memory";
>>>>>>>>    		ret = -ENOMEM;
>>>>>>>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
>>>>>>>>    			cxt.cur_ops->exit();
>>>>>>>>    		cxt.init_called = false;
>>>>>>>>    	}
>>>>>>>> +
>>>>>>>> +	free_cpumask_var(bind_readers);
>>>>>>>> +	free_cpumask_var(bind_writers);
>>>>>>>> +	bind_readers = NULL;
>>>>>>>> +	bind_writers = NULL;
>>>>>>> 同上
>>>>> But here I agree with Wang Liang, as it helps people running debuggers
>>>>> on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
>>>>>
>>>>> Except...  Is this NULLing really the right thing to do for
>>>>> CONFIG_CPUMASK_OFFSTACK=n kernels?
>>>>>
>>>>> 							Thanx, Paul
>>>>>
>>>>>>>> +
>>>>>>>>    	torture_cleanup_end();
>>>>>>>>    }

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Wang Liang 2 weeks ago

在 2025/9/18 17:03, Paul E. McKenney 写道:
> On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
>> 在 2025/9/12 10:16, Zhang Changzhong 写道:
>>> 在 2025/9/12 9:57, Wang Liang 写道:
>>>> When setting the locktorture module parameter 'bind_writers', the variable
>>>> 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
>>>> is not freed, when removing module or setting the parameter again.
>>>>
>>>> Below kmemleak trace is seen for this issue:
>>>>
>>>> unreferenced object 0xffff888100aabff8 (size 8):
>>>>     comm "bash", pid 323, jiffies 4295059233
>>>>     hex dump (first 8 bytes):
>>>>       07 00 00 00 00 00 00 00                          ........
>>>>     backtrace (crc ac50919):
>>>>       __kmalloc_node_noprof+0x2e5/0x420
>>>>       alloc_cpumask_var_node+0x1f/0x30
>>>>       param_set_cpumask+0x26/0xb0 [locktorture]
>>>>       param_attr_store+0x93/0x100
>>>>       module_attr_store+0x1b/0x30
>>>>       kernfs_fop_write_iter+0x114/0x1b0
>>>>       vfs_write+0x300/0x410
>>>>       ksys_write+0x60/0xd0
>>>>       do_syscall_64+0xa4/0x260
>>>>       entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>>
>>>> This issue can be reproduced by:
>>>>     insmod locktorture.ko
>>>>     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>     rmmod locktorture
>>>>
>>>> or:
>>>>     insmod locktorture.ko
>>>>     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
>>>>
>>>> The parameter 'bind_readers' also has the same problem. Free the memory
>>>> when removing module or setting the parameter.
>>>>
>>>> Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
>>>> Signed-off-by: Wang Liang <wangliang74@huawei.com>
>>>> ---
>>>>    kernel/locking/locktorture.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>>>> index ce0362f0a871..cad80c050502 100644
>>>> --- a/kernel/locking/locktorture.c
>>>> +++ b/kernel/locking/locktorture.c
>>>> @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
>>>>    	int ret;
>>>>    	char *s;
>>>> +	free_cpumask_var(*cm_bind);
>>>> +	*cm_bind = NULL;
>>> 这个NULL没必要吧
> Assuming this translates to "This NULL is unnecessary", I have to
> agree with Zhang Changzhong.  I would go further and argue that the
> free_cpumask_var() is also unnecessary here.


Thanks for your replies!

The free_cpumask_var() in param_set_cpumask() may be necessary(). If user
set the parameter for two times, the pointer will point to a new memory,
and no one hold the old memory, which trigger a memory leak.

>> Setting global pointer to NULL after free may be more safe. ^-^
> In lock_torture_cleanup(), you mean?  I would agree with that.
>
>>>> +
>>>>    	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
>>>>    		s = "Out of memory";
>>>>    		ret = -ENOMEM;
>>>> @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
>>>>    			cxt.cur_ops->exit();
>>>>    		cxt.init_called = false;
>>>>    	}
>>>> +
>>>> +	free_cpumask_var(bind_readers);
>>>> +	free_cpumask_var(bind_writers);
>>>> +	bind_readers = NULL;
>>>> +	bind_writers = NULL;
>>> 同上
> But here I agree with Wang Liang, as it helps people running debuggers
> on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
>
> Except...  Is this NULLing really the right thing to do for
> CONFIG_CPUMASK_OFFSTACK=n kernels?
>
> 							Thanx, Paul


For CONFIG_CPUMASK_OFFSTACK=n, the NULLing may be not appropriate. I 
will remove it.

>>>> +
>>>>    	torture_cleanup_end();
>>>>    }

Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

Posted by Paul E. McKenney 2 weeks ago

On Thu, Sep 18, 2025 at 07:17:41PM +0800, Wang Liang wrote:
> 
> 在 2025/9/18 17:03, Paul E. McKenney 写道:
> > On Mon, Sep 15, 2025 at 10:13:33AM +0800, Wang Liang wrote:
> > > 在 2025/9/12 10:16, Zhang Changzhong 写道:
> > > > 在 2025/9/12 9:57, Wang Liang 写道:
> > > > > When setting the locktorture module parameter 'bind_writers', the variable
> > > > > 'cpumask_var_t bind_writers' is allocated in param_set_cpumask(). But it
> > > > > is not freed, when removing module or setting the parameter again.
> > > > > 
> > > > > Below kmemleak trace is seen for this issue:
> > > > > 
> > > > > unreferenced object 0xffff888100aabff8 (size 8):
> > > > >     comm "bash", pid 323, jiffies 4295059233
> > > > >     hex dump (first 8 bytes):
> > > > >       07 00 00 00 00 00 00 00                          ........
> > > > >     backtrace (crc ac50919):
> > > > >       __kmalloc_node_noprof+0x2e5/0x420
> > > > >       alloc_cpumask_var_node+0x1f/0x30
> > > > >       param_set_cpumask+0x26/0xb0 [locktorture]
> > > > >       param_attr_store+0x93/0x100
> > > > >       module_attr_store+0x1b/0x30
> > > > >       kernfs_fop_write_iter+0x114/0x1b0
> > > > >       vfs_write+0x300/0x410
> > > > >       ksys_write+0x60/0xd0
> > > > >       do_syscall_64+0xa4/0x260
> > > > >       entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > > > 
> > > > > This issue can be reproduced by:
> > > > >     insmod locktorture.ko
> > > > >     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> > > > >     rmmod locktorture
> > > > > 
> > > > > or:
> > > > >     insmod locktorture.ko
> > > > >     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> > > > >     echo 0-2 > /sys/module/locktorture/parameters/bind_writers
> > > > > 
> > > > > The parameter 'bind_readers' also has the same problem. Free the memory
> > > > > when removing module or setting the parameter.
> > > > > 
> > > > > Fixes: 73e341242483 ("locktorture: Add readers_bind and writers_bind module parameters")
> > > > > Signed-off-by: Wang Liang <wangliang74@huawei.com>
> > > > > ---
> > > > >    kernel/locking/locktorture.c | 9 +++++++++
> > > > >    1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> > > > > index ce0362f0a871..cad80c050502 100644
> > > > > --- a/kernel/locking/locktorture.c
> > > > > +++ b/kernel/locking/locktorture.c
> > > > > @@ -70,6 +70,9 @@ static int param_set_cpumask(const char *val, const struct kernel_param *kp)
> > > > >    	int ret;
> > > > >    	char *s;
> > > > > +	free_cpumask_var(*cm_bind);
> > > > > +	*cm_bind = NULL;
> > > > 这个NULL没必要吧
> > Assuming this translates to "This NULL is unnecessary", I have to
> > agree with Zhang Changzhong.  I would go further and argue that the
> > free_cpumask_var() is also unnecessary here.
> 
> Thanks for your replies!
> 
> The free_cpumask_var() in param_set_cpumask() may be necessary(). If user
> set the parameter for two times, the pointer will point to a new memory,
> and no one hold the old memory, which trigger a memory leak.

Why wouldn't the free_cpumask_var() you are adding to
lock_torture_cleanup() cover that case?

> > > Setting global pointer to NULL after free may be more safe. ^-^
> > In lock_torture_cleanup(), you mean?  I would agree with that.
> > 
> > > > > +
> > > > >    	if (!alloc_cpumask_var(cm_bind, GFP_KERNEL)) {
> > > > >    		s = "Out of memory";
> > > > >    		ret = -ENOMEM;
> > > > > @@ -1211,6 +1214,12 @@ static void lock_torture_cleanup(void)
> > > > >    			cxt.cur_ops->exit();
> > > > >    		cxt.init_called = false;
> > > > >    	}
> > > > > +
> > > > > +	free_cpumask_var(bind_readers);
> > > > > +	free_cpumask_var(bind_writers);
> > > > > +	bind_readers = NULL;
> > > > > +	bind_writers = NULL;
> > > > 同上
> > But here I agree with Wang Liang, as it helps people running debuggers
> > on the kernel.  Instead of a dangling pointer, they see a NULL pointer.
> > 
> > Except...  Is this NULLing really the right thing to do for
> > CONFIG_CPUMASK_OFFSTACK=n kernels?
> 
> For CONFIG_CPUMASK_OFFSTACK=n, the NULLing may be not appropriate. I will
> remove it.

But if you remove the NULLing entirely, mightn't that inconvenience
people debugging?

							Thanx, Paul

> > > > > +
> > > > >    	torture_cleanup_end();
> > > > >    }