iptunnel: scrub packet in iptunnel_pull_header

PATCH In review Jiri Benc 3 participants 8 messages Feb 18, 2016

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Jiri Benc

author

2016-02-18 10:23 UTC #0 lore raw

cover iptunnel: scrub packet in iptunnel_pull_header

As every IP tunnel has to scrub skb on decapsulation, iptunnel_pull_header tried to do that and open coded part of skb_scrub_packet. Various tunneling protocols (VXLAN, Geneve) then called full skb_scrub_packet on their own, duplicating part of the scrubbing already done.

Consolidate the code, calling skb_scrub_packet from iptunnel_pull_header. This will allow additional cleanups in VXLAN code, as the packet is scrubbed early during rx processing after this patchset and VXLAN can start filling out skb fields earlier.

The full picture of vxlan cleanup patches can be seen at:
https://github.com/jbenc/linux-vxlan/commits/master

Jiri Benc (4):
geneve: implement geneve_get_sk_family helper
geneve: move geneve device lookup before iptunnel_pull_header
vxlan: move vxlan device lookup before iptunnel_pull_header
iptunnel: scrub packet in iptunnel_pull_header

 drivers/net/geneve.c      | 98 +++++++++++++++++++++++++++--------------------
 drivers/net/vxlan.c       | 25 ++++++------
 include/net/ip_tunnels.h  |  3 +-
 net/ipv4/ip_gre.c         |  2 +-
 net/ipv4/ip_tunnel_core.c |  8 ++--
 net/ipv4/ipip.c           |  2 +-
 net/ipv6/sit.c            |  2 +-
 7 files changed, 76 insertions(+), 64 deletions(-)

Signature · 1 line

1.8.3.1

Jiri Benc

author

2016-02-18 10:23 UTC #1 lore raw

PATCH 1/4 geneve: implement geneve_get_sk_family helper

Similarly to the existing vxlan_get_sk_family.

Signed-off-by Jiri Benc <[email protected]>

---
 drivers/net/geneve.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

drivers/net/geneve.c +11 -9

index 6f208132a574..f09de1e30955 100644
@@ -110,6 +110,11 @@ static __be64 vni_to_tunnel_id(const __u8 *vni)
 #endif
 }
 
