[PATCH nf-next 0/4] flowtable: pass dst_entry from ipip tunnel

[PATCH nf-next 0/4] flowtable: pass dst_entry from ipip tunnel

[Pablo Neira Ayuso]

Hi,

This is revisiting the existing approach to avoid a double route lookup
and avoid a late failure to obtain the ipip route leading to an
incomplete forward path.

This is complementary work to Lorenzo's recent updates for .fill_forward_path
for ipip and ip6ip6 to make a more exhaustive route lookup.

Pablo Neira Ayuso (4):
  netfilter: flowtable: consolidate net_device field in nft_forward_info struct
  netfilter: flowtable: consolidate flowtable device check
  net: pass dst via net_device_path in dev_fill_forward_path()
  netfilter: flowtable: release tunnel route on error when building
    forward path

 include/linux/netdevice.h          |  1 +
 net/core/dev.c                     | 45 +++++++++++---
 net/ipv4/ipip.c                    |  2 +-
 net/ipv6/ip6_tunnel.c              |  2 +-
 net/netfilter/nf_flow_table_path.c | 95 ++++++++++--------------------
 5 files changed, 71 insertions(+), 74 deletions(-)

-- 
2.47.3

[PATCH nf-next 1/4] netfilter: flowtable: consolidate net_device field in nft_forward_info struct

[Pablo Neira Ayuso]

info->indev and info->outdev refer to the same device, a single
info->dev field is sufficient.

While at it, remove unused router parameter from the flowtable path
discovery function.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_path.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 98c03b487f52..261bb44d08eb 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -42,8 +42,7 @@ static bool nft_is_valid_ether_device(const struct net_device *dev)
 	return true;
 }
 
-static int nft_dev_fill_forward_path(const struct nf_flow_route *route,
-				     const struct dst_entry *dst_cache,
+static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
 				     const struct nf_conn *ct,
 				     enum ip_conntrack_dir dir, u8 *ha,
 				     struct net_device_path_stack *stack)
@@ -76,8 +75,7 @@ static int nft_dev_fill_forward_path(const struct nf_flow_route *route,
 }
 
 struct nft_forward_info {
-	const struct net_device *indev;
-	const struct net_device *outdev;
+	const struct net_device *dev;
 	struct id {
 		__u16	id;
 		__be16	proto;
@@ -109,7 +107,7 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 		case DEV_PATH_VLAN:
 		case DEV_PATH_PPPOE:
 		case DEV_PATH_TUN:
-			info->indev = path->dev;
+			info->dev = path->dev;
 			if (is_zero_ether_addr(info->h_source))
 				memcpy(info->h_source, path->dev->dev_addr, ETH_ALEN);
 
@@ -179,10 +177,9 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 			return -1;
 		}
 	}
-	info->outdev = info->indev;
 
 	if (nf_flowtable_hw_offload(flowtable) &&
-	    nft_is_valid_ether_device(info->indev))
+	    nft_is_valid_ether_device(info->dev))
 		info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
 
 	return 0;
@@ -255,17 +252,16 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 	unsigned char ha[ETH_ALEN];
 	int i;
 
-	if (nft_dev_fill_forward_path(route, dst, ct, dir, ha, &stack) < 0 ||
+	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
 	    nft_dev_path_info(&stack, &info, ha, &ft->data) < 0)
 		return -ENOENT;
 
-	if (!nft_flowtable_find_dev(info.indev, ft))
+	if (!nft_flowtable_find_dev(info.dev, ft))
 		return -ENOENT;
 
-	if (info.outdev)
-		route->tuple[dir].out.ifindex = info.outdev->ifindex;
+	route->tuple[!dir].in.ifindex = info.dev->ifindex;
+	route->tuple[dir].out.ifindex = info.dev->ifindex;
 
