[PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
to decrease the refcount to zero, which causes device_release never to
be invoked. This leads to memory leak to some resources, like struct
device_private.

Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove

Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 sound/core/control_led.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 25f57c14f294..fff2688b5019 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
 	snd_ctl_led_refresh();
 }
 
+static void snd_ctl_led_release(struct device *dev)
+{
+}
+
 /*
  * sysfs
  */
@@ -663,6 +667,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card)
 		led_card->number = card->number;
 		led_card->led = led;
 		device_initialize(&led_card->dev);
+		led_card->dev.release = snd_ctl_led_release;
 		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
 			goto cerr;
 		led_card->dev.parent = &led->dev;
@@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
 		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
 		sysfs_remove_link(&led_card->dev.kobj, "card");
 		device_del(&led_card->dev);
+		put_device(&led_card->dev);
 		kfree(led_card);
 		led->cards[card->number] = NULL;
 	}
-- 
2.25.1

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> to decrease the refcount to zero, which causes device_release never to
> be invoked. This leads to memory leak to some resources, like struct
> device_private.
> 
> Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> 
> Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  sound/core/control_led.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..fff2688b5019 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
>  	snd_ctl_led_refresh();
>  }
>  
> +static void snd_ctl_led_release(struct device *dev)
> +{
> +}

Whatever you're trying to do, adding a dummy function is never the
answer.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> to decrease the refcount to zero, which causes device_release never to
> be invoked. This leads to memory leak to some resources, like struct
> device_private.
> 
> Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> 
> Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  sound/core/control_led.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..fff2688b5019 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
>  	snd_ctl_led_refresh();
>  }
>  
> +static void snd_ctl_led_release(struct device *dev)
> +{
> +}

Whatever you're trying to do, adding a dummy function is never the
answer.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

-

On Fri, May 28, 2021 at 9:33 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> > The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> > the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> > to decrease the refcount to zero, which causes device_release never to
> > be invoked. This leads to memory leak to some resources, like struct
> > device_private.
> >
> > Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> >
> > Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> > Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  sound/core/control_led.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..fff2688b5019 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
> >       snd_ctl_led_refresh();
> >  }
> >
> > +static void snd_ctl_led_release(struct device *dev)
> > +{
> > +}
>
> Whatever you're trying to do, adding a dummy function is never the
> answer.

I see your point. This function I added is not to fix the root cause,
but to fix an issue caused by the release function when the device is
released.

The put_device is to fix the root cause(i.e., decrease the refcount to
zero), however, the result is dev->p and kobject can be freed, but it
will trigger a WARN [1] as it has no release method.

I don't know how to craft a release method for such a device. So this
dummy function is generated following the default_release, also a
dummy function.

Can you please give some advise on how to fix this WARN issue?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110

>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

-

On Fri, May 28, 2021 at 9:33 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> > The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> > the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> > to decrease the refcount to zero, which causes device_release never to
> > be invoked. This leads to memory leak to some resources, like struct
> > device_private.
> >
> > Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> >
> > Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> > Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  sound/core/control_led.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..fff2688b5019 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
> >       snd_ctl_led_refresh();
> >  }
> >
> > +static void snd_ctl_led_release(struct device *dev)
> > +{
> > +}
>
> Whatever you're trying to do, adding a dummy function is never the
> answer.

I see your point. This function I added is not to fix the root cause,
but to fix an issue caused by the release function when the device is
released.

The put_device is to fix the root cause(i.e., decrease the refcount to
zero), however, the result is dev->p and kobject can be freed, but it
will trigger a WARN [1] as it has no release method.

I don't know how to craft a release method for such a device. So this
dummy function is generated following the default_release, also a
dummy function.

Can you please give some advise on how to fix this WARN issue?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110

>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
>
> Can you please give some advise on how to fix this WARN issue?

But it feels like it spoils the fun if I write the commit...  Anyway:

regards,
dan carpenter

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 25f57c14f294..dd357abc1b58 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
 			for (; group > 0; group--) {
 				led = &snd_ctl_leds[group - 1];
 				device_del(&led->dev);
+				device_put(&led->dev);
 			}
 			device_del(&snd_ctl_led_dev);
 			return -ENOMEM;
@@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
 	for (group = 0; group < MAX_LED; group++) {
 		led = &snd_ctl_leds[group];
 		device_del(&led->dev);
+		device_put(&led->dev);
 	}
 	device_del(&snd_ctl_led_dev);
 	snd_ctl_led_clean(NULL);

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
>
> Can you please give some advise on how to fix this WARN issue?

But it feels like it spoils the fun if I write the commit...  Anyway:

regards,
dan carpenter

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 25f57c14f294..dd357abc1b58 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
 			for (; group > 0; group--) {
 				led = &snd_ctl_leds[group - 1];
 				device_del(&led->dev);
+				device_put(&led->dev);
 			}
 			device_del(&snd_ctl_led_dev);
 			return -ENOMEM;
@@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
 	for (group = 0; group < MAX_LED; group++) {
 		led = &snd_ctl_leds[group];
 		device_del(&led->dev);
+		device_put(&led->dev);
 	}
 	device_del(&snd_ctl_led_dev);
 	snd_ctl_led_clean(NULL);

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[慕冬亮]

> On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
>> 
>> Can you please give some advise on how to fix this WARN issue?
> 
> But it feels like it spoils the fun if I write the commit...  Anyway:

It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.

> 
> regards,
> dan carpenter
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..dd357abc1b58 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> 			for (; group > 0; group--) {
> 				led = &snd_ctl_leds[group - 1];
> 				device_del(&led->dev);
> +				device_put(&led->dev);
> 			}
> 			device_del(&snd_ctl_led_dev);
> 			return -ENOMEM;
> @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> 	for (group = 0; group < MAX_LED; group++) {
> 		led = &snd_ctl_leds[group];
> 		device_del(&led->dev);
> +		device_put(&led->dev);
> 	}
> 	device_del(&snd_ctl_led_dev);
> 	snd_ctl_led_clean(NULL);

Does this patch mean I should add device_put in the init and exit function other than snd_ctl_led_sysfs_remove? This will cause device_release bypass the release method checking?

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[慕冬亮]

> On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
>> 
>> Can you please give some advise on how to fix this WARN issue?
> 
> But it feels like it spoils the fun if I write the commit...  Anyway:

It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.

> 
> regards,
> dan carpenter
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..dd357abc1b58 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> 			for (; group > 0; group--) {
> 				led = &snd_ctl_leds[group - 1];
> 				device_del(&led->dev);
> +				device_put(&led->dev);
> 			}
> 			device_del(&snd_ctl_led_dev);
> 			return -ENOMEM;
> @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> 	for (group = 0; group < MAX_LED; group++) {
> 		led = &snd_ctl_leds[group];
> 		device_del(&led->dev);
> +		device_put(&led->dev);
> 	}
> 	device_del(&snd_ctl_led_dev);
> 	snd_ctl_led_clean(NULL);

Does this patch mean I should add device_put in the init and exit function other than snd_ctl_led_sysfs_remove? This will cause device_release bypass the release method checking?

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
>
>
>
> > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> >>
> >> Can you please give some advise on how to fix this WARN issue?
> >
> > But it feels like it spoils the fun if I write the commit...  Anyway:
>
> It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
>
> >
> > regards,
> > dan carpenter
> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..dd357abc1b58 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> >                       for (; group > 0; group--) {
> >                               led = &snd_ctl_leds[group - 1];
> >                               device_del(&led->dev);
> > +                             device_put(&led->dev);
> >                       }
> >                       device_del(&snd_ctl_led_dev);
> >                       return -ENOMEM;
> > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> >       for (group = 0; group < MAX_LED; group++) {
> >               led = &snd_ctl_leds[group];
> >               device_del(&led->dev);
> > +             device_put(&led->dev);
> >       }
> >       device_del(&snd_ctl_led_dev);
> >       snd_ctl_led_clean(NULL);

Hi Dan,

I tried this patch, and it still triggers the memleak. My
understanding is that the device object is already freed in the
snd_ctl_led_sysfs_remove.

>
> Does this patch mean I should add device_put in the init and exit function other than snd_ctl_led_sysfs_remove? This will cause device_release bypass the release method checking?
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
>
>
>
> > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> >>
> >> Can you please give some advise on how to fix this WARN issue?
> >
> > But it feels like it spoils the fun if I write the commit...  Anyway:
>
> It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
>
> >
> > regards,
> > dan carpenter
> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..dd357abc1b58 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> >                       for (; group > 0; group--) {
> >                               led = &snd_ctl_leds[group - 1];
> >                               device_del(&led->dev);
> > +                             device_put(&led->dev);
> >                       }
> >                       device_del(&snd_ctl_led_dev);
> >                       return -ENOMEM;
> > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> >       for (group = 0; group < MAX_LED; group++) {
> >               led = &snd_ctl_leds[group];
> >               device_del(&led->dev);
> > +             device_put(&led->dev);
> >       }
> >       device_del(&snd_ctl_led_dev);
> >       snd_ctl_led_clean(NULL);

Hi Dan,

I tried this patch, and it still triggers the memleak. My
understanding is that the device object is already freed in the
snd_ctl_led_sysfs_remove.

>
> Does this patch mean I should add device_put in the init and exit function other than snd_ctl_led_sysfs_remove? This will cause device_release bypass the release method checking?
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Sat, May 29, 2021 at 05:35:22AM +0800, 慕冬亮 wrote:
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..dd357abc1b58 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > 			for (; group > 0; group--) {
> > 				led = &snd_ctl_leds[group - 1];
> > 				device_del(&led->dev);
> > +				device_put(&led->dev);
> > 			}
> > 			device_del(&snd_ctl_led_dev);
> > 			return -ENOMEM;
> > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > 	for (group = 0; group < MAX_LED; group++) {
> > 		led = &snd_ctl_leds[group];
> > 		device_del(&led->dev);
> > +		device_put(&led->dev);
> > 	}
> > 	device_del(&snd_ctl_led_dev);
> > 	snd_ctl_led_clean(NULL);
> 
> Does this patch mean I should add device_put in the init and exit
> function other than snd_ctl_led_sysfs_remove? This will cause
> device_release bypass the release method checking?

Please fix your email client to line wrap your emails.

I'm not sure what release method checking you're refering to.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Sat, May 29, 2021 at 05:35:22AM +0800, 慕冬亮 wrote:
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..dd357abc1b58 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > 			for (; group > 0; group--) {
> > 				led = &snd_ctl_leds[group - 1];
> > 				device_del(&led->dev);
> > +				device_put(&led->dev);
> > 			}
> > 			device_del(&snd_ctl_led_dev);
> > 			return -ENOMEM;
> > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > 	for (group = 0; group < MAX_LED; group++) {
> > 		led = &snd_ctl_leds[group];
> > 		device_del(&led->dev);
> > +		device_put(&led->dev);
> > 	}
> > 	device_del(&snd_ctl_led_dev);
> > 	snd_ctl_led_clean(NULL);
> 
> Does this patch mean I should add device_put in the init and exit
> function other than snd_ctl_led_sysfs_remove? This will cause
> device_release bypass the release method checking?

Please fix your email client to line wrap your emails.

I'm not sure what release method checking you're refering to.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> >
> >
> >
> > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > >>
> > >> Can you please give some advise on how to fix this WARN issue?
> > >
> > > But it feels like it spoils the fun if I write the commit...  Anyway:
> >
> > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> >
> > >
> > > regards,
> > > dan carpenter
> > >
> > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > index 25f57c14f294..dd357abc1b58 100644
> > > --- a/sound/core/control_led.c
> > > +++ b/sound/core/control_led.c
> > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > >                       for (; group > 0; group--) {
> > >                               led = &snd_ctl_leds[group - 1];
> > >                               device_del(&led->dev);
> > > +                             device_put(&led->dev);
> > >                       }
> > >                       device_del(&snd_ctl_led_dev);
> > >                       return -ENOMEM;
> > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > >       for (group = 0; group < MAX_LED; group++) {
> > >               led = &snd_ctl_leds[group];
> > >               device_del(&led->dev);
> > > +             device_put(&led->dev);
> > >       }
> > >       device_del(&snd_ctl_led_dev);
> > >       snd_ctl_led_clean(NULL);
> 
> Hi Dan,
> 
> I tried this patch, and it still triggers the memleak.

Did your patch fix the leak?  Because my patch should have been
equivalent except for it fixes an additional leak in the snd_ctl_led_init()
error path.

> My
> understanding is that the device object is already freed in the
> snd_ctl_led_sysfs_remove.
> 

"Already freed"?  Is it a memleak or is it a double free???  I probably
should have read the syzbot email on this...  But you didn't include
a link to it or a reported-by tag so I don't have a way to look at the
actual bug.

I did fix a bug, though...  Just not the one from the report I guess.
Please send a link to the bug report so I can look at that.  ;)

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> >
> >
> >
> > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > >>
> > >> Can you please give some advise on how to fix this WARN issue?
> > >
> > > But it feels like it spoils the fun if I write the commit...  Anyway:
> >
> > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> >
> > >
> > > regards,
> > > dan carpenter
> > >
> > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > index 25f57c14f294..dd357abc1b58 100644
> > > --- a/sound/core/control_led.c
> > > +++ b/sound/core/control_led.c
> > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > >                       for (; group > 0; group--) {
> > >                               led = &snd_ctl_leds[group - 1];
> > >                               device_del(&led->dev);
> > > +                             device_put(&led->dev);
> > >                       }
> > >                       device_del(&snd_ctl_led_dev);
> > >                       return -ENOMEM;
> > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > >       for (group = 0; group < MAX_LED; group++) {
> > >               led = &snd_ctl_leds[group];
> > >               device_del(&led->dev);
> > > +             device_put(&led->dev);
> > >       }
> > >       device_del(&snd_ctl_led_dev);
> > >       snd_ctl_led_clean(NULL);
> 
> Hi Dan,
> 
> I tried this patch, and it still triggers the memleak.

Did your patch fix the leak?  Because my patch should have been
equivalent except for it fixes an additional leak in the snd_ctl_led_init()
error path.

> My
> understanding is that the device object is already freed in the
> snd_ctl_led_sysfs_remove.
> 

"Already freed"?  Is it a memleak or is it a double free???  I probably
should have read the syzbot email on this...  But you didn't include
a link to it or a reported-by tag so I don't have a way to look at the
actual bug.

I did fix a bug, though...  Just not the one from the report I guess.
Please send a link to the bug report so I can look at that.  ;)

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > >
> > >
> > >
> > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > >>
> > > >> Can you please give some advise on how to fix this WARN issue?
> > > >
> > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > >
> > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > >
> > > >
> > > > regards,
> > > > dan carpenter
> > > >
> > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > index 25f57c14f294..dd357abc1b58 100644
> > > > --- a/sound/core/control_led.c
> > > > +++ b/sound/core/control_led.c
> > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > >                       for (; group > 0; group--) {
> > > >                               led = &snd_ctl_leds[group - 1];
> > > >                               device_del(&led->dev);
> > > > +                             device_put(&led->dev);
> > > >                       }
> > > >                       device_del(&snd_ctl_led_dev);
> > > >                       return -ENOMEM;
> > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > >       for (group = 0; group < MAX_LED; group++) {
> > > >               led = &snd_ctl_leds[group];
> > > >               device_del(&led->dev);
> > > > +             device_put(&led->dev);
> > > >       }
> > > >       device_del(&snd_ctl_led_dev);
> > > >       snd_ctl_led_clean(NULL);
> >
> > Hi Dan,
> >
> > I tried this patch, and it still triggers the memleak.
>
> Did your patch fix the leak?  Because my patch should have been
> equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> error path.

