| Message ID | 20210428131147.w2ppmrt6hpcjin5i@Fryzen495 |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Avoid potentially erroneos RST check | expand |
Ali Abdallah <ali.abdallah@suse.com> wrote: > In 'commit b303e7b80ff1 ("Reset the max ACK flag on SYN in ignore state")' > we reset the max ACK number to avoid dropping valid RST that is a > response to a SYN. > > Unfortunately that might not be enough, an out of order ACK in origin > might reset it back, and we might end up again dropping valid RST. > > This patch disables the RST check when we are not in established state > and we receive an RST with SEQ=0 that is most likely a response to a > SYN we had let it go through. > > Signed-off-by: Ali Abdallah <aabdallah@suse.de> Looks good, thanks! Acked-by: Florian Westphal <fw@strlen.de>
Hi, On Wed, Apr 28, 2021 at 03:11:47PM +0200, Ali Abdallah wrote: > In 'commit b303e7b80ff1 ("Reset the max ACK flag on SYN in ignore state")' > we reset the max ACK number to avoid dropping valid RST that is a > response to a SYN. I did not apply: https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210420122415.v2jtayiw3n4ds7t7@Fryzen495/ as you requested to send a v2. Would it make sense to squash this patch and ("Reset the max ACK flag on SYN in ignore state") in one single patch? Thanks.
On 28.04.2021 16:30, Pablo Neira Ayuso wrote: > I did not apply: > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210420122415.v2jtayiw3n4ds7t7@Fryzen495/ > > as you requested to send a v2. > > Would it make sense to squash this patch and ("Reset the max ACK flag > on SYN in ignore state") in one single patch? > > Thanks. Yes, I will send a single patch then. Thanks.
On Fri, Apr 30, 2021 at 11:27:29AM +0200, Ali Abdallah wrote: > On 28.04.2021 16:30, Pablo Neira Ayuso wrote: > > I did not apply: > > > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210420122415.v2jtayiw3n4ds7t7@Fryzen495/ > > > > as you requested to send a v2. > > > > Would it make sense to squash this patch and ("Reset the max ACK flag > > on SYN in ignore state") in one single patch? > > > > Thanks. > > Yes, I will send a single patch then. Thanks. Thanks. There are three patches in patchwork now (they come with no versioning, not sure if one of these is replaced by another). So which ones below should be consider to be applied upstream? https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210420122415.v2jtayiw3n4ds7t7@Fryzen495/ https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210428130911.cteglt52r5if7ynp@Fryzen495/ https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210430093601.zibczc4cjnwx3qwn@Fryzen495/
On 03.05.2021 23:07, Pablo Neira Ayuso wrote: > Thanks. > > There are three patches in patchwork now (they come with no > versioning, not sure if one of these is replaced by another). > > So which ones below should be consider to be applied upstream? > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210420122415.v2jtayiw3n4ds7t7@Fryzen495/ Please discard the above, only kindly apply the following two commits: > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210428130911.cteglt52r5if7ynp@Fryzen495/ > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20210430093601.zibczc4cjnwx3qwn@Fryzen495/
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c index 83890a700ef8..fb1c389a97fe 100644 --- a/net/netfilter/nf_conntrack_proto_tcp.c +++ b/net/netfilter/nf_conntrack_proto_tcp.c @@ -1048,6 +1048,12 @@ 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 we are not in established state, and an RST is + * observed with SEQ=0, this is most likely an answer + * to a SYN we had let go through above. + */ + if (seq == 0 && !nf_conntrack_tcp_established(ct)) + break; + if (before(seq, ct->proto.tcp.seen[!dir].td_maxack) && !tn->tcp_be_liberal) { /* Invalid RST */
In 'commit b303e7b80ff1 ("Reset the max ACK flag on SYN in ignore state")' we reset the max ACK number to avoid dropping valid RST that is a response to a SYN. Unfortunately that might not be enough, an out of order ACK in origin might reset it back, and we might end up again dropping valid RST. This patch disables the RST check when we are not in established state and we receive an RST with SEQ=0 that is most likely a response to a SYN we had let it go through. Signed-off-by: Ali Abdallah <aabdallah@suse.de> --- net/netfilter/nf_conntrack_proto_tcp.c | 6 ++++++ 1 file changed, 6 insertions(+)