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

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

[Pablo Neira Ayuso]

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);
+		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;
 	}
-- 
2.11.0

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

[Eric Dumazet]

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;
>  	}
>