The syzbot link is [1]. I have tested my patch in the syzbot dashboard
and my local workspace.

I think the reason why your patch did not work should be
led_card(struct snd_ctl_led_card) is already freed before returning in
snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
implementation of snd_ctl_led_sysfs_remove for some details. Please
correct me if I make any mistakes.

static void snd_ctl_led_sysfs_remove(struct snd_card *card)
{
        unsigned int group;
        struct snd_ctl_led_card *led_card;
        struct snd_ctl_led *led;
        char link_name[32];

        for (group = 0; group < MAX_LED; group++) {
                led = &snd_ctl_leds[group];
                led_card = led->cards[card->number];
                if (!led_card)
                        continue;
                snprintf(link_name, sizeof(link_name), "led-%s", led->name);
                sysfs_remove_link(&card->ctl_dev.kobj, link_name);
                sysfs_remove_link(&led_card->dev.kobj, "card");
                device_del(&led_card->dev);
                put_device(&led_card->dev);
                kfree(led_card);
                led->cards[card->number] = NULL;
        }
}

[1] https://syzkaller.appspot.com/bug?id=6d9e1e89003c894e7a1855c92dfa558ebcb8f218

>
> > My
> > understanding is that the device object is already freed in the
> > snd_ctl_led_sysfs_remove.
> >
>
> "Already freed"?  Is it a memleak or is it a double free???  I probably
> should have read the syzbot email on this...  But you didn't include
> a link to it or a reported-by tag so I don't have a way to look at the
> actual bug.

I listed the reported-by tag and fixes tag in the first email in this
thread. The syzbot link is [1].

Please take a look at my patch testing request.

>
> I did fix a bug, though...  Just not the one from the report I guess.
> Please send a link to the bug report so I can look at that.  ;)

We should talk about different bugs, memory leak for different objects
and different paths.

>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > >
> > >
> > >
> > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > >>
> > > >> Can you please give some advise on how to fix this WARN issue?
> > > >
> > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > >
> > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > >
> > > >
> > > > regards,
> > > > dan carpenter
> > > >
> > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > index 25f57c14f294..dd357abc1b58 100644
> > > > --- a/sound/core/control_led.c
> > > > +++ b/sound/core/control_led.c
> > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > >                       for (; group > 0; group--) {
> > > >                               led = &snd_ctl_leds[group - 1];
> > > >                               device_del(&led->dev);
> > > > +                             device_put(&led->dev);
> > > >                       }
> > > >                       device_del(&snd_ctl_led_dev);
> > > >                       return -ENOMEM;
> > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > >       for (group = 0; group < MAX_LED; group++) {
> > > >               led = &snd_ctl_leds[group];
> > > >               device_del(&led->dev);
> > > > +             device_put(&led->dev);
> > > >       }
> > > >       device_del(&snd_ctl_led_dev);
> > > >       snd_ctl_led_clean(NULL);
> >
> > Hi Dan,
> >
> > I tried this patch, and it still triggers the memleak.
>
> Did your patch fix the leak?  Because my patch should have been
> equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> error path.

The syzbot link is [1]. I have tested my patch in the syzbot dashboard
and my local workspace.

I think the reason why your patch did not work should be
led_card(struct snd_ctl_led_card) is already freed before returning in
snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
implementation of snd_ctl_led_sysfs_remove for some details. Please
correct me if I make any mistakes.

static void snd_ctl_led_sysfs_remove(struct snd_card *card)
{
        unsigned int group;
        struct snd_ctl_led_card *led_card;
        struct snd_ctl_led *led;
        char link_name[32];

        for (group = 0; group < MAX_LED; group++) {
                led = &snd_ctl_leds[group];
                led_card = led->cards[card->number];
                if (!led_card)
                        continue;
                snprintf(link_name, sizeof(link_name), "led-%s", led->name);
                sysfs_remove_link(&card->ctl_dev.kobj, link_name);
                sysfs_remove_link(&led_card->dev.kobj, "card");
                device_del(&led_card->dev);
                put_device(&led_card->dev);
                kfree(led_card);
                led->cards[card->number] = NULL;
        }
}

[1] https://syzkaller.appspot.com/bug?id=6d9e1e89003c894e7a1855c92dfa558ebcb8f218

>
> > My
> > understanding is that the device object is already freed in the
> > snd_ctl_led_sysfs_remove.
> >
>
> "Already freed"?  Is it a memleak or is it a double free???  I probably
> should have read the syzbot email on this...  But you didn't include
> a link to it or a reported-by tag so I don't have a way to look at the
> actual bug.

I listed the reported-by tag and fixes tag in the first email in this
thread. The syzbot link is [1].

Please take a look at my patch testing request.

>
> I did fix a bug, though...  Just not the one from the report I guess.
> Please send a link to the bug report so I can look at that.  ;)

We should talk about different bugs, memory leak for different objects
and different paths.

>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > >>
> > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > >
> > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > >
> > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > >
> > > > >
> > > > > regards,
> > > > > dan carpenter
> > > > >
> > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > --- a/sound/core/control_led.c
> > > > > +++ b/sound/core/control_led.c
> > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > >                       for (; group > 0; group--) {
> > > > >                               led = &snd_ctl_leds[group - 1];
> > > > >                               device_del(&led->dev);
> > > > > +                             device_put(&led->dev);
> > > > >                       }
> > > > >                       device_del(&snd_ctl_led_dev);
> > > > >                       return -ENOMEM;
> > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > >               led = &snd_ctl_leds[group];
> > > > >               device_del(&led->dev);
> > > > > +             device_put(&led->dev);
> > > > >       }
> > > > >       device_del(&snd_ctl_led_dev);
> > > > >       snd_ctl_led_clean(NULL);
> > >
> > > Hi Dan,
> > >
> > > I tried this patch, and it still triggers the memleak.
> >
> > Did your patch fix the leak?  Because my patch should have been
> > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > error path.
> 
> The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> and my local workspace.
> 
> I think the reason why your patch did not work should be
> led_card(struct snd_ctl_led_card) is already freed before returning in
> snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> implementation of snd_ctl_led_sysfs_remove for some details. Please
> correct me if I make any mistakes.
> 
> static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> {
>         unsigned int group;
>         struct snd_ctl_led_card *led_card;
>         struct snd_ctl_led *led;
>         char link_name[32];
> 
>         for (group = 0; group < MAX_LED; group++) {
>                 led = &snd_ctl_leds[group];
>                 led_card = led->cards[card->number];
>                 if (!led_card)
>                         continue;
>                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
>                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
>                 sysfs_remove_link(&led_card->dev.kobj, "card");
>                 device_del(&led_card->dev);
>                 put_device(&led_card->dev);
>                 kfree(led_card);
>                 led->cards[card->number] = NULL;
>         }
> }

This is frustrating to look at because it's not a diff so it doesn't
show what you changed.  I think you are saying that you added the
put_device(&led_card->dev);.  That's true.  There are some other leaks
as well.  We should just fix them all.  Use device_unregister() because
it's cleaner.

If both device_initialize() and device_add() succeed then call
device_unregister() to unwind.

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 25f57c14f294..561fe45e4449 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
 		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
 		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
 		sysfs_remove_link(&led_card->dev.kobj, "card");
-		device_del(&led_card->dev);
+		device_unregister(&led_card->dev);
 		kfree(led_card);
 		led->cards[card->number] = NULL;
 	}
@@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
 			put_device(&led->dev);
 			for (; group > 0; group--) {
 				led = &snd_ctl_leds[group - 1];
-				device_del(&led->dev);
+				device_unregister(&led->dev);
 			}
-			device_del(&snd_ctl_led_dev);
+			device_unregister(&snd_ctl_led_dev);
 			return -ENOMEM;
 		}
 	}
@@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
 	}
 	for (group = 0; group < MAX_LED; group++) {
 		led = &snd_ctl_leds[group];
-		device_del(&led->dev);
+		device_unregister(&led->dev);
 	}
-	device_del(&snd_ctl_led_dev);
+	device_unregister(&snd_ctl_led_dev);
 	snd_ctl_led_clean(NULL);
 }
 

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > >>
> > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > >
> > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > >
> > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > >
> > > > >
> > > > > regards,
> > > > > dan carpenter
> > > > >
> > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > --- a/sound/core/control_led.c
> > > > > +++ b/sound/core/control_led.c
> > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > >                       for (; group > 0; group--) {
> > > > >                               led = &snd_ctl_leds[group - 1];
> > > > >                               device_del(&led->dev);
> > > > > +                             device_put(&led->dev);
> > > > >                       }
> > > > >                       device_del(&snd_ctl_led_dev);
> > > > >                       return -ENOMEM;
> > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > >               led = &snd_ctl_leds[group];
> > > > >               device_del(&led->dev);
> > > > > +             device_put(&led->dev);
> > > > >       }
> > > > >       device_del(&snd_ctl_led_dev);
> > > > >       snd_ctl_led_clean(NULL);
> > >
> > > Hi Dan,
> > >
> > > I tried this patch, and it still triggers the memleak.
> >
> > Did your patch fix the leak?  Because my patch should have been
> > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > error path.
> 
> The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> and my local workspace.
> 
> I think the reason why your patch did not work should be
> led_card(struct snd_ctl_led_card) is already freed before returning in
> snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> implementation of snd_ctl_led_sysfs_remove for some details. Please
> correct me if I make any mistakes.
> 
> static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> {
>         unsigned int group;
>         struct snd_ctl_led_card *led_card;
>         struct snd_ctl_led *led;
>         char link_name[32];
> 
>         for (group = 0; group < MAX_LED; group++) {
>                 led = &snd_ctl_leds[group];
>                 led_card = led->cards[card->number];
>                 if (!led_card)
>                         continue;
>                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
>                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
>                 sysfs_remove_link(&led_card->dev.kobj, "card");
>                 device_del(&led_card->dev);
>                 put_device(&led_card->dev);
>                 kfree(led_card);
>                 led->cards[card->number] = NULL;
>         }
> }

This is frustrating to look at because it's not a diff so it doesn't
show what you changed.  I think you are saying that you added the
put_device(&led_card->dev);.  That's true.  There are some other leaks
as well.  We should just fix them all.  Use device_unregister() because
it's cleaner.

If both device_initialize() and device_add() succeed then call
device_unregister() to unwind.

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 25f57c14f294..561fe45e4449 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
 		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
 		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
 		sysfs_remove_link(&led_card->dev.kobj, "card");
-		device_del(&led_card->dev);
+		device_unregister(&led_card->dev);
 		kfree(led_card);
 		led->cards[card->number] = NULL;
 	}
@@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
 			put_device(&led->dev);
 			for (; group > 0; group--) {
 				led = &snd_ctl_leds[group - 1];
-				device_del(&led->dev);
+				device_unregister(&led->dev);
 			}
-			device_del(&snd_ctl_led_dev);
+			device_unregister(&snd_ctl_led_dev);
 			return -ENOMEM;
 		}
 	}
@@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
 	}
 	for (group = 0; group < MAX_LED; group++) {
 		led = &snd_ctl_leds[group];
-		device_del(&led->dev);
+		device_unregister(&led->dev);
 	}
-	device_del(&snd_ctl_led_dev);
+	device_unregister(&snd_ctl_led_dev);
 	snd_ctl_led_clean(NULL);
 }
 

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > >
> > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > >>
> > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > >
> > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > >
> > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > >
> > > > > >
> > > > > > regards,
> > > > > > dan carpenter
> > > > > >
> > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > --- a/sound/core/control_led.c
> > > > > > +++ b/sound/core/control_led.c
> > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > >                       for (; group > 0; group--) {
> > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > >                               device_del(&led->dev);
> > > > > > +                             device_put(&led->dev);
> > > > > >                       }
> > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > >                       return -ENOMEM;
> > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > >               led = &snd_ctl_leds[group];
> > > > > >               device_del(&led->dev);
> > > > > > +             device_put(&led->dev);
> > > > > >       }
> > > > > >       device_del(&snd_ctl_led_dev);
> > > > > >       snd_ctl_led_clean(NULL);
> > > >
> > > > Hi Dan,
> > > >
> > > > I tried this patch, and it still triggers the memleak.
> > >
> > > Did your patch fix the leak?  Because my patch should have been
> > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > error path.
> >
> > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > and my local workspace.
> >
> > I think the reason why your patch did not work should be
> > led_card(struct snd_ctl_led_card) is already freed before returning in
> > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > correct me if I make any mistakes.
> >
> > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > {
> >         unsigned int group;
> >         struct snd_ctl_led_card *led_card;
> >         struct snd_ctl_led *led;
> >         char link_name[32];
> >
> >         for (group = 0; group < MAX_LED; group++) {
> >                 led = &snd_ctl_leds[group];
> >                 led_card = led->cards[card->number];
> >                 if (!led_card)
> >                         continue;
> >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> >                 device_del(&led_card->dev);
> >                 put_device(&led_card->dev);
> >                 kfree(led_card);
> >                 led->cards[card->number] = NULL;
> >         }
> > }
>
> This is frustrating to look at because it's not a diff so it doesn't
> show what you changed.  I think you are saying that you added the
> put_device(&led_card->dev);.  That's true.  There are some other leaks
> as well.  We should just fix them all.  Use device_unregister() because
> it's cleaner.

Oh, I see your point. Yeah, we should fix these memory leaks all. I
agree with device_unregister.

>
> If both device_initialize() and device_add() succeed then call
> device_unregister() to unwind.

BTW, have you tested this new patch on two memory leaks?

