All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH nf 0/2] route_me_harder routing loop with tunnels
@ 2020-10-29  2:56 Jason A. Donenfeld
  2020-10-29  2:56 ` [PATCH nf 1/2] wireguard: selftests: check that route_me_harder packets use the right sk Jason A. Donenfeld
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Jason A. Donenfeld @ 2020-10-29  2:56 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel, netdev; +Cc: Jason A. Donenfeld

Hi Pablo,

This series fixes a bug in the route_me_harder family of functions with
regards to tunnel interfaces. The first patch contains an addition to
the wireguard test suite; I normally send my wireguard patches through
Dave's tree, but I thought it'd be nice to send these together here
because the test case is illustrative of the issue. The second patch
then fixes the issue with a lengthy explanation of what's going on.

These are intended for net.git/nf.git, not the -next variety, and to
eventually be backported to stable. So, the second patch has a proper
Fixes: line on it to help with that.

Thanks,
Jason

Jason A. Donenfeld (2):
  wireguard: selftests: check that route_me_harder packets use the right
    sk
  netfilter: use actual socket sk rather than skb sk when routing harder

 include/linux/netfilter_ipv4.h                       |  2 +-
 include/linux/netfilter_ipv6.h                       | 10 +++++-----
 net/ipv4/netfilter.c                                 |  8 +++++---
 net/ipv4/netfilter/iptable_mangle.c                  |  2 +-
 net/ipv4/netfilter/nf_reject_ipv4.c                  |  2 +-
 net/ipv6/netfilter.c                                 |  6 +++---
 net/ipv6/netfilter/ip6table_mangle.c                 |  2 +-
 net/netfilter/ipvs/ip_vs_core.c                      |  4 ++--
 net/netfilter/nf_nat_proto.c                         |  4 ++--
 net/netfilter/nf_synproxy_core.c                     |  2 +-
 net/netfilter/nft_chain_route.c                      |  4 ++--
 net/netfilter/utils.c                                |  4 ++--
 tools/testing/selftests/wireguard/netns.sh           |  8 ++++++++
 tools/testing/selftests/wireguard/qemu/kernel.config |  2 ++
 14 files changed, 36 insertions(+), 24 deletions(-)

-- 
2.29.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH nf 1/2] wireguard: selftests: check that route_me_harder packets use the right sk
  2020-10-29  2:56 [PATCH nf 0/2] route_me_harder routing loop with tunnels Jason A. Donenfeld
@ 2020-10-29  2:56 ` Jason A. Donenfeld
  2020-10-29  2:56 ` [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder Jason A. Donenfeld
  2020-10-30 19:23 ` [PATCH nf 0/2] route_me_harder routing loop with tunnels Pablo Neira Ayuso
  2 siblings, 0 replies; 10+ messages in thread
From: Jason A. Donenfeld @ 2020-10-29  2:56 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel, netdev
  Cc: Jason A. Donenfeld, Chen Minqiang

If netfilter changes the packet mark, the packet is rerouted. The
ip_route_me_harder family of functions fails to use the right sk, opting
to instead use skb->sk, resulting in a routing loop when used with
tunnels. With the next change fixing this issue in netfilter, test for
the relevant condition inside our test suite, since wireguard was where
the bug was discovered.

Reported-by: Chen Minqiang <ptpt52@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 tools/testing/selftests/wireguard/netns.sh           | 8 ++++++++
 tools/testing/selftests/wireguard/qemu/kernel.config | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/tools/testing/selftests/wireguard/netns.sh b/tools/testing/selftests/wireguard/netns.sh
index d77f4829f1e0..74c69b75f6f5 100755
--- a/tools/testing/selftests/wireguard/netns.sh
+++ b/tools/testing/selftests/wireguard/netns.sh
@@ -316,6 +316,14 @@ pp sleep 3
 n2 ping -W 1 -c 1 192.168.241.1
 n1 wg set wg0 peer "$pub2" persistent-keepalive 0
 
