Re: [WireGuard] Source address fib invalidation on IPv6

[Hannes Frederic Sowa]

On Sun, Nov 13, 2016, at 01:43, Jason A. Donenfeld wrote:
> On Sat, Nov 12, 2016 at 8:08 PM, Jason A. Donenfeld 
> wrote:
> >> Gotcha. I don't see any checks that the saddr is valid similar to what 
> >> IPv4 does.
> >>
> >> I think the right place to add a check is in ip6_dst_lookup_tail():
> >> if (!ipv6_addr_any(>saddr)) {
> >> // saddr is valid for L3 domain
> >> }
> >
> > Right. It should probably do the check here, and return
> > ERR_PTR(-EINVAL), the same as the v4 version, so that ret codes can be
> > checked consistently.
> >
> > Thanks for looking into this. If you're backed up and would like me to
> > submit a patch, just let me know, and I'll give it my best shot.
> 
> In perusing through the v6 FIB code, I don't even see an analog of
> __ip_dev_find... Hm?

You probably need some combination of ipv6_chk_addr and/or
ipv6_check_addr_and_flags (where dev can also be NULL). Be careful if a
IFA_HOST or IFA_LINK address switches from one interface to another.

Bye,
Hannes
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [WireGuard] Source address fib invalidation on IPv6

[Jason A. Donenfeld]

On Sun, Nov 13, 2016 at 1:43 AM, Jason A. Donenfeld  wrote:
> In perusing through the v6 FIB code, I don't even see an analog of
> __ip_dev_find... Hm?

Of all places, the iscsi code actually has a nice side-by-side
comparison. So far as I can see, the other protocols just omit this
check in the v6 case, which I believe to be errant behavior. For
example, grep for ip_dev_find in the sctp v4 code. The equivalent v6
code is missing the dev check. Ugly! Here's the block I found in
cxgbit_cm.c:

static struct net_device *cxgbit_ipv4_netdev(__be32 saddr)
{
   struct net_device *ndev;

   ndev = __ip_dev_find(_net, saddr, false);
   if (!ndev)
   return NULL;

   return cxgbit_get_real_dev(ndev);
}

static struct net_device *cxgbit_ipv6_netdev(struct in6_addr *addr6)
{
   struct net_device *ndev = NULL;
   bool found = false;

   if (IS_ENABLED(CONFIG_IPV6)) {
   for_each_netdev_rcu(_net, ndev)
   if (ipv6_chk_addr(_net, addr6, ndev, 1)) {
   found = true;
   break;
   }
   }
   if (!found)
   return NULL;
   return cxgbit_get_real_dev(ndev);
}

It seems like __ip6_dev_find could be made out of that inner loop.
Then existing uses like that iscsi code can be replaced with that
helper function, and the existing ip6 route tail function can be
augmented in the manner you recommended. Seem like a decent
implementation strategy?

I might submit some patches, unless you beat me to it.

Jason
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [WireGuard] Source address fib invalidation on IPv6

[Jason A. Donenfeld]

Hi David,

On Sat, Nov 12, 2016 at 7:14 PM, David Ahern  wrote:
> I believe that is coming from __ip_route_output_key_hash(), line 2232 with 
> __ip_dev_find not finding a device with that address.

It's possible we simply are looking at different source trees, but I
have the -EINVAL return in 4.8 route.c sources happening due to the
assignment on line 2175 and the jump on line 2220.

> Not applicable for your use case, but __ip_dev_find does not have any checks 
> on which L3 domain the device belongs to so the check does not handle VRF for 
> example. I'll take a look at fixing this next week.

Interesting.

>
> Gotcha. I don't see any checks that the saddr is valid similar to what IPv4 
> does.
>
> I think the right place to add a check is in ip6_dst_lookup_tail():
> if (!ipv6_addr_any(>saddr)) {
> // saddr is valid for L3 domain
> }

Right. It should probably do the check here, and return
ERR_PTR(-EINVAL), the same as the v4 version, so that ret codes can be
checked consistently.

Thanks for looking into this. If you're backed up and would like me to
submit a patch, just let me know, and I'll give it my best shot.

Regards,
Jason
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [WireGuard] Source address fib invalidation on IPv6

[Jason A. Donenfeld]

Hi David,