>
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..561fe45e4449 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
>                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
>                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
>                 sysfs_remove_link(&led_card->dev.kobj, "card");
> -               device_del(&led_card->dev);
> +               device_unregister(&led_card->dev);
>                 kfree(led_card);
>                 led->cards[card->number] = NULL;
>         }
> @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
>                         put_device(&led->dev);
>                         for (; group > 0; group--) {
>                                 led = &snd_ctl_leds[group - 1];
> -                               device_del(&led->dev);
> +                               device_unregister(&led->dev);
>                         }
> -                       device_del(&snd_ctl_led_dev);
> +                       device_unregister(&snd_ctl_led_dev);
>                         return -ENOMEM;
>                 }
>         }
> @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
>         }
>         for (group = 0; group < MAX_LED; group++) {
>                 led = &snd_ctl_leds[group];
> -               device_del(&led->dev);
> +               device_unregister(&led->dev);
>         }
> -       device_del(&snd_ctl_led_dev);
> +       device_unregister(&snd_ctl_led_dev);
>         snd_ctl_led_clean(NULL);
>  }
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > >
> > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > >>
> > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > >
> > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > >
> > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > >
> > > > > >
> > > > > > regards,
> > > > > > dan carpenter
> > > > > >
> > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > --- a/sound/core/control_led.c
> > > > > > +++ b/sound/core/control_led.c
> > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > >                       for (; group > 0; group--) {
> > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > >                               device_del(&led->dev);
> > > > > > +                             device_put(&led->dev);
> > > > > >                       }
> > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > >                       return -ENOMEM;
> > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > >               led = &snd_ctl_leds[group];
> > > > > >               device_del(&led->dev);
> > > > > > +             device_put(&led->dev);
> > > > > >       }
> > > > > >       device_del(&snd_ctl_led_dev);
> > > > > >       snd_ctl_led_clean(NULL);
> > > >
> > > > Hi Dan,
> > > >
> > > > I tried this patch, and it still triggers the memleak.
> > >
> > > Did your patch fix the leak?  Because my patch should have been
> > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > error path.
> >
> > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > and my local workspace.
> >
> > I think the reason why your patch did not work should be
> > led_card(struct snd_ctl_led_card) is already freed before returning in
> > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > correct me if I make any mistakes.
> >
> > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > {
> >         unsigned int group;
> >         struct snd_ctl_led_card *led_card;
> >         struct snd_ctl_led *led;
> >         char link_name[32];
> >
> >         for (group = 0; group < MAX_LED; group++) {
> >                 led = &snd_ctl_leds[group];
> >                 led_card = led->cards[card->number];
> >                 if (!led_card)
> >                         continue;
> >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> >                 device_del(&led_card->dev);
> >                 put_device(&led_card->dev);
> >                 kfree(led_card);
> >                 led->cards[card->number] = NULL;
> >         }
> > }
>
> This is frustrating to look at because it's not a diff so it doesn't
> show what you changed.  I think you are saying that you added the
> put_device(&led_card->dev);.  That's true.  There are some other leaks
> as well.  We should just fix them all.  Use device_unregister() because
> it's cleaner.

Oh, I see your point. Yeah, we should fix these memory leaks all. I
agree with device_unregister.

>
> If both device_initialize() and device_add() succeed then call
> device_unregister() to unwind.

BTW, have you tested this new patch on two memory leaks?

>
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..561fe45e4449 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
>                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
>                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
>                 sysfs_remove_link(&led_card->dev.kobj, "card");
> -               device_del(&led_card->dev);
> +               device_unregister(&led_card->dev);
>                 kfree(led_card);
>                 led->cards[card->number] = NULL;
>         }
> @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
>                         put_device(&led->dev);
>                         for (; group > 0; group--) {
>                                 led = &snd_ctl_leds[group - 1];
> -                               device_del(&led->dev);
> +                               device_unregister(&led->dev);
>                         }
> -                       device_del(&snd_ctl_led_dev);
> +                       device_unregister(&snd_ctl_led_dev);
>                         return -ENOMEM;
>                 }
>         }
> @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
>         }
>         for (group = 0; group < MAX_LED; group++) {
>                 led = &snd_ctl_leds[group];
> -               device_del(&led->dev);
> +               device_unregister(&led->dev);
>         }
> -       device_del(&snd_ctl_led_dev);
> +       device_unregister(&snd_ctl_led_dev);
>         snd_ctl_led_clean(NULL);
>  }
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > >
> > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > >>
> > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > >
> > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > >
> > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > >
> > > > > > >
> > > > > > > regards,
> > > > > > > dan carpenter
> > > > > > >
> > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > --- a/sound/core/control_led.c
> > > > > > > +++ b/sound/core/control_led.c
> > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > >                       for (; group > 0; group--) {
> > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > >                               device_del(&led->dev);
> > > > > > > +                             device_put(&led->dev);
> > > > > > >                       }
> > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > >                       return -ENOMEM;
> > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > >               led = &snd_ctl_leds[group];
> > > > > > >               device_del(&led->dev);
> > > > > > > +             device_put(&led->dev);
> > > > > > >       }
> > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > >       snd_ctl_led_clean(NULL);
> > > > >
> > > > > Hi Dan,
> > > > >
> > > > > I tried this patch, and it still triggers the memleak.
> > > >
> > > > Did your patch fix the leak?  Because my patch should have been
> > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > error path.
> > >
> > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > and my local workspace.
> > >
> > > I think the reason why your patch did not work should be
> > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > correct me if I make any mistakes.
> > >
> > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > {
> > >         unsigned int group;
> > >         struct snd_ctl_led_card *led_card;
> > >         struct snd_ctl_led *led;
> > >         char link_name[32];
> > >
> > >         for (group = 0; group < MAX_LED; group++) {
> > >                 led = &snd_ctl_leds[group];
> > >                 led_card = led->cards[card->number];
> > >                 if (!led_card)
> > >                         continue;
> > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > >                 device_del(&led_card->dev);
> > >                 put_device(&led_card->dev);
> > >                 kfree(led_card);
> > >                 led->cards[card->number] = NULL;
> > >         }
> > > }
> >
> > This is frustrating to look at because it's not a diff so it doesn't
> > show what you changed.  I think you are saying that you added the
> > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > as well.  We should just fix them all.  Use device_unregister() because
> > it's cleaner.
>
> Oh, I see your point. Yeah, we should fix these memory leaks all. I
> agree with device_unregister.
>
> >
> > If both device_initialize() and device_add() succeed then call
> > device_unregister() to unwind.
>
> BTW, have you tested this new patch on two memory leaks?
>

Please keep in mind that if we don't have any release method for
struct snd_ctl_led_card, it will trigger a WARN[1] in the
device_release function. That's why I have to add one dummy release
function.

if (dev->release)
        dev->release(dev);
else if (dev->type && dev->type->release)
        dev->type->release(dev);
else if (dev->class && dev->class->dev_release)
        dev->class->dev_release(dev);
else
        WARN(1, KERN_ERR "Device '%s' does not have a release()
function, it is broken and must be fixed. See
Documentation/core-api/kobject.rst.\n",
dev_name(dev));

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110

> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..561fe45e4449 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > -               device_del(&led_card->dev);
> > +               device_unregister(&led_card->dev);
> >                 kfree(led_card);
> >                 led->cards[card->number] = NULL;
> >         }
> > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> >                         put_device(&led->dev);
> >                         for (; group > 0; group--) {
> >                                 led = &snd_ctl_leds[group - 1];
> > -                               device_del(&led->dev);
> > +                               device_unregister(&led->dev);
> >                         }
> > -                       device_del(&snd_ctl_led_dev);
> > +                       device_unregister(&snd_ctl_led_dev);
> >                         return -ENOMEM;
> >                 }
> >         }
> > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> >         }
> >         for (group = 0; group < MAX_LED; group++) {
> >                 led = &snd_ctl_leds[group];
> > -               device_del(&led->dev);
> > +               device_unregister(&led->dev);
> >         }
> > -       device_del(&snd_ctl_led_dev);
> > +       device_unregister(&snd_ctl_led_dev);
> >         snd_ctl_led_clean(NULL);
> >  }
> >

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > >
> > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > >>
> > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > >
> > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > >
> > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > >
> > > > > > >
> > > > > > > regards,
> > > > > > > dan carpenter
> > > > > > >
> > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > --- a/sound/core/control_led.c
> > > > > > > +++ b/sound/core/control_led.c
> > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > >                       for (; group > 0; group--) {
> > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > >                               device_del(&led->dev);
> > > > > > > +                             device_put(&led->dev);
> > > > > > >                       }
> > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > >                       return -ENOMEM;
> > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > >               led = &snd_ctl_leds[group];
> > > > > > >               device_del(&led->dev);
> > > > > > > +             device_put(&led->dev);
> > > > > > >       }
> > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > >       snd_ctl_led_clean(NULL);
> > > > >
> > > > > Hi Dan,
> > > > >
> > > > > I tried this patch, and it still triggers the memleak.
> > > >
> > > > Did your patch fix the leak?  Because my patch should have been
> > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > error path.
> > >
> > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > and my local workspace.
> > >
> > > I think the reason why your patch did not work should be
> > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > correct me if I make any mistakes.
> > >
> > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > {
> > >         unsigned int group;
> > >         struct snd_ctl_led_card *led_card;
> > >         struct snd_ctl_led *led;
> > >         char link_name[32];
> > >
> > >         for (group = 0; group < MAX_LED; group++) {
> > >                 led = &snd_ctl_leds[group];
> > >                 led_card = led->cards[card->number];
> > >                 if (!led_card)
> > >                         continue;
> > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > >                 device_del(&led_card->dev);
> > >                 put_device(&led_card->dev);
> > >                 kfree(led_card);
> > >                 led->cards[card->number] = NULL;
> > >         }
> > > }
> >
> > This is frustrating to look at because it's not a diff so it doesn't
> > show what you changed.  I think you are saying that you added the
> > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > as well.  We should just fix them all.  Use device_unregister() because
> > it's cleaner.
>
> Oh, I see your point. Yeah, we should fix these memory leaks all. I
> agree with device_unregister.
>
> >
> > If both device_initialize() and device_add() succeed then call
> > device_unregister() to unwind.
>
> BTW, have you tested this new patch on two memory leaks?
>

Please keep in mind that if we don't have any release method for
struct snd_ctl_led_card, it will trigger a WARN[1] in the
device_release function. That's why I have to add one dummy release
function.

if (dev->release)
        dev->release(dev);
else if (dev->type && dev->type->release)
        dev->type->release(dev);
else if (dev->class && dev->class->dev_release)
        dev->class->dev_release(dev);
else
        WARN(1, KERN_ERR "Device '%s' does not have a release()
function, it is broken and must be fixed. See
Documentation/core-api/kobject.rst.\n",
dev_name(dev));

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110

> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..561fe45e4449 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > -               device_del(&led_card->dev);
> > +               device_unregister(&led_card->dev);
> >                 kfree(led_card);
> >                 led->cards[card->number] = NULL;
> >         }
> > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> >                         put_device(&led->dev);
> >                         for (; group > 0; group--) {
> >                                 led = &snd_ctl_leds[group - 1];
> > -                               device_del(&led->dev);
> > +                               device_unregister(&led->dev);
> >                         }
> > -                       device_del(&snd_ctl_led_dev);
> > +                       device_unregister(&snd_ctl_led_dev);
> >                         return -ENOMEM;
> >                 }
> >         }
> > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> >         }
> >         for (group = 0; group < MAX_LED; group++) {
> >                 led = &snd_ctl_leds[group];
> > -               device_del(&led->dev);
> > +               device_unregister(&led->dev);
> >         }
> > -       device_del(&snd_ctl_led_dev);
> > +       device_unregister(&snd_ctl_led_dev);
> >         snd_ctl_led_clean(NULL);
> >  }
> >

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 03:34:09PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > >
> > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > >>
> > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > >
> > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > >
> > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > >
> > > > > > >
> > > > > > > regards,
> > > > > > > dan carpenter
> > > > > > >
> > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > --- a/sound/core/control_led.c
> > > > > > > +++ b/sound/core/control_led.c
> > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > >                       for (; group > 0; group--) {
> > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > >                               device_del(&led->dev);
> > > > > > > +                             device_put(&led->dev);
> > > > > > >                       }
> > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > >                       return -ENOMEM;
> > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > >               led = &snd_ctl_leds[group];
> > > > > > >               device_del(&led->dev);
> > > > > > > +             device_put(&led->dev);
> > > > > > >       }
> > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > >       snd_ctl_led_clean(NULL);
> > > > >
> > > > > Hi Dan,
> > > > >
> > > > > I tried this patch, and it still triggers the memleak.
> > > >
> > > > Did your patch fix the leak?  Because my patch should have been
> > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > error path.
> > >
> > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > and my local workspace.
> > >
> > > I think the reason why your patch did not work should be
> > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > correct me if I make any mistakes.
> > >
> > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > {
> > >         unsigned int group;
> > >         struct snd_ctl_led_card *led_card;
> > >         struct snd_ctl_led *led;
> > >         char link_name[32];
> > >
> > >         for (group = 0; group < MAX_LED; group++) {
> > >                 led = &snd_ctl_leds[group];
> > >                 led_card = led->cards[card->number];
> > >                 if (!led_card)
> > >                         continue;
> > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > >                 device_del(&led_card->dev);
> > >                 put_device(&led_card->dev);
> > >                 kfree(led_card);
> > >                 led->cards[card->number] = NULL;
> > >         }
> > > }
> >
> > This is frustrating to look at because it's not a diff so it doesn't
> > show what you changed.  I think you are saying that you added the
> > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > as well.  We should just fix them all.  Use device_unregister() because
> > it's cleaner.
> 
> Oh, I see your point. Yeah, we should fix these memory leaks all. I
> agree with device_unregister.
> 
> >
> > If both device_initialize() and device_add() succeed then call
> > device_unregister() to unwind.
> 
> BTW, have you tested this new patch on two memory leaks?

No I have not tested it at all, and I don't remember if I even compiled
it.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 03:34:09PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > >
> > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > >>
> > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > >
> > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > >
> > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > >
> > > > > > >
> > > > > > > regards,
> > > > > > > dan carpenter
> > > > > > >
> > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > --- a/sound/core/control_led.c
> > > > > > > +++ b/sound/core/control_led.c
> > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > >                       for (; group > 0; group--) {
> > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > >                               device_del(&led->dev);
> > > > > > > +                             device_put(&led->dev);
> > > > > > >                       }
> > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > >                       return -ENOMEM;
> > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > >               led = &snd_ctl_leds[group];
> > > > > > >               device_del(&led->dev);
> > > > > > > +             device_put(&led->dev);
> > > > > > >       }
> > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > >       snd_ctl_led_clean(NULL);
> > > > >
> > > > > Hi Dan,
> > > > >
> > > > > I tried this patch, and it still triggers the memleak.
> > > >
> > > > Did your patch fix the leak?  Because my patch should have been
> > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > error path.
> > >
> > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > and my local workspace.
> > >
> > > I think the reason why your patch did not work should be
> > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > correct me if I make any mistakes.
> > >
> > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > {
> > >         unsigned int group;
> > >         struct snd_ctl_led_card *led_card;
> > >         struct snd_ctl_led *led;
> > >         char link_name[32];
> > >
> > >         for (group = 0; group < MAX_LED; group++) {
> > >                 led = &snd_ctl_leds[group];
> > >                 led_card = led->cards[card->number];
> > >                 if (!led_card)
> > >                         continue;
> > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > >                 device_del(&led_card->dev);
> > >                 put_device(&led_card->dev);
> > >                 kfree(led_card);
> > >                 led->cards[card->number] = NULL;
> > >         }
> > > }
> >
> > This is frustrating to look at because it's not a diff so it doesn't
> > show what you changed.  I think you are saying that you added the
> > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > as well.  We should just fix them all.  Use device_unregister() because
> > it's cleaner.
> 
> Oh, I see your point. Yeah, we should fix these memory leaks all. I
> agree with device_unregister.
> 
> >
> > If both device_initialize() and device_add() succeed then call
> > device_unregister() to unwind.
> 
> BTW, have you tested this new patch on two memory leaks?

