| Message ID | 20230707101701.2474499-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv6/addrconf: fix a potential refcount underflow for idev | expand |
On Fri, Jul 7, 2023 at 12:17 PM 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); > > Hold idev anyway firstly. Then call mod_timer() for rs_timer, put > idev if mod_timer() return 1. This modification takes into account > the case where "when" is 0. > > Fixes: b7b1bfce0bb6 ("ipv6: split duplicate address detection and router solicitation timer") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > --- > net/ipv6/addrconf.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 5479da08ef40..d36e6c5e3081 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -318,9 +318,9 @@ 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)) > - in6_dev_hold(idev); > - mod_timer(&idev->rs_timer, jiffies + when); > + in6_dev_hold(idev); > + if (mod_timer(&idev->rs_timer, jiffies + when)) > + in6_dev_put(idev); > } > All callers own an implicit or explicit reference to idev, so you can use the traditional if (!mod_timer(&idev->rs_timer, jiffies + when)) in6_dev_hold(idev);
> On Fri, Jul 7, 2023 at 12:17 PM 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); >> >> Hold idev anyway firstly. Then call mod_timer() for rs_timer, put >> idev if mod_timer() return 1. This modification takes into account >> the case where "when" is 0. >> >> Fixes: b7b1bfce0bb6 ("ipv6: split duplicate address detection and router solicitation timer") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> --- >> net/ipv6/addrconf.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 5479da08ef40..d36e6c5e3081 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -318,9 +318,9 @@ 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)) >> - in6_dev_hold(idev); >> - mod_timer(&idev->rs_timer, jiffies + when); >> + in6_dev_hold(idev); >> + if (mod_timer(&idev->rs_timer, jiffies + when)) >> + in6_dev_put(idev); >> } >> > > > All callers own an implicit or explicit reference to idev, so you can > use the traditional Yes, thank you for your comment. Thanks, William Xuan > > if (!mod_timer(&idev->rs_timer, jiffies + when)) > in6_dev_hold(idev); > . >
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 5479da08ef40..d36e6c5e3081 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -318,9 +318,9 @@ 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)) - in6_dev_hold(idev); - mod_timer(&idev->rs_timer, jiffies + when); + in6_dev_hold(idev); + if (mod_timer(&idev->rs_timer, jiffies + when)) + in6_dev_put(idev); } static void addrconf_mod_dad_work(struct inet6_ifaddr *ifp,
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); Hold idev anyway firstly. Then call mod_timer() for rs_timer, put idev if mod_timer() return 1. This modification takes into account the case where "when" is 0. Fixes: b7b1bfce0bb6 ("ipv6: split duplicate address detection and router solicitation timer") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/ipv6/addrconf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)