-	route->tuple[!dir].in.ifindex = info.indev->ifindex;
 	for (i = 0; i < info.num_encaps; i++) {
 		route->tuple[!dir].in.encap[i].id = info.encap[i].id;
 		route->tuple[!dir].in.encap[i].proto = info.encap[i].proto;
-- 
2.47.3

Re: [PATCH nf-next 1/4] netfilter: flowtable: consolidate net_device field in nft_forward_info struct

[Lorenzo Bianconi]

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

> info->indev and info->outdev refer to the same device, a single
> info->dev field is sufficient.
> 
> While at it, remove unused router parameter from the flowtable path
> discovery function.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

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

> ---
>  net/netfilter/nf_flow_table_path.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> index 98c03b487f52..261bb44d08eb 100644
> --- a/net/netfilter/nf_flow_table_path.c
> +++ b/net/netfilter/nf_flow_table_path.c
> @@ -42,8 +42,7 @@ static bool nft_is_valid_ether_device(const struct net_device *dev)
>  	return true;
>  }
>  
> -static int nft_dev_fill_forward_path(const struct nf_flow_route *route,
> -				     const struct dst_entry *dst_cache,
> +static int nft_dev_fill_forward_path(const struct dst_entry *dst_cache,
>  				     const struct nf_conn *ct,
>  				     enum ip_conntrack_dir dir, u8 *ha,
>  				     struct net_device_path_stack *stack)
> @@ -76,8 +75,7 @@ static int nft_dev_fill_forward_path(const struct nf_flow_route *route,
>  }
>  
>  struct nft_forward_info {
> -	const struct net_device *indev;
> -	const struct net_device *outdev;
> +	const struct net_device *dev;
>  	struct id {
>  		__u16	id;
>  		__be16	proto;
> @@ -109,7 +107,7 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  		case DEV_PATH_VLAN:
>  		case DEV_PATH_PPPOE:
>  		case DEV_PATH_TUN:
> -			info->indev = path->dev;
> +			info->dev = path->dev;
>  			if (is_zero_ether_addr(info->h_source))
>  				memcpy(info->h_source, path->dev->dev_addr, ETH_ALEN);
>  
> @@ -179,10 +177,9 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  			return -1;
>  		}
>  	}
> -	info->outdev = info->indev;
>  
>  	if (nf_flowtable_hw_offload(flowtable) &&
> -	    nft_is_valid_ether_device(info->indev))
> +	    nft_is_valid_ether_device(info->dev))
>  		info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
>  
>  	return 0;
> @@ -255,17 +252,16 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  	unsigned char ha[ETH_ALEN];
>  	int i;
>  
> -	if (nft_dev_fill_forward_path(route, dst, ct, dir, ha, &stack) < 0 ||
> +	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
>  	    nft_dev_path_info(&stack, &info, ha, &ft->data) < 0)
>  		return -ENOENT;
>  
> -	if (!nft_flowtable_find_dev(info.indev, ft))
> +	if (!nft_flowtable_find_dev(info.dev, ft))
>  		return -ENOENT;
>  
> -	if (info.outdev)
> -		route->tuple[dir].out.ifindex = info.outdev->ifindex;
> +	route->tuple[!dir].in.ifindex = info.dev->ifindex;
> +	route->tuple[dir].out.ifindex = info.dev->ifindex;
>  
> -	route->tuple[!dir].in.ifindex = info.indev->ifindex;
>  	for (i = 0; i < info.num_encaps; i++) {
>  		route->tuple[!dir].in.encap[i].id = info.encap[i].id;
>  		route->tuple[!dir].in.encap[i].proto = info.encap[i].proto;
> -- 
> 2.47.3
> 

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

[PATCH nf-next 2/4] netfilter: flowtable: consolidate flowtable device check

[Pablo Neira Ayuso]

Check that device belongs to the flowtable right after the flowtable
discovery path. This is a preparation patch to obtain the dst entry
from the .fill_forward_path in tunnels.

No functional changes are intended.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_path.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 261bb44d08eb..8f04a4487897 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -90,9 +90,12 @@ struct nft_forward_info {
 	enum flow_offload_xmit_type xmit_type;
 };
 
+static bool nft_flowtable_find_dev(const struct net_device *dev,
+				   struct nft_flowtable *ft);
+
 static int nft_dev_path_info(const struct net_device_path_stack *stack,
 			     struct nft_forward_info *info,
-			     unsigned char *ha, struct nf_flowtable *flowtable)
+			     unsigned char *ha, struct nft_flowtable *ft)
 {
 	const struct net_device_path *path;
 	int i;
@@ -178,10 +181,13 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 		}
 	}
 
