* [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used
@ 2021-05-20 10:53 Florian Westphal
2021-07-05 9:27 ` aabdallah
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2021-05-20 10:53 UTC (permalink / raw)
To: netfilter-devel; +Cc: Ali Abdallah, Florian Westphal
From: Ali Abdallah <aabdallah@suse.de>
If we receive a SYN packet in original direction on an existing
connection tracking entry, we let this SYN through because conntrack
might be out-of-sync.
Conntrack gets back in sync when server responds with SYN/ACK and state
gets updated accordingly.
However, if server replies with RST, this packet might be marked as
INVALID because td_maxack value reflects the *old* conntrack state
and not the state of the originator of the RST.
Avoid td_maxack-based checks if previous packet was a SYN.
Unfortunately that is not be enough: an out of order ACK in original
direction updates last_index, so we still end up marking valid RST.
Thus disable the sequence check when we are not in established state and
the received RST has a sequence of 0.
Because marking RSTs as invalid usually leads to unwanted timeouts,
also skip RST sequence checks if a conntrack entry is already closing.
Such entries can already be evicted via GC in case the table is full.
Co-developed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Ali Abdallah <aabdallah@suse.de>
---
Ali, this is still slightly different to last version.
tcp_can_early_drop() check doesn't need to depend on MAXACK_SET,
so I moved it a bit earlier.
MAXACK flag doesn't have to be unset either, it should be sufficient
to skip the 'old sequence' test when the previous packet was a SYN
packet we let through just before.
I retained your seq == 0 check and added a few comments to explain
why its still required even with the TCP_SYN_SET check in place.
I also ran the packetdrill test from
be0502a3f2e94211a8 ('netfilter: conntrack: tcp: only close if RST
matches exact sequence') and that still has the expected result.
net/netfilter/nf_conntrack_proto_tcp.c | 53 +++++++++++++++++---------
1 file changed, 36 insertions(+), 17 deletions(-)
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 34e22416a721..3e6e2cfc480a 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -822,6 +822,22 @@ static noinline bool tcp_new(struct nf_conn *ct, const struct sk_buff *skb,
return true;
}
+static bool tcp_can_early_drop(const struct nf_conn *ct)
+{
+ switch (ct->proto.tcp.state) {
+ case TCP_CONNTRACK_FIN_WAIT:
+ case TCP_CONNTRACK_LAST_ACK:
+ case TCP_CONNTRACK_TIME_WAIT:
+ case TCP_CONNTRACK_CLOSE:
+ case TCP_CONNTRACK_CLOSE_WAIT:
+ return true;
+ default:
+ break;
+ }
+
+ return false;
+}
+
/* Returns verdict for packet, or -1 for invalid. */
int nf_conntrack_tcp_packet(struct nf_conn *ct,
struct sk_buff *skb,
@@ -1029,9 +1045,28 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
if (index != TCP_RST_SET)
break;
- if (ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET) {
+ /* If we are closing, tuple might have been re-used already.
+ * last_index, last_ack, and all other ct fields used for
+ * sequence/window validation are outdated in that case.
+ *
+ * As the conntrack can already be expired by GC under pressure,
+ * just skip validation checks.
+ */
+ if (tcp_can_early_drop(ct))
+ goto in_window;
+
+ /* td_maxack might be outdated if we let a SYN through earlier */
+ if ((ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET) &&
+ ct->proto.tcp.last_index != TCP_SYN_SET) {
u32 seq = ntohl(th->seq);
+ /* If we are not in established state and SEQ=0 this is most
+ * likely an answer to a SYN we let go through above (last_index
+ * can be updated due to out-of-order ACKs).
+ */
+ if (seq == 0 && !nf_conntrack_tcp_established(ct))
+ break;
+
if (before(seq, ct->proto.tcp.seen[!dir].td_maxack)) {
/* Invalid RST */
spin_unlock_bh(&ct->lock);
@@ -1154,22 +1189,6 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
return NF_ACCEPT;
}
-static bool tcp_can_early_drop(const struct nf_conn *ct)
-{
- switch (ct->proto.tcp.state) {
- case TCP_CONNTRACK_FIN_WAIT:
- case TCP_CONNTRACK_LAST_ACK:
- case TCP_CONNTRACK_TIME_WAIT:
- case TCP_CONNTRACK_CLOSE:
- case TCP_CONNTRACK_CLOSE_WAIT:
- return true;
- default:
- break;
- }
-
- return false;
-}
-
#if IS_ENABLED(CONFIG_NF_CT_NETLINK)
#include <linux/netfilter/nfnetlink.h>
--
2.26.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used
2021-05-20 10:53 [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used Florian Westphal
@ 2021-07-05 9:27 ` aabdallah
2021-07-05 19:23 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: aabdallah @ 2021-07-05 9:27 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
Hi,
I see that this commit [1] is still under review, is there is any change
that it will be reviewed and merged soon? Thanks.
[1]
https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=244902
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used
2021-07-05 9:27 ` aabdallah
@ 2021-07-05 19:23 ` Pablo Neira Ayuso
2021-07-06 10:04 ` Ali Abdallah
0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-07-05 19:23 UTC (permalink / raw)
To: aabdallah; +Cc: Florian Westphal, netfilter-devel
On Mon, Jul 05, 2021 at 11:27:51AM +0200, aabdallah wrote:
> Hi,
>
> I see that this commit [1] is still under review, is there is any change
> that it will be reviewed and merged soon? Thanks.
>
> [1] https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=244902
Ok, I'm assuming that you're fine with Florian's proposal to address
your issue then. I was actually waiting for your ACK.
I'll place this patch:
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210520105311.20745-1-fw@strlen.de/
into nf.git.
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used
2021-07-05 19:23 ` Pablo Neira Ayuso
@ 2021-07-06 10:04 ` Ali Abdallah
2021-07-06 11:53 ` Pablo Neira Ayuso
0 siblings, 1 reply; 5+ messages in thread
From: Ali Abdallah @ 2021-07-06 10:04 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: aabdallah, Florian Westphal, netfilter-devel
[-- Attachment #1: Type: text/plain, Size: 669 bytes --]
On 05.07.2021 21:23, Pablo Neira Ayuso wrote:
> Ok, I'm assuming that you're fine with Florian's proposal to address
> your issue then. I was actually waiting for your ACK.
>
> I'll place this patch:
>
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210520105311.20745-1-fw@strlen.de/
Thanks a lot!
Please also take into account the following patch:
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210527071906.s7z72s7wsene5lib@Fryzen495/
which was already acked by Florian.
> Thanks
Many thanks.
--
Ali Abdallah | SUSE Linux L3 Engineer
GPG fingerprint: 51A0 F4A0 C8CF C98F 842E A9A8 B945 56F8 1C85 D0D5
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used
2021-07-06 10:04 ` Ali Abdallah
@ 2021-07-06 11:53 ` Pablo Neira Ayuso
0 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-07-06 11:53 UTC (permalink / raw)
To: Ali Abdallah; +Cc: aabdallah, Florian Westphal, netfilter-devel
On Tue, Jul 06, 2021 at 12:04:10PM +0200, Ali Abdallah wrote:
> On 05.07.2021 21:23, Pablo Neira Ayuso wrote:
> > Ok, I'm assuming that you're fine with Florian's proposal to address
> > your issue then. I was actually waiting for your ACK.
> >
> > I'll place this patch:
> >
> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210520105311.20745-1-fw@strlen.de/
>
> Thanks a lot!
>
> Please also take into account the following patch:
>
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210527071906.s7z72s7wsene5lib@Fryzen495/
>
> which was already acked by Florian.
Patches applied to nf.git, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-06 12:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20 10:53 [PATCH nf] netfilter: conntrack: improve RST handling when tuple is re-used Florian Westphal
2021-07-05 9:27 ` aabdallah
2021-07-05 19:23 ` Pablo Neira Ayuso
2021-07-06 10:04 ` Ali Abdallah
2021-07-06 11:53 ` Pablo Neira Ayuso
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.