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

["Paul E. McKenney" <paulmck@kernel.org>]

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