All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only
@ 2022-04-25  9:47 Florian Westphal
  2022-04-25 13:41 ` Jozsef Kadlecsik
  2022-04-27 13:49 ` Pablo Neira Ayuso
  0 siblings, 2 replies; 5+ messages in thread
From: Florian Westphal @ 2022-04-25  9:47 UTC (permalink / raw)
  To: netfilter-devel
  Cc: edumazet, ncardwell, Florian Westphal, Jaco Kroon, Jozsef Kadlecsik

Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
pinpointed to nf_conntrack tcp_in_window() bug.

tcp trace shows following sequence:

I > R Flags [S], seq 3451342529, win 62580, options [.. tfo [|tcp]>
R > I Flags [S.], seq 2699962254, ack 3451342530, win 65535, options [..]
R > I Flags [P.], seq 1:89, ack 1, [..]

Note 3rd ACK is from responder to initiator so following branch is taken:
    } else if (((state->state == TCP_CONNTRACK_SYN_SENT
               && dir == IP_CT_DIR_ORIGINAL)
               || (state->state == TCP_CONNTRACK_SYN_RECV
               && dir == IP_CT_DIR_REPLY))
               && after(end, sender->td_end)) {

... because state == TCP_CONNTRACK_SYN_RECV and dir is REPLY.
This causes the scaling factor to be reset to 0: window scale option
is only present in syn(ack) packets.  This in turn makes nf_conntrack
mark valid packets as out-of-window.

This was always broken, it exists even in original commit where
window tracking was added to ip_conntrack (nf_conntrack predecessor)
in 2.6.9-rc1 kernel.

Restrict to 'tcph->syn', just like the 3rd condtional added in
commit 82b72cb94666 ("netfilter: conntrack: re-init state for retransmitted syn-ack").

Upon closer look, those conditionals/branches can be merged:

Because earlier checks prevent syn-ack from showing up in
original direction, the 'dir' checks in the conditional quoted above are
redundant, remove them. Return early for pure syn retransmitted in reply
direction (simultaneous open).

Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
Reported-by: Jaco Kroon <jaco@uls.co.za>
Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 8ec55cd72572..204a5cdff5b1 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -556,24 +556,14 @@ static bool tcp_in_window(struct nf_conn *ct,
 			}
 
 		}
