Re: [PATCH net-next 2/2] vrf: run conntrack only in context of lower/physdev for locally generated packets

[Eugene Crosser <crosser@average.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3919 bytes --]

On 21/10/2021 16:48, Florian Westphal wrote:
> The VRF driver invokes netfilter for output+postrouting hooks so that users
> can create rules that check for 'oif $vrf' rather than lower device name.
> 
> This is a problem when NAT rules are configured.
> 
> To avoid any conntrack involvement in round 1, tag skbs as 'untracked'
> to prevent conntrack from picking them up.
> 
> This gets cleared before the packet gets handed to the ip stack so
> conntrack will be active on the second iteration.
> 
> For ingress, conntrack has already been done before the packet makes it
> to the vrf driver, with this patch egress does connection tracking with
> lower/physical device as well.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  drivers/net/vrf.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index bf2fac913942..c813d03159bf 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -35,6 +35,7 @@
>  #include <net/l3mdev.h>
>  #include <net/fib_rules.h>
>  #include <net/netns/generic.h>
> +#include <net/netfilter/nf_conntrack.h>
>  
>  #define DRV_NAME	"vrf"
>  #define DRV_VERSION	"1.1"
> @@ -424,12 +425,26 @@ static int vrf_local_xmit(struct sk_buff *skb, struct net_device *dev,
>  	return NETDEV_TX_OK;
>  }
>  
> +static void vrf_nf_set_untracked(struct sk_buff *skb)
> +{
> +	if (skb_get_nfct(skb) == 0)
> +		nf_ct_set(skb, 0, IP_CT_UNTRACKED);
> +}
> +
> +static void vrf_nf_reset_ct(struct sk_buff *skb)
> +{
> +	if (skb_get_nfct(skb) == IP_CT_UNTRACKED)
> +		nf_reset_ct(skb);
> +}
> +

Isn't it possible that skb was marked UNTRACKED before entering this path, by a
rule? In such case 'set_untrackd' will do nothing, but 'reset_ct' will clear
UNTRACKED status that was set elswhere. It seems wrong, am I missing something?

>  #if IS_ENABLED(CONFIG_IPV6)
>  static int vrf_ip6_local_out(struct net *net, struct sock *sk,
>  			     struct sk_buff *skb)
>  {
>  	int err;
>  
> +	vrf_nf_reset_ct(skb);
> +
>  	err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net,
>  		      sk, skb, NULL, skb_dst(skb)->dev, dst_output);
>  
> @@ -508,6 +523,8 @@ static int vrf_ip_local_out(struct net *net, struct sock *sk,
>  {
>  	int err;
>  
> +	vrf_nf_reset_ct(skb);
> +
>  	err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk,
>  		      skb, NULL, skb_dst(skb)->dev, dst_output);
>  	if (likely(err == 1))
> @@ -626,8 +643,7 @@ static void vrf_finish_direct(struct sk_buff *skb)
>  		skb_pull(skb, ETH_HLEN);
>  	}
>  
> -	/* reset skb device */
> -	nf_reset_ct(skb);
> +	vrf_nf_reset_ct(skb);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -641,7 +657,7 @@ static int vrf_finish_output6(struct net *net, struct sock *sk,
>  	struct neighbour *neigh;
>  	int ret;
>  
> -	nf_reset_ct(skb);
> +	vrf_nf_reset_ct(skb);
>  
>  	skb->protocol = htons(ETH_P_IPV6);
>  	skb->dev = dev;
> @@ -752,6 +768,8 @@ static struct sk_buff *vrf_ip6_out_direct(struct net_device *vrf_dev,
>  
>  	skb->dev = vrf_dev;
>  
> +	vrf_nf_set_untracked(skb);
> +
>  	err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk,
>  		      skb, NULL, vrf_dev, vrf_ip6_out_direct_finish);
>  
> @@ -858,7 +876,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
>  	struct neighbour *neigh;
>  	bool is_v6gw = false;
>  
> -	nf_reset_ct(skb);
> +	vrf_nf_reset_ct(skb);
>  
>  	/* Be paranoid, rather than too clever. */
>  	if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
> @@ -980,6 +998,8 @@ static struct sk_buff *vrf_ip_out_direct(struct net_device *vrf_dev,
>  
>  	skb->dev = vrf_dev;
>  
> +	vrf_nf_set_untracked(skb);
> +
>  	err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk,
>  		      skb, NULL, vrf_dev, vrf_ip_out_direct_finish);
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]