+# Test that sk_bound_dev_if works
+n1 ping -I wg0 -c 1 -W 1 192.168.241.2
+# What about when the mark changes and the packet must be rerouted?
+n1 iptables -t mangle -I OUTPUT -j MARK --set-xmark 1
+n1 ping -c 1 -W 1 192.168.241.2 # First the boring case
+n1 ping -I wg0 -c 1 -W 1 192.168.241.2 # Then the sk_bound_dev_if case
+n1 iptables -t mangle -D OUTPUT -j MARK --set-xmark 1
+
 # Test that onion routing works, even when it loops
 n1 wg set wg0 peer "$pub3" allowed-ips 192.168.242.2/32 endpoint 192.168.241.2:5
 ip1 addr add 192.168.242.1/24 dev wg0
diff --git a/tools/testing/selftests/wireguard/qemu/kernel.config b/tools/testing/selftests/wireguard/qemu/kernel.config
index d531de13c95b..4eecb432a66c 100644
--- a/tools/testing/selftests/wireguard/qemu/kernel.config
+++ b/tools/testing/selftests/wireguard/qemu/kernel.config
@@ -18,10 +18,12 @@ CONFIG_NF_NAT=y
 CONFIG_NETFILTER_XTABLES=y
 CONFIG_NETFILTER_XT_NAT=y
 CONFIG_NETFILTER_XT_MATCH_LENGTH=y
+CONFIG_NETFILTER_XT_MARK=y
 CONFIG_NF_CONNTRACK_IPV4=y
 CONFIG_NF_NAT_IPV4=y
 CONFIG_IP_NF_IPTABLES=y
 CONFIG_IP_NF_FILTER=y
+CONFIG_IP_NF_MANGLE=y
 CONFIG_IP_NF_NAT=y
 CONFIG_IP_ADVANCED_ROUTER=y
 CONFIG_IP_MULTIPLE_TABLES=y
