[PATCH] Disable RST seq number check when tcp_be_liberal is greater 1

[PATCH] Disable RST seq number check when tcp_be_liberal is greater 1

[Ali Abdallah]

This patch adds the possibility to disable RST seq number check by
setting tcp_be_liberal to a value greater than 1. The default old
behaviour is kept unchanged.

Signed-off-by: Ali Abdallah <aabdallah@suse.de>
---
 Documentation/networking/nf_conntrack-sysctl.rst | 10 ++++++----
 net/netfilter/nf_conntrack_proto_tcp.c           |  3 ++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/nf_conntrack-sysctl.rst b/Documentation/networking/nf_conntrack-sysctl.rst
index 11a9b76786cb..cfcc3bbd5dda 100644
--- a/Documentation/networking/nf_conntrack-sysctl.rst
+++ b/Documentation/networking/nf_conntrack-sysctl.rst
@@ -103,12 +103,14 @@ nf_conntrack_max - INTEGER
 	Size of connection tracking table.  Default value is
 	nf_conntrack_buckets value * 4.
 
-nf_conntrack_tcp_be_liberal - BOOLEAN
+nf_conntrack_tcp_be_liberal - INTEGER
 	- 0 - disabled (default)
-	- not 0 - enabled
+        - 1 - RST sequence number check only
+	- greater than 1 - turns off all sequence number/window checks
 
-	Be conservative in what you do, be liberal in what you accept from others.
-	If it's non-zero, we mark only out of window RST segments as INVALID.
+	Be conservative in what you do, be liberal in what you accept from
+	others. If it is set to 1, we mark only out of window RST segments as
+	INVALID. Values greater than 1 disables also RST sequence numbers check.
 
 nf_conntrack_tcp_loose - BOOLEAN
 	- 0 - disabled
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 34e22416a721..bf4ba89eea6c 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -1032,7 +1032,8 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 		if (ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET) {
 			u32 seq = ntohl(th->seq);
 
-			if (before(seq, ct->proto.tcp.seen[!dir].td_maxack)) {
+			if (before(seq, ct->proto.tcp.seen[!dir].td_maxack) &&
+			    tn->tcp_be_liberal <= 1) {
 				/* Invalid RST  */
 				spin_unlock_bh(&ct->lock);
 				nf_ct_l4proto_log_invalid(skb, ct, "invalid rst");
-- 
2.26.2

Re: [PATCH] Disable RST seq number check when tcp_be_liberal is greater 1

[Nicolas Dichtel]

Le 21/05/2021 à 11:03, Ali Abdallah a écrit :
> This patch adds the possibility to disable RST seq number check by
> setting tcp_be_liberal to a value greater than 1. The default old
> behaviour is kept unchanged.
> 
> Signed-off-by: Ali Abdallah <aabdallah@suse.de>
> ---
>  Documentation/networking/nf_conntrack-sysctl.rst | 10 ++++++----
>  net/netfilter/nf_conntrack_proto_tcp.c           |  3 ++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/networking/nf_conntrack-sysctl.rst b/Documentation/networking/nf_conntrack-sysctl.rst
> index 11a9b76786cb..cfcc3bbd5dda 100644
> --- a/Documentation/networking/nf_conntrack-sysctl.rst
> +++ b/Documentation/networking/nf_conntrack-sysctl.rst
> @@ -103,12 +103,14 @@ nf_conntrack_max - INTEGER
>  	Size of connection tracking table.  Default value is
>  	nf_conntrack_buckets value * 4.
>  
> -nf_conntrack_tcp_be_liberal - BOOLEAN
> +nf_conntrack_tcp_be_liberal - INTEGER
>  	- 0 - disabled (default)
> -	- not 0 - enabled
> +        - 1 - RST sequence number check only
nit: this line is indented with spaces where other are with tabs.

> +	- greater than 1 - turns off all sequence number/window checks
Why not having a fixed value (like 2 for example)? It will allow to add
different behavior in the future.


Regards,
Nicolas

Re: [PATCH] Disable RST seq number check when tcp_be_liberal is greater 1

[Ali Abdallah]

On 24.05.2021 10:39, Nicolas Dichtel wrote:
> >
> > -nf_conntrack_tcp_be_liberal - BOOLEAN
> > +nf_conntrack_tcp_be_liberal - INTEGER
> >  	- 0 - disabled (default)
> > -	- not 0 - enabled
> > +        - 1 - RST sequence number check only
> nit: this line is indented with spaces where other are with tabs.

Yes, will correct that.

Please ignore this patch as I didn't pay attention that tcp_be_liberal
uses proc_dou8vec_minmax (wasn't the case for older releases). Will send
a new patch.

> > +	- greater than 1 - turns off all sequence number/window checks
> Why not having a fixed value (like 2 for example)? It will allow to add
> different behavior in the future.

But then 2 won't disable also other checks?

Anyway, I think a clean solution would be to add another sysctl to
ignore invalid RST.

So please discard this patch.

> Regards,
> Nicolas

Regards,
Ali