Re: [PATCH] ipv6: Reparent temporary address(es) if global address was deleted from userspace

[Heiner Kallweit <heiner.kallweit@web.de>]

Am 06.04.2014 22:27, schrieb Daniel Borkmann:
> On 04/06/2014 09:04 PM, Heiner Kallweit wrote:
>> If the kernel takes care of global and temporary addresses it can happen that the global
>> address is deleted from userspace, e.g. by network managers in case the link goes (temporarily) down.
>> In addition to the then orphaned temporary address(es) the next RA will create a new temporary address.
>> Therefore we might end up with more than one non-deprecated temporary address for the same prefix
>> for a longer period of time (>regen_advance). According to RfC 4941 sect. 3.4 this should not happen.
>
> Nit: your commit description should only be ~75 chars wide max
>
>> Fix this by reparenting orphaned temporary addresses.
>>
>> Signed-off-by: Heiner Kallweit <heiner.kallweit@web.de>
>> ---
>>   net/ipv6/addrconf.c | 34 +++++++++++++++++++++++++++++++---
>>   1 file changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 6c7fa08..4d5595b 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -2077,14 +2077,34 @@ static void manage_tempaddrs(struct inet6_dev *idev,
>>   {
>>       u32 flags;
>>       struct inet6_ifaddr *ift;
>> +    unsigned long regen_advance;
>>
>>       read_lock_bh(&idev->lock);
>> +    regen_advance = idev->cnf.regen_max_retry *
>> +            idev->cnf.dad_transmits *
>> +            NEIGH_VAR(idev->nd_parms, RETRANS_TIME) / HZ;
>>       /* update all temporary addresses in the list */
>>       list_for_each_entry(ift, &idev->tempaddr_list, tmp_list) {
>>           int age, max_valid, max_prefered;
>> +        bool reparent;
>>
>> -        if (ifp != ift->ifpub)
>> -            continue;
>> +        reparent = false;
>
> You can collapse initialization where you declare the variable.
>
>> +        spin_lock(&ift->lock);
>> +        if (ifp != ift->ifpub) {
>> +            /* reparent if orphaned temporary address with same
>> +             * prefix is found
>> +             */
>> +            if (ipv6_prefix_equal(&ift->addr, &ifp->addr,
>> +                          ift->prefix_len)) {
>> +                in6_ifa_hold(ifp);
>> +                in6_ifa_put(ift->ifpub);
>> +                ift->ifpub = ifp;
>> +                reparent = true;
>> +            } else {
>> +                spin_unlock(&ift->lock);
>> +                continue;
>> +            }
>> +        }
>>
>>           /* RFC 4941 section 3.3:
>>            * If a received option will extend the lifetime of a public
>> @@ -2110,7 +2130,6 @@ static void manage_tempaddrs(struct inet6_dev *idev,
>>           if (prefered_lft > max_prefered)
>>               prefered_lft = max_prefered;
>>
>> -        spin_lock(&ift->lock);
>>           flags = ift->flags;
>>           ift->valid_lft = valid_lft;
>>           ift->prefered_lft = prefered_lft;
>> @@ -2119,6 +2138,15 @@ static void manage_tempaddrs(struct inet6_dev *idev,
>>               ift->flags &= ~IFA_F_DEPRECATED;
>>
>>           spin_unlock(&ift->lock);
>> +        /* Don't create a temporary address if we reparented one which
>> +         * is prefered more than regen_advance
>
> Nit: s/prefered/preferred/
>
>> +         */
>> +        if (reparent && prefered_lft > regen_advance) {
>> +            create = false;
>> +            pr_info("%s: reparent orphaned temporary address\n",
>> +                __func__);
>
> The pr_info() should ideally be removed (or at least made pr_debug()).
>
>> +        }
>> +
>>           if (!(flags&IFA_F_TENTATIVE))
>>               ipv6_ifa_notify(0, ift);
>>       }
>>
>
Thanks for the hints, will send a revised patch.
Rgds, Heiner