-- 
2.29.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder
  2020-10-29  2:56 [PATCH nf 0/2] route_me_harder routing loop with tunnels Jason A. Donenfeld
  2020-10-29  2:56 ` [PATCH nf 1/2] wireguard: selftests: check that route_me_harder packets use the right sk Jason A. Donenfeld
@ 2020-10-29  2:56 ` Jason A. Donenfeld
  2020-10-29  9:28   ` Florian Westphal
  2020-10-29 12:01   ` Jason A. Donenfeld
  2020-10-30 19:23 ` [PATCH nf 0/2] route_me_harder routing loop with tunnels Pablo Neira Ayuso
  2 siblings, 2 replies; 10+ messages in thread
From: Jason A. Donenfeld @ 2020-10-29  2:56 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel, netdev; +Cc: Jason A. Donenfeld

If netfilter changes the packet mark when mangling, the packet is
rerouted using the route_me_harder set of functions. Prior to this
commit, there's one big difference between route_me_harder and the
ordinary initial routing functions, described in the comment above
__ip_queue_xmit():

   /* Note: skb->sk can be different from sk, in case of tunnels */
   int __ip_queue_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl,

That function goes on to correctly make use of sk->sk_bound_dev_if,
rather than skb->sk->sk_bound_dev_if. And indeed the comment is true: a
tunnel will receive a packet in ndo_start_xmit with an initial skb->sk.
It will make some transformations to that packet, and then it will send
the encapsulated packet out of a *new* socket. That new socket will
basically always have a different sk_bound_dev_if (otherwise there'd be
a routing loop). So for the purposes of routing the encapsulated packet,
the routing information as it pertains to the socket should come from
that socket's sk, rather than the packet's original skb->sk. For that
reason __ip_queue_xmit() and related functions all do the right thing.

One might argue that all tunnels should just call skb_orphan(skb) before
transmitting the encapsulated packet into the new socket. But tunnels do
*not* do this -- and this is wisely avoided in skb_scrub_packet() too --
because features like TSQ rely on skb->destructor() being called when
that buffer space is truely available again. Calling skb_orphan(skb) too
early would result in buffers filling up unnecessarily and accounting
info being all wrong. Instead, additional routing must take into account
the new sk, just as __ip_queue_xmit() notes.

So, this commit addresses the problem by fishing the correct sk out of
state->sk -- it's already set properly in the call to nf_hook() in
__ip_local_out(), which receives the sk as part of its normal
functionality. So we make sure to plumb state->sk through the various
route_me_harder functions, and then make correct use of it following the
example of __ip_queue_xmit().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 include/linux/netfilter_ipv4.h       |  2 +-
 include/linux/netfilter_ipv6.h       | 10 +++++-----
 net/ipv4/netfilter.c                 |  8 +++++---
 net/ipv4/netfilter/iptable_mangle.c  |  2 +-
 net/ipv4/netfilter/nf_reject_ipv4.c  |  2 +-
 net/ipv6/netfilter.c                 |  6 +++---
 net/ipv6/netfilter/ip6table_mangle.c |  2 +-
 net/netfilter/ipvs/ip_vs_core.c      |  4 ++--
 net/netfilter/nf_nat_proto.c         |  4 ++--
 net/netfilter/nf_synproxy_core.c     |  2 +-
 net/netfilter/nft_chain_route.c      |  4 ++--
 net/netfilter/utils.c                |  4 ++--
 12 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/include/linux/netfilter_ipv4.h b/include/linux/netfilter_ipv4.h
index 082e2c41b7ff..5b70ca868bb1 100644
--- a/include/linux/netfilter_ipv4.h
+++ b/include/linux/netfilter_ipv4.h
@@ -16,7 +16,7 @@ struct ip_rt_info {
 	u_int32_t mark;
 };
 
-int ip_route_me_harder(struct net *net, struct sk_buff *skb, unsigned addr_type);
+int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, unsigned addr_type);
 
 struct nf_queue_entry;
 
diff --git a/include/linux/netfilter_ipv6.h b/include/linux/netfilter_ipv6.h
index 9b67394471e1..48314ade1506 100644
--- a/include/linux/netfilter_ipv6.h
+++ b/include/linux/netfilter_ipv6.h
@@ -42,7 +42,7 @@ struct nf_ipv6_ops {
 #if IS_MODULE(CONFIG_IPV6)
 	int (*chk_addr)(struct net *net, const struct in6_addr *addr,
 			const struct net_device *dev, int strict);
-	int (*route_me_harder)(struct net *net, struct sk_buff *skb);
+	int (*route_me_harder)(struct net *net, struct sock *sk, struct sk_buff *skb);
 	int (*dev_get_saddr)(struct net *net, const struct net_device *dev,
 		       const struct in6_addr *daddr, unsigned int srcprefs,
 		       struct in6_addr *saddr);
@@ -143,9 +143,9 @@ static inline int nf_br_ip6_fragment(struct net *net, struct sock *sk,
 #endif
 }
 
-int ip6_route_me_harder(struct net *net, struct sk_buff *skb);
+int ip6_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb);
 
-static inline int nf_ip6_route_me_harder(struct net *net, struct sk_buff *skb)
+static inline int nf_ip6_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
 #if IS_MODULE(CONFIG_IPV6)
 	const struct nf_ipv6_ops *v6_ops = nf_get_ipv6_ops();
@@ -153,9 +153,9 @@ static inline int nf_ip6_route_me_harder(struct net *net, struct sk_buff *skb)
 	if (!v6_ops)
 		return -EHOSTUNREACH;
 
-	return v6_ops->route_me_harder(net, skb);
+	return v6_ops->route_me_harder(net, sk, skb);
 #elif IS_BUILTIN(CONFIG_IPV6)
-	return ip6_route_me_harder(net, skb);
+	return ip6_route_me_harder(net, sk, skb);
 #else
 	return -EHOSTUNREACH;
 #endif
diff --git a/net/ipv4/netfilter.c b/net/ipv4/netfilter.c
index a058213b77a7..7c841037c533 100644
--- a/net/ipv4/netfilter.c
+++ b/net/ipv4/netfilter.c
@@ -17,17 +17,19 @@
 #include <net/netfilter/nf_queue.h>
 
 /* route_me_harder function, used by iptable_nat, iptable_mangle + ip_queue */
-int ip_route_me_harder(struct net *net, struct sk_buff *skb, unsigned int addr_type)
+int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, unsigned int addr_type)
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	struct rtable *rt;
 	struct flowi4 fl4 = {};
 	__be32 saddr = iph->saddr;
-	const struct sock *sk = skb_to_full_sk(skb);
-	__u8 flags = sk ? inet_sk_flowi_flags(sk) : 0;
+	__u8 flags;
 	struct net_device *dev = skb_dst(skb)->dev;
 	unsigned int hh_len;
 
+	sk = sk_to_full_sk(sk);
+	flags = sk ? inet_sk_flowi_flags(sk) : 0;
+
 	if (addr_type == RTN_UNSPEC)
 		addr_type = inet_addr_type_dev_table(net, dev, saddr);
 	if (addr_type == RTN_LOCAL || addr_type == RTN_UNICAST)
diff --git a/net/ipv4/netfilter/iptable_mangle.c b/net/ipv4/netfilter/iptable_mangle.c
index f703a717ab1d..833079589273 100644
--- a/net/ipv4/netfilter/iptable_mangle.c
+++ b/net/ipv4/netfilter/iptable_mangle.c
@@ -62,7 +62,7 @@ ipt_mangle_out(struct sk_buff *skb, const struct nf_hook_state *state)
 		    iph->daddr != daddr ||
 		    skb->mark != mark ||
 		    iph->tos != tos) {
-			err = ip_route_me_harder(state->net, skb, RTN_UNSPEC);
+			err = ip_route_me_harder(state->net, state->sk, skb, RTN_UNSPEC);
 			if (err < 0)
 				ret = NF_DROP_ERR(err);
 		}
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index 9dcfa4e461b6..93b07739807b 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -145,7 +145,7 @@ void nf_send_reset(struct net *net, struct sk_buff *oldskb, int hook)
 				   ip4_dst_hoplimit(skb_dst(nskb)));
 	nf_reject_ip_tcphdr_put(nskb, oldskb, oth);
 
-	if (ip_route_me_harder(net, nskb, RTN_UNSPEC))
+	if (ip_route_me_harder(net, nskb->sk, nskb, RTN_UNSPEC))
 		goto free_nskb;
 
 	niph = ip_hdr(nskb);
diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
index 6d0e942d082d..ab9a279dd6d4 100644
--- a/net/ipv6/netfilter.c
+++ b/net/ipv6/netfilter.c
@@ -20,10 +20,10 @@
 #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
 #include "../bridge/br_private.h"
 
-int ip6_route_me_harder(struct net *net, struct sk_buff *skb)
+int ip6_route_me_harder(struct net *net, struct sock *sk_partial, struct sk_buff *skb)
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
-	struct sock *sk = sk_to_full_sk(skb->sk);
+	struct sock *sk = sk_to_full_sk(sk_partial);
 	unsigned int hh_len;
 	struct dst_entry *dst;
 	int strict = (ipv6_addr_type(&iph->daddr) &
@@ -84,7 +84,7 @@ static int nf_ip6_reroute(struct sk_buff *skb,
 		if (!ipv6_addr_equal(&iph->daddr, &rt_info->daddr) ||
 		    !ipv6_addr_equal(&iph->saddr, &rt_info->saddr) ||
 		    skb->mark != rt_info->mark)
-			return ip6_route_me_harder(entry->state.net, skb);
+			return ip6_route_me_harder(entry->state.net, entry->state.sk, skb);
 	}
 	return 0;
 }
diff --git a/net/ipv6/netfilter/ip6table_mangle.c b/net/ipv6/netfilter/ip6table_mangle.c
index 1a2748611e00..cee74803d7a1 100644
--- a/net/ipv6/netfilter/ip6table_mangle.c
+++ b/net/ipv6/netfilter/ip6table_mangle.c
@@ -57,7 +57,7 @@ ip6t_mangle_out(struct sk_buff *skb, const struct nf_hook_state *state)
 	     skb->mark != mark ||
 	     ipv6_hdr(skb)->hop_limit != hop_limit ||
 	     flowlabel != *((u_int32_t *)ipv6_hdr(skb)))) {
-		err = ip6_route_me_harder(state->net, skb);
+		err = ip6_route_me_harder(state->net, state->sk, skb);
 		if (err < 0)
 			ret = NF_DROP_ERR(err);
 	}
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index cc3c275934f4..c0b8215ab3d4 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -742,12 +742,12 @@ static int ip_vs_route_me_harder(struct netns_ipvs *ipvs, int af,
 		struct dst_entry *dst = skb_dst(skb);
 
 		if (dst->dev && !(dst->dev->flags & IFF_LOOPBACK) &&
-		    ip6_route_me_harder(ipvs->net, skb) != 0)
+		    ip6_route_me_harder(ipvs->net, skb->sk, skb) != 0)
 			return 1;
 	} else
 #endif
 		if (!(skb_rtable(skb)->rt_flags & RTCF_LOCAL) &&
-		    ip_route_me_harder(ipvs->net, skb, RTN_LOCAL) != 0)
+		    ip_route_me_harder(ipvs->net, skb->sk, skb, RTN_LOCAL) != 0)
 			return 1;
 
 	return 0;
diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
index 59151dc07fdc..e87b6bd6b3cd 100644
--- a/net/netfilter/nf_nat_proto.c
+++ b/net/netfilter/nf_nat_proto.c
@@ -715,7 +715,7 @@ nf_nat_ipv4_local_fn(void *priv, struct sk_buff *skb,
 
 		if (ct->tuplehash[dir].tuple.dst.u3.ip !=
 		    ct->tuplehash[!dir].tuple.src.u3.ip) {
-			err = ip_route_me_harder(state->net, skb, RTN_UNSPEC);
+			err = ip_route_me_harder(state->net, state->sk, skb, RTN_UNSPEC);
 			if (err < 0)
 				ret = NF_DROP_ERR(err);
 		}
@@ -953,7 +953,7 @@ nf_nat_ipv6_local_fn(void *priv, struct sk_buff *skb,
 
 		if (!nf_inet_addr_cmp(&ct->tuplehash[dir].tuple.dst.u3,
 				      &ct->tuplehash[!dir].tuple.src.u3)) {
-			err = nf_ip6_route_me_harder(state->net, skb);
+			err = nf_ip6_route_me_harder(state->net, state->sk, skb);
 			if (err < 0)
 				ret = NF_DROP_ERR(err);
 		}
diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 9cca35d22927..d7d34a62d3bf 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -446,7 +446,7 @@ synproxy_send_tcp(struct net *net,
 
 	skb_dst_set_noref(nskb, skb_dst(skb));
 	nskb->protocol = htons(ETH_P_IP);
-	if (ip_route_me_harder(net, nskb, RTN_UNSPEC))
+	if (ip_route_me_harder(net, nskb->sk, nskb, RTN_UNSPEC))
 		goto free_nskb;
 
 	if (nfct) {
diff --git a/net/netfilter/nft_chain_route.c b/net/netfilter/nft_chain_route.c
index 8826bbe71136..edd02cda57fc 100644
--- a/net/netfilter/nft_chain_route.c
+++ b/net/netfilter/nft_chain_route.c
@@ -42,7 +42,7 @@ static unsigned int nf_route_table_hook4(void *priv,
 		    iph->daddr != daddr ||
 		    skb->mark != mark ||
 		    iph->tos != tos) {
-			err = ip_route_me_harder(state->net, skb, RTN_UNSPEC);
+			err = ip_route_me_harder(state->net, state->sk, skb, RTN_UNSPEC);
 			if (err < 0)
 				ret = NF_DROP_ERR(err);
 		}
@@ -92,7 +92,7 @@ static unsigned int nf_route_table_hook6(void *priv,
 	     skb->mark != mark ||
 	     ipv6_hdr(skb)->hop_limit != hop_limit ||
 	     flowlabel != *((u32 *)ipv6_hdr(skb)))) {
-		err = nf_ip6_route_me_harder(state->net, skb);
+		err = nf_ip6_route_me_harder(state->net, state->sk, skb);
 		if (err < 0)
 			ret = NF_DROP_ERR(err);
 	}
diff --git a/net/netfilter/utils.c b/net/netfilter/utils.c
index cedf47ab3c6f..2182d361e273 100644
--- a/net/netfilter/utils.c
+++ b/net/netfilter/utils.c
@@ -191,8 +191,8 @@ static int nf_ip_reroute(struct sk_buff *skb, const struct nf_queue_entry *entry
 		      skb->mark == rt_info->mark &&
 		      iph->daddr == rt_info->daddr &&
 		      iph->saddr == rt_info->saddr))
-			return ip_route_me_harder(entry->state.net, skb,
-						  RTN_UNSPEC);
+			return ip_route_me_harder(entry->state.net, entry->state.sk,
+						  skb, RTN_UNSPEC);
 	}
 #endif
 	return 0;
-- 
2.29.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder
  2020-10-29  2:56 ` [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder Jason A. Donenfeld
@ 2020-10-29  9:28   ` Florian Westphal
  2020-10-29 12:01   ` Jason A. Donenfeld
  1 sibling, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2020-10-29  9:28 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: Pablo Neira Ayuso, netfilter-devel, netdev

Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> If netfilter changes the packet mark when mangling, the packet is
> rerouted using the route_me_harder set of functions. Prior to this
> commit, there's one big difference between route_me_harder and the
> ordinary initial routing functions, described in the comment above
> __ip_queue_xmit():
> 
>    /* Note: skb->sk can be different from sk, in case of tunnels */
>    int __ip_queue_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl,
> 
> That function goes on to correctly make use of sk->sk_bound_dev_if,
> rather than skb->sk->sk_bound_dev_if. And indeed the comment is true: a
> tunnel will receive a packet in ndo_start_xmit with an initial skb->sk.
> It will make some transformations to that packet, and then it will send
> the encapsulated packet out of a *new* socket. That new socket will
> basically always have a different sk_bound_dev_if (otherwise there'd be
> a routing loop). So for the purposes of routing the encapsulated packet,
> the routing information as it pertains to the socket should come from
> that socket's sk, rather than the packet's original skb->sk. For that
> reason __ip_queue_xmit() and related functions all do the right thing.
> 
> One might argue that all tunnels should just call skb_orphan(skb) before
> transmitting the encapsulated packet into the new socket. But tunnels do
> *not* do this -- and this is wisely avoided in skb_scrub_packet() too --
> because features like TSQ rely on skb->destructor() being called when
> that buffer space is truely available again. Calling skb_orphan(skb) too
> early would result in buffers filling up unnecessarily and accounting
> info being all wrong. Instead, additional routing must take into account
> the new sk, just as __ip_queue_xmit() notes.
> 
> So, this commit addresses the problem by fishing the correct sk out of
> state->sk -- it's already set properly in the call to nf_hook() in
> __ip_local_out(), which receives the sk as part of its normal
> functionality. So we make sure to plumb state->sk through the various
> route_me_harder functions, and then make correct use of it following the
> example of __ip_queue_xmit().

Reviewed-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder
  2020-10-29  2:56 ` [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder Jason A. Donenfeld
  2020-10-29  9:28   ` Florian Westphal
@ 2020-10-29 12:01   ` Jason A. Donenfeld
  2020-10-29 12:02     ` Jason A. Donenfeld
  1 sibling, 1 reply; 10+ messages in thread
From: Jason A. Donenfeld @ 2020-10-29 12:01 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel, Netdev

As a historical note, this code goes all the way back to Rusty in
2.3.14, where it looked like this:

#ifdef CONFIG_NETFILTER
/* To preserve the cute illusion that a locally-generated packet can
  be mangled before routing, we actually reroute if a hook altered
  the packet. -RR */
static int route_me_harder(struct sk_buff *skb)
{
       struct iphdr *iph = skb->nh.iph;
       struct rtable *rt;

       if (ip_route_output(&rt, iph->daddr, iph->saddr,
                           RT_TOS(iph->tos) | RTO_CONN,
                           skb->sk ? skb->sk->bound_dev_if : 0)) {
               printk("route_me_harder: No more route.\n");
               return -EINVAL;
       }

       /* Drop old route. */
       dst_release(skb->dst);

       skb->dst = &rt->u.dst;
       return 0;
}
#endif

And until now, it was never updated to take the separate sock *sk parameter.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder
  2020-10-29 12:01   ` Jason A. Donenfeld
@ 2020-10-29 12:02     ` Jason A. Donenfeld
  0 siblings, 0 replies; 10+ messages in thread
From: Jason A. Donenfeld @ 2020-10-29 12:02 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel, Netdev

On Thu, Oct 29, 2020 at 1:01 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> As a historical note, this code goes all the way back to Rusty in
> 2.3.14, where it looked like this:

Grrr, typo. 2.3.15.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 0/2] route_me_harder routing loop with tunnels
  2020-10-29  2:56 [PATCH nf 0/2] route_me_harder routing loop with tunnels Jason A. Donenfeld
  2020-10-29  2:56 ` [PATCH nf 1/2] wireguard: selftests: check that route_me_harder packets use the right sk Jason A. Donenfeld
  2020-10-29  2:56 ` [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder Jason A. Donenfeld
@ 2020-10-30 19:23 ` Pablo Neira Ayuso
  2020-10-31  0:05   ` Jason A. Donenfeld
  2 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2020-10-30 19:23 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: netfilter-devel, netdev

On Thu, Oct 29, 2020 at 03:56:04AM +0100, Jason A. Donenfeld wrote:
> Hi Pablo,
> 
> This series fixes a bug in the route_me_harder family of functions with
> regards to tunnel interfaces. The first patch contains an addition to
> the wireguard test suite; I normally send my wireguard patches through
> Dave's tree, but I thought it'd be nice to send these together here
> because the test case is illustrative of the issue. The second patch
> then fixes the issue with a lengthy explanation of what's going on.
> 
> These are intended for net.git/nf.git, not the -next variety, and to
> eventually be backported to stable. So, the second patch has a proper
> Fixes: line on it to help with that.

Series applied, thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 0/2] route_me_harder routing loop with tunnels
  2020-10-30 19:23 ` [PATCH nf 0/2] route_me_harder routing loop with tunnels Pablo Neira Ayuso
@ 2020-10-31  0:05   ` Jason A. Donenfeld
  2020-10-31  1:00     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Jason A. Donenfeld @ 2020-10-31  0:05 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, Netdev

