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@xxxxxxxxxxxxxx/ [1] > > Signed-off-by: Jakob Koschel <jkl820.git@xxxxxxxxx> > > --- > > 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, >
