Re: [PATCH nf] netfilter: nft_flow_offload: skip tcp rst and fin packets

[Eric Dumazet <eric.dumazet@gmail.com>]

On 8/13/19 10:51 AM, Pablo Neira Ayuso wrote:
> TCP rst and fin packets do not qualify to place a flow into the
> flowtable. Most likely there will be no more packets after connection
> closure. Without this patch, this flow entry expires and connection
> tracking picks up the entry in ESTABLISHED state using the fixup
> timeout, which makes this look inconsistent to the user for a connection
> that is actually already closed.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nft_flow_offload.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
> index aa5f571d4361..6dc54c2ca856 100644
> --- a/net/netfilter/nft_flow_offload.c
> +++ b/net/netfilter/nft_flow_offload.c
> @@ -73,10 +73,10 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
>  	struct nft_flow_offload *priv = nft_expr_priv(expr);
>  	struct nf_flowtable *flowtable = &priv->flowtable->data;
>  	enum ip_conntrack_info ctinfo;
> +	struct tcphdr *tcph = NULL;
>  	struct nf_flow_route route;
>  	struct flow_offload *flow;
>  	enum ip_conntrack_dir dir;
> -	bool is_tcp = false;
>  	struct nf_conn *ct;
>  	int ret;
>  
> @@ -89,7 +89,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
>  
>  	switch (ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum) {
>  	case IPPROTO_TCP:
> -		is_tcp = true;
> +		tcph = (void *)(skb_network_header(pkt->skb) + pkt->xt.thoff);

Don't you need something like :

tcph = skb_header_pointer(pkt->skb, pkt->xt.thoff, sizeof(*tcph), buffer);


> +		if (unlikely(tcph->fin || tcph->rst))
> +			goto out;
>  		break;
>  	case IPPROTO_UDP:
>  		break;
> @@ -115,7 +117,7 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
>  	if (!flow)
>  		goto err_flow_alloc;
>  
> -	if (is_tcp) {
> +	if (tcph) {
>  		ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
>  		ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
>  	}
>