Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC addresses

[Sabrina Dubroca <sd@queasysnail.net>]

2018-02-20, 10:25:41 -0700, David Ahern wrote:
> On 2/20/18 9:43 AM, Sabrina Dubroca wrote:
> > According to RFC 4429 (section 3.1), adding new IPv6 addresses as
> > optimistic addresses is acceptable, as long as the implementation
> > follows some rules:
> > 
> >    * Optimistic DAD SHOULD only be used when the implementation is aware
> >         that the address is based on a most likely unique interface
> >         identifier (such as in [RFC2464]), generated randomly [RFC3041],
> >         or by a well-distributed hash function [RFC3972] or assigned by
> >         Dynamic Host Configuration Protocol for IPv6 (DHCPv6) [RFC3315].
> >         Optimistic DAD SHOULD NOT be used for manually entered
> >         addresses.
> 
> That last line suggests this patch should not be allowed.

I think it should. Some tools perform autoconfiguration in userspace,
why should the kernel prevent them from requesting optimistic DAD?

If the administrator decides to enable optimistic DAD on
poorly-choosen addresses, or to disable DAD entirely, that's their
problem.


> But if it is ...
> 
> > 
> > Thus, it seems reasonable to allow userspace to set the optimistic flag
> > when adding new addresses.
> > 
> > We must not let userspace set NODAD + OPTIMISTIC, since if the kernel is
> > not performing DAD we would never clear the optimistic flag. We must
> > also ignore userspace's request to add OPTIMISTIC flag to addresses that
> > have already completed DAD.
> > 
> > Then we also need to clear the OPTIMISTIC flag on permanent addresses
> > when DAD fails. Otherwise, IFA_F_OPTIMISTIC addresses added by userspace
> > can still be used after DAD has failed, because in
> > ipv6_chk_addr_and_flags(), IFA_F_OPTIMISTIC overrides IFA_F_TENTATIVE.
> > 
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> >  net/ipv6/addrconf.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 4facfe0b1888..652285bae801 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -1968,6 +1968,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp, int dad_failed)
> >  		spin_lock_bh(&ifp->lock);
> >  		addrconf_del_dad_work(ifp);
> >  		ifp->flags |= IFA_F_TENTATIVE;
> > +		ifp->flags &= ~IFA_F_OPTIMISTIC;
> >  		spin_unlock_bh(&ifp->lock);
> >  		if (dad_failed)
> >  			ipv6_ifa_notify(0, ifp);
> > @@ -4501,6 +4502,9 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
> >  	    (ifp->flags & IFA_F_TEMPORARY || ifp->prefix_len != 64))
> >  		return -EINVAL;
> >  
> > +	if (!(ifp->flags & (IFA_F_TENTATIVE | IFA_F_DADFAILED)))
> > +		ifa_flags &= ~IFA_F_OPTIMISTIC;
> > +
> >  	timeout = addrconf_timeout_fixup(valid_lft, HZ);
> >  	if (addrconf_finite_timeout(timeout)) {
> >  		expires = jiffies_to_clock_t(timeout * HZ);
> > @@ -4607,7 +4611,10 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  
> >  	/* We ignore other flags so far. */
> >  	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
> > -		     IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN;
> > +		     IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN | IFA_F_OPTIMISTIC;
> > +
> > +	if (ifa_flags & IFA_F_NODAD && ifa_flags & IFA_F_OPTIMISTIC)
> > +		return -EINVAL;
> 
> ... add an extack message telling users nodad and optimistic are
> mutually exclusive.

Ok, I'll do that in v2.


> Also, it seems like this feature needs to be wrapped in
> CONFIG_IPV6_OPTIMISTIC_DAD and optimistic checks for linklocal and
> autoconf are wrapped in sysctl checks. Why shouldn't manual addresses
> follow suit?

Good catch, they should, will fix in v2.

And while we're talking about CONFIG_IPV6_OPTIMISTIC_DAD: it has been
labeled "experimental" for almost 11 years, maybe it's time to drop
the label?  Most distributions seem to enable it despite the
"If unsure, say N." advice in Kconfig.

Thanks for the comments.

-- 
Sabrina