On Fri, Oct 30, 2020 at 8:23 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Thu, Oct 29, 2020 at 03:56:04AM +0100, Jason A. Donenfeld wrote:
> > Hi Pablo,
> >
> > This series fixes a bug in the route_me_harder family of functions with
> > regards to tunnel interfaces. The first patch contains an addition to
> > the wireguard test suite; I normally send my wireguard patches through
> > Dave's tree, but I thought it'd be nice to send these together here
> > because the test case is illustrative of the issue. The second patch
> > then fixes the issue with a lengthy explanation of what's going on.
> >
> > These are intended for net.git/nf.git, not the -next variety, and to
> > eventually be backported to stable. So, the second patch has a proper
> > Fixes: line on it to help with that.
>
> Series applied, thanks.

To nf.git? Did you forget to git-push perhaps?

Jason

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 0/2] route_me_harder routing loop with tunnels
  2020-10-31  0:05   ` Jason A. Donenfeld
@ 2020-10-31  1:00     ` Pablo Neira Ayuso
  2020-10-31  1:08       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 10+ messages in thread
From: Pablo Neira Ayuso @ 2020-10-31  1:00 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: netfilter-devel, Netdev

On Sat, Oct 31, 2020 at 01:05:03AM +0100, Jason A. Donenfeld wrote:
> On Fri, Oct 30, 2020 at 8:23 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Thu, Oct 29, 2020 at 03:56:04AM +0100, Jason A. Donenfeld wrote:
> > > Hi Pablo,
> > >
> > > This series fixes a bug in the route_me_harder family of functions with
> > > regards to tunnel interfaces. The first patch contains an addition to
> > > the wireguard test suite; I normally send my wireguard patches through
> > > Dave's tree, but I thought it'd be nice to send these together here
> > > because the test case is illustrative of the issue. The second patch
> > > then fixes the issue with a lengthy explanation of what's going on.
> > >
> > > These are intended for net.git/nf.git, not the -next variety, and to
> > > eventually be backported to stable. So, the second patch has a proper
> > > Fixes: line on it to help with that.
> >
> > Series applied, thanks.
> 
> To nf.git?

