Message-ID: <9d291c0f-17f0-4559-8b20-bb6186a8646b@huawei.com>
Date: Tue, 23 Sep 2025 11:42:38 +0800
From: Wang Liang <wangliang74@...wei.com>
To: <paulmck@...nel.org>, Zhang Changzhong <zhangchangzhong@...wei.com>
CC: <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/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@...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


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

Powered by blists - more mailing lists