2023-03-13 14:00:19

by Jakob Koschel

Subject: [PATCH v2] watchdog: avoid usage of iterator after loop

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 macro 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 <[email protected]>
---
Changes in v2:
- avoid returning within the list iterator
- Link to v1: https://lore.kernel.org/r/20230301-watchdog-avoid-iter-after-loop-v1-1-851f583be9f7@gmail.com
---
drivers/watchdog/watchdog_pretimeout.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
index 376a495ab80c..e5295c990fa1 100644
--- a/drivers/watchdog/watchdog_pretimeout.c
+++ b/drivers/watchdog/watchdog_pretimeout.c
@@ -207,10 +207,9 @@ 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);
+ kfree(p);
break;
}
}
spin_unlock_irq(&pretimeout_lock);
-
- kfree(p);
}

---
base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
change-id: 20230301-watchdog-avoid-iter-after-loop-a197bf201435

Best regards,
--
Jakob Koschel <[email protected]>

2023-03-13 15:01:01

by Guenter Roeck

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

On 3/13/23 06:59, 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 macro 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 <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> Changes in v2:
> - avoid returning within the list iterator
> - Link to v1: https://lore.kernel.org/r/20230301-watchdog-avoid-iter-after-loop-v1-1-851f583be9f7@gmail.com
> ---
> drivers/watchdog/watchdog_pretimeout.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> index 376a495ab80c..e5295c990fa1 100644
> --- a/drivers/watchdog/watchdog_pretimeout.c
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -207,10 +207,9 @@ 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);
> + kfree(p);
> break;
> }
> }
> spin_unlock_irq(&pretimeout_lock);
> -
> - kfree(p);
> }
>
> ---
> base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
> change-id: 20230301-watchdog-avoid-iter-after-loop-a197bf201435
>
> Best regards,