Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nf] netfilter: nft_flow_offload: skip tcp rst and fin packets
@ 2019-08-13  8:51 Pablo Neira Ayuso
  2019-08-13 14:34 ` Eric Dumazet
  0 siblings, 1 reply; 2+ messages in thread
From: Pablo Neira Ayuso @ 2019-08-13  8:51 UTC (permalink / raw)
  To: netfilter-devel

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



^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH nf] netfilter: nft_flow_offload: skip tcp rst and fin packets
  2019-08-13  8:51 [PATCH nf] netfilter: nft_flow_offload: skip tcp rst and fin packets Pablo Neira Ayuso
@ 2019-08-13 14:34 ` Eric Dumazet
  0 siblings, 0 replies; 2+ messages in thread
From: Eric Dumazet @ 2019-08-13 14:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel



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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13  8:51 [PATCH nf] netfilter: nft_flow_offload: skip tcp rst and fin packets Pablo Neira Ayuso
2019-08-13 14:34 ` Eric Dumazet

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org netfilter-devel@archiver.kernel.org
	public-inbox-index netfilter-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox