Re: [PATCH] vrf: Revert:"run conntrack only in context of lower/physdev for locally generated packets"

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi,

On Sat, Dec 21, 2024 at 07:33:08PM +0800, hanhuihui wrote:
> In commit 8e0538d8, netfilter skips the NAT hook in the VRF context. This solves the problems mentioned in commit 
> in 8c9c296 and d43b75f. Therefore, we no longer need to set "untracked" to avoid any conntrack 
> participation in round 1.So maybe we can reverts commit 8c9c296a and d43b75fb because we don't need them now.

Did you run netfilter selftests?

> Fixes: 8c9c296 ("vrf: run conntrack only in context of lower/physdev for locally generated packets")
> Fixes: d43b75f ("vrf: don't run conntrack on vrf with !dflt qdisc")

These tags do not look fine.

Thanks.

> Signed-off-by: hanhuihui hanhuihui5@huawei.com
> ---
>  drivers/net/vrf.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index b90dccdc2..7b0c35003 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -36,7 +36,6 @@
>  #include <net/fib_rules.h>
>  #include <net/sch_generic.h>
>  #include <net/netns/generic.h>
> -#include <net/netfilter/nf_conntrack.h>
>  
>  #define DRV_NAME	"vrf"
>  #define DRV_VERSION	"1.1"
> @@ -416,26 +415,12 @@ 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, NULL, 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);
> -}
> -
>  #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);
>  
> @@ -514,8 +499,6 @@ 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))
> @@ -633,7 +616,8 @@ static void vrf_finish_direct(struct sk_buff *skb)
>  		skb_pull(skb, ETH_HLEN);
>  	}
>  
> -	vrf_nf_reset_ct(skb);
> +	/* reset skb device */
> +	nf_reset_ct(skb);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -647,7 +631,7 @@ static int vrf_finish_output6(struct net *net, struct sock *sk,
>  	struct neighbour *neigh;
>  	int ret;
>  
> -	vrf_nf_reset_ct(skb);
> +	nf_reset_ct(skb);
>  
>  	skb->protocol = htons(ETH_P_IPV6);
>  	skb->dev = dev;
> @@ -778,8 +762,6 @@ static struct sk_buff *vrf_ip6_out(struct net_device *vrf_dev,
>  	if (rt6_need_strict(&ipv6_hdr(skb)->daddr))
>  		return skb;
>  
> -	vrf_nf_set_untracked(skb);
> -
>  	if (qdisc_tx_is_default(vrf_dev) ||
>  	    IP6CB(skb)->flags & IP6SKB_XFRM_TRANSFORMED)
>  		return vrf_ip6_out_direct(vrf_dev, sk, skb);
> @@ -866,7 +848,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
>  	struct neighbour *neigh;
>  	bool is_v6gw = false;
>  
> -	vrf_nf_reset_ct(skb);
> +	nf_reset_ct(skb);
>  
>  	/* Be paranoid, rather than too clever. */
>  	if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
> @@ -1009,8 +991,6 @@ static struct sk_buff *vrf_ip_out(struct net_device *vrf_dev,
>  	    ipv4_is_lbcast(ip_hdr(skb)->daddr))
>  		return skb;
>  
> -	vrf_nf_set_untracked(skb);
> -
>  	if (qdisc_tx_is_default(vrf_dev) ||
>  	    IPCB(skb)->flags & IPSKB_XFRM_TRANSFORMED)
>  		return vrf_ip_out_direct(vrf_dev, sk, skb);
> -- 
> 2.43.0
> 
>