No I have not tested it at all, and I don't remember if I even compiled
it.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 04:08:04PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >
> > On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > > >>
> > > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > > >
> > > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > > >
> > > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > > >
> > > > > > > >
> > > > > > > > regards,
> > > > > > > > dan carpenter
> > > > > > > >
> > > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > > --- a/sound/core/control_led.c
> > > > > > > > +++ b/sound/core/control_led.c
> > > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > > >                       for (; group > 0; group--) {
> > > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > > >                               device_del(&led->dev);
> > > > > > > > +                             device_put(&led->dev);
> > > > > > > >                       }
> > > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > > >                       return -ENOMEM;
> > > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > > >               led = &snd_ctl_leds[group];
> > > > > > > >               device_del(&led->dev);
> > > > > > > > +             device_put(&led->dev);
> > > > > > > >       }
> > > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > > >       snd_ctl_led_clean(NULL);
> > > > > >
> > > > > > Hi Dan,
> > > > > >
> > > > > > I tried this patch, and it still triggers the memleak.
> > > > >
> > > > > Did your patch fix the leak?  Because my patch should have been
> > > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > > error path.
> > > >
> > > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > > and my local workspace.
> > > >
> > > > I think the reason why your patch did not work should be
> > > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > > correct me if I make any mistakes.
> > > >
> > > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > {
> > > >         unsigned int group;
> > > >         struct snd_ctl_led_card *led_card;
> > > >         struct snd_ctl_led *led;
> > > >         char link_name[32];
> > > >
> > > >         for (group = 0; group < MAX_LED; group++) {
> > > >                 led = &snd_ctl_leds[group];
> > > >                 led_card = led->cards[card->number];
> > > >                 if (!led_card)
> > > >                         continue;
> > > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > >                 device_del(&led_card->dev);
> > > >                 put_device(&led_card->dev);
> > > >                 kfree(led_card);
> > > >                 led->cards[card->number] = NULL;
> > > >         }
> > > > }
> > >
> > > This is frustrating to look at because it's not a diff so it doesn't
> > > show what you changed.  I think you are saying that you added the
> > > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > > as well.  We should just fix them all.  Use device_unregister() because
> > > it's cleaner.
> >
> > Oh, I see your point. Yeah, we should fix these memory leaks all. I
> > agree with device_unregister.
> >
> > >
> > > If both device_initialize() and device_add() succeed then call
> > > device_unregister() to unwind.
> >
> > BTW, have you tested this new patch on two memory leaks?
> >
> 
> Please keep in mind that if we don't have any release method for
> struct snd_ctl_led_card, it will trigger a WARN[1] in the
> device_release function. That's why I have to add one dummy release
> function.
> 
> if (dev->release)
>         dev->release(dev);
> else if (dev->type && dev->type->release)
>         dev->type->release(dev);
> else if (dev->class && dev->class->dev_release)
>         dev->class->dev_release(dev);
> else
>         WARN(1, KERN_ERR "Device '%s' does not have a release()
> function, it is broken and must be fixed. See
> Documentation/core-api/kobject.rst.\n",
> dev_name(dev));
> 

Oh yeah.  You're right.  The "kfree(led_card);" needs to be moved to a
release function or it can lead to a use after free.  For the others,
I think a dummy release function is ok (because it is static data).

It feels like there should be a standard way to say that there is no
need to release any data.  That way it could be verified by static
analysis tools.

regards,
dan carpenter

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110 
> 
> > >
> > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > index 25f57c14f294..561fe45e4449 100644
> > > --- a/sound/core/control_led.c
> > > +++ b/sound/core/control_led.c
> > > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > -               device_del(&led_card->dev);
> > > +               device_unregister(&led_card->dev);
> > >                 kfree(led_card);
> > >                 led->cards[card->number] = NULL;
> > >         }
> > > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> > >                         put_device(&led->dev);
> > >                         for (; group > 0; group--) {
> > >                                 led = &snd_ctl_leds[group - 1];
> > > -                               device_del(&led->dev);
> > > +                               device_unregister(&led->dev);
> > >                         }
> > > -                       device_del(&snd_ctl_led_dev);
> > > +                       device_unregister(&snd_ctl_led_dev);
> > >                         return -ENOMEM;
> > >                 }
> > >         }
> > > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> > >         }
> > >         for (group = 0; group < MAX_LED; group++) {
> > >                 led = &snd_ctl_leds[group];
> > > -               device_del(&led->dev);
> > > +               device_unregister(&led->dev);
> > >         }
> > > -       device_del(&snd_ctl_led_dev);
> > > +       device_unregister(&snd_ctl_led_dev);
> > >         snd_ctl_led_clean(NULL);
> > >  }
> > >

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 04:08:04PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >
> > On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > > >>
> > > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > > >
> > > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > > >
> > > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > > >
> > > > > > > >
> > > > > > > > regards,
> > > > > > > > dan carpenter
> > > > > > > >
> > > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > > --- a/sound/core/control_led.c
> > > > > > > > +++ b/sound/core/control_led.c
> > > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > > >                       for (; group > 0; group--) {
> > > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > > >                               device_del(&led->dev);
> > > > > > > > +                             device_put(&led->dev);
> > > > > > > >                       }
> > > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > > >                       return -ENOMEM;
> > > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > > >               led = &snd_ctl_leds[group];
> > > > > > > >               device_del(&led->dev);
> > > > > > > > +             device_put(&led->dev);
> > > > > > > >       }
> > > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > > >       snd_ctl_led_clean(NULL);
> > > > > >
> > > > > > Hi Dan,
> > > > > >
> > > > > > I tried this patch, and it still triggers the memleak.
> > > > >
> > > > > Did your patch fix the leak?  Because my patch should have been
> > > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > > error path.
> > > >
> > > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > > and my local workspace.
> > > >
> > > > I think the reason why your patch did not work should be
> > > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > > correct me if I make any mistakes.
> > > >
> > > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > {
> > > >         unsigned int group;
> > > >         struct snd_ctl_led_card *led_card;
> > > >         struct snd_ctl_led *led;
> > > >         char link_name[32];
> > > >
> > > >         for (group = 0; group < MAX_LED; group++) {
> > > >                 led = &snd_ctl_leds[group];
> > > >                 led_card = led->cards[card->number];
> > > >                 if (!led_card)
> > > >                         continue;
> > > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > >                 device_del(&led_card->dev);
> > > >                 put_device(&led_card->dev);
> > > >                 kfree(led_card);
> > > >                 led->cards[card->number] = NULL;
> > > >         }
> > > > }
> > >
> > > This is frustrating to look at because it's not a diff so it doesn't
> > > show what you changed.  I think you are saying that you added the
> > > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > > as well.  We should just fix them all.  Use device_unregister() because
> > > it's cleaner.
> >
> > Oh, I see your point. Yeah, we should fix these memory leaks all. I
> > agree with device_unregister.
> >
> > >
> > > If both device_initialize() and device_add() succeed then call
> > > device_unregister() to unwind.
> >
> > BTW, have you tested this new patch on two memory leaks?
> >
> 
> Please keep in mind that if we don't have any release method for
> struct snd_ctl_led_card, it will trigger a WARN[1] in the
> device_release function. That's why I have to add one dummy release
> function.
> 
> if (dev->release)
>         dev->release(dev);
> else if (dev->type && dev->type->release)
>         dev->type->release(dev);
> else if (dev->class && dev->class->dev_release)
>         dev->class->dev_release(dev);
> else
>         WARN(1, KERN_ERR "Device '%s' does not have a release()
> function, it is broken and must be fixed. See
> Documentation/core-api/kobject.rst.\n",
> dev_name(dev));
> 

Oh yeah.  You're right.  The "kfree(led_card);" needs to be moved to a
release function or it can lead to a use after free.  For the others,
I think a dummy release function is ok (because it is static data).

It feels like there should be a standard way to say that there is no
need to release any data.  That way it could be verified by static
analysis tools.

regards,
dan carpenter

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110 
> 
> > >
> > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > index 25f57c14f294..561fe45e4449 100644
> > > --- a/sound/core/control_led.c
> > > +++ b/sound/core/control_led.c
> > > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > -               device_del(&led_card->dev);
> > > +               device_unregister(&led_card->dev);
> > >                 kfree(led_card);
> > >                 led->cards[card->number] = NULL;
> > >         }
> > > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> > >                         put_device(&led->dev);
> > >                         for (; group > 0; group--) {
> > >                                 led = &snd_ctl_leds[group - 1];
> > > -                               device_del(&led->dev);
> > > +                               device_unregister(&led->dev);
> > >                         }
> > > -                       device_del(&snd_ctl_led_dev);
> > > +                       device_unregister(&snd_ctl_led_dev);
> > >                         return -ENOMEM;
> > >                 }
> > >         }
> > > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> > >         }
> > >         for (group = 0; group < MAX_LED; group++) {
> > >                 led = &snd_ctl_leds[group];
> > > -               device_del(&led->dev);
> > > +               device_unregister(&led->dev);
> > >         }
> > > -       device_del(&snd_ctl_led_dev);
> > > +       device_unregister(&snd_ctl_led_dev);
> > >         snd_ctl_led_clean(NULL);
> > >  }
> > >

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 4:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 04:08:04PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > >
> > > > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > > > >>
> > > > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > > > >
> > > > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > > > >
> > > > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > regards,
> > > > > > > > > dan carpenter
> > > > > > > > >
> > > > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > > > --- a/sound/core/control_led.c
> > > > > > > > > +++ b/sound/core/control_led.c
> > > > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > > > >                       for (; group > 0; group--) {
> > > > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > > > >                               device_del(&led->dev);
> > > > > > > > > +                             device_put(&led->dev);
> > > > > > > > >                       }
> > > > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > > > >                       return -ENOMEM;
> > > > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > > > >               led = &snd_ctl_leds[group];
> > > > > > > > >               device_del(&led->dev);
> > > > > > > > > +             device_put(&led->dev);
> > > > > > > > >       }
> > > > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > > > >       snd_ctl_led_clean(NULL);
> > > > > > >
> > > > > > > Hi Dan,
> > > > > > >
> > > > > > > I tried this patch, and it still triggers the memleak.
> > > > > >
> > > > > > Did your patch fix the leak?  Because my patch should have been
> > > > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > > > error path.
> > > > >
> > > > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > > > and my local workspace.
> > > > >
> > > > > I think the reason why your patch did not work should be
> > > > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > > > correct me if I make any mistakes.
> > > > >
> > > > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > > {
> > > > >         unsigned int group;
> > > > >         struct snd_ctl_led_card *led_card;
> > > > >         struct snd_ctl_led *led;
> > > > >         char link_name[32];
> > > > >
> > > > >         for (group = 0; group < MAX_LED; group++) {
> > > > >                 led = &snd_ctl_leds[group];
> > > > >                 led_card = led->cards[card->number];
> > > > >                 if (!led_card)
> > > > >                         continue;
> > > > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > >                 device_del(&led_card->dev);
> > > > >                 put_device(&led_card->dev);
> > > > >                 kfree(led_card);
> > > > >                 led->cards[card->number] = NULL;
> > > > >         }
> > > > > }
> > > >
> > > > This is frustrating to look at because it's not a diff so it doesn't
> > > > show what you changed.  I think you are saying that you added the
> > > > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > > > as well.  We should just fix them all.  Use device_unregister() because
> > > > it's cleaner.
> > >
> > > Oh, I see your point. Yeah, we should fix these memory leaks all. I
> > > agree with device_unregister.
> > >
> > > >
> > > > If both device_initialize() and device_add() succeed then call
> > > > device_unregister() to unwind.
> > >
> > > BTW, have you tested this new patch on two memory leaks?
> > >
> >
> > Please keep in mind that if we don't have any release method for
> > struct snd_ctl_led_card, it will trigger a WARN[1] in the
> > device_release function. That's why I have to add one dummy release
> > function.
> >
> > if (dev->release)
> >         dev->release(dev);
> > else if (dev->type && dev->type->release)
> >         dev->type->release(dev);
> > else if (dev->class && dev->class->dev_release)
> >         dev->class->dev_release(dev);
> > else
> >         WARN(1, KERN_ERR "Device '%s' does not have a release()
> > function, it is broken and must be fixed. See
> > Documentation/core-api/kobject.rst.\n",
> > dev_name(dev));
> >
>
> Oh yeah.  You're right.  The "kfree(led_card);" needs to be moved to a
> release function or it can lead to a use after free.  For the others,
> I think a dummy release function is ok (because it is static data).
>

Hi Dan,

I wonder if we shall split the current patch into two patches, one
patch for each memory leak. It is better to satisfy the rule - "one
patch only fixes one issue".

We should absolutely fix all these memory leaks. But one patch for two
different bugs with different objects and different paths is not very
suitable.

