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: <679d81f3-2610-44b9-bc9a-30ef0f70fa36@paulmck-laptop>
Date: Thu, 18 Sep 2025 02:03:35 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Wang Liang <wangliang74@...wei.com>
Cc: Zhang Changzhong <zhangchangzhong@...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 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.

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