[PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[Mike Manning]

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();
 }
 
-- 
2.1.4

Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[Mike Manning]

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();
>  }
>  
> 

Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[David Miller]

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.

Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[Andrey Konovalov]

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();
>>  }
>>
>>
>

Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[Cong Wang]

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

Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[Mike Manning]

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.

Re: [PATCH] net: ipv6: Fix warning of freeing alive inet6 address

[Mike Manning]

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?