* [PATCH] netfilter: conntrack: add new sysctl to disable RST check
@ 2021-05-26 9:24 Ali Abdallah
2021-05-26 14:29 ` Nicolas Dichtel
2021-05-28 9:15 ` Florian Westphal
0 siblings, 2 replies; 6+ messages in thread
From: Ali Abdallah @ 2021-05-26 9:24 UTC (permalink / raw)
To: netfilter-devel
This patch adds a new sysctl tcp_ignore_invalid_rst to disable marking
out of segments RSTs as INVALID.
Signed-off-by: Ali Abdallah <aabdallah@suse.de>
---
Documentation/networking/nf_conntrack-sysctl.rst | 6 ++++++
include/net/netns/conntrack.h | 1 +
net/netfilter/nf_conntrack_proto_tcp.c | 6 +++++-
net/netfilter/nf_conntrack_standalone.c | 10 ++++++++++
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/nf_conntrack-sysctl.rst b/Documentation/networking/nf_conntrack-sysctl.rst
index 11a9b76786cb..45f5a9690172 100644
--- a/Documentation/networking/nf_conntrack-sysctl.rst
+++ b/Documentation/networking/nf_conntrack-sysctl.rst
@@ -110,6 +110,12 @@ nf_conntrack_tcp_be_liberal - BOOLEAN
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.
+nf_conntrack_tcp_ignore_invalid_rst - BOOLEAN
+ - 0 - disabled (default)
+ - not 0 - enabled
+
+ If it's non-zero, we don't mark out of window RST segments as INVALID.
+
nf_conntrack_tcp_loose - BOOLEAN
- 0 - disabled
- not 0 - enabled (default)
diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
index ad0a95c2335e..473acd7cce9c 100644
--- a/include/net/netns/conntrack.h
+++ b/include/net/netns/conntrack.h
@@ -27,6 +27,7 @@ struct nf_tcp_net {
u8 tcp_loose;
u8 tcp_be_liberal;
u8 tcp_max_retrans;
+ u8 tcp_ignore_invalid_rst;
};
enum udp_conntrack {
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 34e22416a721..1a5e77b05514 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_ignore_invalid_rst) {
/* Invalid RST */
spin_unlock_bh(&ct->lock);
nf_ct_l4proto_log_invalid(skb, ct, "invalid rst");
@@ -1436,6 +1437,9 @@ void nf_conntrack_tcp_init_net(struct net *net)
*/
tn->tcp_be_liberal = 0;
+ /* If it's non-zero, we turn off RST sequence number check */
+ tn->tcp_ignore_invalid_rst = 0;
+
/* Max number of the retransmitted packets without receiving an (acceptable)
* ACK from the destination. If this number is reached, a shorter timer
* will be started.
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index aaa55246d0ca..9341be6b142f 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -577,6 +577,7 @@ enum nf_ct_sysctl_index {
NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_UNACK,
NF_SYSCTL_CT_PROTO_TCP_LOOSE,
NF_SYSCTL_CT_PROTO_TCP_LIBERAL,
+ NF_SYSCTL_CT_PROTO_TCP_IGNORE_INVALID_RST,
NF_SYSCTL_CT_PROTO_TCP_MAX_RETRANS,
NF_SYSCTL_CT_PROTO_TIMEOUT_UDP,
NF_SYSCTL_CT_PROTO_TIMEOUT_UDP_STREAM,
@@ -778,6 +779,14 @@ static struct ctl_table nf_ct_sysctl_table[] = {
.extra1 = SYSCTL_ZERO,
.extra2 = SYSCTL_ONE,
},
+ [NF_SYSCTL_CT_PROTO_TCP_IGNORE_INVALID_RST] = {
+ .procname = "nf_conntrack_tcp_ignore_invalid_rst",
+ .maxlen = sizeof(u8),
+ .mode = 0644,
+ .proc_handler = proc_dou8vec_minmax,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE,
+ },
[NF_SYSCTL_CT_PROTO_TCP_MAX_RETRANS] = {
.procname = "nf_conntrack_tcp_max_retrans",
.maxlen = sizeof(u8),
@@ -970,6 +979,7 @@ static void nf_conntrack_standalone_init_tcp_sysctl(struct net *net,
XASSIGN(LOOSE, &tn->tcp_loose);
XASSIGN(LIBERAL, &tn->tcp_be_liberal);
XASSIGN(MAX_RETRANS, &tn->tcp_max_retrans);
+ XASSIGN(IGNORE_INVALID_RST, &tn->tcp_ignore_invalid_rst);
#undef XASSIGN
}
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: conntrack: add new sysctl to disable RST check
2021-05-26 9:24 [PATCH] netfilter: conntrack: add new sysctl to disable RST check Ali Abdallah
@ 2021-05-26 14:29 ` Nicolas Dichtel
2021-05-26 14:34 ` Ali Abdallah
2021-05-28 9:15 ` Florian Westphal
1 sibling, 1 reply; 6+ messages in thread
From: Nicolas Dichtel @ 2021-05-26 14:29 UTC (permalink / raw)
To: Ali Abdallah, netfilter-devel
Le 26/05/2021 à 11:24, Ali Abdallah a écrit :
> This patch adds a new sysctl tcp_ignore_invalid_rst to disable marking
> out of segments RSTs as INVALID.
>
> Signed-off-by: Ali Abdallah <aabdallah@suse.de>
> ---
> Documentation/networking/nf_conntrack-sysctl.rst | 6 ++++++
> include/net/netns/conntrack.h | 1 +
> net/netfilter/nf_conntrack_proto_tcp.c | 6 +++++-
> net/netfilter/nf_conntrack_standalone.c | 10 ++++++++++
> 4 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/networking/nf_conntrack-sysctl.rst b/Documentation/networking/nf_conntrack-sysctl.rst
> index 11a9b76786cb..45f5a9690172 100644
> --- a/Documentation/networking/nf_conntrack-sysctl.rst
> +++ b/Documentation/networking/nf_conntrack-sysctl.rst
> @@ -110,6 +110,12 @@ nf_conntrack_tcp_be_liberal - BOOLEAN
> 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.
>
> +nf_conntrack_tcp_ignore_invalid_rst - BOOLEAN
> + - 0 - disabled (default)
> + - not 0 - enabled
If I correctly read the patch, the only "not 0" possible value is 1. Why not
using explicitly "1"?
[snip]
> @@ -778,6 +779,14 @@ static struct ctl_table nf_ct_sysctl_table[] = {
> .extra1 = SYSCTL_ZERO,
> .extra2 = SYSCTL_ONE,
> },
> + [NF_SYSCTL_CT_PROTO_TCP_IGNORE_INVALID_RST] = {
> + .procname = "nf_conntrack_tcp_ignore_invalid_rst",
> + .maxlen = sizeof(u8),
> + .mode = 0644,
> + .proc_handler = proc_dou8vec_minmax,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = SYSCTL_ONE,
Max == 1.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: conntrack: add new sysctl to disable RST check
2021-05-26 14:29 ` Nicolas Dichtel
@ 2021-05-26 14:34 ` Ali Abdallah
2021-05-26 15:06 ` Nicolas Dichtel
0 siblings, 1 reply; 6+ messages in thread
From: Ali Abdallah @ 2021-05-26 14:34 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: netfilter-devel
On 26.05.2021 16:29, Nicolas Dichtel wrote:
> > +nf_conntrack_tcp_ignore_invalid_rst - BOOLEAN
> > + - 0 - disabled (default)
> > + - not 0 - enabled
> If I correctly read the patch, the only "not 0" possible value is 1. Why not
> using explicitly "1"?
That what the doc on nf_conntrack_tcp_be_liberal says as well, logically
not 0 is 1, so IMHO I don't think that can lead to confusion.
Kind regards,
Ali
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: conntrack: add new sysctl to disable RST check
2021-05-26 14:34 ` Ali Abdallah
@ 2021-05-26 15:06 ` Nicolas Dichtel
2021-05-26 15:12 ` Ali Abdallah
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Dichtel @ 2021-05-26 15:06 UTC (permalink / raw)
To: Ali Abdallah; +Cc: netfilter-devel
Le 26/05/2021 à 16:34, Ali Abdallah a écrit :
> On 26.05.2021 16:29, Nicolas Dichtel wrote:
>>> +nf_conntrack_tcp_ignore_invalid_rst - BOOLEAN
>>> + - 0 - disabled (default)
>>> + - not 0 - enabled
>> If I correctly read the patch, the only "not 0" possible value is 1. Why not
>> using explicitly "1"?
>
> That what the doc on nf_conntrack_tcp_be_liberal says as well, logically
> not 0 is 1, so IMHO I don't think that can lead to confusion.
There is a lot of sysctl that have several magic values, see
https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
And some act like boolean but accept all values,
/proc/sys/net/ipv4/conf/*/forwarding for example.
There is nothing obvious with sysctl values (and a lot of inconsistencies), it's
why I suggest to be explicit.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: conntrack: add new sysctl to disable RST check
2021-05-26 15:06 ` Nicolas Dichtel
@ 2021-05-26 15:12 ` Ali Abdallah
0 siblings, 0 replies; 6+ messages in thread
From: Ali Abdallah @ 2021-05-26 15:12 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: netfilter-devel
On 26.05.2021 17:06, Nicolas Dichtel wrote:
> Le 26/05/2021 à 16:34, Ali Abdallah a écrit :
> > That what the doc on nf_conntrack_tcp_be_liberal says as well, logically
> > not 0 is 1, so IMHO I don't think that can lead to confusion.
>
> There is a lot of sysctl that have several magic values, see
> https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
>
> And some act like boolean but accept all values,
> /proc/sys/net/ipv4/conf/*/forwarding for example.
>
> There is nothing obvious with sysctl values (and a lot of inconsistencies), it's
> why I suggest to be explicit.
Yes, I absolutely see your point, and I have no problem in making that
explicit, I will sent a v2 patch saying explicitly that only "1" would
disable RST seq number checks, hopefully it gets merged soon.
> Regards,
> Nicolas
Kind Regards,
Ali
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: conntrack: add new sysctl to disable RST check
2021-05-26 9:24 [PATCH] netfilter: conntrack: add new sysctl to disable RST check Ali Abdallah
2021-05-26 14:29 ` Nicolas Dichtel
@ 2021-05-28 9:15 ` Florian Westphal
1 sibling, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2021-05-28 9:15 UTC (permalink / raw)
To: Ali Abdallah; +Cc: netfilter-devel
Ali Abdallah <ali.abdallah@suse.com> wrote:
> This patch adds a new sysctl tcp_ignore_invalid_rst to disable marking
> out of segments RSTs as INVALID.
Just for archives:
I am not sure this is still needed after the recent change, but I can
see why its desirable to keep NAT working even when RST is invalid.
I think that the more fundamental problem is the inability to permit
setting a conntrack as INVALID while allowing NAT to work, i.e. move
drop decision to ruleset.
However, I see that this isn't easy to change. Therefore,
Reviewed-by: Florian Westphal <fw@strlen.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-28 9:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 9:24 [PATCH] netfilter: conntrack: add new sysctl to disable RST check Ali Abdallah
2021-05-26 14:29 ` Nicolas Dichtel
2021-05-26 14:34 ` Ali Abdallah
2021-05-26 15:06 ` Nicolas Dichtel
2021-05-26 15:12 ` Ali Abdallah
2021-05-28 9:15 ` Florian Westphal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.