> It feels like there should be a standard way to say that there is no
> need to release any data.  That way it could be verified by static
> analysis tools.
>
> regards,
> dan carpenter
>
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110
> >
> > > >
> > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > index 25f57c14f294..561fe45e4449 100644
> > > > --- a/sound/core/control_led.c
> > > > +++ b/sound/core/control_led.c
> > > > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > -               device_del(&led_card->dev);
> > > > +               device_unregister(&led_card->dev);
> > > >                 kfree(led_card);
> > > >                 led->cards[card->number] = NULL;
> > > >         }
> > > > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> > > >                         put_device(&led->dev);
> > > >                         for (; group > 0; group--) {
> > > >                                 led = &snd_ctl_leds[group - 1];
> > > > -                               device_del(&led->dev);
> > > > +                               device_unregister(&led->dev);
> > > >                         }
> > > > -                       device_del(&snd_ctl_led_dev);
> > > > +                       device_unregister(&snd_ctl_led_dev);
> > > >                         return -ENOMEM;
> > > >                 }
> > > >         }
> > > > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> > > >         }
> > > >         for (group = 0; group < MAX_LED; group++) {
> > > >                 led = &snd_ctl_leds[group];
> > > > -               device_del(&led->dev);
> > > > +               device_unregister(&led->dev);
> > > >         }
> > > > -       device_del(&snd_ctl_led_dev);
> > > > +       device_unregister(&snd_ctl_led_dev);
> > > >         snd_ctl_led_clean(NULL);
> > > >  }
> > > >

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 4:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 04:08:04PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 3:34 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 3:03 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 02:20:37PM +0800, Dongliang Mu wrote:
> > > > > On Mon, May 31, 2021 at 12:40 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > >
> > > > > > On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> > > > > > > On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@gmail.com> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > > > > > > > >>
> > > > > > > > >> Can you please give some advise on how to fix this WARN issue?
> > > > > > > > >
> > > > > > > > > But it feels like it spoils the fun if I write the commit...  Anyway:
> > > > > > > >
> > > > > > > > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > regards,
> > > > > > > > > dan carpenter
> > > > > > > > >
> > > > > > > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > > > > > > index 25f57c14f294..dd357abc1b58 100644
> > > > > > > > > --- a/sound/core/control_led.c
> > > > > > > > > +++ b/sound/core/control_led.c
> > > > > > > > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > > > > > > > >                       for (; group > 0; group--) {
> > > > > > > > >                               led = &snd_ctl_leds[group - 1];
> > > > > > > > >                               device_del(&led->dev);
> > > > > > > > > +                             device_put(&led->dev);
> > > > > > > > >                       }
> > > > > > > > >                       device_del(&snd_ctl_led_dev);
> > > > > > > > >                       return -ENOMEM;
> > > > > > > > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > > > > > > > >       for (group = 0; group < MAX_LED; group++) {
> > > > > > > > >               led = &snd_ctl_leds[group];
> > > > > > > > >               device_del(&led->dev);
> > > > > > > > > +             device_put(&led->dev);
> > > > > > > > >       }
> > > > > > > > >       device_del(&snd_ctl_led_dev);
> > > > > > > > >       snd_ctl_led_clean(NULL);
> > > > > > >
> > > > > > > Hi Dan,
> > > > > > >
> > > > > > > I tried this patch, and it still triggers the memleak.
> > > > > >
> > > > > > Did your patch fix the leak?  Because my patch should have been
> > > > > > equivalent except for it fixes an additional leak in the snd_ctl_led_init()
> > > > > > error path.
> > > > >
> > > > > The syzbot link is [1]. I have tested my patch in the syzbot dashboard
> > > > > and my local workspace.
> > > > >
> > > > > I think the reason why your patch did not work should be
> > > > > led_card(struct snd_ctl_led_card) is already freed before returning in
> > > > > snd_ctl_led_sysfs_remove, rather than led(struct snd_ctl_led). See the
> > > > > implementation of snd_ctl_led_sysfs_remove for some details. Please
> > > > > correct me if I make any mistakes.
> > > > >
> > > > > static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > > {
> > > > >         unsigned int group;
> > > > >         struct snd_ctl_led_card *led_card;
> > > > >         struct snd_ctl_led *led;
> > > > >         char link_name[32];
> > > > >
> > > > >         for (group = 0; group < MAX_LED; group++) {
> > > > >                 led = &snd_ctl_leds[group];
> > > > >                 led_card = led->cards[card->number];
> > > > >                 if (!led_card)
> > > > >                         continue;
> > > > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > >                 device_del(&led_card->dev);
> > > > >                 put_device(&led_card->dev);
> > > > >                 kfree(led_card);
> > > > >                 led->cards[card->number] = NULL;
> > > > >         }
> > > > > }
> > > >
> > > > This is frustrating to look at because it's not a diff so it doesn't
> > > > show what you changed.  I think you are saying that you added the
> > > > put_device(&led_card->dev);.  That's true.  There are some other leaks
> > > > as well.  We should just fix them all.  Use device_unregister() because
> > > > it's cleaner.
> > >
> > > Oh, I see your point. Yeah, we should fix these memory leaks all. I
> > > agree with device_unregister.
> > >
> > > >
> > > > If both device_initialize() and device_add() succeed then call
> > > > device_unregister() to unwind.
> > >
> > > BTW, have you tested this new patch on two memory leaks?
> > >
> >
> > Please keep in mind that if we don't have any release method for
> > struct snd_ctl_led_card, it will trigger a WARN[1] in the
> > device_release function. That's why I have to add one dummy release
> > function.
> >
> > if (dev->release)
> >         dev->release(dev);
> > else if (dev->type && dev->type->release)
> >         dev->type->release(dev);
> > else if (dev->class && dev->class->dev_release)
> >         dev->class->dev_release(dev);
> > else
> >         WARN(1, KERN_ERR "Device '%s' does not have a release()
> > function, it is broken and must be fixed. See
> > Documentation/core-api/kobject.rst.\n",
> > dev_name(dev));
> >
>
> Oh yeah.  You're right.  The "kfree(led_card);" needs to be moved to a
> release function or it can lead to a use after free.  For the others,
> I think a dummy release function is ok (because it is static data).
>

Hi Dan,

I wonder if we shall split the current patch into two patches, one
patch for each memory leak. It is better to satisfy the rule - "one
patch only fixes one issue".

We should absolutely fix all these memory leaks. But one patch for two
different bugs with different objects and different paths is not very
suitable.

> It feels like there should be a standard way to say that there is no
> need to release any data.  That way it could be verified by static
> analysis tools.
>
> regards,
> dan carpenter
>
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2110
> >
> > > >
> > > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > > index 25f57c14f294..561fe45e4449 100644
> > > > --- a/sound/core/control_led.c
> > > > +++ b/sound/core/control_led.c
> > > > @@ -700,7 +700,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > >                 snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > > >                 sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >                 sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > -               device_del(&led_card->dev);
> > > > +               device_unregister(&led_card->dev);
> > > >                 kfree(led_card);
> > > >                 led->cards[card->number] = NULL;
> > > >         }
> > > > @@ -739,9 +739,9 @@ static int __init snd_ctl_led_init(void)
> > > >                         put_device(&led->dev);
> > > >                         for (; group > 0; group--) {
> > > >                                 led = &snd_ctl_leds[group - 1];
> > > > -                               device_del(&led->dev);
> > > > +                               device_unregister(&led->dev);
> > > >                         }
> > > > -                       device_del(&snd_ctl_led_dev);
> > > > +                       device_unregister(&snd_ctl_led_dev);
> > > >                         return -ENOMEM;
> > > >                 }
> > > >         }
> > > > @@ -767,9 +767,9 @@ static void __exit snd_ctl_led_exit(void)
> > > >         }
> > > >         for (group = 0; group < MAX_LED; group++) {
> > > >                 led = &snd_ctl_leds[group];
> > > > -               device_del(&led->dev);
> > > > +               device_unregister(&led->dev);
> > > >         }
> > > > -       device_del(&snd_ctl_led_dev);
> > > > +       device_unregister(&snd_ctl_led_dev);
> > > >         snd_ctl_led_clean(NULL);
> > > >  }
> > > >

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> Hi Dan,
> 
> I wonder if we shall split the current patch into two patches, one
> patch for each memory leak. It is better to satisfy the rule - "one
> patch only fixes one issue".
> 
> We should absolutely fix all these memory leaks. But one patch for two
> different bugs with different objects and different paths is not very
> suitable.
> 

I would just send one patch, because I only see it as one bug.  But you
do what you think is best.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> Hi Dan,
> 
> I wonder if we shall split the current patch into two patches, one
> patch for each memory leak. It is better to satisfy the rule - "one
> patch only fixes one issue".
> 
> We should absolutely fix all these memory leaks. But one patch for two
> different bugs with different objects and different paths is not very
> suitable.
> 

I would just send one patch, because I only see it as one bug.  But you
do what you think is best.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 5:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> > Hi Dan,
> >
> > I wonder if we shall split the current patch into two patches, one
> > patch for each memory leak. It is better to satisfy the rule - "one
> > patch only fixes one issue".
> >
> > We should absolutely fix all these memory leaks. But one patch for two
> > different bugs with different objects and different paths is not very
> > suitable.
> >
>
> I would just send one patch, because I only see it as one bug.  But you
> do what you think is best.

If you think they are the same bug, that's great. Just send and apply
only one patch as it is. I will not send version v2.

BTW, could you please show me the syzbot link for the bug you tried to
resolve? If it is not from syzbot dashboard, can you please show the
bug report?

>
> regards,
> dan carpenter
>
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 5:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> > Hi Dan,
> >
> > I wonder if we shall split the current patch into two patches, one
> > patch for each memory leak. It is better to satisfy the rule - "one
> > patch only fixes one issue".
> >
> > We should absolutely fix all these memory leaks. But one patch for two
> > different bugs with different objects and different paths is not very
> > suitable.
> >
>
> I would just send one patch, because I only see it as one bug.  But you
> do what you think is best.

If you think they are the same bug, that's great. Just send and apply
only one patch as it is. I will not send version v2.

BTW, could you please show me the syzbot link for the bug you tried to
resolve? If it is not from syzbot dashboard, can you please show the
bug report?

>
> regards,
> dan carpenter
>
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 06:35:33PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 5:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> > > Hi Dan,
> > >
> > > I wonder if we shall split the current patch into two patches, one
> > > patch for each memory leak. It is better to satisfy the rule - "one
> > > patch only fixes one issue".
> > >
> > > We should absolutely fix all these memory leaks. But one patch for two
> > > different bugs with different objects and different paths is not very
> > > suitable.
> > >
> >
> > I would just send one patch, because I only see it as one bug.  But you
> > do what you think is best.
> 
> If you think they are the same bug, that's great. Just send and apply
> only one patch as it is. I will not send version v2.

Sorry for the miscommunication.  That's not what I meant at all.

Your patch only introduces one put_device().  We need all five to fix
the bug, and we'll have to change the kfree(link_whatever).  Use
device_unregister() instead put_device().  Include a Reported-by with
the correct syzkaller information.

> 
> BTW, could you please show me the syzbot link for the bug you tried to
> resolve? If it is not from syzbot dashboard, can you please show the
> bug report?

What are you talking about?  I'm not trying to fix a syzkaller bug.  I'm
just reviewing your patch.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Mon, May 31, 2021 at 06:35:33PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 5:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> > > Hi Dan,
> > >
> > > I wonder if we shall split the current patch into two patches, one
> > > patch for each memory leak. It is better to satisfy the rule - "one
> > > patch only fixes one issue".
> > >
> > > We should absolutely fix all these memory leaks. But one patch for two
> > > different bugs with different objects and different paths is not very
> > > suitable.
> > >
> >
> > I would just send one patch, because I only see it as one bug.  But you
> > do what you think is best.
> 
> If you think they are the same bug, that's great. Just send and apply
> only one patch as it is. I will not send version v2.

Sorry for the miscommunication.  That's not what I meant at all.

Your patch only introduces one put_device().  We need all five to fix
the bug, and we'll have to change the kfree(link_whatever).  Use
device_unregister() instead put_device().  Include a Reported-by with
the correct syzkaller information.

> 
> BTW, could you please show me the syzbot link for the bug you tried to
> resolve? If it is not from syzbot dashboard, can you please show the
> bug report?

What are you talking about?  I'm not trying to fix a syzkaller bug.  I'm
just reviewing your patch.

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 6:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 06:35:33PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 5:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> > > > Hi Dan,
> > > >
> > > > I wonder if we shall split the current patch into two patches, one
> > > > patch for each memory leak. It is better to satisfy the rule - "one
> > > > patch only fixes one issue".
> > > >
> > > > We should absolutely fix all these memory leaks. But one patch for two
> > > > different bugs with different objects and different paths is not very
> > > > suitable.
> > > >
> > >
> > > I would just send one patch, because I only see it as one bug.  But you
> > > do what you think is best.
> >
> > If you think they are the same bug, that's great. Just send and apply
> > only one patch as it is. I will not send version v2.
>
> Sorry for the miscommunication.  That's not what I meant at all.
>
> Your patch only introduces one put_device().  We need all five to fix
> the bug, and we'll have to change the kfree(link_whatever).  Use
> device_unregister() instead put_device().  Include a Reported-by with
> the correct syzkaller information.
>
> >
> > BTW, could you please show me the syzbot link for the bug you tried to
> > resolve? If it is not from syzbot dashboard, can you please show the
> > bug report?
>
> What are you talking about?  I'm not trying to fix a syzkaller bug.  I'm
> just reviewing your patch.

It seems we truly have some miscommunication. Your message makes me
think you are fixing another bug report that shares the same root
cause with this bug.

Now let's sync and get on the same page.

You are trying to give me some suggestions to fix this bug. I need to
listen to your advice and send another version v2 to you developers.
Right?

>
> regards,
> dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 6:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, May 31, 2021 at 06:35:33PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 5:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Mon, May 31, 2021 at 05:10:49PM +0800, Dongliang Mu wrote:
> > > > Hi Dan,
> > > >
> > > > I wonder if we shall split the current patch into two patches, one
> > > > patch for each memory leak. It is better to satisfy the rule - "one
> > > > patch only fixes one issue".
> > > >
> > > > We should absolutely fix all these memory leaks. But one patch for two
> > > > different bugs with different objects and different paths is not very
> > > > suitable.
> > > >
> > >
> > > I would just send one patch, because I only see it as one bug.  But you
> > > do what you think is best.
> >
> > If you think they are the same bug, that's great. Just send and apply
> > only one patch as it is. I will not send version v2.
>
> Sorry for the miscommunication.  That's not what I meant at all.
>
> Your patch only introduces one put_device().  We need all five to fix
> the bug, and we'll have to change the kfree(link_whatever).  Use
> device_unregister() instead put_device().  Include a Reported-by with
> the correct syzkaller information.
>
> >
> > BTW, could you please show me the syzbot link for the bug you tried to
> > resolve? If it is not from syzbot dashboard, can you please show the
> > bug report?
>
> What are you talking about?  I'm not trying to fix a syzkaller bug.  I'm
> just reviewing your patch.

It seems we truly have some miscommunication. Your message makes me
think you are fixing another bug report that shares the same root
cause with this bug.

Now let's sync and get on the same page.

You are trying to give me some suggestions to fix this bug. I need to
listen to your advice and send another version v2 to you developers.
Right?

>
> regards,
> dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> to decrease the refcount to zero, which causes device_release never to
> be invoked. This leads to memory leak to some resources, like struct
> device_private.
> 
> Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> 
> Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  sound/core/control_led.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..fff2688b5019 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
>  	snd_ctl_led_refresh();
>  }
>  
> +static void snd_ctl_led_release(struct device *dev)
> +{
> +}

Just to clarify again some more, this call back has to free "led_card".
This patch changes the memory leak into a use after free bug. (A use
after free bug is worse than a memory leak).

There were some other leaks as discussed where a dummy free function is
fine because they were dealing with static data structures (not
allocated memory).

> +
>  /*
>   * sysfs
>   */
> @@ -663,6 +667,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card)
>  		led_card->number = card->number;
>  		led_card->led = led;
>  		device_initialize(&led_card->dev);
> +		led_card->dev.release = snd_ctl_led_release;
>  		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
>  			goto cerr;
>  		led_card->dev.parent = &led->dev;
> @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
>  		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
>  		sysfs_remove_link(&led_card->dev.kobj, "card");
>  		device_del(&led_card->dev);
> +		put_device(&led_card->dev);
>  		kfree(led_card);
>  		led->cards[card->number] = NULL;
>  	}

Btw, I have created a Smatch warning for this type of code where we
have:

	put_device(&foo->dev);
	kfree(foo);

sound/core/control_led.c:709 snd_ctl_led_sysfs_remove() warn: freeing device managed memory: 'led_card'

So hopefully that will prevent future similar bugs.  I'll test it out
overnight and report back tomorrow how it works.

regards,
dan carpenter