-	if (nf_flowtable_hw_offload(flowtable) &&
+	if (nf_flowtable_hw_offload(&ft->data) &&
 	    nft_is_valid_ether_device(info->dev))
 		info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
 
+	if (!nft_flowtable_find_dev(info->dev, ft))
+		return -1;
+
 	return 0;
 }
 
@@ -253,10 +259,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 	int i;
 
 	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
-	    nft_dev_path_info(&stack, &info, ha, &ft->data) < 0)
-		return -ENOENT;
-
-	if (!nft_flowtable_find_dev(info.dev, ft))
+	    nft_dev_path_info(&stack, &info, ha, ft) < 0)
 		return -ENOENT;
 
 	route->tuple[!dir].in.ifindex = info.dev->ifindex;
-- 
2.47.3

Re: [PATCH nf-next 2/4] netfilter: flowtable: consolidate flowtable device check

[Lorenzo Bianconi]

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

> Check that device belongs to the flowtable right after the flowtable
> discovery path. This is a preparation patch to obtain the dst entry
> from the .fill_forward_path in tunnels.
> 
> No functional changes are intended.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

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

> ---
>  net/netfilter/nf_flow_table_path.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> index 261bb44d08eb..8f04a4487897 100644
> --- a/net/netfilter/nf_flow_table_path.c
> +++ b/net/netfilter/nf_flow_table_path.c
> @@ -90,9 +90,12 @@ struct nft_forward_info {
>  	enum flow_offload_xmit_type xmit_type;
>  };
>  
> +static bool nft_flowtable_find_dev(const struct net_device *dev,
> +				   struct nft_flowtable *ft);
> +
>  static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  			     struct nft_forward_info *info,
> -			     unsigned char *ha, struct nf_flowtable *flowtable)
> +			     unsigned char *ha, struct nft_flowtable *ft)
>  {
>  	const struct net_device_path *path;
>  	int i;
> @@ -178,10 +181,13 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  		}
>  	}
>  
> -	if (nf_flowtable_hw_offload(flowtable) &&
> +	if (nf_flowtable_hw_offload(&ft->data) &&
>  	    nft_is_valid_ether_device(info->dev))
>  		info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
>  
> +	if (!nft_flowtable_find_dev(info->dev, ft))
> +		return -1;
> +
>  	return 0;
>  }
>  
> @@ -253,10 +259,7 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  	int i;
>  
>  	if (nft_dev_fill_forward_path(dst, ct, dir, ha, &stack) < 0 ||
> -	    nft_dev_path_info(&stack, &info, ha, &ft->data) < 0)
> -		return -ENOENT;
> -
> -	if (!nft_flowtable_find_dev(info.dev, ft))
> +	    nft_dev_path_info(&stack, &info, ha, ft) < 0)
>  		return -ENOENT;
>  
>  	route->tuple[!dir].in.ifindex = info.dev->ifindex;
> -- 
> 2.47.3
> 

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

[PATCH nf-next 3/4] net: pass dst via net_device_path in dev_fill_forward_path()

[Pablo Neira Ayuso]

Add dst_entry to tunnel device path, this will allow us to remove
a duplicated route lookup.

This is a preparation patch to retrieve the tunnel route directly
from the .fill_forward_path. This new dst_entry in the tunnel will be
used by a follow up patch.

Since dst_release() works fine on NULL interface, this is still
noop until the flowtable starts using this.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/netdevice.h |  1 +
 net/core/dev.c            | 45 +++++++++++++++++++++++++++++++--------
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8db25b79573e..30dc78b4f1a3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -902,6 +902,7 @@ struct net_device_path {
 			};
 
 			u8	l3_proto;
