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

[Zhang Changzhong <zhangchangzhong@huawei.com>]

在 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();
>>>>   }
>