[PATCH mptcp-net] Squash-to: "mptcp: fix possible divide by zero"

* [PATCH mptcp-net] Squash-to: "mptcp: fix possible divide by zero"
@ 2021-08-24  9:16 Paolo Abeni
  2021-08-24 10:07 ` Matthieu Baerts
  0 siblings, 1 reply; 2+ messages in thread
From: Paolo Abeni @ 2021-08-24  9:16 UTC (permalink / raw)
  To: mptcp

With the current 'can_collapse' definition, skb collapsing/coalescing
may not happen have if can_collapse is true. Since we fill the
tx cache only if 'can_collapse' is false, the TCP code can end-up
allocating an skb without mptcp extension and triggering a later
WARN

Fix the issue using a much more restrictive check for collapsing, so
that if such check is true, collapsing will happen for sure.
Additionally change the variable name accordingly and remove a now
erronous WARN_ON - collapsing can happen even if must_collapse is false.

Note that if by some bug we hit the opposite erroneous condition -
collapsing did not happen even if must_collapse is true - we will hit
the existing WARN_ON_ONCE(!mpext) check.

The coming message in the squash to commit should be updated replacing:

"""
This change addresses the issue refactoring the skb allocation
code so that skb allocation always happens after the call
to tcp_send_mss() correctly initialize mss_now.
"""

with:
"""
This change addresses the issue refactoring the skb allocation
code checking if skb collapsing will happen for sure and doing
the skb allocation only after such check. Skb allocation will happens
only after that the call to tcp_send_mss() correctly initializes mss_now.
"""

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index cd690794e22b..af8f677162f3 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1290,7 +1290,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	bool zero_window_probe = false;
 	struct mptcp_ext *mpext = NULL;
 	struct sk_buff *skb, *tail;
-	bool can_collapse = false;
+	bool must_collapse = false;
 	int size_bias = 0;
 	int avail_size;
 	size_t ret = 0;
@@ -1310,17 +1310,21 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 		 * SSN association set here
 		 */
 		mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
-		can_collapse = (info->size_goal - skb->len > 0) &&
-			 mptcp_skb_can_collapse_to(data_seq, skb, mpext);
-		if (!can_collapse) {
+		if (!mptcp_skb_can_collapse_to(data_seq, skb, mpext)) {
 			TCP_SKB_CB(skb)->eor = 1;
-		} else {
+			goto alloc_skb;
+		}
+
+		must_collapse = (info->size_goal - skb->len > 0) &&
+				(skb_shinfo(skb)->nr_frags < sysctl_max_skb_frags);
+		if (must_collapse) {
 			size_bias = skb->len;
 			avail_size = info->size_goal - skb->len;
 		}
 	}
 
-	if (!can_collapse && !ssk->sk_tx_skb_cache &&
+alloc_skb:
+	if (!must_collapse && !ssk->sk_tx_skb_cache &&
 	    !mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
 		return 0;
 
@@ -1353,7 +1357,6 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	if (skb == tail) {
 		TCP_SKB_CB(tail)->tcp_flags &= ~TCPHDR_PSH;
 		mpext->data_len += ret;
-		WARN_ON_ONCE(!can_collapse);
 		WARN_ON_ONCE(zero_window_probe);
 		goto out;
 	}
-- 
2.26.3


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