+			struct dst_entry *dst;
 		} tun;
 		struct {
 			enum {
diff --git a/net/core/dev.c b/net/core/dev.c
index c1c1be1a6962..fed101f54a8a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -750,6 +750,23 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
 	return &stack->path[k];
 }
 
+static void dev_fill_forward_path_release(struct net_device_path_stack *stack)
+{
+	struct net_device_path *path;
+	int k;
+
+	for (k = stack->num_paths; k >= 0; k--) {
+		path = &stack->path[k];
+		switch (path->type) {
+		case DEV_PATH_TUN:
+			dst_release(path->tun.dst);
+			break;
+		default:
+			break;
+		}
+	}
+}
+
 int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
 			  struct net_device_path_stack *stack)
 {
@@ -765,28 +782,38 @@ int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
 	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
 		last_dev = ctx.dev;
 		path = dev_fwd_path(stack);
-		if (!path)
-			return -1;
+		if (!path) {
+			ret = -1;
+			goto err_out;
+		}
 
 		memset(path, 0, sizeof(struct net_device_path));
 		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
 		if (ret < 0)
-			return -1;
+			goto err_out;
 
-		if (WARN_ON_ONCE(last_dev == ctx.dev))
-			return -1;
+		if (WARN_ON_ONCE(last_dev == ctx.dev)) {
+			ret = -1;
+			goto err_out;
+		}
 	}
 
 	if (!ctx.dev)
-		return ret;
+		goto err_out;
 
 	path = dev_fwd_path(stack);
-	if (!path)
-		return -1;
+	if (!path) {
+		ret = -1;
+		goto err_out;
+	}
 	path->type = DEV_PATH_ETHERNET;
 	path->dev = ctx.dev;
 
-	return ret;
+	return 0;
+err_out:
+	dev_fill_forward_path_release(stack);
+
+	return -1;
 }
 EXPORT_SYMBOL_GPL(dev_fill_forward_path);
 
-- 
2.47.3

Re: [PATCH nf-next 3/4] net: pass dst via net_device_path in dev_fill_forward_path()

[Lorenzo Bianconi]

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

> Add dst_entry to tunnel device path, this will allow us to remove
> a duplicated route lookup.
> 
> This is a preparation patch to retrieve the tunnel route directly
> from the .fill_forward_path. This new dst_entry in the tunnel will be
> used by a follow up patch.
> 
> Since dst_release() works fine on NULL interface, this is still
> noop until the flowtable starts using this.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

just few nits inline. Fixing them:

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

> ---
>  include/linux/netdevice.h |  1 +
>  net/core/dev.c            | 45 +++++++++++++++++++++++++++++++--------
>  2 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8db25b79573e..30dc78b4f1a3 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -902,6 +902,7 @@ struct net_device_path {
>  			};
>  
>  			u8	l3_proto;
> +			struct dst_entry *dst;

nit: I guess if you move dst_entry pointer at the beginning of the tun struct
we avoid 7 bytes compiler padding.

