linux-kernel - Re: [PATCH] locktorture: Fix memory leak in param_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d07dd312-295e-4703-895b-8ce438acea3c@huawei.com>
Date: Fri, 19 Sep 2025 18:22:40 +0800
From: Zhang Changzhong <zhangchangzhong@...wei.com>
To: <paulmck@...nel.org>
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()

在 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?

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