Re: [RFC PATCH v2 net-next] net: ipv6: Add a sysctl to make optimistic addresses useful candidates

[Hannes Frederic Sowa <hannes@stressinduktion.org>]

Hi,

On Mi, 2014-10-22 at 20:15 +0900, Erik Kline wrote:
> >> >>       case IPV6_SADDR_RULE_PREFIX:
> >> >>               /* Rule 8: Use longest matching prefix */
> >> >>               ret = ipv6_addr_diff(&score->ifa->addr, dst->addr);
> >> >> @@ -3222,8 +3240,13 @@ static void addrconf_dad_begin(struct inet6_ifaddr *ifp)
> >> >>        * Optimistic nodes can start receiving
> >> >>        * Frames right away
> >> >>        */
> >> >> -     if (ifp->flags & IFA_F_OPTIMISTIC)
> >> >> +     if (ifp->flags & IFA_F_OPTIMISTIC) {
> >> >>               ip6_ins_rt(ifp->rt);
> >> >> +             /* Because optimistic nodes can receive frames, notify
> >> >> +              * listeners. If DAD fails, RTM_DELADDR is sent.
> >> >> +              */
> >> >> +             ipv6_ifa_notify(RTM_NEWADDR, ifp);
> >> >> +     }
> >> >
> >> > I wonder if we can now delete the ipv6_ifa_notify(RTM_NEWADDR, ifp) in
> >> > addrconf_dad_completed.
> >>
> >> I don't know what everyone's general preference would be, but mine
> >> would be to err on the side of notifying on state changes.  It seems
> >> harmless to me to keep it in, and something in userspace might want to
> >> know if/when DAD completes.
> >
> > Userspace expects to communicate with an address which gets announced
> > via RTM_NEWADDR, so I would make the RTM_NEWADDR notifications
> > conditional on use_optimistic flag in both, the completed and the
> > dad_begin function. This sounds like the best option to me, no?
> 
> That's easy enough to do in addrconf_dad_begin().  Unfortunately, by
> the time we get to addrconf_dad_completed() the IFA_F_OPTIMISTIC flag
> has already been cleared.
> 
> I have a version that attempts to take its best guess as to whether an
> RTM_NEWADDR _should_ have already been sent--something like:
> 
> #ifdef CONFIG_IPV6_OPTIMISTIC_DAD
>         // We probably already sent a notification in addrconf_dad_begin().
>         if (!ifp->idev->cnf.optimistic_dad || !ifp->idev->cnf.use_optimistic)
> #endif
>         ipv6_ifa_notify(RTM_NEWADDR, ifp);
> 
> but that doesn't seem that clean to me (apart from the ifdef-y
> messiness of it).  Do you think this "best guess" approach is
> reasonable?

I would definitely ack a patch which removes the macros and the config
option CONFIG_IPV6_OPTIMISTIC_DAD completely.

You also can split the ifdefed part into a small helper function:

static inline bool ipv6_use_optimistic_addr(struct inet6_dev *idev)
{
#ifdef CONFIG_IPV6_OPTIMISTIC_DAD
        return idev->cnf.optimistic_dad && idev->cnf.use_optimistic;
#else
        return false;
#endif
}

...

and then in both locations update the RTM_NEWADDR like this:

addrconf_dad_begin:

if (ipv6_use_optimisitic_addr(idev))
   notify...

and the dad_completed with a negated check. I think this is the easiest
option.

> With just the "use_optimistic" check in addrconf_dad_begin(),
> userspace can still communicate with addresses it gets via
> RTM_NEWADDR, it will just get /two/ such notifications: one when it
> can probably use it and one when it definitely can.
> 
> Thoughts?

In the near future we also must introduce specific DAD events needed for
RFC7217 addresses (to count DAD progress and safe the information per
prefix in user space). I would prefer to keep the logic for RTM_NEWADDR
that the kernel will definitely allow the use of the address but
RTM_NEWADDR should be handled idempotent by user space.

Thanks,
Hannes