-	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
-		     && dir == IP_CT_DIR_ORIGINAL)
-		   || (state->state == TCP_CONNTRACK_SYN_RECV
-		     && dir == IP_CT_DIR_REPLY))
-		   && after(end, sender->td_end)) {
+	} else if (tcph->syn &&
+		   after(end, sender->td_end) &&
+		   (state->state == TCP_CONNTRACK_SYN_SENT ||
+		    state->state == TCP_CONNTRACK_SYN_RECV)) {
 		/*
 		 * RFC 793: "if a TCP is reinitialized ... then it need
 		 * not wait at all; it must only be sure to use sequence
 		 * numbers larger than those recently used."
-		 */
-		sender->td_end =
-		sender->td_maxend = end;
-		sender->td_maxwin = (win == 0 ? 1 : win);
-
-		tcp_options(skb, dataoff, tcph, sender);
-	} else if (tcph->syn && dir == IP_CT_DIR_REPLY &&
-		   state->state == TCP_CONNTRACK_SYN_SENT) {
-		/* Retransmitted syn-ack, or syn (simultaneous open).
 		 *
 		 * Re-init state for this direction, just like for the first
 		 * syn(-ack) reply, it might differ in seq, ack or tcp options.
@@ -581,7 +571,8 @@ static bool tcp_in_window(struct nf_conn *ct,
 		tcp_init_sender(sender, receiver,
 				skb, dataoff, tcph,
 				end, win);
-		if (!tcph->ack)
+
+		if (dir == IP_CT_DIR_REPLY && !tcph->ack)
 			return true;
 	}
 
-- 
2.35.1


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

* Re: [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only
  2022-04-25  9:47 [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only Florian Westphal
@ 2022-04-25 13:41 ` Jozsef Kadlecsik
  2022-04-25 14:55   ` Eric Dumazet
  2022-04-27 13:49 ` Pablo Neira Ayuso
  1 sibling, 1 reply; 5+ messages in thread
From: Jozsef Kadlecsik @ 2022-04-25 13:41 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel, edumazet, ncardwell, Jaco Kroon

On Mon, 25 Apr 2022, Florian Westphal wrote:

> Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
> pinpointed to nf_conntrack tcp_in_window() bug.
> 
> tcp trace shows following sequence:
> 
> I > R Flags [S], seq 3451342529, win 62580, options [.. tfo [|tcp]>
> R > I Flags [S.], seq 2699962254, ack 3451342530, win 65535, options [..]
> R > I Flags [P.], seq 1:89, ack 1, [..]
> 
> Note 3rd ACK is from responder to initiator so following branch is taken:
>     } else if (((state->state == TCP_CONNTRACK_SYN_SENT
>                && dir == IP_CT_DIR_ORIGINAL)
>                || (state->state == TCP_CONNTRACK_SYN_RECV
>                && dir == IP_CT_DIR_REPLY))
>                && after(end, sender->td_end)) {
> 
> ... because state == TCP_CONNTRACK_SYN_RECV and dir is REPLY.
> This causes the scaling factor to be reset to 0: window scale option
> is only present in syn(ack) packets.  This in turn makes nf_conntrack
> mark valid packets as out-of-window.
> 
> This was always broken, it exists even in original commit where
> window tracking was added to ip_conntrack (nf_conntrack predecessor)
> in 2.6.9-rc1 kernel.
> 
> Restrict to 'tcph->syn', just like the 3rd condtional added in
> commit 82b72cb94666 ("netfilter: conntrack: re-init state for retransmitted syn-ack").
> 
> Upon closer look, those conditionals/branches can be merged:
> 
> Because earlier checks prevent syn-ack from showing up in
> original direction, the 'dir' checks in the conditional quoted above are
> redundant, remove them. Return early for pure syn retransmitted in reply
> direction (simultaneous open).
> 
> Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
> Reported-by: Jaco Kroon <jaco@uls.co.za>
> Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

[Sorry, I was away whole last week as well.]

Best regards,
Jozsef

> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 8ec55cd72572..204a5cdff5b1 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -556,24 +556,14 @@ static bool tcp_in_window(struct nf_conn *ct,
>  			}
>  
>  		}
> -	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
> -		     && dir == IP_CT_DIR_ORIGINAL)
> -		   || (state->state == TCP_CONNTRACK_SYN_RECV
> -		     && dir == IP_CT_DIR_REPLY))
> -		   && after(end, sender->td_end)) {
> +	} else if (tcph->syn &&
> +		   after(end, sender->td_end) &&
> +		   (state->state == TCP_CONNTRACK_SYN_SENT ||
> +		    state->state == TCP_CONNTRACK_SYN_RECV)) {
>  		/*
>  		 * RFC 793: "if a TCP is reinitialized ... then it need
>  		 * not wait at all; it must only be sure to use sequence
>  		 * numbers larger than those recently used."
> -		 */
> -		sender->td_end =
> -		sender->td_maxend = end;
> -		sender->td_maxwin = (win == 0 ? 1 : win);
> -
> -		tcp_options(skb, dataoff, tcph, sender);
> -	} else if (tcph->syn && dir == IP_CT_DIR_REPLY &&
> -		   state->state == TCP_CONNTRACK_SYN_SENT) {
> -		/* Retransmitted syn-ack, or syn (simultaneous open).
>  		 *
>  		 * Re-init state for this direction, just like for the first
>  		 * syn(-ack) reply, it might differ in seq, ack or tcp options.
> @@ -581,7 +571,8 @@ static bool tcp_in_window(struct nf_conn *ct,
>  		tcp_init_sender(sender, receiver,
>  				skb, dataoff, tcph,
>  				end, win);
> -		if (!tcph->ack)
> +
> +		if (dir == IP_CT_DIR_REPLY && !tcph->ack)
>  			return true;
>  	}
>  
> -- 
> 2.35.1
> 
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

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

* Re: [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only
  2022-04-25 13:41 ` Jozsef Kadlecsik
@ 2022-04-25 14:55   ` Eric Dumazet
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-04-25 14:55 UTC (permalink / raw)
  To: Jozsef Kadlecsik
  Cc: Florian Westphal, netfilter-devel, Neal Cardwell, Jaco Kroon

On Mon, Apr 25, 2022 at 6:41 AM Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
>
> On Mon, 25 Apr 2022, Florian Westphal wrote:
>
> > Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
> > pinpointed to nf_conntrack tcp_in_window() bug.
> >
> > tcp trace shows following sequence:
> >
> > I > R Flags [S], seq 3451342529, win 62580, options [.. tfo [|tcp]>
> > R > I Flags [S.], seq 2699962254, ack 3451342530, win 65535, options [..]
> > R > I Flags [P.], seq 1:89, ack 1, [..]
> >
> > Note 3rd ACK is from responder to initiator so following branch is taken:
> >     } else if (((state->state == TCP_CONNTRACK_SYN_SENT
> >                && dir == IP_CT_DIR_ORIGINAL)
> >                || (state->state == TCP_CONNTRACK_SYN_RECV
> >                && dir == IP_CT_DIR_REPLY))
> >                && after(end, sender->td_end)) {
> >
> > ... because state == TCP_CONNTRACK_SYN_RECV and dir is REPLY.
> > This causes the scaling factor to be reset to 0: window scale option
> > is only present in syn(ack) packets.  This in turn makes nf_conntrack
> > mark valid packets as out-of-window.
> >
> > This was always broken, it exists even in original commit where
> > window tracking was added to ip_conntrack (nf_conntrack predecessor)
> > in 2.6.9-rc1 kernel.
> >
> > Restrict to 'tcph->syn', just like the 3rd condtional added in
> > commit 82b72cb94666 ("netfilter: conntrack: re-init state for retransmitted syn-ack").
> >
> > Upon closer look, those conditionals/branches can be merged:
> >
> > Because earlier checks prevent syn-ack from showing up in
> > original direction, the 'dir' checks in the conditional quoted above are
> > redundant, remove them. Return early for pure syn retransmitted in reply
> > direction (simultaneous open).
> >
> > Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
> > Reported-by: Jaco Kroon <jaco@uls.co.za>
> > Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
>
> Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>
>
> [Sorry, I was away whole last week as well.]
>

Thanks a lot Florian and Jozsef !

> Best regards,
> Jozsef
>
> > ---
> >  net/netfilter/nf_conntrack_proto_tcp.c | 21 ++++++---------------
> >  1 file changed, 6 insertions(+), 15 deletions(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > index 8ec55cd72572..204a5cdff5b1 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > @@ -556,24 +556,14 @@ static bool tcp_in_window(struct nf_conn *ct,
> >                       }
> >
> >               }
> > -     } else if (((state->state == TCP_CONNTRACK_SYN_SENT
> > -                  && dir == IP_CT_DIR_ORIGINAL)
> > -                || (state->state == TCP_CONNTRACK_SYN_RECV
> > -                  && dir == IP_CT_DIR_REPLY))
> > -                && after(end, sender->td_end)) {
> > +     } else if (tcph->syn &&
> > +                after(end, sender->td_end) &&
> > +                (state->state == TCP_CONNTRACK_SYN_SENT ||
> > +                 state->state == TCP_CONNTRACK_SYN_RECV)) {
> >               /*
> >                * RFC 793: "if a TCP is reinitialized ... then it need
> >                * not wait at all; it must only be sure to use sequence
> >                * numbers larger than those recently used."
> > -              */
> > -             sender->td_end =
> > -             sender->td_maxend = end;
> > -             sender->td_maxwin = (win == 0 ? 1 : win);
> > -
> > -             tcp_options(skb, dataoff, tcph, sender);
> > -     } else if (tcph->syn && dir == IP_CT_DIR_REPLY &&
> > -                state->state == TCP_CONNTRACK_SYN_SENT) {
> > -             /* Retransmitted syn-ack, or syn (simultaneous open).
> >                *
> >                * Re-init state for this direction, just like for the first
> >                * syn(-ack) reply, it might differ in seq, ack or tcp options.
> > @@ -581,7 +571,8 @@ static bool tcp_in_window(struct nf_conn *ct,
> >               tcp_init_sender(sender, receiver,
> >                               skb, dataoff, tcph,
> >                               end, win);
> > -             if (!tcph->ack)
> > +
> > +             if (dir == IP_CT_DIR_REPLY && !tcph->ack)
> >                       return true;
> >       }
> >
> > --
> > 2.35.1
> >
> >
>
> -
> E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
> PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
> Address : Wigner Research Centre for Physics
>           H-1525 Budapest 114, POB. 49, Hungary

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

* Re: [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only
  2022-04-25  9:47 [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only Florian Westphal
  2022-04-25 13:41 ` Jozsef Kadlecsik
@ 2022-04-27 13:49 ` Pablo Neira Ayuso
  2022-06-10  8:32   ` Jaco Kroon
  1 sibling, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2022-04-27 13:49 UTC (permalink / raw)
  To: Florian Westphal
  Cc: netfilter-devel, edumazet, ncardwell, Jaco Kroon, Jozsef Kadlecsik

On Mon, Apr 25, 2022 at 11:47:11AM +0200, Florian Westphal wrote:
> Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
> pinpointed to nf_conntrack tcp_in_window() bug.
> 
> tcp trace shows following sequence:
> 
> I > R Flags [S], seq 3451342529, win 62580, options [.. tfo [|tcp]>
> R > I Flags [S.], seq 2699962254, ack 3451342530, win 65535, options [..]
> R > I Flags [P.], seq 1:89, ack 1, [..]
> 
> Note 3rd ACK is from responder to initiator so following branch is taken:
>     } else if (((state->state == TCP_CONNTRACK_SYN_SENT
>                && dir == IP_CT_DIR_ORIGINAL)
>                || (state->state == TCP_CONNTRACK_SYN_RECV
>                && dir == IP_CT_DIR_REPLY))
>                && after(end, sender->td_end)) {
> 
> ... because state == TCP_CONNTRACK_SYN_RECV and dir is REPLY.
> This causes the scaling factor to be reset to 0: window scale option
> is only present in syn(ack) packets.  This in turn makes nf_conntrack
> mark valid packets as out-of-window.
> 
> This was always broken, it exists even in original commit where
> window tracking was added to ip_conntrack (nf_conntrack predecessor)
> in 2.6.9-rc1 kernel.
> 
> Restrict to 'tcph->syn', just like the 3rd condtional added in
> commit 82b72cb94666 ("netfilter: conntrack: re-init state for retransmitted syn-ack").
> 
> Upon closer look, those conditionals/branches can be merged:
> 
> Because earlier checks prevent syn-ack from showing up in
> original direction, the 'dir' checks in the conditional quoted above are
> redundant, remove them. Return early for pure syn retransmitted in reply
> direction (simultaneous open).

Applied, thanks

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

* Re: [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only
  2022-04-27 13:49 ` Pablo Neira Ayuso
@ 2022-06-10  8:32   ` Jaco Kroon
  0 siblings, 0 replies; 5+ messages in thread
From: Jaco Kroon @ 2022-06-10  8:32 UTC (permalink / raw)
  To: Pablo Neira Ayuso, Florian Westphal
  Cc: netfilter-devel, edumazet, ncardwell, Jozsef Kadlecsik

Thank you all.  Got entangled in other stuff again, just wanted to say I
appreciate the effort that went into this.

Kind Regards,
Jaco

On 2022/04/27 15:49, Pablo Neira Ayuso wrote:
> On Mon, Apr 25, 2022 at 11:47:11AM +0200, Florian Westphal wrote:
>> Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
>> pinpointed to nf_conntrack tcp_in_window() bug.
>>
>> tcp trace shows following sequence:
>>
>> I > R Flags [S], seq 3451342529, win 62580, options [.. tfo [|tcp]>
>> R > I Flags [S.], seq 2699962254, ack 3451342530, win 65535, options [..]
>> R > I Flags [P.], seq 1:89, ack 1, [..]
>>
>> Note 3rd ACK is from responder to initiator so following branch is taken:
>>     } else if (((state->state == TCP_CONNTRACK_SYN_SENT
>>                && dir == IP_CT_DIR_ORIGINAL)
>>                || (state->state == TCP_CONNTRACK_SYN_RECV
>>                && dir == IP_CT_DIR_REPLY))
>>                && after(end, sender->td_end)) {
>>
>> ... because state == TCP_CONNTRACK_SYN_RECV and dir is REPLY.
>> This causes the scaling factor to be reset to 0: window scale option
>> is only present in syn(ack) packets.  This in turn makes nf_conntrack
>> mark valid packets as out-of-window.
>>
>> This was always broken, it exists even in original commit where
>> window tracking was added to ip_conntrack (nf_conntrack predecessor)
>> in 2.6.9-rc1 kernel.
>>
>> Restrict to 'tcph->syn', just like the 3rd condtional added in
>> commit 82b72cb94666 ("netfilter: conntrack: re-init state for retransmitted syn-ack").
>>
>> Upon closer look, those conditionals/branches can be merged:
>>
>> Because earlier checks prevent syn-ack from showing up in
>> original direction, the 'dir' checks in the conditional quoted above are
>> redundant, remove them. Return early for pure syn retransmitted in reply
>> direction (simultaneous open).
> Applied, thanks

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

end of thread, other threads:[~2022-06-10  8:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-25  9:47 [PATCH nf] netfilter: nf_conntrack_tcp: re-init for syn packets only Florian Westphal
2022-04-25 13:41 ` Jozsef Kadlecsik
2022-04-25 14:55   ` Eric Dumazet
2022-04-27 13:49 ` Pablo Neira Ayuso
2022-06-10  8:32   ` Jaco Kroon

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.