[PATCH net] ipv6: set lwtstate for ns and na dst

[PATCH net] ipv6: set lwtstate for ns and na dst

[Xin Long]

This patch is to fix an issue that ipv6 ns and na can't go out with the
correct tunnel info, and it can be reproduced by:

  # ip net a a; ip net a b
  # ip -n a l a eth0 type veth peer name eth0 netns b
  # ip -n a l s eth0 up; ip -n b link set eth0 up
  # ip -n a a a 10.1.0.1/24 dev eth0; ip -n b a a 10.1.0.2/24 dev eth0
  # ip -n b l a vxlan1 type vxlan id 1 local 10.1.0.2 remote 10.1.0.1 \
      dstport 0 dev eth0 ttl 64
  # ip -n b a a 1000::1/24 dev vxlan1; ip -n b l s vxlan1 up
  # ip -n b r a 2000::/24 dev vxlan1; sleep 3
  # ip -n a l a vxlan1 type vxlan local 10.1.0.1 dev eth0 ttl 64 \
      dstport 0 external
  # ip -n a a a 2000::1/24 dev vxlan1; ip -n a l s vxlan1 up
  # ip -n a r a 1000::/24 encap ip id 1 dst 10.1.0.2 dev vxlan1; sleep 3
  # ip net exec a ping6 1000::1 -c 1

ping6 doesn't work, as ns and na are icmpv6 packets which don't look up
a dst, but build a dst on its own. It will cause the lwstate to be lost
and the packets can never go out with the correct tunnel info.

Also unlike arp packets process where arp_request will get dst from the
data packet and arp_reply will get dst->lwstate from peer arp_request,
now ns doesn't get dst from the data and na can't get dst->lwstata as
peer ns packet lwstate will be lost after doing input route.

Since ns and na are ipv6 packets, a proper fix is to look up a dst and
set its lwstate to the new allocated dst.

Fixes: 19e42e451506 ("ipv6: support for fib route lwtunnel encap attributes")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv6/ndisc.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 53caf59..963172d96 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -517,6 +517,9 @@ void ndisc_send_na(struct net_device *dev, const struct in6_addr *daddr,
 		   const struct in6_addr *solicited_addr,
 		   bool router, bool solicited, bool override, bool inc_opt)
 {
+	struct sock *sk = dev_net(dev)->ipv6.ndisc_sk;
+	struct dst_entry *dst, *dst_orig;
+	struct flowi6 fl6;
 	struct sk_buff *skb;
 	struct in6_addr tmpaddr;
 	struct inet6_ifaddr *ifp;
@@ -566,6 +569,20 @@ void ndisc_send_na(struct net_device *dev, const struct in6_addr *daddr,
 				       dev->dev_addr,
 				       NDISC_NEIGHBOUR_ADVERTISEMENT);
 
+	icmpv6_flow_init(sk, &fl6, msg->icmph.icmp6_type, src_addr,
+			 daddr, dev->ifindex);
+	dst = icmp6_dst_alloc(dev, &fl6);
+	if (IS_ERR(dst)) {
+		kfree_skb(skb);
+		return;
+	}
+
+	dst_orig = ip6_route_output(dev_net(dev), NULL, &fl6);
+	if (!dst_orig->error)
+		dst->lwtstate = lwtstate_get(dst_orig->lwtstate);
+	dst_release(dst_orig);
+
+	skb_dst_set(skb, dst);
 	ndisc_send_skb(skb, daddr, src_addr);
 }
 
@@ -599,6 +616,9 @@ void ndisc_send_ns(struct net_device *dev, const struct in6_addr *solicit,
 		   const struct in6_addr *daddr, const struct in6_addr *saddr,
 		   u64 nonce)
 {
+	struct sock *sk = dev_net(dev)->ipv6.ndisc_sk;
+	struct dst_entry *dst, *dst_orig;
+	struct flowi6 fl6;
 	struct sk_buff *skb;
 	struct in6_addr addr_buf;
 	int inc_opt = dev->addr_len;
@@ -644,6 +664,21 @@ void ndisc_send_ns(struct net_device *dev, const struct in6_addr *solicit,
 		memcpy(opt + 2, &nonce, 6);
 	}
 
+	icmpv6_flow_init(sk, &fl6, msg->icmph.icmp6_type, saddr,
+			 daddr, dev->ifindex);
+	dst = icmp6_dst_alloc(dev, &fl6);
+	if (IS_ERR(dst)) {
+		kfree_skb(skb);
+		return;
+	}
+
+	fl6.daddr = *solicit;
+	dst_orig = ip6_route_output(dev_net(dev), NULL, &fl6);
+	if (!dst_orig->error)
+		dst->lwtstate = lwtstate_get(dst_orig->lwtstate);
+	dst_release(dst_orig);
+
+	skb_dst_set(skb, dst);
 	ndisc_send_skb(skb, daddr, saddr);
 }
 
-- 
2.1.0

Re: [PATCH net] ipv6: set lwtstate for ns and na dst

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 25 Nov 2019 14:23:12 +0800

> This patch is to fix an issue that ipv6 ns and na can't go out with the
> correct tunnel info, and it can be reproduced by:

And why shouldn't RS and redirects get this treatment too?

And then, at that point, all callers of ndisc_send_skb() have this
early route lookup code (and thus the "!dst" code path is unused), and
the question ultimately becomes why doesn't ndisc_send_skb() itself
have the dst lookup modification?

I'm not too sure about this change and will not apply it.

Re: [PATCH net] ipv6: set lwtstate for ns and na dst

[Xin Long]

On Tue, Nov 26, 2019 at 6:53 AM David Miller <davem@davemloft.net> wrote:
>
> From: Xin Long <lucien.xin@gmail.com>
> Date: Mon, 25 Nov 2019 14:23:12 +0800
>
> > This patch is to fix an issue that ipv6 ns and na can't go out with the
> > correct tunnel info, and it can be reproduced by:
>
> And why shouldn't RS and redirects get this treatment too?
RS daddr is using IN6ADDR_LINKLOCAL_ALLROUTERS_INIT.
I didn't think it could be an address that users may use to configure
a lwtunnel state.

>
> And then, at that point, all callers of ndisc_send_skb() have this
> early route lookup code (and thus the "!dst" code path is unused), and
> the question ultimately becomes why doesn't ndisc_send_skb() itself
> have the dst lookup modification?
>
> I'm not too sure about this change and will not apply it.
I'm not so sure about this fix either, but I couldn't see other fixes for it.
Hope to get more developer's opinions, especially from router's.
besides the above, do you have other concerns?

Thanks.