| Message ID | 20230301-watchdog-avoid-iter-after-loop-v1-1-851f583be9f7@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | watchdog: avoid usage of iterator after loop | expand |
On 3/1/23 09:17, Jakob Koschel wrote: > If potentially no valid element is found, 'p' would contain an invalid > pointer past the iterator loop. To ensure 'p' is valid under any > circumstances, the kfree() should be within the loop body. > > Additionally, Linus proposed to avoid any use of the list iterator > variable after the loop, in the attempt to move the list iterator > variable declaration into the marcro to avoid any potential misuse after macro > the loop [1]. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> > --- > drivers/watchdog/watchdog_pretimeout.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c > index 376a495ab80c..d8c78696eaf5 100644 > --- a/drivers/watchdog/watchdog_pretimeout.c > +++ b/drivers/watchdog/watchdog_pretimeout.c > @@ -207,10 +207,10 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd) > list_for_each_entry_safe(p, t, &pretimeout_list, entry) { > if (p->wdd == wdd) { > list_del(&p->entry); > - break; > + spin_unlock_irq(&pretimeout_lock); > + kfree(p); > + return; Please just make it kfree(p); break; There is no need to drop the spinlock here and/or to return directly. Thanks, Guenter > } > } > spin_unlock_irq(&pretimeout_lock); > - > - kfree(p); > } > > --- > base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9 > change-id: 20230301-watchdog-avoid-iter-after-loop-a197bf201435 > > Best regards,
On 23/03/01 10:31AM, Guenter Roeck wrote: > On 3/1/23 09:17, Jakob Koschel wrote: > > If potentially no valid element is found, 'p' would contain an invalid > > pointer past the iterator loop. To ensure 'p' is valid under any > > circumstances, the kfree() should be within the loop body. > > > > Additionally, Linus proposed to avoid any use of the list iterator > > variable after the loop, in the attempt to move the list iterator > > variable declaration into the marcro to avoid any potential misuse after > > macro > > > the loop [1]. > > > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > > Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> > > --- > > drivers/watchdog/watchdog_pretimeout.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c > > index 376a495ab80c..d8c78696eaf5 100644 > > --- a/drivers/watchdog/watchdog_pretimeout.c > > +++ b/drivers/watchdog/watchdog_pretimeout.c > > @@ -207,10 +207,10 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd) > > list_for_each_entry_safe(p, t, &pretimeout_list, entry) { > > if (p->wdd == wdd) { > > list_del(&p->entry); > > - break; > > + spin_unlock_irq(&pretimeout_lock); > > + kfree(p); > > + return; > > Please just make it > kfree(p); > break; > > There is no need to drop the spinlock here and/or to return > directly. Ok great, I'll fix that in v2. I wasn't sure if something breaks if 'p' is released if the spinlock is still hold. ~ jakob > > Thanks, > Guenter > > > } > > } > > spin_unlock_irq(&pretimeout_lock); > > - > > - kfree(p); > > } > > > > --- > > base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9 > > change-id: 20230301-watchdog-avoid-iter-after-loop-a197bf201435 > > > > Best regards, >
On 3/1/23 14:42, Jakob Koschel wrote: > On 23/03/01 10:31AM, Guenter Roeck wrote: >> On 3/1/23 09:17, Jakob Koschel wrote: >>> If potentially no valid element is found, 'p' would contain an invalid >>> pointer past the iterator loop. To ensure 'p' is valid under any >>> circumstances, the kfree() should be within the loop body. >>> >>> Additionally, Linus proposed to avoid any use of the list iterator >>> variable after the loop, in the attempt to move the list iterator >>> variable declaration into the marcro to avoid any potential misuse after >> >> macro >> >>> the loop [1]. >>> >>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] >>> Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> >>> --- >>> drivers/watchdog/watchdog_pretimeout.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c >>> index 376a495ab80c..d8c78696eaf5 100644 >>> --- a/drivers/watchdog/watchdog_pretimeout.c >>> +++ b/drivers/watchdog/watchdog_pretimeout.c >>> @@ -207,10 +207,10 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd) >>> list_for_each_entry_safe(p, t, &pretimeout_list, entry) { >>> if (p->wdd == wdd) { >>> list_del(&p->entry); >>> - break; >>> + spin_unlock_irq(&pretimeout_lock); >>> + kfree(p); >>> + return; >> >> Please just make it >> kfree(p); >> break; >> >> There is no need to drop the spinlock here and/or to return >> directly. > > Ok great, I'll fix that in v2. I wasn't sure if something breaks if 'p' is released if the spinlock is still hold. > Ah, interesting question. Looking into it, I don't think that is a problem. Just to be sure, I wrote a little coccinelle script to find calls to kfree() under spinlock_irq() and found several instances. Guenter
diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c index 376a495ab80c..d8c78696eaf5 100644 --- a/drivers/watchdog/watchdog_pretimeout.c +++ b/drivers/watchdog/watchdog_pretimeout.c @@ -207,10 +207,10 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd) list_for_each_entry_safe(p, t, &pretimeout_list, entry) { if (p->wdd == wdd) { list_del(&p->entry); - break; + spin_unlock_irq(&pretimeout_lock); + kfree(p); + return; } } spin_unlock_irq(&pretimeout_lock); - - kfree(p); }
If potentially no valid element is found, 'p' would contain an invalid pointer past the iterator loop. To ensure 'p' is valid under any circumstances, the kfree() should be within the loop body. Additionally, Linus proposed to avoid any use of the list iterator variable after the loop, in the attempt to move the list iterator variable declaration into the marcro to avoid any potential misuse after the loop [1]. Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> --- drivers/watchdog/watchdog_pretimeout.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9 change-id: 20230301-watchdog-avoid-iter-after-loop-a197bf201435 Best regards,