/*
 * Copyright (C) 2021 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"

static int my_id;

STATE(managed);

static void set_ignore(struct sm_state *sm, struct expression *mod_expr)
{
	set_state(my_id, sm->name, sm->sym, &undefined);
}

static void match_put_device(const char *fn, struct expression *expr, void *param)
{
	struct expression *arg;

	arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
	arg = strip_expr(arg);
	if (!arg || arg->type != EXPR_PREOP || arg->op != '&')
		return;
	arg = strip_expr(arg->unop);
	if (!arg || arg->type != EXPR_DEREF)
		return;
	arg = strip_expr(arg->deref);
	if (arg && arg->type == EXPR_PREOP && arg->op == '*')
		arg = arg->unop;
	set_state_expr(my_id, arg, &managed);
}

static void match_free(const char *fn, struct expression *expr, void *param)
{
	struct expression *arg;
	char *name;

	arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
	if (get_state_expr(my_id, arg) != &managed)
		return;
	name = expr_to_str(arg);
	sm_warning("freeing device managed memory: '%s'", name);
	free_string(name);
}

void check_put_device(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_hook("put_device", &match_put_device, INT_PTR(0));
	add_function_hook("device_unregister", &match_put_device, INT_PTR(0));

	add_function_hook("kfree", &match_free, INT_PTR(0));
	add_modification_hook(my_id, &set_ignore);
}

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> to decrease the refcount to zero, which causes device_release never to
> be invoked. This leads to memory leak to some resources, like struct
> device_private.
> 
> Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> 
> Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  sound/core/control_led.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 25f57c14f294..fff2688b5019 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
>  	snd_ctl_led_refresh();
>  }
>  
> +static void snd_ctl_led_release(struct device *dev)
> +{
> +}

Just to clarify again some more, this call back has to free "led_card".
This patch changes the memory leak into a use after free bug. (A use
after free bug is worse than a memory leak).

There were some other leaks as discussed where a dummy free function is
fine because they were dealing with static data structures (not
allocated memory).

> +
>  /*
>   * sysfs
>   */
> @@ -663,6 +667,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card)
>  		led_card->number = card->number;
>  		led_card->led = led;
>  		device_initialize(&led_card->dev);
> +		led_card->dev.release = snd_ctl_led_release;
>  		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
>  			goto cerr;
>  		led_card->dev.parent = &led->dev;
> @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
>  		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
>  		sysfs_remove_link(&led_card->dev.kobj, "card");
>  		device_del(&led_card->dev);
> +		put_device(&led_card->dev);
>  		kfree(led_card);
>  		led->cards[card->number] = NULL;
>  	}

Btw, I have created a Smatch warning for this type of code where we
have:

	put_device(&foo->dev);
	kfree(foo);

sound/core/control_led.c:709 snd_ctl_led_sysfs_remove() warn: freeing device managed memory: 'led_card'

So hopefully that will prevent future similar bugs.  I'll test it out
overnight and report back tomorrow how it works.

regards,
dan carpenter

/*
 * Copyright (C) 2021 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"

static int my_id;

STATE(managed);

static void set_ignore(struct sm_state *sm, struct expression *mod_expr)
{
	set_state(my_id, sm->name, sm->sym, &undefined);
}

static void match_put_device(const char *fn, struct expression *expr, void *param)
{
	struct expression *arg;

	arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
	arg = strip_expr(arg);
	if (!arg || arg->type != EXPR_PREOP || arg->op != '&')
		return;
	arg = strip_expr(arg->unop);
	if (!arg || arg->type != EXPR_DEREF)
		return;
	arg = strip_expr(arg->deref);
	if (arg && arg->type == EXPR_PREOP && arg->op == '*')
		arg = arg->unop;
	set_state_expr(my_id, arg, &managed);
}

static void match_free(const char *fn, struct expression *expr, void *param)
{
	struct expression *arg;
	char *name;

	arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
	if (get_state_expr(my_id, arg) != &managed)
		return;
	name = expr_to_str(arg);
	sm_warning("freeing device managed memory: '%s'", name);
	free_string(name);
}

void check_put_device(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_hook("put_device", &match_put_device, INT_PTR(0));
	add_function_hook("device_unregister", &match_put_device, INT_PTR(0));

	add_function_hook("kfree", &match_free, INT_PTR(0));
	add_modification_hook(my_id, &set_ignore);
}

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

I've also created a Smatch check for missing ->release() functions.  It
find one bug in that file.  There are other bugs, but the static checker
is not clever enough to find them.  I expect Smatch will get smarter
about this in the coming year.

sound/core/control_led.c:685 snd_ctl_led_sysfs_add() warn: calling put_device() with no ->release() function

So, yeay, I feel like this thread has been useful and I now understand
put_device() a little better.  Please review the email thread again and
send a v2 patch.  :)

regards,
dan carpenter

/*
 * Copyright (C) 2021 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static void match_put_device(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	struct smatch_state *state;

	state = get_state(SMATCH_EXTRA, name, sym);
	if (!state ||
	    estate_min(state).value != 0 ||
	    estate_max(state).value != 0)
		return;

	sm_warning("calling put_device() with no ->release() function");
}

void check_no_release(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_param_key_hook("put_device", &match_put_device, 0, "$->release", NULL);
	add_function_param_key_hook("device_unregister", &match_put_device, 0, "$->release", NULL);
}

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

I've also created a Smatch check for missing ->release() functions.  It
find one bug in that file.  There are other bugs, but the static checker
is not clever enough to find them.  I expect Smatch will get smarter
about this in the coming year.

sound/core/control_led.c:685 snd_ctl_led_sysfs_add() warn: calling put_device() with no ->release() function

So, yeay, I feel like this thread has been useful and I now understand
put_device() a little better.  Please review the email thread again and
send a v2 patch.  :)

regards,
dan carpenter

/*
 * Copyright (C) 2021 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static void match_put_device(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	struct smatch_state *state;

	state = get_state(SMATCH_EXTRA, name, sym);
	if (!state ||
	    estate_min(state).value != 0 ||
	    estate_max(state).value != 0)
		return;

	sm_warning("calling put_device() with no ->release() function");
}

void check_no_release(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_param_key_hook("put_device", &match_put_device, 0, "$->release", NULL);
	add_function_param_key_hook("device_unregister", &match_put_device, 0, "$->release", NULL);
}

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 7:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> I've also created a Smatch check for missing ->release() functions.  It
> find one bug in that file.  There are other bugs, but the static checker
> is not clever enough to find them.  I expect Smatch will get smarter
> about this in the coming year.
>
> sound/core/control_led.c:685 snd_ctl_led_sysfs_add() warn: calling put_device() with no ->release() function
>
> So, yeay, I feel like this thread has been useful and I now understand
> put_device() a little better.  Please review the email thread again and
> send a v2 patch.  :)

Sure. No problem. I will send a v2 patch after reading the whole thread again.

>
> regards,
> dan carpenter
>
> /*
>  * Copyright (C) 2021 Oracle.
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License
>  * as published by the Free Software Foundation; either version 2
>  * of the License, or (at your option) any later version.
>  *
>  * This program is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
>  */
>
> #include "smatch.h"
> #include "smatch_extra.h"
>
> static int my_id;
>
> static void match_put_device(struct expression *expr, const char *name, struct symbol *sym, void *data)
> {
>         struct smatch_state *state;
>
>         state = get_state(SMATCH_EXTRA, name, sym);
>         if (!state ||
>             estate_min(state).value != 0 ||
>             estate_max(state).value != 0)
>                 return;
>
>         sm_warning("calling put_device() with no ->release() function");
> }
>
> void check_no_release(int id)
> {
>         my_id = id;
>
>         if (option_project != PROJ_KERNEL)
>                 return;
>
>         add_function_param_key_hook("put_device", &match_put_device, 0, "$->release", NULL);
>         add_function_param_key_hook("device_unregister", &match_put_device, 0, "$->release", NULL);
> }
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 7:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> I've also created a Smatch check for missing ->release() functions.  It
> find one bug in that file.  There are other bugs, but the static checker
> is not clever enough to find them.  I expect Smatch will get smarter
> about this in the coming year.
>
> sound/core/control_led.c:685 snd_ctl_led_sysfs_add() warn: calling put_device() with no ->release() function
>
> So, yeay, I feel like this thread has been useful and I now understand
> put_device() a little better.  Please review the email thread again and
> send a v2 patch.  :)

Sure. No problem. I will send a v2 patch after reading the whole thread again.

>
> regards,
> dan carpenter
>
> /*
>  * Copyright (C) 2021 Oracle.
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License
>  * as published by the Free Software Foundation; either version 2
>  * of the License, or (at your option) any later version.
>  *
>  * This program is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
>  */
>
> #include "smatch.h"
> #include "smatch_extra.h"
>
> static int my_id;
>
> static void match_put_device(struct expression *expr, const char *name, struct symbol *sym, void *data)
> {
>         struct smatch_state *state;
>
>         state = get_state(SMATCH_EXTRA, name, sym);
>         if (!state ||
>             estate_min(state).value != 0 ||
>             estate_max(state).value != 0)
>                 return;
>
>         sm_warning("calling put_device() with no ->release() function");
> }
>
> void check_no_release(int id)
> {
>         my_id = id;
>
>         if (option_project != PROJ_KERNEL)
>                 return;
>
>         add_function_param_key_hook("put_device", &match_put_device, 0, "$->release", NULL);
>         add_function_param_key_hook("device_unregister", &match_put_device, 0, "$->release", NULL);
> }
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> > The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> > the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> > to decrease the refcount to zero, which causes device_release never to
> > be invoked. This leads to memory leak to some resources, like struct
> > device_private.
> >
> > Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> >
> > Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> > Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  sound/core/control_led.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..fff2688b5019 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
> >       snd_ctl_led_refresh();
> >  }
> >
> > +static void snd_ctl_led_release(struct device *dev)
> > +{
> > +}
>
> Just to clarify again some more, this call back has to free "led_card".
> This patch changes the memory leak into a use after free bug. (A use
> after free bug is worse than a memory leak).

Hi Dan,

I have read the whole thread several times. I don't quite understand
why you think this call back needs to free "led_card". In current
implementation, the led_card is allocated in snd_ctl_led_sysfs_add,
and released in snd_ctl_led_sysfs_remove. It seems there is no logic
issue. If we keep a dump function here, I think there should no UAF.

I agree with you. We shall be very careful about any added release
function. It might turn a memory leak into double-free or
use-after-free.

>
> There were some other leaks as discussed where a dummy free function is
> fine because they were dealing with static data structures (not
> allocated memory).
>
> > +
> >  /*
> >   * sysfs
> >   */
> > @@ -663,6 +667,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card)
> >               led_card->number = card->number;
> >               led_card->led = led;
> >               device_initialize(&led_card->dev);
> > +             led_card->dev.release = snd_ctl_led_release;
> >               if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
> >                       goto cerr;
> >               led_card->dev.parent = &led->dev;
> > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> >               sysfs_remove_link(&led_card->dev.kobj, "card");
> >               device_del(&led_card->dev);
> > +             put_device(&led_card->dev);
> >               kfree(led_card);
> >               led->cards[card->number] = NULL;
> >       }
>
> Btw, I have created a Smatch warning for this type of code where we
> have:
>
>         put_device(&foo->dev);
>         kfree(foo);

I don't think this should be a bug pattern. put_device will drop the
final reference of one object with struct device and invoke
device_release to release some resources.

The release function should only clean up the internal resources in
the device object. It should not touch the led_card which contains the
device object.

>
> sound/core/control_led.c:709 snd_ctl_led_sysfs_remove() warn: freeing device managed memory: 'led_card'
>
> So hopefully that will prevent future similar bugs.  I'll test it out
> overnight and report back tomorrow how it works.
>
> regards,
> dan carpenter
>
> /*
>  * Copyright (C) 2021 Oracle.
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License
>  * as published by the Free Software Foundation; either version 2
>  * of the License, or (at your option) any later version.
>  *
>  * This program is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
>  */
>
> #include "smatch.h"
>
> static int my_id;
>
> STATE(managed);
>
> static void set_ignore(struct sm_state *sm, struct expression *mod_expr)
> {
>         set_state(my_id, sm->name, sm->sym, &undefined);
> }
>
> static void match_put_device(const char *fn, struct expression *expr, void *param)
> {
>         struct expression *arg;
>
>         arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
>         arg = strip_expr(arg);
>         if (!arg || arg->type != EXPR_PREOP || arg->op != '&')
>                 return;
>         arg = strip_expr(arg->unop);
>         if (!arg || arg->type != EXPR_DEREF)
>                 return;
>         arg = strip_expr(arg->deref);
>         if (arg && arg->type == EXPR_PREOP && arg->op == '*')
>                 arg = arg->unop;
>         set_state_expr(my_id, arg, &managed);
> }
>
> static void match_free(const char *fn, struct expression *expr, void *param)
> {
>         struct expression *arg;
>         char *name;
>
>         arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
>         if (get_state_expr(my_id, arg) != &managed)
>                 return;
>         name = expr_to_str(arg);
>         sm_warning("freeing device managed memory: '%s'", name);
>         free_string(name);
> }
>
> void check_put_device(int id)
> {
>         my_id = id;
>
>         if (option_project != PROJ_KERNEL)
>                 return;
>
>         add_function_hook("put_device", &match_put_device, INT_PTR(0));
>         add_function_hook("device_unregister", &match_put_device, INT_PTR(0));
>
>         add_function_hook("kfree", &match_free, INT_PTR(0));
>         add_modification_hook(my_id, &set_ignore);
> }

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
> > The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain
> > the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails
> > to decrease the refcount to zero, which causes device_release never to
> > be invoked. This leads to memory leak to some resources, like struct
> > device_private.
> >
> > Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
> >
> > Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com
> > Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  sound/core/control_led.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index 25f57c14f294..fff2688b5019 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
> >       snd_ctl_led_refresh();
> >  }
> >
> > +static void snd_ctl_led_release(struct device *dev)
> > +{
> > +}
>
> Just to clarify again some more, this call back has to free "led_card".
> This patch changes the memory leak into a use after free bug. (A use
> after free bug is worse than a memory leak).

Hi Dan,

I have read the whole thread several times. I don't quite understand
why you think this call back needs to free "led_card". In current
implementation, the led_card is allocated in snd_ctl_led_sysfs_add,
and released in snd_ctl_led_sysfs_remove. It seems there is no logic
issue. If we keep a dump function here, I think there should no UAF.

I agree with you. We shall be very careful about any added release
function. It might turn a memory leak into double-free or
use-after-free.

>
> There were some other leaks as discussed where a dummy free function is
> fine because they were dealing with static data structures (not
> allocated memory).
>
> > +
> >  /*
> >   * sysfs
> >   */
> > @@ -663,6 +667,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card)
> >               led_card->number = card->number;
> >               led_card->led = led;
> >               device_initialize(&led_card->dev);
> > +             led_card->dev.release = snd_ctl_led_release;
> >               if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
> >                       goto cerr;
> >               led_card->dev.parent = &led->dev;
> > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> >               sysfs_remove_link(&led_card->dev.kobj, "card");
> >               device_del(&led_card->dev);
> > +             put_device(&led_card->dev);
> >               kfree(led_card);
> >               led->cards[card->number] = NULL;
> >       }
>
> Btw, I have created a Smatch warning for this type of code where we
> have:
>
>         put_device(&foo->dev);
>         kfree(foo);

