[PATCH net-next 0/4] iptunnel: scrub packet in iptunnel_pull_header

[PATCH net-next 0/4] iptunnel: scrub packet in iptunnel_pull_header

[Jiri Benc]

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(-)

-- 
1.8.3.1

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

[Jiri Benc]

Similarly to the existing vxlan_get_sk_family.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/geneve.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6f208132a574..f09de1e30955 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -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;
 		}
 	}
-- 
1.8.3.1

[PATCH net-next 2/4] geneve: move geneve device lookup before iptunnel_pull_header

[Jiri Benc]

This is in preparation for iptunnel_pull_header calling skb_scrub_packet.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/geneve.c | 76 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 32 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index f09de1e30955..4ceccf871b3f 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -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:
-- 
1.8.3.1

[PATCH net-next 3/4] vxlan: move vxlan device lookup before iptunnel_pull_header

[Jiri Benc]

This is in preparation for iptunnel_pull_header calling skb_scrub_packet.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/vxlan.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 3a84680b5117..b43981416fb9 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -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:
-- 
1.8.3.1

[PATCH net-next 4/4] iptunnel: scrub packet in iptunnel_pull_header

[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 <jbenc@redhat.com>
---
 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(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 4ceccf871b3f..dfbe3ca687f7 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -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);
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index b43981416fb9..56cb9d23a6f3 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -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)) {
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 87408ab80856..4dd616376fec 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -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);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 917c2c1bfadd..12071e28d958 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -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,
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index a6e58b6141cd..eaca2449a09a 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -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);
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index 6ec5b42fd172..ec51d02166de 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -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);
 	}
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 0625ac6356b5..f45b8ffc2840 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -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);
 	}
-- 
1.8.3.1

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

[John W. Linville]

On Thu, Feb 18, 2016 at 11:22:49AM +0100, Jiri Benc wrote:
> Similarly to the existing vxlan_get_sk_family.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  drivers/net/geneve.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 6f208132a574..f09de1e30955 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -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?

>  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)

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.

> @@ -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();

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

> @@ -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
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

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

[Jiri Benc]

On Thu, 18 Feb 2016 09:44:10 -0500, John W. Linville wrote:
> On Thu, Feb 18, 2016 at 11:22:49AM +0100, Jiri Benc wrote:
> > +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.

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

Yes.

> Otherwise, we are calling the same function as many as three times
> on receive.
[...]
> 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.

> > @@ -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.

> 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

Re: [PATCH net-next 0/4] iptunnel: scrub packet in iptunnel_pull_header

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Thu, 18 Feb 2016 11:22:48 +0100

> 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

Very nice cleanup, patch applied.