Re: [PATCH nf-next v9 2/3] net: netfilter: Add IPIP flowtable tx sw acceleration

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4067 bytes --]

> Hi Lorenzo,
> 
> I found one more issue: caching the ip6 daddr does not work because
> skb_cow_head() can reallocate the skb header, invalidating all
> pointers.
> 
> I went back to use the other_tuple, it is safer, new branch:
> 
>         flowtable-consolidate-xmit+ipip3

Hi Pablo,

thx for fixing it. I tested the branch above and it works fine with IPIP tunnel
flowtable offloading.

> 
> Hopefully, this is the last iteration for this series.
> 
> I am attaching a diff that compares flowtable-consolidate-xmit+ipip2
> vs. flowtable-consolidate-xmit+ipip3 branches.
> 
> I also made a few cosmetic edits.

> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index ee81ee3a5110..e128b0fe9a7b 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -512,13 +512,14 @@ static int nf_flow_pppoe_push(struct sk_buff *skb, u16 id)
>  }
>  
>  static int nf_flow_tunnel_ipip_push(struct net *net, struct sk_buff *skb,
> -				    struct flow_offload_tuple *tuple)
> +				    struct flow_offload_tuple *tuple,
> +				    __be32 *ip_daddr)
>  {
>  	struct iphdr *iph = (struct iphdr *)skb_network_header(skb);
>  	struct rtable *rt = dst_rtable(tuple->dst_cache);
> -	__be16	frag_off = iph->frag_off;
> -	u32 headroom = sizeof(*iph);
>  	u8 tos = iph->tos, ttl = iph->ttl;
> +	__be16 frag_off = iph->frag_off;
> +	u32 headroom = sizeof(*iph);
>  	int err;
>  
>  	err = iptunnel_handle_offloads(skb, SKB_GSO_IPXIP4);
> @@ -551,14 +552,17 @@ static int nf_flow_tunnel_ipip_push(struct net *net, struct sk_buff *skb,
>  	__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
>  	ip_send_check(iph);
>  
> +	*ip_daddr = tuple->tun.src_v4.s_addr;
> +
>  	return 0;
>  }
>  
>  static int nf_flow_tunnel_v4_push(struct net *net, struct sk_buff *skb,
> -				  struct flow_offload_tuple *tuple)
> +				  struct flow_offload_tuple *tuple,
> +				  __be32 *ip_daddr)
>  {
>  	if (tuple->tun_num)
> -		return nf_flow_tunnel_ipip_push(net, skb, tuple);
> +		return nf_flow_tunnel_ipip_push(net, skb, tuple, ip_daddr);
>  
>  	return 0;
>  }
> @@ -572,7 +576,8 @@ static int nf_flow_encap_push(struct sk_buff *skb,
>  		switch (tuple->encap[i].proto) {
>  		case htons(ETH_P_8021Q):
>  		case htons(ETH_P_8021AD):
> -			if (skb_vlan_push(skb, tuple->encap[i].proto, tuple->encap[i].id) < 0)
> +			if (skb_vlan_push(skb, tuple->encap[i].proto,
> +					  tuple->encap[i].id) < 0)
>  				return -1;
>  			break;
>  		case htons(ETH_P_PPP_SES):
> @@ -624,12 +629,11 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
>  	dir = tuplehash->tuple.dir;
>  	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
>  	other_tuple = &flow->tuplehash[!dir].tuple;
> +	ip_daddr = other_tuple->src_v4.s_addr;
>  
> -	if (nf_flow_tunnel_v4_push(state->net, skb, other_tuple) < 0)
> +	if (nf_flow_tunnel_v4_push(state->net, skb, other_tuple, &ip_daddr) < 0)
>  		return NF_DROP;
>  
> -	ip_daddr = ip_hdr(skb)->daddr;
> -
>  	if (nf_flow_encap_push(skb, other_tuple) < 0)
>  		return NF_DROP;
>  
> @@ -906,6 +910,7 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
>  {
>  	struct flow_offload_tuple_rhash *tuplehash;
>  	struct nf_flowtable *flow_table = priv;
> +	struct flow_offload_tuple *other_tuple;
>  	enum flow_offload_tuple_dir dir;
>  	struct nf_flowtable_ctx ctx = {
>  		.in	= state->in,
> @@ -937,9 +942,10 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
>  
>  	dir = tuplehash->tuple.dir;
>  	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
> -	ip6_daddr = &ipv6_hdr(skb)->daddr;
> +	other_tuple = &flow->tuplehash[!dir].tuple;
> +	ip6_daddr = &other_tuple->src_v6;
>  
> -	if (nf_flow_encap_push(skb, &flow->tuplehash[!dir].tuple) < 0)
> +	if (nf_flow_encap_push(skb, other_tuple) < 0)
>  		return NF_DROP;
>  
>  	switch (tuplehash->tuple.xmit_type) {

ack, the above changes are fine to me.

Regards,
Lorenzo



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]