Re: [PATCH] watchdog: avoid usage of iterator after loop

[Guenter Roeck]

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@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.

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