| Message ID | 1493749857-18032-1-git-send-email-mmanning@brocade.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
On reflection, please put this on hold subject to testing with syzkaller. I have not had a repro of the issue and so the fix even though harmless may not be effective. Thanks Mike On 02/05/17 19:30, Mike Manning wrote: > While this is not reproducible manually, Andrey's syzkaller program hit > the warning "IPv6: Freeing alive inet6 address" with this part trace: > > inet6_ifa_finish_destroy+0x12e/0x190 c:894 > in6_ifa_put ./include/net/addrconf.h:330 > addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963 > > The fix is to call in6_ifa_put() for the inet6_ifaddr before rather > than after calling addrconf_ifdown(), as the latter may remove it from > the address hash table. > > Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD") > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Signed-off-by: Mike Manning <mmanning@brocade.com> > --- > net/ipv6/addrconf.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 80ce478..361993a 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w) > } else if (action == DAD_ABORT) { > in6_ifa_hold(ifp); > addrconf_dad_stop(ifp, 1); > - if (disable_ipv6) > + if (disable_ipv6) { > + in6_ifa_put(ifp); > addrconf_ifdown(idev->dev, 0); > + goto unlock; > + } > goto out; > } > > @@ -3950,6 +3953,7 @@ static void addrconf_dad_work(struct work_struct *w) > ifp->dad_nonce); > out: > in6_ifa_put(ifp); > +unlock: > rtnl_unlock(); > } > >
From: Mike Manning <mmanning@brocade.com> Date: Wed, 3 May 2017 15:16:20 +0100 > On reflection, please put this on hold subject to testing with > syzkaller. I have not had a repro of the issue and so the fix even > though harmless may not be effective. Ok.
On Wed, May 3, 2017 at 4:16 PM, Mike Manning <mmanning@brocade.com> wrote: > On reflection, please put this on hold subject to testing with syzkaller. I have not had a repro of the issue and so the fix even though harmless may not be effective. Hi Mike, I didn't see your patch, you think you might have forgotten to add me to recipients. Did you try building the kernel with the provided config and then running the reproducer? Would you like me to test if this patch fixes the warning? Thanks! > > Thanks > Mike > > On 02/05/17 19:30, Mike Manning wrote: >> While this is not reproducible manually, Andrey's syzkaller program hit >> the warning "IPv6: Freeing alive inet6 address" with this part trace: >> >> inet6_ifa_finish_destroy+0x12e/0x190 c:894 >> in6_ifa_put ./include/net/addrconf.h:330 >> addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963 >> >> The fix is to call in6_ifa_put() for the inet6_ifaddr before rather >> than after calling addrconf_ifdown(), as the latter may remove it from >> the address hash table. >> >> Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD") >> Reported-by: Andrey Konovalov <andreyknvl@google.com> >> Signed-off-by: Mike Manning <mmanning@brocade.com> >> --- >> net/ipv6/addrconf.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 80ce478..361993a 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w) >> } else if (action == DAD_ABORT) { >> in6_ifa_hold(ifp); >> addrconf_dad_stop(ifp, 1); >> - if (disable_ipv6) >> + if (disable_ipv6) { >> + in6_ifa_put(ifp); >> addrconf_ifdown(idev->dev, 0); >> + goto unlock; >> + } >> goto out; >> } >> >> @@ -3950,6 +3953,7 @@ static void addrconf_dad_work(struct work_struct *w) >> ifp->dad_nonce); >> out: >> in6_ifa_put(ifp); >> +unlock: >> rtnl_unlock(); >> } >> >> >
On Tue, May 2, 2017 at 11:30 AM, Mike Manning <mmanning@brocade.com> wrote: > While this is not reproducible manually, Andrey's syzkaller program hit > the warning "IPv6: Freeing alive inet6 address" with this part trace: > > inet6_ifa_finish_destroy+0x12e/0x190 c:894 > in6_ifa_put ./include/net/addrconf.h:330 > addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963 > > The fix is to call in6_ifa_put() for the inet6_ifaddr before rather > than after calling addrconf_ifdown(), as the latter may remove it from > the address hash table. > > Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD") > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Signed-off-by: Mike Manning <mmanning@brocade.com> > --- > net/ipv6/addrconf.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 80ce478..361993a 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w) > } else if (action == DAD_ABORT) { > in6_ifa_hold(ifp); > addrconf_dad_stop(ifp, 1); > - if (disable_ipv6) > + if (disable_ipv6) { > + in6_ifa_put(ifp); > addrconf_ifdown(idev->dev, 0); > + goto unlock; > + } But addrconf_dad_stop() calls ipv6_del_addr() which could unhash the addr too...
On 03/05/17 18:58, Cong Wang wrote: > On Tue, May 2, 2017 at 11:30 AM, Mike Manning <mmanning@brocade.com> wrote: >> While this is not reproducible manually, Andrey's syzkaller program hit >> the warning "IPv6: Freeing alive inet6 address" with this part trace: >> >> inet6_ifa_finish_destroy+0x12e/0x190 c:894 >> in6_ifa_put ./include/net/addrconf.h:330 >> addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963 >> >> The fix is to call in6_ifa_put() for the inet6_ifaddr before rather >> than after calling addrconf_ifdown(), as the latter may remove it from >> the address hash table. >> >> Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD") >> Reported-by: Andrey Konovalov <andreyknvl@google.com> >> Signed-off-by: Mike Manning <mmanning@brocade.com> >> --- >> net/ipv6/addrconf.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 80ce478..361993a 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w) >> } else if (action == DAD_ABORT) { >> in6_ifa_hold(ifp); >> addrconf_dad_stop(ifp, 1); >> - if (disable_ipv6) >> + if (disable_ipv6) { >> + in6_ifa_put(ifp); >> addrconf_ifdown(idev->dev, 0); >> + goto unlock; >> + } > > > But addrconf_dad_stop() calls ipv6_del_addr() which could unhash > the addr too... > Agreed, and in the mean time Andrey has confirmed that this v1 patch does not resolve the issue. The problem is not specific to my change for removing addresses. It seems that generally here the in6_ifa_hold() and matching in6_ifa_put() are surplus to requirement, as the address refcnt is 2 even without the hold before calling DAD stop.
On 03/05/17 19:24, Mike Manning wrote: > On 03/05/17 18:58, Cong Wang wrote: >> On Tue, May 2, 2017 at 11:30 AM, Mike Manning <mmanning@brocade.com> wrote: >>> While this is not reproducible manually, Andrey's syzkaller program hit >>> the warning "IPv6: Freeing alive inet6 address" with this part trace: >>> >>> inet6_ifa_finish_destroy+0x12e/0x190 c:894 >>> in6_ifa_put ./include/net/addrconf.h:330 >>> addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963 >>> >>> The fix is to call in6_ifa_put() for the inet6_ifaddr before rather >>> than after calling addrconf_ifdown(), as the latter may remove it from >>> the address hash table. >>> >>> Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD") >>> Reported-by: Andrey Konovalov <andreyknvl@google.com> >>> Signed-off-by: Mike Manning <mmanning@brocade.com> >>> --- >>> net/ipv6/addrconf.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >>> index 80ce478..361993a 100644 >>> --- a/net/ipv6/addrconf.c >>> +++ b/net/ipv6/addrconf.c >>> @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w) >>> } else if (action == DAD_ABORT) { >>> in6_ifa_hold(ifp); >>> addrconf_dad_stop(ifp, 1); >>> - if (disable_ipv6) >>> + if (disable_ipv6) { >>> + in6_ifa_put(ifp); >>> addrconf_ifdown(idev->dev, 0); >>> + goto unlock; >>> + } >> >> >> But addrconf_dad_stop() calls ipv6_del_addr() which could unhash >> the addr too... >> Further investigation shows that none of the code block above is at fault. Debugging shows that the problem is happening with DAD_BEGIN and not DAD_ABORT. Follows more detail on the issue, but as I do not have a fix at this stage, I retract this submission altogether. The problem is due to rapidly adding the same address fd00::bb on ip6tnl0, and also without running DAD (accept_dad < 1), so it's an edge case. Typically the call to addrconf_dad_work() starts with an ifp refcnt of 3. Then via addrconf_dad_begin() and addrconf_dad_completed(), the call to addrconf_del_dad_work() results in a dec of the refcnt to 2 due to the call to cancel_delayed_work() returning 1. The 2nd normal case is if the call to addrconf_dad_work() starts with an ifp refcnt of 2, in which case the call to cancel_delayed_work() returns 0 and so no decrement of the refcnt, which correctly stays at 2. The error case is when the call to addrconf_dad_work() starts with an ifp refcnt of 2, but the call to cancel_delayed_work() then also results in a dec of the refcnt to 1, so the final in6_ifa_put() detects that the refcnt is being reduced to 0 for an active address. So the question is whether the interaction of cancel_delayed_work() in addrconf_dad_work(), delayed_work_pending() in addrconf_mod_dad_work() and INIT_DELAYED_WORK in ipv6_add_addr() [along with the handling for this when deleting addresses] needs improving, and if so how?
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 80ce478..361993a 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -3902,8 +3902,11 @@ static void addrconf_dad_work(struct work_struct *w) } else if (action == DAD_ABORT) { in6_ifa_hold(ifp); addrconf_dad_stop(ifp, 1); - if (disable_ipv6) + if (disable_ipv6) { + in6_ifa_put(ifp); addrconf_ifdown(idev->dev, 0); + goto unlock; + } goto out; } @@ -3950,6 +3953,7 @@ static void addrconf_dad_work(struct work_struct *w) ifp->dad_nonce); out: in6_ifa_put(ifp); +unlock: rtnl_unlock(); }
While this is not reproducible manually, Andrey's syzkaller program hit the warning "IPv6: Freeing alive inet6 address" with this part trace: inet6_ifa_finish_destroy+0x12e/0x190 c:894 in6_ifa_put ./include/net/addrconf.h:330 addrconf_dad_work+0x4e9/0x1040 net/ipv6/addrconf.c:3963 The fix is to call in6_ifa_put() for the inet6_ifaddr before rather than after calling addrconf_ifdown(), as the latter may remove it from the address hash table. Fixes: 85b51b12115c ("net: ipv6: Remove addresses for failures with strict DAD") Reported-by: Andrey Konovalov <andreyknvl@google.com> Signed-off-by: Mike Manning <mmanning@brocade.com> --- net/ipv6/addrconf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)