Re: [PATCH v6 2/2] netfilter: add netfilter hooks to SRv6 data plane

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

Hi,

A few more comments.

On Tue, Aug 17, 2021 at 06:34:53AM +0000, Ryoga Saito wrote:
> This patch introduces netfilter hooks for solving the problem that
> conntrack couldn't record both inner flows and outer flows.
> 
> Signed-off-by: Ryoga Saito <contact@proelbtn.com>
> ---
>  net/ipv6/seg6_iptunnel.c |  79 +++++++++++++++++++++++++--
>  net/ipv6/seg6_local.c    | 114 +++++++++++++++++++++++++++------------
>  2 files changed, 153 insertions(+), 40 deletions(-)
> 
> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
> index 897fa59c47de..83074b9f4b7e 100644
> --- a/net/ipv6/seg6_iptunnel.c
> +++ b/net/ipv6/seg6_iptunnel.c
> @@ -26,6 +26,8 @@
>  #ifdef CONFIG_IPV6_SEG6_HMAC
>  #include <net/seg6_hmac.h>
>  #endif
> +#include <net/lwtunnel.h>
> +#include <net/netfilter/nf_conntrack.h>

I think this #include <net/netfilter/nf_conntrack.h> is not required,
nf_reset_ct() is available through linux/skbuff.h. Same comment
applies to net/ipv6/seg6_local.c

>  static size_t seg6_lwt_headroom(struct seg6_iptunnel_encap *tuninfo)
>  {
> @@ -295,25 +297,33 @@ static int seg6_do_srh(struct sk_buff *skb)
>  
>  	ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
>  	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
> +	nf_reset_ct(skb);
>  
>  	return 0;
>  }
>  
> -static int seg6_input(struct sk_buff *skb)
> +static int seg6_input_finish(struct net *net, struct sock *sk,
> +			     struct sk_buff *skb)
> +{
> +	return dst_input(skb);
> +}
> +
> +static int seg6_input_core(struct net *net, struct sock *sk,
> +			   struct sk_buff *skb)
>  {
>  	struct dst_entry *orig_dst = skb_dst(skb);
>  	struct dst_entry *dst = NULL;
>  	struct seg6_lwt *slwt;
>  	int err;
>  
> +	slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate);

No need to move this assignement before calling seg6_do_srh()...

> +
>  	err = seg6_do_srh(skb);
>  	if (unlikely(err)) {
>  		kfree_skb(skb);
>  		return err;
>  	}
>  
> -	slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate);

... it could stay still here, right?

> -
>  	preempt_disable();
>  	dst = dst_cache_get(&slwt->cache);
>  	preempt_enable();