I don't think this should be a bug pattern. put_device will drop the
final reference of one object with struct device and invoke
device_release to release some resources.

The release function should only clean up the internal resources in
the device object. It should not touch the led_card which contains the
device object.

>
> sound/core/control_led.c:709 snd_ctl_led_sysfs_remove() warn: freeing device managed memory: 'led_card'
>
> So hopefully that will prevent future similar bugs.  I'll test it out
> overnight and report back tomorrow how it works.
>
> regards,
> dan carpenter
>
> /*
>  * Copyright (C) 2021 Oracle.
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License
>  * as published by the Free Software Foundation; either version 2
>  * of the License, or (at your option) any later version.
>  *
>  * This program is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
>  */
>
> #include "smatch.h"
>
> static int my_id;
>
> STATE(managed);
>
> static void set_ignore(struct sm_state *sm, struct expression *mod_expr)
> {
>         set_state(my_id, sm->name, sm->sym, &undefined);
> }
>
> static void match_put_device(const char *fn, struct expression *expr, void *param)
> {
>         struct expression *arg;
>
>         arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
>         arg = strip_expr(arg);
>         if (!arg || arg->type != EXPR_PREOP || arg->op != '&')
>                 return;
>         arg = strip_expr(arg->unop);
>         if (!arg || arg->type != EXPR_DEREF)
>                 return;
>         arg = strip_expr(arg->deref);
>         if (arg && arg->type == EXPR_PREOP && arg->op == '*')
>                 arg = arg->unop;
>         set_state_expr(my_id, arg, &managed);
> }
>
> static void match_free(const char *fn, struct expression *expr, void *param)
> {
>         struct expression *arg;
>         char *name;
>
>         arg = get_argument_from_call_expr(expr->args, PTR_INT(param));
>         if (get_state_expr(my_id, arg) != &managed)
>                 return;
>         name = expr_to_str(arg);
>         sm_warning("freeing device managed memory: '%s'", name);
>         free_string(name);
> }
>
> void check_put_device(int id)
> {
>         my_id = id;
>
>         if (option_project != PROJ_KERNEL)
>                 return;
>
>         add_function_hook("put_device", &match_put_device, INT_PTR(0));
>         add_function_hook("device_unregister", &match_put_device, INT_PTR(0));
>
>         add_function_hook("kfree", &match_free, INT_PTR(0));
>         add_modification_hook(my_id, &set_ignore);
> }

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > >               device_del(&led_card->dev);
> > > +             put_device(&led_card->dev);
> > >               kfree(led_card);
> > >               led->cards[card->number] = NULL;
> > >       }
> >
> > Btw, I have created a Smatch warning for this type of code where we
> > have:
> >
> >         put_device(&foo->dev);
> >         kfree(foo);
> 
> I don't think this should be a bug pattern. put_device will drop the
> final reference of one object with struct device and invoke
> device_release to release some resources.
> 
> The release function should only clean up the internal resources in
> the device object. It should not touch the led_card which contains the
> device object.
> 

It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
debugging on, which you would never do in a production environment.  The
put_device() function calls kobject_release():

lib/kobject.c
   725  static void kobject_release(struct kref *kref)
   726  {
   727          struct kobject *kobj = container_of(kref, struct kobject, kref);
   728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
   729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
   730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
   731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
   732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
                                                  ^^^^^^^^^^^^^^^^^^^^^^^

   733  
   734          schedule_delayed_work(&kobj->release, delay);
   735  #else
   736          kobject_cleanup(kobj);
   737  #endif
   738  }

This release will be done later and it references led_card->dev which is
now freed.

The Smatch check did work pretty decently.  These are all use after free
bugs if you have CONFIG_DEBUG_KOBJECT_RELEASE enabled.  (Line numbers
from Friday's linux-next).  I'm not going to bother fixing them because
they're only an issue for CONFIG_DEBUG_KOBJECT_RELEASE and not for
production but I will email people when more of these bugs are
introduced.

sound/core/control_led.c:688 snd_ctl_led_sysfs_add() warn: freeing device managed memory (UAF): 'led_card'
drivers/usb/gadget/function/f_mass_storage.c:2649 fsg_common_remove_lun() warn: freeing device managed memory (UAF): 'lun'
drivers/usb/gadget/function/f_mass_storage.c:2818 fsg_common_create_lun() warn: freeing device managed memory (UAF): 'lun'
drivers/usb/gadget/function/f_mass_storage.c:2881 fsg_common_release() warn: freeing device managed memory (UAF): 'lun'
drivers/w1/w1.c:810 w1_unref_slave() warn: freeing device managed memory (UAF): 'sl'
drivers/pci/endpoint/pci-epc-core.c:671 pci_epc_destroy() warn: freeing device managed memory (UAF): 'epc'
drivers/pci/endpoint/pci-epc-core.c:742 __pci_epc_create() warn: freeing device managed memory (UAF): 'epc'
drivers/infiniband/ulp/srp/ib_srp.c:3930 srp_add_port() warn: freeing device managed memory (UAF): 'host'
drivers/infiniband/ulp/srp/ib_srp.c:4058 srp_remove_one() warn: freeing device managed memory (UAF): 'host'
drivers/infiniband/ulp/rtrs/rtrs-clt.c:2695 alloc_clt() warn: freeing device managed memory (UAF): 'clt'
drivers/media/pci/solo6x10/solo6x10-core.c:156 free_solo_dev() warn: freeing device managed memory (UAF): 'solo_dev'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:203 nfp_cpp_free() warn: freeing device managed memory (UAF): 'cpp'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:1258 nfp_cpp_from_operations() warn: freeing device managed memory (UAF): 'cpp'
drivers/net/netdevsim/bus.c:354 nsim_bus_dev_del() warn: freeing device managed memory (UAF): 'nsim_bus_dev'
drivers/thermal/thermal_core.c:1002 __thermal_cooling_device_register() warn: freeing device managed memory (UAF): 'cdev'

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > >               device_del(&led_card->dev);
> > > +             put_device(&led_card->dev);
> > >               kfree(led_card);
> > >               led->cards[card->number] = NULL;
> > >       }
> >
> > Btw, I have created a Smatch warning for this type of code where we
> > have:
> >
> >         put_device(&foo->dev);
> >         kfree(foo);
> 
> I don't think this should be a bug pattern. put_device will drop the
> final reference of one object with struct device and invoke
> device_release to release some resources.
> 
> The release function should only clean up the internal resources in
> the device object. It should not touch the led_card which contains the
> device object.
> 

It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
debugging on, which you would never do in a production environment.  The
put_device() function calls kobject_release():

lib/kobject.c
   725  static void kobject_release(struct kref *kref)
   726  {
   727          struct kobject *kobj = container_of(kref, struct kobject, kref);
   728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
   729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
   730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
   731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
   732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
                                                  ^^^^^^^^^^^^^^^^^^^^^^^

   733  
   734          schedule_delayed_work(&kobj->release, delay);
   735  #else
   736          kobject_cleanup(kobj);
   737  #endif
   738  }

This release will be done later and it references led_card->dev which is
now freed.

The Smatch check did work pretty decently.  These are all use after free
bugs if you have CONFIG_DEBUG_KOBJECT_RELEASE enabled.  (Line numbers
from Friday's linux-next).  I'm not going to bother fixing them because
they're only an issue for CONFIG_DEBUG_KOBJECT_RELEASE and not for
production but I will email people when more of these bugs are
introduced.

sound/core/control_led.c:688 snd_ctl_led_sysfs_add() warn: freeing device managed memory (UAF): 'led_card'
drivers/usb/gadget/function/f_mass_storage.c:2649 fsg_common_remove_lun() warn: freeing device managed memory (UAF): 'lun'
drivers/usb/gadget/function/f_mass_storage.c:2818 fsg_common_create_lun() warn: freeing device managed memory (UAF): 'lun'
drivers/usb/gadget/function/f_mass_storage.c:2881 fsg_common_release() warn: freeing device managed memory (UAF): 'lun'
drivers/w1/w1.c:810 w1_unref_slave() warn: freeing device managed memory (UAF): 'sl'
drivers/pci/endpoint/pci-epc-core.c:671 pci_epc_destroy() warn: freeing device managed memory (UAF): 'epc'
drivers/pci/endpoint/pci-epc-core.c:742 __pci_epc_create() warn: freeing device managed memory (UAF): 'epc'
drivers/infiniband/ulp/srp/ib_srp.c:3930 srp_add_port() warn: freeing device managed memory (UAF): 'host'
drivers/infiniband/ulp/srp/ib_srp.c:4058 srp_remove_one() warn: freeing device managed memory (UAF): 'host'
drivers/infiniband/ulp/rtrs/rtrs-clt.c:2695 alloc_clt() warn: freeing device managed memory (UAF): 'clt'
drivers/media/pci/solo6x10/solo6x10-core.c:156 free_solo_dev() warn: freeing device managed memory (UAF): 'solo_dev'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:203 nfp_cpp_free() warn: freeing device managed memory (UAF): 'cpp'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:1258 nfp_cpp_from_operations() warn: freeing device managed memory (UAF): 'cpp'
drivers/net/netdevsim/bus.c:354 nsim_bus_dev_del() warn: freeing device managed memory (UAF): 'nsim_bus_dev'
drivers/thermal/thermal_core.c:1002 __thermal_cooling_device_register() warn: freeing device managed memory (UAF): 'cdev'

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > >               device_del(&led_card->dev);
> > > > +             put_device(&led_card->dev);
> > > >               kfree(led_card);
> > > >               led->cards[card->number] = NULL;
> > > >       }
> > >
> > > Btw, I have created a Smatch warning for this type of code where we
> > > have:
> > >
> > >         put_device(&foo->dev);
> > >         kfree(foo);
> >
> > I don't think this should be a bug pattern. put_device will drop the
> > final reference of one object with struct device and invoke
> > device_release to release some resources.
> >
> > The release function should only clean up the internal resources in
> > the device object. It should not touch the led_card which contains the
> > device object.
> >
>
> It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> debugging on, which you would never do in a production environment.  The
> put_device() function calls kobject_release():

This is interesting. Let's dig a little deeper.

>
> lib/kobject.c
>    725  static void kobject_release(struct kref *kref)
>    726  {
>    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
>    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
>    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
>    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
>    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
>    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
>                                                   ^^^^^^^^^^^^^^^^^^^^^^^
>
>    733
>    734          schedule_delayed_work(&kobj->release, delay);
>    735  #else
>    736          kobject_cleanup(kobj);
>    737  #endif
>    738  }
>
> This release will be done later and it references led_card->dev which is
> now freed.

The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
-> kobject_cleanup. From the comment, kobject_cleanup should only
clean the resources in the kobject, without touching the dev object.
To further confirm, I checked the implementation and found out there
seem no references to the dev object. Would you mind pointing out the
reference to dev object? Moreover, if kobject_cleanup touches the
resources out of kobject, shall we directly change this function other
than its callees?

#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
static void kobject_delayed_cleanup(struct work_struct *work)
{
kobject_cleanup(container_of(to_delayed_work(work),
    struct kobject, release));
}
#endif

/*
 * kobject_cleanup - free kobject resources.
 * @kobj: object to cleanup
 */
static void kobject_cleanup(struct kobject *kobj)
{
struct kobject *parent = kobj->parent;
struct kobj_type *t = get_ktype(kobj);
const char *name = kobj->name;

pr_debug("kobject: '%s' (%p): %s, parent %p\n",
kobject_name(kobj), kobj, __func__, kobj->parent);

if (t && !t->release)
pr_debug("kobject: '%s' (%p): does not have a release() function, it
is broken and must be fixed. See
Documentation/core-api/kobject.rst.\n",
kobject_name(kobj), kobj);

/* remove from sysfs if the caller did not do it */
if (kobj->state_in_sysfs) {
pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
kobject_name(kobj), kobj);
__kobject_del(kobj);
} else {
/* avoid dropping the parent reference unnecessarily */
parent = NULL;
}

if (t && t->release) {
pr_debug("kobject: '%s' (%p): calling ktype release\n",
kobject_name(kobj), kobj);
t->release(kobj);
}

/* free name if we allocated it */
if (name) {
pr_debug("kobject: '%s': free name\n", name);
kfree_const(name);
}

kobject_put(parent);
}

>
> The Smatch check did work pretty decently.  These are all use after free
> bugs if you have CONFIG_DEBUG_KOBJECT_RELEASE enabled.  (Line numbers
> from Friday's linux-next).  I'm not going to bother fixing them because
> they're only an issue for CONFIG_DEBUG_KOBJECT_RELEASE and not for
> production but I will email people when more of these bugs are
> introduced.
>
> sound/core/control_led.c:688 snd_ctl_led_sysfs_add() warn: freeing device managed memory (UAF): 'led_card'
> drivers/usb/gadget/function/f_mass_storage.c:2649 fsg_common_remove_lun() warn: freeing device managed memory (UAF): 'lun'
> drivers/usb/gadget/function/f_mass_storage.c:2818 fsg_common_create_lun() warn: freeing device managed memory (UAF): 'lun'
> drivers/usb/gadget/function/f_mass_storage.c:2881 fsg_common_release() warn: freeing device managed memory (UAF): 'lun'
> drivers/w1/w1.c:810 w1_unref_slave() warn: freeing device managed memory (UAF): 'sl'
> drivers/pci/endpoint/pci-epc-core.c:671 pci_epc_destroy() warn: freeing device managed memory (UAF): 'epc'
> drivers/pci/endpoint/pci-epc-core.c:742 __pci_epc_create() warn: freeing device managed memory (UAF): 'epc'
> drivers/infiniband/ulp/srp/ib_srp.c:3930 srp_add_port() warn: freeing device managed memory (UAF): 'host'
> drivers/infiniband/ulp/srp/ib_srp.c:4058 srp_remove_one() warn: freeing device managed memory (UAF): 'host'
> drivers/infiniband/ulp/rtrs/rtrs-clt.c:2695 alloc_clt() warn: freeing device managed memory (UAF): 'clt'
> drivers/media/pci/solo6x10/solo6x10-core.c:156 free_solo_dev() warn: freeing device managed memory (UAF): 'solo_dev'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:203 nfp_cpp_free() warn: freeing device managed memory (UAF): 'cpp'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:1258 nfp_cpp_from_operations() warn: freeing device managed memory (UAF): 'cpp'
> drivers/net/netdevsim/bus.c:354 nsim_bus_dev_del() warn: freeing device managed memory (UAF): 'nsim_bus_dev'
> drivers/thermal/thermal_core.c:1002 __thermal_cooling_device_register() warn: freeing device managed memory (UAF): 'cdev'
>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > >               device_del(&led_card->dev);
> > > > +             put_device(&led_card->dev);
> > > >               kfree(led_card);
> > > >               led->cards[card->number] = NULL;
> > > >       }
> > >
> > > Btw, I have created a Smatch warning for this type of code where we
> > > have:
> > >
> > >         put_device(&foo->dev);
> > >         kfree(foo);
> >
> > I don't think this should be a bug pattern. put_device will drop the
> > final reference of one object with struct device and invoke
> > device_release to release some resources.
> >
> > The release function should only clean up the internal resources in
> > the device object. It should not touch the led_card which contains the
> > device object.
> >
>
> It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> debugging on, which you would never do in a production environment.  The
> put_device() function calls kobject_release():