+static sa_family_t geneve_get_sk_family(struct geneve_sock *gs)
+{
+	return gs->sock->sk->sk_family;
+}
+
 static struct geneve_dev *geneve_lookup(struct geneve_sock *gs,
 					__be32 addr, u8 vni[])
 {

@@ -165,16 +170,13 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 	static u8 zero_vni[3];
 	u8 *vni;
 	int err = 0;
-	sa_family_t sa_family;
 #if IS_ENABLED(CONFIG_IPV6)
 	struct ipv6hdr *ip6h = NULL;
 	struct in6_addr addr6;
 	static struct in6_addr zero_addr6;
 #endif
 
-	sa_family = gs->sock->sk->sk_family;
-
-	if (sa_family == AF_INET) {
+	if (geneve_get_sk_family(gs) == AF_INET) {
 		iph = ip_hdr(skb); /* outer IP header... */
 
 		if (gs->collect_md) {

@@ -188,7 +190,7 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 
 		geneve = geneve_lookup(gs, addr, vni);
 #if IS_ENABLED(CONFIG_IPV6)
-	} else if (sa_family == AF_INET6) {
+	} else if (geneve_get_sk_family(gs) == AF_INET6) {
 		ip6h = ipv6_hdr(skb); /* outer IPv6 header... */
 
 		if (gs->collect_md) {

@@ -213,7 +215,7 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 			(gnvh->oam ? TUNNEL_OAM : 0) |
 			(gnvh->critical ? TUNNEL_CRIT_OPT : 0);
 
-		tun_dst = udp_tun_rx_dst(skb, sa_family, flags,
+		tun_dst = udp_tun_rx_dst(skb, geneve_get_sk_family(gs), flags,
 					 vni_to_tunnel_id(gnvh->vni),
 					 gnvh->opt_len * 4);
 		if (!tun_dst)

@@ -392,7 +394,7 @@ static void geneve_notify_add_rx_port(struct geneve_sock *gs)
 	struct net_device *dev;
 	struct sock *sk = gs->sock->sk;
 	struct net *net = sock_net(sk);
-	sa_family_t sa_family = sk->sk_family;
+	sa_family_t sa_family = geneve_get_sk_family(gs);
 	__be16 port = inet_sk(sk)->inet_sport;
 	int err;

@@ -553,7 +555,7 @@ static void geneve_notify_del_rx_port(struct geneve_sock *gs)
 	struct net_device *dev;
 	struct sock *sk = gs->sock->sk;
 	struct net *net = sock_net(sk);
-	sa_family_t sa_family = sk->sk_family;
+	sa_family_t sa_family = geneve_get_sk_family(gs);
 	__be16 port = inet_sk(sk)->inet_sport;
 
 	rcu_read_lock();

@@ -596,7 +598,7 @@ static struct geneve_sock *geneve_find_sock(struct geneve_net *gn,
 
 	list_for_each_entry(gs, &gn->sock_list, list) {
 		if (inet_sk(gs->sock->sk)->inet_sport == dst_port &&
-		    inet_sk(gs->sock->sk)->sk.sk_family == family) {
+		    geneve_get_sk_family(gs) == family) {
 			return gs;
 		}
 	}

Signature · 1 line

1.8.3.1

John W. Linville

2016-02-18 14:45 UTC #2 lore raw

Re: [PATCH net-next 1/4] geneve: implement geneve_get_sk_family helper

Quoted 12 lines · Jiri Benc

Similarly to the existing vxlan_get_sk_family.
Signed-off-by Jiri Benc <[email protected]>
---
 drivers/net/geneve.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/net/geneve.c +0 -0
index 6f208132a574..f09de1e30955 100644

drivers/net/geneve.c static __be64 vni_to_tunnel_id(const __u8 *vni) · line 117

Quoted 8 lines

drivers/net/geneve.c +5 -0

@@ -110,6 +110,11 @@ static __be64 vni_to_tunnel_id(const __u8 *vni)
 #endif
 }
 
+static sa_family_t geneve_get_sk_family(struct geneve_sock *gs)
+{
+	return gs->sock->sk->sk_family;
+}
+

Should this be inline?

Quoted 31 lines

drivers/net/geneve.c +2 -5

 static struct geneve_dev *geneve_lookup(struct geneve_sock *gs,
 					__be32 addr, u8 vni[])
 {

@@ -165,16 +170,13 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 	static u8 zero_vni[3];
 	u8 *vni;
 	int err = 0;
-	sa_family_t sa_family;
 #if IS_ENABLED(CONFIG_IPV6)
 	struct ipv6hdr *ip6h = NULL;
 	struct in6_addr addr6;
 	static struct in6_addr zero_addr6;
 #endif
 
-	sa_family = gs->sock->sk->sk_family;
-
-	if (sa_family == AF_INET) {
+	if (geneve_get_sk_family(gs) == AF_INET) {
 		iph = ip_hdr(skb); /* outer IP header... */
 
 		if (gs->collect_md) {

@@ -188,7 +190,7 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 
 		geneve = geneve_lookup(gs, addr, vni);
 #if IS_ENABLED(CONFIG_IPV6)
-	} else if (sa_family == AF_INET6) {
+	} else if (geneve_get_sk_family(gs) == AF_INET6) {
 		ip6h = ipv6_hdr(skb); /* outer IPv6 header... */
 
 		if (gs->collect_md) {

drivers/net/geneve.c static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb) · line 221

Quoted 8 lines

drivers/net/geneve.c +1 -1

@@ -213,7 +215,7 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 			(gnvh->oam ? TUNNEL_OAM : 0) |
 			(gnvh->critical ? TUNNEL_CRIT_OPT : 0);
 
-		tun_dst = udp_tun_rx_dst(skb, sa_family, flags,
+		tun_dst = udp_tun_rx_dst(skb, geneve_get_sk_family(gs), flags,
 					 vni_to_tunnel_id(gnvh->vni),
 					 gnvh->opt_len * 4);
 		if (!tun_dst)

Can we count on GCC to inline geneve_get_sk_family on its own? Otherwise, we are calling the same function as many as three times on receive.

Quoted 10 lines

drivers/net/geneve.c +1 -1

@@ -392,7 +394,7 @@ static void geneve_notify_add_rx_port(struct geneve_sock *gs)
 	struct net_device *dev;
 	struct sock *sk = gs->sock->sk;
 	struct net *net = sock_net(sk);
-	sa_family_t sa_family = sk->sk_family;
+	sa_family_t sa_family = geneve_get_sk_family(gs);
 	__be16 port = inet_sk(sk)->inet_sport;
 	int err;

drivers/net/geneve.c static void geneve_notify_del_rx_port(struct geneve_sock *gs) · line 561

Quoted 8 lines

drivers/net/geneve.c +1 -1

@@ -553,7 +555,7 @@ static void geneve_notify_del_rx_port(struct geneve_sock *gs)
 	struct net_device *dev;
 	struct sock *sk = gs->sock->sk;
 	struct net *net = sock_net(sk);
-	sa_family_t sa_family = sk->sk_family;
+	sa_family_t sa_family = geneve_get_sk_family(gs);
 	__be16 port = inet_sk(sk)->inet_sport;
 
 	rcu_read_lock();

Then on these three you are caching the results of the function call instead...

drivers/net/geneve.c static struct geneve_sock *geneve_find_sock(struct geneve_net *gn, · line 604

Quoted 9 lines

drivers/net/geneve.c +1 -1

@@ -596,7 +598,7 @@ static struct geneve_sock *geneve_find_sock(struct geneve_net *gn,
 
 	list_for_each_entry(gs, &gn->sock_list, list) {
 		if (inet_sk(gs->sock->sk)->inet_sport == dst_port &&
-		    inet_sk(gs->sock->sk)->sk.sk_family == family) {
+		    geneve_get_sk_family(gs) == family) {
 			return gs;
 		}
 	}

And back to calling it each time (as necessary inside the list_for_each_entry).

So, it seems like geneve_get_sk_family needs to be inline -- maybe GCC is doing that on its own? FWIW, I probably would cache the results inside of geneve_rx like you have done in geneve_notify_add_rx_port and geneve_notify_del_rx_port.

Thanks for the attention to geneve!

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

Jiri Benc

author

2016-02-18 15:24 UTC #3 lore raw

Re: [PATCH net-next 1/4] geneve: implement geneve_get_sk_family helper

Quoted 8 lines · John W. Linville

drivers/net/geneve.c static __be64 vni_to_tunnel_id(const __u8 *vni) · line 117
Quoted 5 lines · Jiri Benc
drivers/net/geneve.c +5 -0
+static sa_family_t geneve_get_sk_family(struct geneve_sock *gs)
+{
+	return gs->sock->sk->sk_family;
+}
+
Should this be inline?

AFAIK the kernel policy is to not add inlines in .c files, instead let the compiler do its job.

Quoted 1 line

Can we count on GCC to inline geneve_get_sk_family on its own?

Yes.

Quoted 2 lines

Otherwise, we are calling the same function as many as three times on receive.

[...]

Quoted 2 lines

Then on these three you are caching the results of the function call instead...

The reason I removed the sa_family local variable from geneve_rx is the next patch in the set where I need to put part of the family references into a separate function.

I kept the rest of the file as it was - I thought about removing the local variables in geneve_notify_add/del_rx_port, too, but it would lead to ugly long lines here:

        for_each_netdev_rcu(net, dev) {
                if (dev->netdev_ops->ndo_add_geneve_port)
                        dev->netdev_ops->ndo_add_geneve_port(dev, sa_family,
                                                             port);
        }

Thus, the local variable makes sense in the these two functions, makes the code more comprehensible.

Quoted 12 lines

drivers/net/geneve.c static struct geneve_sock *geneve_find_sock(struct geneve_net *gn, · line 604
Quoted 9 lines
drivers/net/geneve.c +1 -1
@@ -596,7 +598,7 @@ static struct geneve_sock *geneve_find_sock(struct geneve_net *gn,
 
 	list_for_each_entry(gs, &gn->sock_list, list) {
 		if (inet_sk(gs->sock->sk)->inet_sport == dst_port &&
-		    inet_sk(gs->sock->sk)->sk.sk_family == family) {
+		    geneve_get_sk_family(gs) == family) {
 			return gs;
 		}
 	}
And back to calling it each time (as necessary inside the list_for_each_entry).

Just replacing the direct sk_family dereference with the helper, no real change here. No need to introduce a local variable here, this is still short enough.

Quoted 4 lines

So, it seems like geneve_get_sk_family needs to be inline -- maybe GCC is doing that on its own? FWIW, I probably would cache the results inside of geneve_rx like you have done in geneve_notify_add_rx_port and geneve_notify_del_rx_port.

Then I would just have to remove or duplicate the local variable in the next patch. Not worth it, I think.

Thanks for review,

Jiri

Jiri Benc

author

2016-02-18 10:23 UTC #4 lore raw

PATCH 2/4 geneve: move geneve device lookup before iptunnel_pull_header

In reply to Jiri Benc

This is in preparation for iptunnel_pull_header calling skb_scrub_packet.

Signed-off-by Jiri Benc <[email protected]>

---
 drivers/net/geneve.c | 76 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 32 deletions(-)

drivers/net/geneve.c +44 ~~-32~~

index f09de1e30955..4ceccf871b3f 100644
@@ -158,55 +158,60 @@ static inline struct genevehdr *geneve_hdr(const struct sk_buff *skb)
 	return (struct genevehdr *)(udp_hdr(skb) + 1);
 }
 
-/* geneve receive/decap routine */
-static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
+static struct geneve_dev *geneve_lookup_skb(struct geneve_sock *gs,
+					    struct sk_buff *skb)
 {
-	struct genevehdr *gnvh = geneve_hdr(skb);
-	struct metadata_dst *tun_dst = NULL;
-	struct geneve_dev *geneve = NULL;
-	struct pcpu_sw_netstats *stats;
-	struct iphdr *iph = NULL;
+	u8 *vni;
 	__be32 addr;
 	static u8 zero_vni[3];
-	u8 *vni;
-	int err = 0;
 #if IS_ENABLED(CONFIG_IPV6)
-	struct ipv6hdr *ip6h = NULL;
-	struct in6_addr addr6;
 	static struct in6_addr zero_addr6;
 #endif
 
 	if (geneve_get_sk_family(gs) == AF_INET) {
+		struct iphdr *iph;
+
 		iph = ip_hdr(skb); /* outer IP header... */
 
 		if (gs->collect_md) {
 			vni = zero_vni;
 			addr = 0;
 		} else {
-			vni = gnvh->vni;
-
+			vni = geneve_hdr(skb)->vni;
 			addr = iph->saddr;
 		}
 
-		geneve = geneve_lookup(gs, addr, vni);
+		return geneve_lookup(gs, addr, vni);
 #if IS_ENABLED(CONFIG_IPV6)
 	} else if (geneve_get_sk_family(gs) == AF_INET6) {
+		struct ipv6hdr *ip6h;
+		struct in6_addr addr6;
+
 		ip6h = ipv6_hdr(skb); /* outer IPv6 header... */
 
 		if (gs->collect_md) {
 			vni = zero_vni;
 			addr6 = zero_addr6;
 		} else {
-			vni = gnvh->vni;
-
+			vni = geneve_hdr(skb)->vni;
 			addr6 = ip6h->saddr;
 		}
 
-		geneve = geneve6_lookup(gs, addr6, vni);
+		return geneve6_lookup(gs, addr6, vni);
 #endif
 	}
-	if (!geneve)
-		goto drop;
+	return NULL;
+}
+
+/* geneve receive/decap routine */
+static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
+		      struct sk_buff *skb)
+{
+	struct genevehdr *gnvh = geneve_hdr(skb);
+	struct metadata_dst *tun_dst = NULL;
+	struct pcpu_sw_netstats *stats;
+	int err = 0;
+	void *oiph;
 
 	if (ip_tunnel_collect_metadata() || gs->collect_md) {
 		__be16 flags;

@@ -243,25 +248,27 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 	if (ether_addr_equal(eth_hdr(skb)->h_source, geneve->dev->dev_addr))
 		goto drop;
 
+	oiph = skb_network_header(skb);
 	skb_reset_network_header(skb);
 
-	if (iph)
-		err = IP_ECN_decapsulate(iph, skb);
+	if (geneve_get_sk_family(gs) == AF_INET)
+		err = IP_ECN_decapsulate(oiph, skb);
 #if IS_ENABLED(CONFIG_IPV6)
-	if (ip6h)
-		err = IP6_ECN_decapsulate(ip6h, skb);
+	else
+		err = IP6_ECN_decapsulate(oiph, skb);
 #endif
 
 	if (unlikely(err)) {
 		if (log_ecn_error) {
-			if (iph)
+			if (geneve_get_sk_family(gs) == AF_INET)
 				net_info_ratelimited("non-ECT from %pI4 "
 						     "with TOS=%#x\n",
-						     &iph->saddr, iph->tos);
+						     &((struct iphdr *)oiph)->saddr,
+						     ((struct iphdr *)oiph)->tos);
 #if IS_ENABLED(CONFIG_IPV6)
-			if (ip6h)
+			else
 				net_info_ratelimited("non-ECT from %pI6\n",
-						     &ip6h->saddr);
+						     &((struct ipv6hdr *)oiph)->saddr);
 #endif
 		}
 		if (err > 1) {

@@ -323,6 +330,7 @@ static void geneve_uninit(struct net_device *dev)
 static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct genevehdr *geneveh;
+	struct geneve_dev *geneve;
 	struct geneve_sock *gs;
 	int opts_len;

@@ -338,16 +346,20 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (unlikely(geneveh->proto_type != htons(ETH_P_TEB)))
 		goto error;
 
+	gs = rcu_dereference_sk_user_data(sk);
+	if (!gs)
+		goto drop;
+
+	geneve = geneve_lookup_skb(gs, skb);
+	if (!geneve)
+		goto drop;
+
 	opts_len = geneveh->opt_len * 4;
 	if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
 				 htons(ETH_P_TEB)))
 		goto drop;
 
-	gs = rcu_dereference_sk_user_data(sk);
-	if (!gs)
-		goto drop;
-
-	geneve_rx(gs, skb);
+	geneve_rx(geneve, gs, skb);
 	return 0;
 
 drop:

Signature · 1 line

1.8.3.1

Jiri Benc

author

2016-02-18 10:23 UTC #5 lore raw

PATCH 3/4 vxlan: move vxlan device lookup before iptunnel_pull_header

In reply to Jiri Benc

This is in preparation for iptunnel_pull_header calling skb_scrub_packet.

Signed-off-by Jiri Benc <[email protected]>

---
 drivers/net/vxlan.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

drivers/net/vxlan.c +11 ~~-12~~

index 3a84680b5117..b43981416fb9 100644
@@ -1187,22 +1187,16 @@ out:
 	unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
 }
 
-static void vxlan_rcv(struct vxlan_sock *vs, struct sk_buff *skb,
-		      struct vxlan_metadata *md, __be32 vni,
+static void vxlan_rcv(struct vxlan_dev *vxlan, struct vxlan_sock *vs,
+		      struct sk_buff *skb, struct vxlan_metadata *md,
 		      struct metadata_dst *tun_dst)
 {
 	struct iphdr *oip = NULL;
 	struct ipv6hdr *oip6 = NULL;
-	struct vxlan_dev *vxlan;
 	struct pcpu_sw_netstats *stats;
 	union vxlan_addr saddr;
 	int err = 0;
 
-	/* Is this VNI defined? */
-	vxlan = vxlan_vs_find_vni(vs, vni);
-	if (!vxlan)
-		goto drop;
-
 	skb_reset_mac_header(skb);
 	skb_scrub_packet(skb, !net_eq(vxlan->net, dev_net(vxlan->dev)));
 	skb->protocol = eth_type_trans(skb, vxlan->dev);

@@ -1281,6 +1275,7 @@ drop:
 static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct metadata_dst *tun_dst = NULL;
+	struct vxlan_dev *vxlan;
 	struct vxlan_sock *vs;
 	struct vxlanhdr unparsed;
 	struct vxlan_metadata _md;

@@ -1302,13 +1297,17 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	unparsed.vx_flags &= ~VXLAN_HF_VNI;
 	unparsed.vx_vni &= ~VXLAN_VNI_MASK;
 
-	if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
-		goto drop;
-
 	vs = rcu_dereference_sk_user_data(sk);
 	if (!vs)
 		goto drop;
 
+	vxlan = vxlan_vs_find_vni(vs, vxlan_vni(vxlan_hdr(skb)->vx_vni));
+	if (!vxlan)
+		goto drop;
+
+	if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
+		goto drop;
+
 	if (vxlan_collect_metadata(vs)) {
 		tun_dst = udp_tun_rx_dst(skb, vxlan_get_sk_family(vs), TUNNEL_KEY,
 					 vxlan_vni(vxlan_hdr(skb)->vx_vni),

@@ -1343,7 +1342,7 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		goto drop;
 	}
 
-	vxlan_rcv(vs, skb, md, vxlan_vni(vxlan_hdr(skb)->vx_vni), tun_dst);
+	vxlan_rcv(vxlan, vs, skb, md, tun_dst);
 	return 0;
 
 drop:

Signature · 1 line

1.8.3.1

Jiri Benc

author

2016-02-18 10:23 UTC #6 lore raw

PATCH 4/4 iptunnel: scrub packet in iptunnel_pull_header

In reply to Jiri Benc

Part of skb_scrub_packet was open coded in iptunnel_pull_header. Let it call skb_scrub_packet directly instead.

Signed-off-by Jiri Benc <[email protected]>

---
 drivers/net/geneve.c      | 4 ++--
 drivers/net/vxlan.c       | 4 ++--
 include/net/ip_tunnels.h  | 3 ++-
 net/ipv4/ip_gre.c         | 2 +-
 net/ipv4/ip_tunnel_core.c | 8 +++-----
 net/ipv4/ipip.c           | 2 +-
 net/ipv6/sit.c            | 2 +-
 7 files changed, 12 insertions(+), 13 deletions(-)

drivers/net/geneve.c +2 -2

index 4ceccf871b3f..dfbe3ca687f7 100644
@@ -237,7 +237,6 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
 	}
 
 	skb_reset_mac_header(skb);
-	skb_scrub_packet(skb, !net_eq(geneve->net, dev_net(geneve->dev)));
 	skb->protocol = eth_type_trans(skb, geneve->dev);
 	skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);

@@ -356,7 +355,8 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 
 	opts_len = geneveh->opt_len * 4;
 	if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
-				 htons(ETH_P_TEB)))
+				 htons(ETH_P_TEB),
+				 !net_eq(geneve->net, dev_net(geneve->dev))))
 		goto drop;
 
 	geneve_rx(geneve, gs, skb);

drivers/net/vxlan.c +2 -2

index b43981416fb9..56cb9d23a6f3 100644
@@ -1198,7 +1198,6 @@ static void vxlan_rcv(struct vxlan_dev *vxlan, struct vxlan_sock *vs,
 	int err = 0;
 
 	skb_reset_mac_header(skb);
-	skb_scrub_packet(skb, !net_eq(vxlan->net, dev_net(vxlan->dev)));
 	skb->protocol = eth_type_trans(skb, vxlan->dev);
 	skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);

@@ -1305,7 +1304,8 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (!vxlan)
 		goto drop;
 
-	if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
+	if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB),
+				 !net_eq(vxlan->net, dev_net(vxlan->dev))))
 		goto drop;
 
 	if (vxlan_collect_metadata(vs)) {

include/net/ip_tunnels.h +2 -1

index 87408ab80856..4dd616376fec 100644
@@ -270,7 +270,8 @@ static inline u8 ip_tunnel_ecn_encap(u8 tos, const struct iphdr *iph,
 	return INET_ECN_encapsulate(tos, inner);
 }
 
-int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto);
+int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto,
+			 bool xnet);
 void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
 		   __be32 src, __be32 dst, u8 proto,
 		   u8 tos, u8 ttl, __be16 df, bool xnet);

net/ipv4/ip_gre.c +1 -1

index 917c2c1bfadd..12071e28d958 100644
@@ -238,7 +238,7 @@ static int parse_gre_header(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 				return -EINVAL;
 		}
 	}
-	return iptunnel_pull_header(skb, hdr_len, tpi->proto);
+	return iptunnel_pull_header(skb, hdr_len, tpi->proto, false);
 }
 
 static void ipgre_err(struct sk_buff *skb, u32 info,

net/ipv4/ip_tunnel_core.c +3 -5

index a6e58b6141cd..eaca2449a09a 100644
@@ -86,7 +86,8 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
 }
 EXPORT_SYMBOL_GPL(iptunnel_xmit);
 
-int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto)
+int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto,
+			 bool xnet)
 {
 	if (unlikely(!pskb_may_pull(skb, hdr_len)))
 		return -ENOMEM;

@@ -109,13 +110,10 @@ int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto)
 		skb->protocol = inner_proto;
 	}
 