On Fri, Nov 11, 2016 at 11:14 PM, David Ahern  wrote:
> What do you mean by 'valid dst'? ipv6 returns net->ipv6.ip6_null_entry on 
> lookup failures so yes dst is non-NULL but that does not mean the lookup 
> succeeded.

What I mean is that it returns an ordinary dst, as if that souce
address _hadn't_ been removed from the interface, even though I just
removed it. Is this buggy behavior? If so, let me know and I'll try to
track it down. The expected behavior, as far as I can see, would be
the same that ip_route_output_flow has -- returning -EINVAL when the
saddr isn't valid. At the moment, when the saddr is invalid,
ipv6_stub->ipv6_dst_lookup returns 0 and  contains a real entry.

Regards,
Jason
___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard

Re: [WireGuard] Source address fib invalidation on IPv6

[David Ahern]

On 11/11/16 12:29 PM, Jason A. Donenfeld wrote:
> Hi folks,
> 
> If I'm replying to a UDP packet, I generally want to use a source
> address that's the same as the destination address of the packet to
> which I'm replying. For example:
> 
> Peer A sends packet: src = 10.0.0.1,  dst = 10.0.0.3
> Peer B replies with: src = 10.0.0.3, dst = 10.0.0.1
> 
> But let's complicate things. Let's say Peer B has multiple IPs on an
> interface: 10.0.0.2, 10.0.0.3. The default route uses 10.0.0.2. In
> this case what do you think should happen?
> 
> Case 1:
> Peer A sends packet: src = 10.0.0.1,  dst = 10.0.0.3
> Peer B replies with: src = 10.0.0.2, dst = 10.0.0.1
> 
> Case 2:
> Peer A sends packet: src = 10.0.0.1,  dst = 10.0.0.3
> Peer B replies with: src = 10.0.0.3, dst = 10.0.0.1
> 
> Intuition tells me the answer is "Case 2". If you agree, keep reading.
> If you disagree, stop reading here, and instead correct my poor
> intuition.
> 
> So, assuming "Case 2", when Peer B receives the first packet, he notes
> that packet's destination address, so that he can use it as a source
> address next. When replying, Peer B sets the stored source address and
> calls the routing function:
> 
> struct flowi4 fl = {
>.saddr = from_daddr_of_previous_packet,
>.daddr = from_saddr_of_previous_packet,
> };
> rt = ip_route_output_flow(sock_net(sock), , sock);
> 
> What if, however, by the time Peer B chooses to reply, his interface
> no longer has that source address? No problem, because
> ip_route_output_flow will return -EINVAL in that case. So, we can do
> this:
> 
> struct flowi4 fl = {
>.saddr = from_daddr_of_previous_packet,
>.daddr = from_saddr_of_previous_packet,
> };
> rt = ip_route_output_flow(sock_net(sock), , sock);
> if (unlikely(IS_ERR(rt))) {
> fl.saddr = 0;
> rt = ip_route_output_flow(sock_net(sock), , sock);
> }
> 
> And then all is good in the neighborhood. This solution works. Done.
> 
> But what about IPv6? That's where we get into trouble:
> 
> struct flowi6 fl = {
>.saddr = from_daddr_of_previous_packet,
>.daddr = from_saddr_of_previous_packet,
> };
> ret = ipv6_stub->ipv6_dst_lookup(sock_net(sock), sock, , );
> 
> In this case, IPv6 returns a valid dst, when no interface has the
> source address anymore! So, there's no way to know whether or not the
> source address for replying has gone stale. We don't have a means of
> falling back to inaddr_any for the source address.

What do you mean by 'valid dst'? ipv6 returns net->ipv6.ip6_null_entry on 
lookup failures so yes dst is non-NULL but that does not mean the lookup 
succeeded.

For example take a look at ip6_dst_lookup_tail():
if (!*dst)
*dst = ip6_route_output_flags(net, sk, fl6, flags);

err = (*dst)->error;
if (err)
goto out_err_release;


perhaps I should add dst->error to the fib tracepoints ...

> 
> Primary question: is this behavior a bug? Or is this some consequence
> of a fundamental IPv6 difference with v4? Or is something else
> happening here?
> 
> Thanks,
> Jason
> 

___
WireGuard mailing list
WireGuard@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/wireguard