[PATCH nf 0/3] flowtable fixes for ipip tunnels

[PATCH nf 0/3] flowtable fixes for ipip tunnels

[Pablo Neira Ayuso]

Hi,

The following patchset contains fixes for the flowtable ipip support:

Patch #1 updates nf_flow_tunnel_ip{6}ip{6}_push() to use the dst_entry
         in this direction to calculate headroom and set iph->frag_off.
	 Apparently, nft_flow_tunnel_update_route() sets the right
	 dst_entry in this direction, but datapath uses the other direction.

Patch #2 adds a function to bail out early on any attempt to hardware
	 offload a flow entry which is not supported. Currently, ->num_tun
	 has no drivers supporting this.

Patch #3 makes dst_entry available for all xmit modes which is required
	 to reach the dst_entry when pushing the ipip header (see related
	 patch #1). Based on patch from Rein Wein.

This series is passing nft_flowtable.sh selftest here, including Ren Wei's
patch:

  [nf,v3,2/2] selftests: netfilter: add bridge tunnel flowtable regression
  https://patchwork.ozlabs.org/project/netfilter-devel/patch/5b8a9e87ff7b47401612bb0e0fc841d8bfdd333d.1782092221.git.chzhengyang2023@lzu.edu.cn/

which should be also be taken upstream.

Pablo Neira Ayuso (3):
  netfilter: flowtable: use dst in this direction when pushing IPIP header
  netfilter: flowtable: IPIP tunnel hardware offload is not yet support
  netfilter: flowtable: support IPIP tunnel with direct xmit

 include/net/netfilter/nf_flow_table.h |  4 ++--
 net/netfilter/nf_flow_table_core.c    | 15 +++++++++++----
 net/netfilter/nf_flow_table_ip.c      | 21 ++++++++++++---------
 net/netfilter/nf_flow_table_offload.c | 17 +++++++++++++++++
 4 files changed, 42 insertions(+), 15 deletions(-)

-- 
2.47.3

[PATCH nf 1/3] netfilter: flowtable: use dst in this direction when pushing IPIP header

[Pablo Neira Ayuso]

When pushing the IPIP header, the route of the other direction is used
to calculate the headroom, use the route in this direction. Accessing
the other tuple to set the IP source and destination is fine because
this tuple does not provide such information to avoid storing redundant
information. However, this tuple already provides the dst for this
direction, this went unnoticed because this bug affects headroom and
iph->frag_off only at this stage.

Fixes: d30301ba4b07 ("netfilter: flowtable: Add IPIP tx sw acceleration")
Fixes: 93cf357fa797 ("netfilter: flowtable: Add IP6IP6 tx sw acceleration")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_ip.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 29e93ac1e2e4..089f2bc19972 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -590,10 +590,10 @@ 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,
-				    __be32 *ip_daddr)
+				    struct dst_entry *dst, __be32 *ip_daddr)
 {
 	struct iphdr *iph = (struct iphdr *)skb_network_header(skb);
-	struct rtable *rt = dst_rtable(tuple->dst_cache);
+	struct rtable *rt = dst_rtable(dst);
 	u8 tos = iph->tos, ttl = iph->ttl;
 	__be16 frag_off = iph->frag_off;
 	u32 headroom = sizeof(*iph);
@@ -636,21 +636,22 @@ static int nf_flow_tunnel_ipip_push(struct net *net, struct sk_buff *skb,
 
 static int nf_flow_tunnel_v4_push(struct net *net, struct sk_buff *skb,
 				  struct flow_offload_tuple *tuple,
-				  __be32 *ip_daddr)
+				  struct dst_entry *dst,  __be32 *ip_daddr)
 {
 	if (tuple->tun_num)
-		return nf_flow_tunnel_ipip_push(net, skb, tuple, ip_daddr);
+		return nf_flow_tunnel_ipip_push(net, skb, tuple, dst, ip_daddr);
 
 	return 0;
 }
 
 static int nf_flow_tunnel_ip6ip6_push(struct net *net, struct sk_buff *skb,
 				      struct flow_offload_tuple *tuple,
+				      struct dst_entry *dst,
 				      struct in6_addr **ip6_daddr)
 {
 	struct ipv6hdr *ip6h = (struct ipv6hdr *)skb_network_header(skb);
-	struct rtable *rt = dst_rtable(tuple->dst_cache);
 	__u8 dsfield = ipv6_get_dsfield(ip6h);
+	struct rtable *rt = dst_rtable(dst);
 	struct flowi6 fl6 = {
 		.daddr = tuple->tun.src_v6,
 		.saddr = tuple->tun.dst_v6,
@@ -696,10 +697,11 @@ static int nf_flow_tunnel_ip6ip6_push(struct net *net, struct sk_buff *skb,
 
 static int nf_flow_tunnel_v6_push(struct net *net, struct sk_buff *skb,
 				  struct flow_offload_tuple *tuple,
+				  struct dst_entry *dst,
 				  struct in6_addr **ip6_daddr)
 {
 	if (tuple->tun_num)
-		return nf_flow_tunnel_ip6ip6_push(net, skb, tuple, ip6_daddr);
+		return nf_flow_tunnel_ip6ip6_push(net, skb, tuple, dst, ip6_daddr);
 
 	return 0;
 }
@@ -842,7 +844,8 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
 	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, &ip_daddr) < 0)
+	if (nf_flow_tunnel_v4_push(state->net, skb, other_tuple,
+				   tuplehash->tuple.dst_cache, &ip_daddr) < 0)
 		return NF_DROP;
 
 	switch (tuplehash->tuple.xmit_type) {
@@ -1158,6 +1161,7 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
 	ip6_daddr = &other_tuple->src_v6;
 
 	if (nf_flow_tunnel_v6_push(state->net, skb, other_tuple,
+				   tuplehash->tuple.dst_cache,
 				   &ip6_daddr) < 0)
 		return NF_DROP;
 
-- 
2.47.3

[PATCH nf 2/3] netfilter: flowtable: IPIP tunnel hardware offload is not yet support

[Pablo Neira Ayuso]

No driver supports for IPIP tunnels yet, give up early on setting up the
hardware offload for this scenario.

This patch adds a stub that can be enhanced to add more configuration
that are currently not supported. As of now, the offload work is
enqueued to the worker, then ignored if the hardware offload
configuration is not supported.

This can be updated later on to skip hardware offload work to be queued
in case hardware offload does not support it.

Fixes: d98103575dcd ("netfilter: flowtable: Add IP6IP6 rx sw acceleration")
Fixes: ab427db17885 ("netfilter: flowtable: Add IPIP rx sw acceleration")
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Reported-by: Zhengyang Chen <chzhengyang2023@lzu.edu.cn>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_offload.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 002ec15d988b..3e87117e724b 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -1101,12 +1101,23 @@ nf_flow_offload_work_alloc(struct nf_flowtable *flowtable,
 	return offload;
 }
 
+static bool nf_flow_offload_unsupported(struct flow_offload *flow)
+{
+	if (flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.tun_num ||
+	    flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.tun_num)
+		return true;
+
+	return false;
+}
 
 void nf_flow_offload_add(struct nf_flowtable *flowtable,
 			 struct flow_offload *flow)
 {
 	struct flow_offload_work *offload;
 
+	if (nf_flow_offload_unsupported(flow))
+		return;
+
 	offload = nf_flow_offload_work_alloc(flowtable, flow, FLOW_CLS_REPLACE);
 	if (!offload)
 		return;
@@ -1119,6 +1130,9 @@ void nf_flow_offload_del(struct nf_flowtable *flowtable,
 {
 	struct flow_offload_work *offload;
 
+	if (nf_flow_offload_unsupported(flow))
+		return;
+
 	offload = nf_flow_offload_work_alloc(flowtable, flow, FLOW_CLS_DESTROY);
 	if (!offload)
 		return;
@@ -1133,6 +1147,9 @@ void nf_flow_offload_stats(struct nf_flowtable *flowtable,
 	struct flow_offload_work *offload;
 	__s32 delta;
 
+	if (nf_flow_offload_unsupported(flow))
+		return;
+
 	delta = nf_flow_timeout_delta(flow->timeout);
 	if ((delta >= (9 * flow_offload_get_timeout(flow)) / 10))
 		return;
-- 
2.47.3

[PATCH nf 3/3] netfilter: flowtable: support IPIP tunnel with direct xmit

[Pablo Neira Ayuso]

The combination of IPIP tunnel with direct xmit, eg. bridge device,
breaks because no dst_entry is provided to check the skb headroom and to
set the iph->frag_off field. This leads to invalid dst usage and can
trigger a crash in the tunnel transmit path.

Fix this by moving dst_cache and dst_cookie out of the runtime union so
that they can be shared by neighbour, xfrm, and direct tunnel flows.
For FLOW_OFFLOAD_XMIT_DIRECT tuples carrying tunnel metadata, preserve
route state in these shared fields and release it through the common
dst release path.

Since dst_entry is now available to the three supported xmit modes and
dst_release() already deals with NULL dst, remove the xmit type check
in nft_flow_dst_release(). Moreover, skip the check if the dst entry
is NULL in nf_flow_dst_check() which is now the case for the direct
xmit case.

Based on patch from Rein Wei <n05ec@lzu.edu.cn>.

Fixes: d30301ba4b07 ("netfilter: flowtable: Add IPIP tx sw acceleration")
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Reported-by: Zhengyang Chen <chzhengyang2023@lzu.edu.cn>
Reported-by: Ren Wei <n05ec@lzu.edu.cn>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_flow_table.h |  4 ++--
 net/netfilter/nf_flow_table_core.c    | 15 +++++++++++----
 net/netfilter/nf_flow_table_ip.c      |  3 +--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index 7b23b245a5a8..369f6a717811 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -155,11 +155,11 @@ struct flow_offload_tuple {
 					tun_num:2,
 					in_vlan_ingress:2;
 	u16				mtu;
+	struct dst_entry		*dst_cache;
+	u32				dst_cookie;
 	union {
 		struct {
-			struct dst_entry *dst_cache;
 			u32		ifidx;
-			u32		dst_cookie;
 		};
 		struct {
 			u32		ifidx;
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 99c5b9d671a0..6f195ccf222a 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -127,12 +127,21 @@ static int flow_offload_fill_route(struct flow_offload *flow,
 
 	switch (route->tuple[dir].xmit_type) {
 	case FLOW_OFFLOAD_XMIT_DIRECT:
+		if (flow_tuple->tun_num) {
+			flow_tuple->dst_cache = dst;
+			flow_tuple->dst_cookie =
+				flow_offload_dst_cookie(flow_tuple);
+		} else {
+			flow_tuple->dst_cache = NULL;
+			flow_tuple->dst_cookie = 0;
+		}
 		memcpy(flow_tuple->out.h_dest, route->tuple[dir].out.h_dest,
 		       ETH_ALEN);
 		memcpy(flow_tuple->out.h_source, route->tuple[dir].out.h_source,
 		       ETH_ALEN);
 		flow_tuple->out.ifidx = route->tuple[dir].out.ifindex;
-		dst_release(dst);
+		if (!flow_tuple->tun_num)
+			dst_release(dst);
 		break;
 	case FLOW_OFFLOAD_XMIT_XFRM:
 	case FLOW_OFFLOAD_XMIT_NEIGH:
@@ -152,9 +161,7 @@ static int flow_offload_fill_route(struct flow_offload *flow,
 static void nft_flow_dst_release(struct flow_offload *flow,
 				 enum flow_offload_tuple_dir dir)
 {
-	if (flow->tuplehash[dir].tuple.xmit_type == FLOW_OFFLOAD_XMIT_NEIGH ||
-	    flow->tuplehash[dir].tuple.xmit_type == FLOW_OFFLOAD_XMIT_XFRM)
-		dst_release(flow->tuplehash[dir].tuple.dst_cache);
+	dst_release(flow->tuplehash[dir].tuple.dst_cache);
 }
 
 void flow_offload_route_init(struct flow_offload *flow,
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 089f2bc19972..0b78decce8a9 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -299,8 +299,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
 
 static inline bool nf_flow_dst_check(struct flow_offload_tuple *tuple)
 {
-	if (tuple->xmit_type != FLOW_OFFLOAD_XMIT_NEIGH &&
-	    tuple->xmit_type != FLOW_OFFLOAD_XMIT_XFRM)
+	if (!tuple->dst_cache)
 		return true;
 
 	return dst_check(tuple->dst_cache, tuple->dst_cookie);
-- 
2.47.3

Re: [PATCH nf 3/3] netfilter: flowtable: support IPIP tunnel with direct xmit

[Lorenzo Bianconi]

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

On Jun 29, Pablo Neira Ayuso wrote:
> The combination of IPIP tunnel with direct xmit, eg. bridge device,
> breaks because no dst_entry is provided to check the skb headroom and to
> set the iph->frag_off field. This leads to invalid dst usage and can
> trigger a crash in the tunnel transmit path.
> 
> Fix this by moving dst_cache and dst_cookie out of the runtime union so
> that they can be shared by neighbour, xfrm, and direct tunnel flows.
> For FLOW_OFFLOAD_XMIT_DIRECT tuples carrying tunnel metadata, preserve
> route state in these shared fields and release it through the common
> dst release path.
> 
> Since dst_entry is now available to the three supported xmit modes and
> dst_release() already deals with NULL dst, remove the xmit type check
> in nft_flow_dst_release(). Moreover, skip the check if the dst entry
> is NULL in nf_flow_dst_check() which is now the case for the direct
> xmit case.
> 
> Based on patch from Rein Wei <n05ec@lzu.edu.cn>.
> 
> Fixes: d30301ba4b07 ("netfilter: flowtable: Add IPIP tx sw acceleration")
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Reported-by: Zhengyang Chen <chzhengyang2023@lzu.edu.cn>
> Reported-by: Ren Wei <n05ec@lzu.edu.cn>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  include/net/netfilter/nf_flow_table.h |  4 ++--
>  net/netfilter/nf_flow_table_core.c    | 15 +++++++++++----
>  net/netfilter/nf_flow_table_ip.c      |  3 +--
>  3 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index 7b23b245a5a8..369f6a717811 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -155,11 +155,11 @@ struct flow_offload_tuple {
>  					tun_num:2,
>  					in_vlan_ingress:2;
>  	u16				mtu;
> +	struct dst_entry		*dst_cache;
> +	u32				dst_cookie;

nit: if you swap dst_cache and dst_cookie we can avoid a 4 bytes padding hole.

>  	union {
>  		struct {
> -			struct dst_entry *dst_cache;
>  			u32		ifidx;
> -			u32		dst_cookie;
>  		};
>  		struct {
>  			u32		ifidx;
> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index 99c5b9d671a0..6f195ccf222a 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -127,12 +127,21 @@ static int flow_offload_fill_route(struct flow_offload *flow,
>  
>  	switch (route->tuple[dir].xmit_type) {
>  	case FLOW_OFFLOAD_XMIT_DIRECT:
> +		if (flow_tuple->tun_num) {
> +			flow_tuple->dst_cache = dst;
> +			flow_tuple->dst_cookie =
> +				flow_offload_dst_cookie(flow_tuple);
> +		} else {
> +			flow_tuple->dst_cache = NULL;
> +			flow_tuple->dst_cookie = 0;

nit: since we use kmem_cache_zalloc() in flow_offload_alloc() do we need the else
branch?

> +		}
>  		memcpy(flow_tuple->out.h_dest, route->tuple[dir].out.h_dest,
>  		       ETH_ALEN);
>  		memcpy(flow_tuple->out.h_source, route->tuple[dir].out.h_source,
>  		       ETH_ALEN);
>  		flow_tuple->out.ifidx = route->tuple[dir].out.ifindex;
> -		dst_release(dst);
> +		if (!flow_tuple->tun_num)
> +			dst_release(dst);
>  		break;
>  	case FLOW_OFFLOAD_XMIT_XFRM:
>  	case FLOW_OFFLOAD_XMIT_NEIGH:
> @@ -152,9 +161,7 @@ static int flow_offload_fill_route(struct flow_offload *flow,
>  static void nft_flow_dst_release(struct flow_offload *flow,
>  				 enum flow_offload_tuple_dir dir)
>  {
> -	if (flow->tuplehash[dir].tuple.xmit_type == FLOW_OFFLOAD_XMIT_NEIGH ||
> -	    flow->tuplehash[dir].tuple.xmit_type == FLOW_OFFLOAD_XMIT_XFRM)
> -		dst_release(flow->tuplehash[dir].tuple.dst_cache);
> +	dst_release(flow->tuplehash[dir].tuple.dst_cache);
>  }
>  
>  void flow_offload_route_init(struct flow_offload *flow,
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 089f2bc19972..0b78decce8a9 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -299,8 +299,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
>  
>  static inline bool nf_flow_dst_check(struct flow_offload_tuple *tuple)
>  {
> -	if (tuple->xmit_type != FLOW_OFFLOAD_XMIT_NEIGH &&
> -	    tuple->xmit_type != FLOW_OFFLOAD_XMIT_XFRM)
> +	if (!tuple->dst_cache)
>  		return true;
>  
>  	return dst_check(tuple->dst_cache, tuple->dst_cookie);
> -- 
> 2.47.3
> 

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

Re: [PATCH nf 2/3] netfilter: flowtable: IPIP tunnel hardware offload is not yet support

[Lorenzo Bianconi]

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

On Jun 29, Pablo Neira Ayuso wrote:
> No driver supports for IPIP tunnels yet, give up early on setting up the
> hardware offload for this scenario.
> 
> This patch adds a stub that can be enhanced to add more configuration
> that are currently not supported. As of now, the offload work is
> enqueued to the worker, then ignored if the hardware offload
> configuration is not supported.
> 
> This can be updated later on to skip hardware offload work to be queued
> in case hardware offload does not support it.
> 
> Fixes: d98103575dcd ("netfilter: flowtable: Add IP6IP6 rx sw acceleration")
> Fixes: ab427db17885 ("netfilter: flowtable: Add IPIP rx sw acceleration")
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Reported-by: Zhengyang Chen <chzhengyang2023@lzu.edu.cn>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  net/netfilter/nf_flow_table_offload.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 002ec15d988b..3e87117e724b 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -1101,12 +1101,23 @@ nf_flow_offload_work_alloc(struct nf_flowtable *flowtable,
>  	return offload;
>  }
>  
> +static bool nf_flow_offload_unsupported(struct flow_offload *flow)
> +{
> +	if (flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.tun_num ||
> +	    flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.tun_num)
> +		return true;
> +
> +	return false;
> +}
>  
>  void nf_flow_offload_add(struct nf_flowtable *flowtable,
>  			 struct flow_offload *flow)
>  {
>  	struct flow_offload_work *offload;
>  
> +	if (nf_flow_offload_unsupported(flow))
> +		return;
> +
>  	offload = nf_flow_offload_work_alloc(flowtable, flow, FLOW_CLS_REPLACE);
>  	if (!offload)
>  		return;
> @@ -1119,6 +1130,9 @@ void nf_flow_offload_del(struct nf_flowtable *flowtable,
>  {
>  	struct flow_offload_work *offload;
>  
> +	if (nf_flow_offload_unsupported(flow))
> +		return;
> +
>  	offload = nf_flow_offload_work_alloc(flowtable, flow, FLOW_CLS_DESTROY);
>  	if (!offload)
>  		return;
> @@ -1133,6 +1147,9 @@ void nf_flow_offload_stats(struct nf_flowtable *flowtable,
>  	struct flow_offload_work *offload;
>  	__s32 delta;
>  
> +	if (nf_flow_offload_unsupported(flow))
> +		return;
> +
>  	delta = nf_flow_timeout_delta(flow->timeout);
>  	if ((delta >= (9 * flow_offload_get_timeout(flow)) / 10))
>  		return;
> -- 
> 2.47.3
> 

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

Re: [PATCH nf 1/3] netfilter: flowtable: use dst in this direction when pushing IPIP header

[Lorenzo Bianconi]

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

On Jun 29, Pablo Neira Ayuso wrote:
> When pushing the IPIP header, the route of the other direction is used
> to calculate the headroom, use the route in this direction. Accessing
> the other tuple to set the IP source and destination is fine because
> this tuple does not provide such information to avoid storing redundant
> information. However, this tuple already provides the dst for this
> direction, this went unnoticed because this bug affects headroom and
> iph->frag_off only at this stage.
> 
> Fixes: d30301ba4b07 ("netfilter: flowtable: Add IPIP tx sw acceleration")
> Fixes: 93cf357fa797 ("netfilter: flowtable: Add IP6IP6 tx sw acceleration")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> ---
>  net/netfilter/nf_flow_table_ip.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 29e93ac1e2e4..089f2bc19972 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -590,10 +590,10 @@ 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,
> -				    __be32 *ip_daddr)
> +				    struct dst_entry *dst, __be32 *ip_daddr)
>  {
>  	struct iphdr *iph = (struct iphdr *)skb_network_header(skb);
> -	struct rtable *rt = dst_rtable(tuple->dst_cache);
> +	struct rtable *rt = dst_rtable(dst);
>  	u8 tos = iph->tos, ttl = iph->ttl;
>  	__be16 frag_off = iph->frag_off;
>  	u32 headroom = sizeof(*iph);
> @@ -636,21 +636,22 @@ static int nf_flow_tunnel_ipip_push(struct net *net, struct sk_buff *skb,
>  
>  static int nf_flow_tunnel_v4_push(struct net *net, struct sk_buff *skb,
>  				  struct flow_offload_tuple *tuple,
> -				  __be32 *ip_daddr)
> +				  struct dst_entry *dst,  __be32 *ip_daddr)
>  {
>  	if (tuple->tun_num)
> -		return nf_flow_tunnel_ipip_push(net, skb, tuple, ip_daddr);
> +		return nf_flow_tunnel_ipip_push(net, skb, tuple, dst, ip_daddr);
>  
>  	return 0;
>  }
>  
>  static int nf_flow_tunnel_ip6ip6_push(struct net *net, struct sk_buff *skb,
>  				      struct flow_offload_tuple *tuple,
> +				      struct dst_entry *dst,
>  				      struct in6_addr **ip6_daddr)
>  {
>  	struct ipv6hdr *ip6h = (struct ipv6hdr *)skb_network_header(skb);
> -	struct rtable *rt = dst_rtable(tuple->dst_cache);
>  	__u8 dsfield = ipv6_get_dsfield(ip6h);
> +	struct rtable *rt = dst_rtable(dst);
>  	struct flowi6 fl6 = {
>  		.daddr = tuple->tun.src_v6,
>  		.saddr = tuple->tun.dst_v6,
> @@ -696,10 +697,11 @@ static int nf_flow_tunnel_ip6ip6_push(struct net *net, struct sk_buff *skb,
>  
>  static int nf_flow_tunnel_v6_push(struct net *net, struct sk_buff *skb,
>  				  struct flow_offload_tuple *tuple,
> +				  struct dst_entry *dst,
>  				  struct in6_addr **ip6_daddr)
>  {
>  	if (tuple->tun_num)
> -		return nf_flow_tunnel_ip6ip6_push(net, skb, tuple, ip6_daddr);
> +		return nf_flow_tunnel_ip6ip6_push(net, skb, tuple, dst, ip6_daddr);
>  
>  	return 0;
>  }
> @@ -842,7 +844,8 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
>  	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, &ip_daddr) < 0)
> +	if (nf_flow_tunnel_v4_push(state->net, skb, other_tuple,
> +				   tuplehash->tuple.dst_cache, &ip_daddr) < 0)
>  		return NF_DROP;
>  
>  	switch (tuplehash->tuple.xmit_type) {
> @@ -1158,6 +1161,7 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
>  	ip6_daddr = &other_tuple->src_v6;
>  
>  	if (nf_flow_tunnel_v6_push(state->net, skb, other_tuple,
> +				   tuplehash->tuple.dst_cache,
>  				   &ip6_daddr) < 0)
>  		return NF_DROP;
>  
> -- 
> 2.47.3
> 

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

Re: [PATCH nf 0/3] flowtable fixes for ipip tunnels

[陈正阳]

  Hi Pablo,

  > Hi,
  >
  > The following patchset contains fixes for the flowtable ipip support:
  >
  > Patch #1 updates nf_flow_tunnel_ip{6}ip{6}_push() to use the dst_entry
  >          in this direction to calculate headroom and set iph->frag_off.
  >      Apparently, nft_flow_tunnel_update_route() sets the right
  >      dst_entry in this direction, but datapath uses the other direction.
  >
  > Patch #2 adds a function to bail out early on any attempt to hardware
  >      offload a flow entry which is not supported. Currently, ->num_tun
  >      has no drivers supporting this.

  One small attribution note for patch #2: I noticed that my name is listed
  as Reported-by in 0002. The final patch is reworked by you, but it appears
  to be based on the unsupported tunnel hardware offload handling from my v3
  submission. Since I was the patch author/contributor rather than the bug
  reporter for this part, could you please replace my Reported-by tag with:

  Co-developed-by: Zhengyang Chen <chzhengyang2023@lzu.edu.cn>
  Signed-off-by: Zhengyang Chen <chzhengyang2023@lzu.edu.cn>

  The Signed-off-by was already present in my v2/v3 submissions.

  > Patch #3 makes dst_entry available for all xmit modes which is required
  >      to reach the dst_entry when pushing the ipip header (see related
  >      patch #1). Based on patch from Rein Wein.

  Small correction here: Ren Wei helped review/send the patch on our side,
  but the original patch was authored by me, Zhengyang Chen
  <chzhengyang2023@lzu.edu.cn>.

  Patch #3 keeps the main logic from my v2/v3 submission, including moving
  dst_cache/dst_cookie out of the runtime union, preserving dst state for
  DIRECT tunnel flows, and releasing dst through the common path. So could
  this attribution please be changed to:

  Based on patch from Zhengyang Chen <chzhengyang2023@lzu.edu.cn>.

  Also, for 0003, could my Reported-by tag be replaced with the same
  Co-developed-by and Signed-off-by tags above? I do not mean to change the
  Reported-by tags for the original reporters; this is only about my own
  attribution.

  > This series is passing nft_flowtable.sh selftest here, including Ren Wei's
  > patch:
  >
  >   [nf,v3,2/2] selftests: netfilter: add bridge tunnel flowtable regression
  >   https://patchwork.ozlabs.org/project/netfilter-devel/patch/5b8a9e87ff7b47401612bb0e0fc841d8bfdd333d.1782092221.git.chzhengyang2023@lzu.edu.cn/
  >
  > which should be also be taken upstream.
  >
  > Pablo Neira Ayuso (3):
  >   netfilter: flowtable: use dst in this direction when pushing IPIP header
  >   netfilter: flowtable: IPIP tunnel hardware offload is not yet support
  >   netfilter: flowtable: support IPIP tunnel with direct xmit
  >
  >  include/net/netfilter/nf_flow_table.h |  4 ++--
  >  net/netfilter/nf_flow_table_core.c    | 15 +++++++++++----
  >  net/netfilter/nf_flow_table_ip.c      | 21 ++++++++++++---------
  >  net/netfilter/nf_flow_table_offload.c | 17 +++++++++++++++++
  >  4 files changed, 42 insertions(+), 15 deletions(-)
  >
  > --
  > 2.47.3

  Thanks for picking this up and for the rework/testing. I only wanted to
  clarify my specific contributions before the series moves further upstream.

  Best regards,
  Zhengyang