This is interesting. Let's dig a little deeper.

>
> lib/kobject.c
>    725  static void kobject_release(struct kref *kref)
>    726  {
>    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
>    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
>    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
>    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
>    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
>    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
>                                                   ^^^^^^^^^^^^^^^^^^^^^^^
>
>    733
>    734          schedule_delayed_work(&kobj->release, delay);
>    735  #else
>    736          kobject_cleanup(kobj);
>    737  #endif
>    738  }
>
> This release will be done later and it references led_card->dev which is
> now freed.

The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
-> kobject_cleanup. From the comment, kobject_cleanup should only
clean the resources in the kobject, without touching the dev object.
To further confirm, I checked the implementation and found out there
seem no references to the dev object. Would you mind pointing out the
reference to dev object? Moreover, if kobject_cleanup touches the
resources out of kobject, shall we directly change this function other
than its callees?

#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
static void kobject_delayed_cleanup(struct work_struct *work)
{
kobject_cleanup(container_of(to_delayed_work(work),
    struct kobject, release));
}
#endif

/*
 * kobject_cleanup - free kobject resources.
 * @kobj: object to cleanup
 */
static void kobject_cleanup(struct kobject *kobj)
{
struct kobject *parent = kobj->parent;
struct kobj_type *t = get_ktype(kobj);
const char *name = kobj->name;

pr_debug("kobject: '%s' (%p): %s, parent %p\n",
kobject_name(kobj), kobj, __func__, kobj->parent);

if (t && !t->release)
pr_debug("kobject: '%s' (%p): does not have a release() function, it
is broken and must be fixed. See
Documentation/core-api/kobject.rst.\n",
kobject_name(kobj), kobj);

/* remove from sysfs if the caller did not do it */
if (kobj->state_in_sysfs) {
pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
kobject_name(kobj), kobj);
__kobject_del(kobj);
} else {
/* avoid dropping the parent reference unnecessarily */
parent = NULL;
}

if (t && t->release) {
pr_debug("kobject: '%s' (%p): calling ktype release\n",
kobject_name(kobj), kobj);
t->release(kobj);
}

/* free name if we allocated it */
if (name) {
pr_debug("kobject: '%s': free name\n", name);
kfree_const(name);
}

kobject_put(parent);
}

>
> The Smatch check did work pretty decently.  These are all use after free
> bugs if you have CONFIG_DEBUG_KOBJECT_RELEASE enabled.  (Line numbers
> from Friday's linux-next).  I'm not going to bother fixing them because
> they're only an issue for CONFIG_DEBUG_KOBJECT_RELEASE and not for
> production but I will email people when more of these bugs are
> introduced.
>
> sound/core/control_led.c:688 snd_ctl_led_sysfs_add() warn: freeing device managed memory (UAF): 'led_card'
> drivers/usb/gadget/function/f_mass_storage.c:2649 fsg_common_remove_lun() warn: freeing device managed memory (UAF): 'lun'
> drivers/usb/gadget/function/f_mass_storage.c:2818 fsg_common_create_lun() warn: freeing device managed memory (UAF): 'lun'
> drivers/usb/gadget/function/f_mass_storage.c:2881 fsg_common_release() warn: freeing device managed memory (UAF): 'lun'
> drivers/w1/w1.c:810 w1_unref_slave() warn: freeing device managed memory (UAF): 'sl'
> drivers/pci/endpoint/pci-epc-core.c:671 pci_epc_destroy() warn: freeing device managed memory (UAF): 'epc'
> drivers/pci/endpoint/pci-epc-core.c:742 __pci_epc_create() warn: freeing device managed memory (UAF): 'epc'
> drivers/infiniband/ulp/srp/ib_srp.c:3930 srp_add_port() warn: freeing device managed memory (UAF): 'host'
> drivers/infiniband/ulp/srp/ib_srp.c:4058 srp_remove_one() warn: freeing device managed memory (UAF): 'host'
> drivers/infiniband/ulp/rtrs/rtrs-clt.c:2695 alloc_clt() warn: freeing device managed memory (UAF): 'clt'
> drivers/media/pci/solo6x10/solo6x10-core.c:156 free_solo_dev() warn: freeing device managed memory (UAF): 'solo_dev'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:203 nfp_cpp_free() warn: freeing device managed memory (UAF): 'cpp'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:1258 nfp_cpp_from_operations() warn: freeing device managed memory (UAF): 'cpp'
> drivers/net/netdevsim/bus.c:354 nsim_bus_dev_del() warn: freeing device managed memory (UAF): 'nsim_bus_dev'
> drivers/thermal/thermal_core.c:1002 __thermal_cooling_device_register() warn: freeing device managed memory (UAF): 'cdev'
>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Tue, Jun 01, 2021 at 10:19:22PM +0800, Dongliang Mu wrote:
> On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > >               device_del(&led_card->dev);
> > > > > +             put_device(&led_card->dev);
> > > > >               kfree(led_card);
> > > > >               led->cards[card->number] = NULL;
> > > > >       }
> > > >
> > > > Btw, I have created a Smatch warning for this type of code where we
> > > > have:
> > > >
> > > >         put_device(&foo->dev);
> > > >         kfree(foo);
> > >
> > > I don't think this should be a bug pattern. put_device will drop the
> > > final reference of one object with struct device and invoke
> > > device_release to release some resources.
> > >
> > > The release function should only clean up the internal resources in
> > > the device object. It should not touch the led_card which contains the
> > > device object.
> > >
> >
> > It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> > debugging on, which you would never do in a production environment.  The
> > put_device() function calls kobject_release():
> 
> This is interesting. Let's dig a little deeper.
> 
> >
> > lib/kobject.c
> >    725  static void kobject_release(struct kref *kref)
> >    726  {
> >    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
> >    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> >    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> >    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
> >    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
> >    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> >                                                   ^^^^^^^^^^^^^^^^^^^^^^^
> >
> >    733
> >    734          schedule_delayed_work(&kobj->release, delay);
> >    735  #else
> >    736          kobject_cleanup(kobj);
> >    737  #endif
> >    738  }
> >
> > This release will be done later and it references led_card->dev which is
> > now freed.
> 
> The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
> -> kobject_cleanup. From the comment, kobject_cleanup should only
> clean the resources in the kobject, without touching the dev object.
> To further confirm, I checked the implementation and found out there
> seem no references to the dev object. Would you mind pointing out the
> reference to dev object?

The kobj struct is included in the dev struct, it's not a pointer.

	led_card->dev.kobj.name

See all the '.' characters and only one "->"?  If you kfree(led_card)
then you can't use led_card->dev.kobj any more.

> Moreover, if kobject_cleanup touches the
> resources out of kobject, shall we directly change this function other
> than its callees?
> 

I don't understand your question here.  The rest of the email looks like
some copy and pasted code but I don't know what I'm supposed to be
looking for.

I really feel like I have explained things very as well as I can and I'm
not sure what more I can do to help... :/

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter]

On Tue, Jun 01, 2021 at 10:19:22PM +0800, Dongliang Mu wrote:
> On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > >               device_del(&led_card->dev);
> > > > > +             put_device(&led_card->dev);
> > > > >               kfree(led_card);
> > > > >               led->cards[card->number] = NULL;
> > > > >       }
> > > >
> > > > Btw, I have created a Smatch warning for this type of code where we
> > > > have:
> > > >
> > > >         put_device(&foo->dev);
> > > >         kfree(foo);
> > >
> > > I don't think this should be a bug pattern. put_device will drop the
> > > final reference of one object with struct device and invoke
> > > device_release to release some resources.
> > >
> > > The release function should only clean up the internal resources in
> > > the device object. It should not touch the led_card which contains the
> > > device object.
> > >
> >
> > It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> > debugging on, which you would never do in a production environment.  The
> > put_device() function calls kobject_release():
> 
> This is interesting. Let's dig a little deeper.
> 
> >
> > lib/kobject.c
> >    725  static void kobject_release(struct kref *kref)
> >    726  {
> >    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
> >    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> >    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> >    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
> >    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
> >    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> >                                                   ^^^^^^^^^^^^^^^^^^^^^^^
> >
> >    733
> >    734          schedule_delayed_work(&kobj->release, delay);
> >    735  #else
> >    736          kobject_cleanup(kobj);
> >    737  #endif
> >    738  }
> >
> > This release will be done later and it references led_card->dev which is
> > now freed.
> 
> The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
> -> kobject_cleanup. From the comment, kobject_cleanup should only
> clean the resources in the kobject, without touching the dev object.
> To further confirm, I checked the implementation and found out there
> seem no references to the dev object. Would you mind pointing out the
> reference to dev object?

The kobj struct is included in the dev struct, it's not a pointer.

	led_card->dev.kobj.name

See all the '.' characters and only one "->"?  If you kfree(led_card)
then you can't use led_card->dev.kobj any more.

> Moreover, if kobject_cleanup touches the
> resources out of kobject, shall we directly change this function other
> than its callees?
> 

I don't understand your question here.  The rest of the email looks like
some copy and pasted code but I don't know what I'm supposed to be
looking for.

I really feel like I have explained things very as well as I can and I'm
not sure what more I can do to help... :/

regards,
dan carpenter

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Tue, Jun 1, 2021 at 10:43 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jun 01, 2021 at 10:19:22PM +0800, Dongliang Mu wrote:
> > On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > > > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > > >               device_del(&led_card->dev);
> > > > > > +             put_device(&led_card->dev);
> > > > > >               kfree(led_card);
> > > > > >               led->cards[card->number] = NULL;
> > > > > >       }
> > > > >
> > > > > Btw, I have created a Smatch warning for this type of code where we
> > > > > have:
> > > > >
> > > > >         put_device(&foo->dev);
> > > > >         kfree(foo);
> > > >
> > > > I don't think this should be a bug pattern. put_device will drop the
> > > > final reference of one object with struct device and invoke
> > > > device_release to release some resources.
> > > >
> > > > The release function should only clean up the internal resources in
> > > > the device object. It should not touch the led_card which contains the
> > > > device object.
> > > >
> > >
> > > It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> > > debugging on, which you would never do in a production environment.  The
> > > put_device() function calls kobject_release():
> >
> > This is interesting. Let's dig a little deeper.
> >
> > >
> > > lib/kobject.c
> > >    725  static void kobject_release(struct kref *kref)
> > >    726  {
> > >    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
> > >    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > >    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> > >    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
> > >    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
> > >    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> > >                                                   ^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > >    733
> > >    734          schedule_delayed_work(&kobj->release, delay);
> > >    735  #else
> > >    736          kobject_cleanup(kobj);
> > >    737  #endif
> > >    738  }
> > >
> > > This release will be done later and it references led_card->dev which is
> > > now freed.
> >
> > The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
> > -> kobject_cleanup. From the comment, kobject_cleanup should only
> > clean the resources in the kobject, without touching the dev object.
> > To further confirm, I checked the implementation and found out there
> > seem no references to the dev object. Would you mind pointing out the
> > reference to dev object?
>
> The kobj struct is included in the dev struct, it's not a pointer.
>
>         led_card->dev.kobj.name
>
> See all the '.' characters and only one "->"?  If you kfree(led_card)
> then you can't use led_card->dev.kobj any more.

Yeah, you're right. I originally thought the field kobj is a pointer
and there should no problem. Please leave alone the question below. I
thought up this question based on the assumption before.

>
> > Moreover, if kobject_cleanup touches the
> > resources out of kobject, shall we directly change this function other
> > than its callees?
> >
>
> I don't understand your question here.  The rest of the email looks like
> some copy and pasted code but I don't know what I'm supposed to be
> looking for.
>
> I really feel like I have explained things very as well as I can and I'm
> not sure what more I can do to help... :/

You already helped too much, and I learned a lot from the discussion
with you. Don't be bothered by my stupid questions. :)

>
> regards,
> dan carpenter
>

Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dongliang Mu]

On Tue, Jun 1, 2021 at 10:43 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jun 01, 2021 at 10:19:22PM +0800, Dongliang Mu wrote:
> > On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > > > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > > > >               device_del(&led_card->dev);
> > > > > > +             put_device(&led_card->dev);
> > > > > >               kfree(led_card);
> > > > > >               led->cards[card->number] = NULL;
> > > > > >       }
> > > > >
> > > > > Btw, I have created a Smatch warning for this type of code where we
> > > > > have:
> > > > >
> > > > >         put_device(&foo->dev);
> > > > >         kfree(foo);
> > > >
> > > > I don't think this should be a bug pattern. put_device will drop the
> > > > final reference of one object with struct device and invoke
> > > > device_release to release some resources.
> > > >
> > > > The release function should only clean up the internal resources in
> > > > the device object. It should not touch the led_card which contains the
> > > > device object.
> > > >
> > >
> > > It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> > > debugging on, which you would never do in a production environment.  The
> > > put_device() function calls kobject_release():
> >
> > This is interesting. Let's dig a little deeper.
> >
> > >
> > > lib/kobject.c
> > >    725  static void kobject_release(struct kref *kref)
> > >    726  {
> > >    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
> > >    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> > >    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
> > >    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
> > >    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
> > >    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
> > >                                                   ^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > >    733
> > >    734          schedule_delayed_work(&kobj->release, delay);
> > >    735  #else
> > >    736          kobject_cleanup(kobj);
> > >    737  #endif
> > >    738  }
> > >
> > > This release will be done later and it references led_card->dev which is
> > > now freed.
> >
> > The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
> > -> kobject_cleanup. From the comment, kobject_cleanup should only
> > clean the resources in the kobject, without touching the dev object.
> > To further confirm, I checked the implementation and found out there
> > seem no references to the dev object. Would you mind pointing out the
> > reference to dev object?
>
> The kobj struct is included in the dev struct, it's not a pointer.
>
>         led_card->dev.kobj.name
>
> See all the '.' characters and only one "->"?  If you kfree(led_card)
> then you can't use led_card->dev.kobj any more.

Yeah, you're right. I originally thought the field kobj is a pointer
and there should no problem. Please leave alone the question below. I
thought up this question based on the assumption before.

>
> > Moreover, if kobject_cleanup touches the
> > resources out of kobject, shall we directly change this function other
> > than its callees?
> >
>
> I don't understand your question here.  The rest of the email looks like
> some copy and pasted code but I don't know what I'm supposed to be
> looking for.
>
> I really feel like I have explained things very as well as I can and I'm
> not sure what more I can do to help... :/

You already helped too much, and I learned a lot from the discussion
with you. Don't be bothered by my stupid questions. :)

>
> regards,
> dan carpenter
>