Suggestion on how to fix "memory leak in snd_ctl_led_register"

Suggestion on how to fix "memory leak in snd_ctl_led_register"

[慕冬亮]

Hi kernel developers,

The root cause of "memory leak in snd_ctl_led_register" [1] is
unbalanced refcount operations in the device_add function [2]. This
refcount issue causes struct device_private allocated in
device_private_init unable to be deallocated, leading to memory leak.
The details are as follows:

First, the get_device and put_device are balanced to keep the refcount
unchanged. And there seem no other refcount operations in the
following code. However, in my debugging, the klist_add_tail would
increase the refcount of dev with the get_device in the
klist_chldren_get. This is kind of surprising. Let's go deeper.

int device_add(struct device *dev)
{
......
dev = get_device(dev);
if (!dev)
goto done;

if (!dev->p) {
error = device_private_init(dev);
if (error)
goto done;
}
......
if (parent)
klist_add_tail(&dev->p->knode_parent,
       &parent->p->klist_children);
......
done:
put_device(dev);
return error;
......
}

In the klist_add_tail [3], n is &dev->p->knode_parent, k is
&parent->p->klist_children. Then it goes to klist_node_init. Here, in
my debugging, k->get is klist_children_get. In this function, it calls
get_device to increase the refcount.

void klist_add_tail(struct klist_node *n, struct klist *k)
{
klist_node_init(k, n);
add_tail(k, n);
}

static void klist_node_init(struct klist *k, struct klist_node *n)
{
INIT_LIST_HEAD(&n->n_node);
kref_init(&n->n_ref);
knode_set_klist(n, k);
if (k->get)
k->get(n);
}

static void klist_children_get(struct klist_node *n)
{
struct device_private *p = to_device_private_parent(n);
struct device *dev = p->device;

get_device(dev);
}

It seems that klist_children_get is initialized in device_private_init function.

Finally, I tried some patches, but they are not working. Now I have no
idea how to fix this bug. I will really appreciate if anyone can give
some suggestion on how to fix this bug.

BTW, if there are any issues in the root cause analysis, please let me know.

[1] https://syzkaller.appspot.com/bug?id=6d9e1e89003c894e7a1855c92dfa558ebcb8f218
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/core.c?id=e48661230cc35b3d0f4367eddfc19f86463ab917#n3198
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/klist.c?id=e48661230cc35b3d0f4367eddfc19f86463ab917#n134
--
My best regards to you.

     No System Is Safe!
     Dongliang Mu

Re: Suggestion on how to fix "memory leak in snd_ctl_led_register"

[Greg KH]

On Thu, May 27, 2021 at 10:49:22AM +0800, 慕冬亮 wrote:
> Hi kernel developers,
> 
> The root cause of "memory leak in snd_ctl_led_register" [1] is
> unbalanced refcount operations in the device_add function [2]. This
> refcount issue causes struct device_private allocated in
> device_private_init unable to be deallocated, leading to memory leak.
> The details are as follows:

<snip>

Great, can you just send a patch to us to resolve the issue?  That's the
easiest way to resolve all of this.

thanks,

greg k-h

Re: Suggestion on how to fix "memory leak in snd_ctl_led_register"

[慕冬亮]

On Thu, May 27, 2021 at 12:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 27, 2021 at 10:49:22AM +0800, 慕冬亮 wrote:
> > Hi kernel developers,
> >
> > The root cause of "memory leak in snd_ctl_led_register" [1] is
> > unbalanced refcount operations in the device_add function [2]. This
> > refcount issue causes struct device_private allocated in
> > device_private_init unable to be deallocated, leading to memory leak.
> > The details are as follows:
>
> <snip>
>
> Great, can you just send a patch to us to resolve the issue?  That's the
> easiest way to resolve all of this.

As I said in the last, I think up some fixes, but they are not working
in my local testing. So I have no idea how to fix this bug. Then I ask
for help.

>
> thanks,
>
> greg k-h

Re: Suggestion on how to fix "memory leak in snd_ctl_led_register"

[慕冬亮]

On Thu, May 27, 2021 at 12:20 PM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
>
> On Thu, May 27, 2021 at 12:17 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 27, 2021 at 10:49:22AM +0800, 慕冬亮 wrote:
> > > Hi kernel developers,
> > >
> > > The root cause of "memory leak in snd_ctl_led_register" [1] is
> > > unbalanced refcount operations in the device_add function [2]. This
> > > refcount issue causes struct device_private allocated in
> > > device_private_init unable to be deallocated, leading to memory leak.
> > > The details are as follows:
> >
> > <snip>
> >
> > Great, can you just send a patch to us to resolve the issue?  That's the
> > easiest way to resolve all of this.

Let me think more about the fix. I will send a patch if I can test it
successfully in my local workspace.

>
> As I said in the last, I think up some fixes, but they are not working
> in my local testing. So I have no idea how to fix this bug. Then I ask
> for help.
>
> >
> > thanks,
> >
> > greg k-h