[PATCH net v2] ipv6/addrconf: fix a potential refcount underflow for idev

[PATCH net v2] ipv6/addrconf: fix a potential refcount underflow for idev

[Ziyang Xuan]

Now in addrconf_mod_rs_timer(), reference idev depends on whether
rs_timer is not pending. Then modify rs_timer timeout.

There is a time gap in [1], during which if the pending rs_timer
becomes not pending. It will miss to hold idev, but the rs_timer
is activated. Thus rs_timer callback function addrconf_rs_timer()
will be executed and put idev later without holding idev. A refcount
underflow issue for idev can be caused by this.

	if (!timer_pending(&idev->rs_timer))
		in6_dev_hold(idev);
		  <--------------[1]
	mod_timer(&idev->rs_timer, jiffies + when);

To fix the issue, hold idev if mod_timer() return 0.

Fixes: b7b1bfce0bb6 ("ipv6: split duplicate address detection and router solicitation timer")
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
v2:
  - Do not hold idev anyway firstly.
---
 net/ipv6/addrconf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 5479da08ef40..e5213e598a04 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -318,9 +318,8 @@ static void addrconf_del_dad_work(struct inet6_ifaddr *ifp)
 static void addrconf_mod_rs_timer(struct inet6_dev *idev,
 				  unsigned long when)
 {
-	if (!timer_pending(&idev->rs_timer))
+	if (!mod_timer(&idev->rs_timer, jiffies + when))
 		in6_dev_hold(idev);
-	mod_timer(&idev->rs_timer, jiffies + when);
 }
 
 static void addrconf_mod_dad_work(struct inet6_ifaddr *ifp,
-- 
2.25.1

Re: [PATCH net v2] ipv6/addrconf: fix a potential refcount underflow for idev

[Eric Dumazet]

On Sat, Jul 8, 2023 at 8:59 AM Ziyang Xuan
<william.xuanziyang@huawei.com> wrote:
>
> Now in addrconf_mod_rs_timer(), reference idev depends on whether
> rs_timer is not pending. Then modify rs_timer timeout.
>
> There is a time gap in [1], during which if the pending rs_timer
> becomes not pending. It will miss to hold idev, but the rs_timer
> is activated. Thus rs_timer callback function addrconf_rs_timer()
> will be executed and put idev later without holding idev. A refcount
> underflow issue for idev can be caused by this.
>
>         if (!timer_pending(&idev->rs_timer))
>                 in6_dev_hold(idev);
>                   <--------------[1]
>         mod_timer(&idev->rs_timer, jiffies + when);
>
> To fix the issue, hold idev if mod_timer() return 0.
>
> Fixes: b7b1bfce0bb6 ("ipv6: split duplicate address detection and router solicitation timer")
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks !

Re: [PATCH net v2] ipv6/addrconf: fix a potential refcount underflow for idev

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat, 8 Jul 2023 14:59:10 +0800 you wrote:
> Now in addrconf_mod_rs_timer(), reference idev depends on whether
> rs_timer is not pending. Then modify rs_timer timeout.
> 
> There is a time gap in [1], during which if the pending rs_timer
> becomes not pending. It will miss to hold idev, but the rs_timer
> is activated. Thus rs_timer callback function addrconf_rs_timer()
> will be executed and put idev later without holding idev. A refcount
> underflow issue for idev can be caused by this.
> 
> [...]

Here is the summary with links:
  - [net,v2] ipv6/addrconf: fix a potential refcount underflow for idev
    https://git.kernel.org/netdev/net/c/06a0716949c2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html