Yes.

> Did you forget to git-push perhaps?

I usually take a little time to compile test and run tests before
pushing it out.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH nf 0/2] route_me_harder routing loop with tunnels
  2020-10-31  1:00     ` Pablo Neira Ayuso
@ 2020-10-31  1:08       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2020-10-31  1:08 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: netfilter-devel, Netdev

On Sat, Oct 31, 2020 at 02:00:03AM +0100, Pablo Neira Ayuso wrote:
> On Sat, Oct 31, 2020 at 01:05:03AM +0100, Jason A. Donenfeld wrote:
> > On Fri, Oct 30, 2020 at 8:23 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >
> > > On Thu, Oct 29, 2020 at 03:56:04AM +0100, Jason A. Donenfeld wrote:
> > > > Hi Pablo,
> > > >
> > > > This series fixes a bug in the route_me_harder family of functions with
> > > > regards to tunnel interfaces. The first patch contains an addition to
> > > > the wireguard test suite; I normally send my wireguard patches through
> > > > Dave's tree, but I thought it'd be nice to send these together here
> > > > because the test case is illustrative of the issue. The second patch
> > > > then fixes the issue with a lengthy explanation of what's going on.
> > > >
> > > > These are intended for net.git/nf.git, not the -next variety, and to
> > > > eventually be backported to stable. So, the second patch has a proper
> > > > Fixes: line on it to help with that.
> > >
> > > Series applied, thanks.
> > 
> > To nf.git?
> 
> Yes.
> 
> > Did you forget to git-push perhaps?
> 
> I usually take a little time to compile test and run tests before
> pushing it out.

offlist.

JFYI: I'll be scheduling a pull request by tomorrow to request
inclusion of these updates into net.git.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-10-31  1:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-29  2:56 [PATCH nf 0/2] route_me_harder routing loop with tunnels Jason A. Donenfeld
2020-10-29  2:56 ` [PATCH nf 1/2] wireguard: selftests: check that route_me_harder packets use the right sk Jason A. Donenfeld
2020-10-29  2:56 ` [PATCH nf 2/2] netfilter: use actual socket sk rather than skb sk when routing harder Jason A. Donenfeld
2020-10-29  9:28   ` Florian Westphal
2020-10-29 12:01   ` Jason A. Donenfeld
2020-10-29 12:02     ` Jason A. Donenfeld
2020-10-30 19:23 ` [PATCH nf 0/2] route_me_harder routing loop with tunnels Pablo Neira Ayuso
2020-10-31  0:05   ` Jason A. Donenfeld
2020-10-31  1:00     ` Pablo Neira Ayuso
2020-10-31  1:08       ` Pablo Neira Ayuso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.