Message-ID: <06f32215-ec2c-4d8f-8ccd-bc8dc250ec77@paulmck-laptop>
Date: Fri, 19 Sep 2025 03:41:02 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Zhang Changzhong <zhangchangzhong@...wei.com>
Cc: Wang Liang <wangliang74@...wei.com>, dave@...olabs.net,
	josh@...htriplett.org, frederic@...nel.org, yuehaibing@...wei.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] locktorture: Fix memory leak in param_set_cpumask()

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

Powered by blists - more mailing lists