Re: [PATCH net] net: ipv6: allow explicitly choosing optimistic addresses

Re: [PATCH net] net: ipv6: allow explicitly choosing optimistic addresses

[Lorenzo Colitti]

On Fri, Dec 12, 2014 at 12:50 PM, Erik Kline <ek@google.com> wrote:
>
> @@ -1527,7 +1527,8 @@ int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>                 if (!net_eq(dev_net(ifp->idev->dev), net))
>                         continue;
>                 if (ipv6_addr_equal(&ifp->addr, addr) &&
> -                   !(ifp->flags&IFA_F_TENTATIVE) &&
> +                   (!(ifp->flags&IFA_F_TENTATIVE) ||
> +                    ifp->flags&IFA_F_OPTIMISTIC) &&
>                     (dev == NULL || ifp->idev->dev == dev ||
>                      !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>                         rcu_read_unlock_bh();

I looked at the callers of ipv6_chk_addr (lxr finds 19 files); from
what I saw, this change will make all of them more correct except for
ndisc_solicit. With this change, ndisc_solicit could now send
neighbour solicitations from optimistic addresses, which is not
allowed by the RFC.

Re: [PATCH net] net: ipv6: allow explicitly choosing optimistic addresses

[Erik Kline]

On Fri, Dec 12, 2014 at 4:37 PM, Lorenzo Colitti <lorenzo@google.com> wrote:
> On Fri, Dec 12, 2014 at 12:50 PM, Erik Kline <ek@google.com> wrote:
>>
>> @@ -1527,7 +1527,8 @@ int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>>                 if (!net_eq(dev_net(ifp->idev->dev), net))
>>                         continue;
>>                 if (ipv6_addr_equal(&ifp->addr, addr) &&
>> -                   !(ifp->flags&IFA_F_TENTATIVE) &&
>> +                   (!(ifp->flags&IFA_F_TENTATIVE) ||
>> +                    ifp->flags&IFA_F_OPTIMISTIC) &&
>>                     (dev == NULL || ifp->idev->dev == dev ||
>>                      !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>>                         rcu_read_unlock_bh();
>
> I looked at the callers of ipv6_chk_addr (lxr finds 19 files); from
> what I saw, this change will make all of them more correct except for
> ndisc_solicit. With this change, ndisc_solicit could now send
> neighbour solicitations from optimistic addresses, which is not
> allowed by the RFC.

True.  I have a version that fixes this, but ended up splitting
ipv6_chk_addr into two:

    - ipv6_chk_addr (same as today), and
    - ipv6_chk_addr_and_flags (which takes a "u32 banned_flags" argument)

The ugliness of this naming is not lost on me.  I briefly tried a
version that reuses the "strict" argument to achieve the same thing,
but that doesn't seem to be quite correct in all cases either.

[PATCH net] net: ipv6: allow explicitly choosing optimistic addresses

[Erik Kline]

RFC 4429 ("Optimistic DAD") states that optimistic addresses
should be treated as deprecated addresses.  From section 2.1:

   Unless noted otherwise, components of the IPv6 protocol stack
   should treat addresses in the Optimistic state equivalently to
   those in the Deprecated state, indicating that the address is
   available for use but should not be used if another suitable
   address is available.

Optimistic addresses are indeed avoided when other addresses are
available (i.e. at source address selection time), but they have
not heretofore been available for things like explicit bind() and
sendmsg() with struct in6_pktinfo, etc.

This change makes optimistic addresses treated more like
deprecated addresses than tentative ones.

Signed-off-by: Erik Kline <ek@google.com>
---
 include/net/addrconf.h |  3 +++
 net/ipv6/addrconf.c    | 14 ++++++++++++--
 net/ipv6/ndisc.c       |  3 ++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index d13573b..80456f7 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -62,6 +62,9 @@ int addrconf_set_dstaddr(struct net *net, void __user *arg);
 
 int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 		  const struct net_device *dev, int strict);
+int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
+			    const struct net_device *dev, int strict,
+			    u32 banned_flags);
 
 #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
 int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f7c8bbe..422976b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1519,6 +1519,14 @@ static int ipv6_count_addresses(struct inet6_dev *idev)
 int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 		  const struct net_device *dev, int strict)
 {
+	return ipv6_chk_addr_and_flags(net, addr, dev, strict, 0);
+}
+EXPORT_SYMBOL(ipv6_chk_addr);
+
+int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
+			    const struct net_device *dev, int strict,
+			    u32 banned_flags)
+{
 	struct inet6_ifaddr *ifp;
 	unsigned int hash = inet6_addr_hash(addr);
 
@@ -1527,7 +1535,9 @@ int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 		if (!net_eq(dev_net(ifp->idev->dev), net))
 			continue;
 		if (ipv6_addr_equal(&ifp->addr, addr) &&
-		    !(ifp->flags&IFA_F_TENTATIVE) &&
+		    (!(ifp->flags&IFA_F_TENTATIVE) ||
+		     ifp->flags&IFA_F_OPTIMISTIC) &&
+		    !(ifp->flags&banned_flags) &&
 		    (dev == NULL || ifp->idev->dev == dev ||
 		     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
 			rcu_read_unlock_bh();
@@ -1538,7 +1548,7 @@ int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 	rcu_read_unlock_bh();
 	return 0;
 }
-EXPORT_SYMBOL(ipv6_chk_addr);
+EXPORT_SYMBOL(ipv6_chk_addr_and_flags);
 
 static bool ipv6_chk_same_addr(struct net *net, const struct in6_addr *addr,
 			       struct net_device *dev)
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 6828667..a726c7a 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -655,7 +655,8 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
 	struct in6_addr *target = (struct in6_addr *)&neigh->primary_key;
 	int probes = atomic_read(&neigh->probes);
 
-	if (skb && ipv6_chk_addr(dev_net(dev), &ipv6_hdr(skb)->saddr, dev, 1))
+	if (skb && ipv6_chk_addr_and_flags(dev_net(dev), &ipv6_hdr(skb)->saddr,
+					   dev, 1, IFA_F_OPTIMISTIC))
 		saddr = &ipv6_hdr(skb)->saddr;
 	probes -= NEIGH_VAR(neigh->parms, UCAST_PROBES);
 	if (probes < 0) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net] net: ipv6: allow explicitly choosing optimistic addresses

[Hannes Frederic Sowa]

Hi,

On Wed, Jan 21, 2015, at 08:02, Erik Kline wrote:
>  		if (ipv6_addr_equal(&ifp->addr, addr) &&
> -                   !(ifp->flags&IFA_F_TENTATIVE) &&
> +                   (!(ifp->flags&IFA_F_TENTATIVE) ||
> +                    ifp->flags&IFA_F_OPTIMISTIC) &&
> +                   !(ifp->flags&banned_flags) &&
>  		    (dev == NULL || ifp->idev->dev == dev ||

Can the wrapper take the IFA_F_TENTATIVE and IFA_F_OPTIMISTIC into the
banned_flags argument, so this condition becomes easier to read? The new
caller could also specify them verbatim. I think it would improve
readability.

Thanks,
Hannes

Re: [PATCH net] net: ipv6: allow explicitly choosing optimistic addresses

[Erik Kline]

On Sun, Jan 25, 2015 at 9:46 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi,
>
> On Wed, Jan 21, 2015, at 08:02, Erik Kline wrote:
>>               if (ipv6_addr_equal(&ifp->addr, addr) &&
>> -                   !(ifp->flags&IFA_F_TENTATIVE) &&
>> +                   (!(ifp->flags&IFA_F_TENTATIVE) ||
>> +                    ifp->flags&IFA_F_OPTIMISTIC) &&
>> +                   !(ifp->flags&banned_flags) &&
>>                   (dev == NULL || ifp->idev->dev == dev ||
>
> Can the wrapper take the IFA_F_TENTATIVE and IFA_F_OPTIMISTIC into the
> banned_flags argument, so this condition becomes easier to read? The new
> caller could also specify them verbatim. I think it would improve
> readability.

So I did a bit of fiddling (and some fiddling of bits) and I've got a
working version with your suggestion.

Note that because we use 2 flags to represent optimistic state we
still have a bit of complication in order to check whether or not
optimistic is explicitly banned or not (as opposed to accidentally
banned by virtue of its pairing with tentative).

Thanks,
-Erik