-	nf_reset(skb);
-	secpath_reset(skb);
 	skb_clear_hash_if_not_l4(skb);
-	skb_dst_drop(skb);
 	skb->vlan_tci = 0;
 	skb_set_queue_mapping(skb, 0);
-	skb->pkt_type = PACKET_HOST;
+	skb_scrub_packet(skb, xnet);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iptunnel_pull_header);

net/ipv4/ipip.c +1 -1

index 6ec5b42fd172..ec51d02166de 100644
@@ -195,7 +195,7 @@ static int ipip_rcv(struct sk_buff *skb)
 	if (tunnel) {
 		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 			goto drop;
-		if (iptunnel_pull_header(skb, 0, tpi.proto))
+		if (iptunnel_pull_header(skb, 0, tpi.proto, false))
 			goto drop;
 		return ip_tunnel_rcv(tunnel, skb, &tpi, NULL, log_ecn_error);
 	}

net/ipv6/sit.c +1 -1

index 0625ac6356b5..f45b8ffc2840 100644
@@ -740,7 +740,7 @@ static int ipip_rcv(struct sk_buff *skb)
 
 		if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 			goto drop;
-		if (iptunnel_pull_header(skb, 0, tpi.proto))
+		if (iptunnel_pull_header(skb, 0, tpi.proto, false))
 			goto drop;
 		return ip_tunnel_rcv(tunnel, skb, &tpi, NULL, log_ecn_error);
 	}

Signature · 1 line

1.8.3.1

David Miller

maintainer

2016-02-18 19:35 UTC #7 lore raw

In reply to Jiri Benc

From: Jiri Benc <[email protected]>
Date: Thu, 18 Feb 2016 11:22:48 +0100

Quoted 5 lines

As every IP tunnel has to scrub skb on decapsulation, iptunnel_pull_header tried to do that and open coded part of skb_scrub_packet. Various tunneling protocols (VXLAN, Geneve) then called full skb_scrub_packet on their own, duplicating part of the scrubbing already done.

Quoted 7 lines

Consolidate the code, calling skb_scrub_packet from iptunnel_pull_header. This will allow additional cleanups in VXLAN code, as the packet is scrubbed early during rx processing after this patchset and VXLAN can start filling out skb fields earlier.
The full picture of vxlan cleanup patches can be seen at:
https://github.com/jbenc/linux-vxlan/commits/master

Very nice cleanup, patch applied.