netdev - Re: The mystery of optimistic ipv6 DAD handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Dec 2011 14:53:05 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	netdev@...r.kernel.org
Cc:	nhorman@...driver.com
Subject: Re: The mystery of optimistic ipv6 DAD handling

From: David Miller <davem@...emloft.net>
Date: Tue, 27 Dec 2011 14:06:52 -0500 (EST)

> However, two things strike me:
> 
> 1) even beforehand we'd only get a NULL neigh when the device pointer
>    is NULL, that can't happen here, rt->rt6i_dev will be non-NULL always
>    in these circumstances, it's forced to be net->loopback_dev by the
>    ip6_dst_alloc() call made by addrconf_dst_alloc() (the code back
>    in 2.6.21 does the same exact thing, just inline)
> 
> 2) on top of that it's looking up a neighbour using rt->rt6i_gateway
>    as the key, that's either bogus or pointless.  As far as I can tell
>    it's uninitialized at this point and therefore always all-zeros.
> 
> My quick hunch is that the neigh should be looked up based upon 'addr', and
> that ipv6_add_addr() should test if the neigh is resolved in order to determine
> if the optimistic flag should be cleared.  That matches the logic in the comment
> "do not yet know the link layer address of our nexthop router."
> 
> Neil, any chance you can help me unravel this?

So the fundamental thing is that these addrconf routes are mainly for
for input and looping back traffic on output.

We create the addrconf route "to us", for input.  That's why the lookup
key is the ipv6 address we are configuring on the interface.

For output, the neigh is "don't care".  It goes to the loopback device
and for loopback ->hard_header is NULL so
net/ipv6/ndisc.c:ndisc_constructor() directly hooks up
neigh->ops->queue_xmit as the neigh->output method, which is
dev_queue_xmit().  So it doesn't matter that we lookup the neigh in
addrconf_dst_alloc() using the all-zeros rt->rt6i_gateway.

So this route and it's neigh have nothing to do with link layer
address of any nexthop gatway(s) in this interface.

Therefore the test is completely wrong, and all I can determine is
that we should flat out remove it.  It never triggers anyways.

Perhaps we should find another appropriate spot to put this kind of
test, but in this spot and against this route object seems totally not
right.

Neil, what say you?

--------------------
ipv6: Remove optimistic DAD flag test in ipv6_add_addr()

The route we have here is for the address being added to the interface,
ie. for input packet processing.

Therefore using that route to determine whether an output nexthop gateway
is known and resolved doesn't make any sense.

So, simply remove this test, it never triggered anyways.

Signed-off-by: David S. Miller <davem@...emloft.net>

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 59a9d0e..85421cc 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -650,16 +650,6 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr, int pfxlen,

 	ifa->rt = rt;

-	/*
-	 * part one of RFC 4429, section 3.3
-	 * We should not configure an address as
-	 * optimistic if we do not yet know the link
-	 * layer address of our nexhop router
-	 */
-
-	if (dst_get_neighbour_noref_raw(&rt->dst) == NULL)
-		ifa->flags &= ~IFA_F_OPTIMISTIC;
-
 	ifa->idev = idev;
 	in6_dev_hold(idev);
 	/* For caller */
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html