>  		} tun;
>  		struct {
>  			enum {
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c1c1be1a6962..fed101f54a8a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -750,6 +750,23 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
>  	return &stack->path[k];
>  }
>  
> +static void dev_fill_forward_path_release(struct net_device_path_stack *stack)
> +{
> +	struct net_device_path *path;
> +	int k;
> +
> +	for (k = stack->num_paths; k >= 0; k--) {
> +		path = &stack->path[k];
> +		switch (path->type) {
> +		case DEV_PATH_TUN:
> +			dst_release(path->tun.dst);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
>  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>  			  struct net_device_path_stack *stack)
>  {
> @@ -765,28 +782,38 @@ int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>  	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
>  		last_dev = ctx.dev;
>  		path = dev_fwd_path(stack);
> -		if (!path)
> -			return -1;
> +		if (!path) {
> +			ret = -1;

nit: I guess we can drop ret variable now since we do not use it in the return
path.

> +			goto err_out;
> +		}
>  
>  		memset(path, 0, sizeof(struct net_device_path));
>  		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
>  		if (ret < 0)
> -			return -1;
> +			goto err_out;
>  
> -		if (WARN_ON_ONCE(last_dev == ctx.dev))
> -			return -1;
> +		if (WARN_ON_ONCE(last_dev == ctx.dev)) {
> +			ret = -1;
> +			goto err_out;
> +		}
>  	}
>  
>  	if (!ctx.dev)
> -		return ret;
> +		goto err_out;
>  
>  	path = dev_fwd_path(stack);
> -	if (!path)
> -		return -1;
> +	if (!path) {
> +		ret = -1;
> +		goto err_out;
> +	}
>  	path->type = DEV_PATH_ETHERNET;
>  	path->dev = ctx.dev;
>  
> -	return ret;
> +	return 0;
> +err_out:
> +	dev_fill_forward_path_release(stack);
> +
> +	return -1;
>  }
>  EXPORT_SYMBOL_GPL(dev_fill_forward_path);
>  
> -- 
> 2.47.3
> 

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

[PATCH nf-next 4/4] netfilter: flowtable: release tunnel route on error when building forward path

[Pablo Neira Ayuso]

nft_flow_tunnel_update_route() can lazy fail, leaving an incomplete
forward path set ip. The route lookup also happens twice, once from
dev_fill_forward_path() and again in this aforementioned function.

Update ipip and ip6ip6 not to release the dst_entry and pass it on
via the tunnel forward path information.

In case of failure when setting up the forwarding path, release the
tunnel dst that was provided via dev_fill_forward_path().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv4/ipip.c                    |  2 +-
 net/ipv6/ip6_tunnel.c              |  2 +-
 net/netfilter/nf_flow_table_path.c | 62 ++++++++----------------------
 3 files changed, 18 insertions(+), 48 deletions(-)

diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
index d1aa048a6099..0a4fa1351dd7 100644
--- a/net/ipv4/ipip.c
+++ b/net/ipv4/ipip.c
@@ -370,10 +370,10 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
 	path->tun.src_v4.s_addr = tiph->saddr;
 	path->tun.dst_v4.s_addr = tiph->daddr;
 	path->tun.l3_proto = IPPROTO_IPIP;
+	path->tun.dst = &rt->dst;
 	path->dev = ctx->dev;
 
 	ctx->dev = rt->dst.dev;
-	ip_rt_put(rt);
 
 	return 0;
 }
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index bf8e40af60b0..312de4a6e160 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1864,12 +1864,12 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
 		path->tun.src_v6 = t->parms.laddr;
 		path->tun.dst_v6 = t->parms.raddr;
 		path->tun.l3_proto = IPPROTO_IPV6;
+		path->tun.dst = dst;
 		path->dev = ctx->dev;
 		ctx->dev = dst->dev;
 	}
 
 	err = dst->error;
-	dst_release(dst);
 
 	return err;
 }
diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
index 8f04a4487897..9f6fc29c4e16 100644
--- a/net/netfilter/nf_flow_table_path.c
+++ b/net/netfilter/nf_flow_table_path.c
@@ -82,6 +82,7 @@ struct nft_forward_info {
 	} encap[NF_FLOW_TABLE_ENCAP_MAX];
 	u8 num_encaps;
 	struct flow_offload_tunnel tun;
+	struct dst_entry *tun_dst;
 	u8 num_tuns;
 	u8 ingress_vlans;
 	u8 h_source[ETH_ALEN];
@@ -124,15 +125,16 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 			/* DEV_PATH_VLAN, DEV_PATH_PPPOE and DEV_PATH_TUN */
 			if (path->type == DEV_PATH_TUN) {
 				if (info->num_tuns)
-					return -1;
+					goto err_out;
 
 				info->tun.src_v6 = path->tun.src_v6;
 				info->tun.dst_v6 = path->tun.dst_v6;
 				info->tun.l3_proto = path->tun.l3_proto;
+				info->tun_dst = path->tun.dst;
 				info->num_tuns++;
 			} else {
 				if (info->num_encaps >= NF_FLOW_TABLE_ENCAP_MAX)
-					return -1;
+					goto err_out;
 
 				info->encap[info->num_encaps].id =
 					path->encap.id;
@@ -153,13 +155,13 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 			switch (path->bridge.vlan_mode) {
 			case DEV_PATH_BR_VLAN_UNTAG_HW:
 				if (info->num_encaps == 0)
-					return -1;
+					goto err_out;
 
 				info->ingress_vlans |= BIT(info->num_encaps - 1);
 				break;
 			case DEV_PATH_BR_VLAN_TAG:
 				if (info->num_encaps >= NF_FLOW_TABLE_ENCAP_MAX)
-					return -1;
+					goto err_out;
 
 				info->encap[info->num_encaps].id = path->bridge.vlan_id;
 				info->encap[info->num_encaps].proto = path->bridge.vlan_proto;
@@ -167,7 +169,7 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 				break;
 			case DEV_PATH_BR_VLAN_UNTAG:
 				if (info->num_encaps == 0)
-					return -1;
+					goto err_out;
 
 				info->num_encaps--;
 				break;
@@ -177,7 +179,7 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 			info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
 			break;
 		default:
-			return -1;
+			goto err_out;
 		}
 	}
 
