On Sun, Oct 15, 2017 at 09:24:07AM -0600, David Ahern wrote:
> On 10/15/17 1:50 AM, Ido Schimmel wrote:
> > On Fri, Oct 13, 2017 at 04:02:09PM -0700, David Ahern wrote:
> >> ipv6_add_addr is called in process context with rtnl lock held
> >> (e.g., manual config of an address) or during softirq processing
> >> (e.g., autoconf and address from a router advertisement).
> >>
> >> Currently, ipv6_add_addr calls rcu_read_lock_bh shortly after entry
> >> and does not call unlock until exit, minus the call around the address
> >> validator notifier. Similarly, addrconf_hash_lock is taken after the
> >> validator notifier and held until exit. This forces the allocation of
> >> inet6_ifaddr to always be atomic.
> >>
> >> Refactor ipv6_add_addr as follows:
> >> 1. add an input boolean to discriminate the call path (process context
> >> or softirq). This new flag controls whether the alloc can be done
> >> with GFP_KERNEL or GFP_ATOMIC.
> >>
> >> 2. Move the rcu_read_lock_bh and unlock calls only around functions that
> >> do rcu updates.
> >>
> >> 3. Remove the in6_dev_hold and put added by 3ad7d2468f79f ("Ipvlan should
> >> return an error when an address is already in use."). This was done
> >> presumably because rcu_read_unlock_bh needs to be called before calling
> >> the validator. Since rcu_read_lock is not needed before the validator
> >> runs revert the hold and put added by 3ad7d2468f79f and only do the
> >> hold when setting ifp->idev.
> >>
> >> 4. move duplicate address check and insertion of new address in the global
> >> address hash into a helper. The helper is called after an ifa is
> >> allocated and filled in.
> >>
> >> This allows the ifa for manually configured addresses to be done with
> >> GFP_KERNEL and reduces the overall amount of time with rcu_read_lock held
> >> and hash table spinlock held.
> >>
> >> Signed-off-by: David Ahern <[email protected]>
> >
> > [...]
> >
> >> @@ -1073,21 +1085,19 @@ ipv6_add_addr(struct inet6_dev *idev, const struct
> >> in6_addr *addr,
> >>
> >> in6_ifa_hold(ifa);
> >> write_unlock(&idev->lock);
> >> -out2:
> >> +
> >> rcu_read_unlock_bh();
> >>
> >> - if (likely(err == 0))
> >> - inet6addr_notifier_call_chain(NETDEV_UP, ifa);
> >> - else {
> >> + inet6addr_notifier_call_chain(NETDEV_UP, ifa);
> >> +out:
> >> + if (unlikely(err < 0)) {
> >> + if (rt)
> >> + ip6_rt_put(rt);
> >
> > I believe 'rt' needs to be set to NULL after addrconf_dst_alloc()
> > fails.
>
> The above frees rt and the line below frees the ifa and resets the value
> to an error, so after the line above rt is no longer referenced.
Earlier in the code we have:
rt = addrconf_dst_alloc(idev, addr, false);
if (IS_ERR(rt)) {
err = PTR_ERR(rt);
goto out;
}
So we end up calling ip6_rt_put() with an error value. I believe it
should be:
rt = addrconf_dst_alloc(idev, addr, false);
if (IS_ERR(rt)) {
err = PTR_ERR(rt);
rt = NULL;
goto out;
}
