diff mbox series

Squash-to: tcp: Check for filled TCP option space before SACK

Message ID 7ad85d0d66f3dcbebbd9d2ee6896cfedd1620812.1576499809.git.pabeni@redhat.com
State Accepted, archived
Delegated to: Matthieu Baerts
Headers show
Series Squash-to: tcp: Check for filled TCP option space before SACK | expand

Commit Message

Paolo Abeni Dec. 16, 2019, 12:37 p.m. UTC 
Less invasive checks, so that the number of branchs when hitting
the sack code path decreses compared to the current vanilla tree.
--
Some addictional check in tcp_established_options() is needed, otherwise
the tcp header will be corrupted when mptcp_established_options() fully
consumes the TCP option space. This condition can be reached without
MPTCP, so not sure if the result patch is still worthy for -net ?!?
---
 net/ipv4/tcp_output.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Matthieu Baerts Dec. 16, 2019, 1:05 p.m. UTC | #1 
Hi Paolo,

On 16/12/2019 13:37, Paolo Abeni wrote:
> Less invasive checks, so that the number of branchs when hitting
> the sack code path decreses compared to the current vanilla tree.
> --
> Some addictional check in tcp_established_options() is needed, otherwise
> the tcp header will be corrupted when mptcp_established_options() fully
> consumes the TCP option space. This condition can be reached without
> MPTCP, so not sure if the result patch is still worthy for -net ?!?


Thank you for the patch!

It looks good to me!

I guess we will need to update the commit message for "tcp: Check for 
filled TCP option space before SACK" :)

Cheers,
Matth
Matthieu Baerts Dec. 16, 2019, 6:28 p.m. UTC | #2 
Hi Paolo,

On 16/12/2019 14:05, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 16/12/2019 13:37, Paolo Abeni wrote:
>> Less invasive checks, so that the number of branchs when hitting
>> the sack code path decreses compared to the current vanilla tree.
>> -- 
>> Some addictional check in tcp_established_options() is needed, otherwise
>> the tcp header will be corrupted when mptcp_established_options() fully
>> consumes the TCP option space. This condition can be reached without
>> MPTCP, so not sure if the result patch is still worthy for -net ?!?
> 
> 
> Thank you for the patch!
> 
> It looks good to me!

- b474fba77e6f: "squashed" in "tcp: Check for filled TCP option space 
before SACK"
- 5920a223da80: "Signed-off-by" + "Co-developed-by"
- 7ed2a1728574: conflict in 
t/mptcp-Handle-MP_CAPABLE-options-for-outgoing-connections
- b8aff0a1a12d..66880827b48f: result

> I guess we will need to update the commit message for "tcp: Check for 
> filled TCP option space before SACK" :)

What can I put here? :)

   v1:
     - less invasive checks (Eric Dumazet)

Should we also mention that with MPTCP and TS, we can use all the option 
space?

I will launch the tests + export soon.

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 710ab45badfa..e797ca6c6d7d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -748,19 +748,20 @@  static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
 		size += TCPOLEN_TSTAMP_ALIGNED;
 	}
 
-	if (size + TCPOLEN_SACK_BASE_ALIGNED >= MAX_TCP_OPTION_SPACE)
-		return size;
-
 	eff_sacks = tp->rx_opt.num_sacks + tp->rx_opt.dsack;
 	if (unlikely(eff_sacks)) {
 		const unsigned int remaining = MAX_TCP_OPTION_SPACE - size;
+		if (unlikely(remaining < TCPOLEN_SACK_BASE_ALIGNED +
+					 TCPOLEN_SACK_PERBLOCK))
+			return size;
+
 		opts->num_sack_blocks =
 			min_t(unsigned int, eff_sacks,
 			      (remaining - TCPOLEN_SACK_BASE_ALIGNED) /
 			      TCPOLEN_SACK_PERBLOCK);
-		if (likely(opts->num_sack_blocks))
-			size += TCPOLEN_SACK_BASE_ALIGNED +
-				opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
+
+		size += TCPOLEN_SACK_BASE_ALIGNED +
+			opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
 	}
 
 	return size;