@@ -189,6 +191,11 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
 		return -1;
 
 	return 0;
+err_out:
+	if (info->num_tuns)
+		dst_release(info->tun_dst);
+
+	return -1;
 }
 
 static bool nft_flowtable_find_dev(const struct net_device *dev,
@@ -208,44 +215,6 @@ static bool nft_flowtable_find_dev(const struct net_device *dev,
 	return found;
 }
 
-static int nft_flow_tunnel_update_route(const struct nft_pktinfo *pkt,
-					struct flow_offload_tunnel *tun,
-					struct nf_flow_route *route,
-					enum ip_conntrack_dir dir)
-{
-	struct dst_entry *cur_dst = route->tuple[dir].dst;
-	struct dst_entry *tun_dst = NULL;
-	struct flowi fl = {};
-
-	switch (nft_pf(pkt)) {
-	case NFPROTO_IPV4:
-		fl.u.ip4.daddr = tun->dst_v4.s_addr;
-		fl.u.ip4.saddr = tun->src_v4.s_addr;
-		fl.u.ip4.flowi4_iif = nft_in(pkt)->ifindex;
-		fl.u.ip4.flowi4_dscp = ip4h_dscp(ip_hdr(pkt->skb));
-		fl.u.ip4.flowi4_mark = pkt->skb->mark;
-		fl.u.ip4.flowi4_flags = FLOWI_FLAG_ANYSRC;
-		break;
-	case NFPROTO_IPV6:
-		fl.u.ip6.daddr = tun->dst_v6;
-		fl.u.ip6.saddr = tun->src_v6;
-		fl.u.ip6.flowi6_iif = nft_in(pkt)->ifindex;
-		fl.u.ip6.flowlabel = ip6_flowinfo(ipv6_hdr(pkt->skb));
-		fl.u.ip6.flowi6_mark = pkt->skb->mark;
-		fl.u.ip6.flowi6_flags = FLOWI_FLAG_ANYSRC;
-		break;
-	}
-
-	nf_route(nft_net(pkt), &tun_dst, &fl, false, nft_pf(pkt));
-	if (!tun_dst)
-		return -ENOENT;
-
-	route->tuple[dir].dst = tun_dst;
-	dst_release(cur_dst);
-
-	return 0;
-}
-
 static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 				struct nf_flow_route *route,
 				const struct nf_conn *ct,
@@ -270,12 +239,13 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
 		route->tuple[!dir].in.encap[i].proto = info.encap[i].proto;
 	}
 
