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=6d9e1e89003c894e7a1855c92dfa558ebcb8f21...
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