-	if (info.num_tuns &&
-	    !nft_flow_tunnel_update_route(pkt, &info.tun, route, dir)) {
+	if (info.num_tuns) {
 		route->tuple[!dir].in.tun.src_v6 = info.tun.dst_v6;
 		route->tuple[!dir].in.tun.dst_v6 = info.tun.src_v6;
 		route->tuple[!dir].in.tun.l3_proto = info.tun.l3_proto;
 		route->tuple[!dir].in.num_tuns = info.num_tuns;
+		dst_release(route->tuple[dir].dst);
+		route->tuple[dir].dst = info.tun_dst;
 	}
 
 	route->tuple[!dir].in.num_encaps = info.num_encaps;
-- 
2.47.3

Re: [PATCH nf-next 4/4] netfilter: flowtable: release tunnel route on error when building forward path

[Lorenzo Bianconi]

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

> nft_flow_tunnel_update_route() can lazy fail, leaving an incomplete
> forward path set ip. The route lookup also happens twice, once from
> dev_fill_forward_path() and again in this aforementioned function.
> 
> Update ipip and ip6ip6 not to release the dst_entry and pass it on
> via the tunnel forward path information.
> 
> In case of failure when setting up the forwarding path, release the
> tunnel dst that was provided via dev_fill_forward_path().
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

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

> ---
>  net/ipv4/ipip.c                    |  2 +-
>  net/ipv6/ip6_tunnel.c              |  2 +-
>  net/netfilter/nf_flow_table_path.c | 62 ++++++++----------------------
>  3 files changed, 18 insertions(+), 48 deletions(-)
> 
> diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c
> index d1aa048a6099..0a4fa1351dd7 100644
> --- a/net/ipv4/ipip.c
> +++ b/net/ipv4/ipip.c
> @@ -370,10 +370,10 @@ static int ipip_fill_forward_path(struct net_device_path_ctx *ctx,
>  	path->tun.src_v4.s_addr = tiph->saddr;
>  	path->tun.dst_v4.s_addr = tiph->daddr;
>  	path->tun.l3_proto = IPPROTO_IPIP;
> +	path->tun.dst = &rt->dst;
>  	path->dev = ctx->dev;
>  
>  	ctx->dev = rt->dst.dev;
> -	ip_rt_put(rt);
>  
>  	return 0;
>  }
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index bf8e40af60b0..312de4a6e160 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1864,12 +1864,12 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
>  		path->tun.src_v6 = t->parms.laddr;
>  		path->tun.dst_v6 = t->parms.raddr;
>  		path->tun.l3_proto = IPPROTO_IPV6;
> +		path->tun.dst = dst;
>  		path->dev = ctx->dev;
>  		ctx->dev = dst->dev;
>  	}
>  
>  	err = dst->error;
> -	dst_release(dst);
>  
>  	return err;
>  }
> diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> index 8f04a4487897..9f6fc29c4e16 100644
> --- a/net/netfilter/nf_flow_table_path.c
> +++ b/net/netfilter/nf_flow_table_path.c
> @@ -82,6 +82,7 @@ struct nft_forward_info {
>  	} encap[NF_FLOW_TABLE_ENCAP_MAX];
>  	u8 num_encaps;
>  	struct flow_offload_tunnel tun;
> +	struct dst_entry *tun_dst;
>  	u8 num_tuns;
>  	u8 ingress_vlans;
>  	u8 h_source[ETH_ALEN];
> @@ -124,15 +125,16 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  			/* DEV_PATH_VLAN, DEV_PATH_PPPOE and DEV_PATH_TUN */
>  			if (path->type == DEV_PATH_TUN) {
>  				if (info->num_tuns)
> -					return -1;
> +					goto err_out;
>  
>  				info->tun.src_v6 = path->tun.src_v6;
>  				info->tun.dst_v6 = path->tun.dst_v6;
>  				info->tun.l3_proto = path->tun.l3_proto;
> +				info->tun_dst = path->tun.dst;
>  				info->num_tuns++;
>  			} else {
>  				if (info->num_encaps >= NF_FLOW_TABLE_ENCAP_MAX)
> -					return -1;
> +					goto err_out;
>  
>  				info->encap[info->num_encaps].id =
>  					path->encap.id;
> @@ -153,13 +155,13 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  			switch (path->bridge.vlan_mode) {
>  			case DEV_PATH_BR_VLAN_UNTAG_HW:
>  				if (info->num_encaps == 0)
> -					return -1;
> +					goto err_out;
>  
>  				info->ingress_vlans |= BIT(info->num_encaps - 1);
>  				break;
>  			case DEV_PATH_BR_VLAN_TAG:
>  				if (info->num_encaps >= NF_FLOW_TABLE_ENCAP_MAX)
> -					return -1;
> +					goto err_out;
>  
>  				info->encap[info->num_encaps].id = path->bridge.vlan_id;
>  				info->encap[info->num_encaps].proto = path->bridge.vlan_proto;
> @@ -167,7 +169,7 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  				break;
>  			case DEV_PATH_BR_VLAN_UNTAG:
>  				if (info->num_encaps == 0)
> -					return -1;
> +					goto err_out;
>  
>  				info->num_encaps--;
>  				break;
> @@ -177,7 +179,7 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  			info->xmit_type = FLOW_OFFLOAD_XMIT_DIRECT;
>  			break;
>  		default:
> -			return -1;
> +			goto err_out;
>  		}
>  	}
>  
> @@ -189,6 +191,11 @@ static int nft_dev_path_info(const struct net_device_path_stack *stack,
>  		return -1;
>  
>  	return 0;
> +err_out:
> +	if (info->num_tuns)
> +		dst_release(info->tun_dst);
> +
> +	return -1;
>  }
>  
>  static bool nft_flowtable_find_dev(const struct net_device *dev,
> @@ -208,44 +215,6 @@ static bool nft_flowtable_find_dev(const struct net_device *dev,
>  	return found;
>  }
>  
> -static int nft_flow_tunnel_update_route(const struct nft_pktinfo *pkt,
> -					struct flow_offload_tunnel *tun,
> -					struct nf_flow_route *route,
> -					enum ip_conntrack_dir dir)
> -{
> -	struct dst_entry *cur_dst = route->tuple[dir].dst;
> -	struct dst_entry *tun_dst = NULL;
> -	struct flowi fl = {};
> -
> -	switch (nft_pf(pkt)) {
> -	case NFPROTO_IPV4:
> -		fl.u.ip4.daddr = tun->dst_v4.s_addr;
> -		fl.u.ip4.saddr = tun->src_v4.s_addr;
> -		fl.u.ip4.flowi4_iif = nft_in(pkt)->ifindex;
> -		fl.u.ip4.flowi4_dscp = ip4h_dscp(ip_hdr(pkt->skb));
> -		fl.u.ip4.flowi4_mark = pkt->skb->mark;
> -		fl.u.ip4.flowi4_flags = FLOWI_FLAG_ANYSRC;
> -		break;
> -	case NFPROTO_IPV6:
> -		fl.u.ip6.daddr = tun->dst_v6;
> -		fl.u.ip6.saddr = tun->src_v6;
> -		fl.u.ip6.flowi6_iif = nft_in(pkt)->ifindex;
> -		fl.u.ip6.flowlabel = ip6_flowinfo(ipv6_hdr(pkt->skb));
> -		fl.u.ip6.flowi6_mark = pkt->skb->mark;
> -		fl.u.ip6.flowi6_flags = FLOWI_FLAG_ANYSRC;
> -		break;
> -	}
> -
> -	nf_route(nft_net(pkt), &tun_dst, &fl, false, nft_pf(pkt));
> -	if (!tun_dst)
> -		return -ENOENT;
> -
> -	route->tuple[dir].dst = tun_dst;
> -	dst_release(cur_dst);
> -
> -	return 0;
> -}
> -
>  static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  				struct nf_flow_route *route,
>  				const struct nf_conn *ct,
> @@ -270,12 +239,13 @@ static int nft_dev_forward_path(const struct nft_pktinfo *pkt,
>  		route->tuple[!dir].in.encap[i].proto = info.encap[i].proto;
>  	}
>  
> -	if (info.num_tuns &&
> -	    !nft_flow_tunnel_update_route(pkt, &info.tun, route, dir)) {
> +	if (info.num_tuns) {
>  		route->tuple[!dir].in.tun.src_v6 = info.tun.dst_v6;
>  		route->tuple[!dir].in.tun.dst_v6 = info.tun.src_v6;
>  		route->tuple[!dir].in.tun.l3_proto = info.tun.l3_proto;
>  		route->tuple[!dir].in.num_tuns = info.num_tuns;
> +		dst_release(route->tuple[dir].dst);
> +		route->tuple[dir].dst = info.tun_dst;
>  	}
>  
>  	route->tuple[!dir].in.num_encaps = info.num_encaps;
> -- 
> 2.47.3
> 

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