Squash-to: "mptcp: give rcvlowat some love"

[PATCH mptcp-next] Squash-to: "mptcp: give rcvlowat some love"

Posted by Paolo Abeni 7 months ago

Christoph reported a couple of serious splat caused by
the mentioned patch.

mptcp_set_rcvlowat() can use msk->scaling_ratio, before
such field is initialized, causing a divide by zero: we
need to init it in the sock constructor.

Additionally the same function bogusly cast an msk to a
tcp_sock, causing memory corruption. The reproducer likely
clears the sk refcount for the next msk allocated into the
same slab.

The intent was to properly propagate the rcvbuf changes to
the subflows. Let's do that explicitly.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
--
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/442
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/443

since the above issues are introduced by the squash-to patch, I think
we can't have the tag in the final patch.
---
 net/mptcp/protocol.c |  2 ++
 net/mptcp/sockopt.c  | 17 ++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 6f9e116598ed..3ef6368e26f6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2758,6 +2758,8 @@ static void __mptcp_init_sock(struct sock *sk)
 	msk->rmem_fwd_alloc = 0;
 	WRITE_ONCE(msk->rmem_released, 0);
 	msk->timer_ival = TCP_RTO_MIN;
+	msk->scaling_ratio = (1200 << TCP_RMEM_TO_WIN_SCALE) /
+			     SKB_TRUESIZE(4096);
 
 	WRITE_ONCE(msk->first, NULL);
 	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 340e87195a27..6b37946b5c52 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -1473,6 +1473,7 @@ void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)
  */
 int mptcp_set_rcvlowat(struct sock *sk, int val)
 {
+	struct mptcp_subflow_context *subflow;
 	int space, cap;
 
 	if (sk->sk_userlocks & SOCK_RCVBUF_LOCK)
@@ -1490,9 +1491,19 @@ int mptcp_set_rcvlowat(struct sock *sk, int val)
 		return 0;
 
 	space = __tcp_space_from_win(mptcp_sk(sk)->scaling_ratio, val);
-	if (space > sk->sk_rcvbuf) {
-		WRITE_ONCE(sk->sk_rcvbuf, space);
-		tcp_sk(sk)->window_clamp = val;
+	if (space <= sk->sk_rcvbuf)
+		return 0;
+
+	/* propagate the rcvbuf changes to all the subflows */
+	WRITE_ONCE(sk->sk_rcvbuf, space);
+	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		bool slow;
+
+		slow = lock_sock_fast(ssk);
+		WRITE_ONCE(ssk->sk_rcvbuf, space);
+		tcp_sk(ssk)->window_clamp = val;
+		unlock_sock_fast(ssk, slow);
 	}
 	return 0;
 }
-- 
2.41.0

Re: [PATCH mptcp-next] Squash-to: "mptcp: give rcvlowat some love"

Posted by Matthieu Baerts 7 months ago

Hi Paolo,

On 28/09/2023 17:52, Paolo Abeni wrote:
> Christoph reported a couple of serious splat caused by
> the mentioned patch.
> 
> mptcp_set_rcvlowat() can use msk->scaling_ratio, before
> such field is initialized, causing a divide by zero: we
> need to init it in the sock constructor.
> 
> Additionally the same function bogusly cast an msk to a
> tcp_sock, causing memory corruption. The reproducer likely
> clears the sk refcount for the next msk allocated into the
> same slab.
> 
> The intent was to properly propagate the rcvbuf changes to
> the subflows. Let's do that explicitly.

Thank you for fixing all that!

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> --
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/442
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/443
> 
> since the above issues are introduced by the squash-to patch, I think
> we can't have the tag in the final patch.
> ---
>  net/mptcp/protocol.c |  2 ++
>  net/mptcp/sockopt.c  | 17 ++++++++++++++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 6f9e116598ed..3ef6368e26f6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2758,6 +2758,8 @@ static void __mptcp_init_sock(struct sock *sk)
>  	msk->rmem_fwd_alloc = 0;
>  	WRITE_ONCE(msk->rmem_released, 0);
>  	msk->timer_ival = TCP_RTO_MIN;
> +	msk->scaling_ratio = (1200 << TCP_RMEM_TO_WIN_SCALE) /
> +			     SKB_TRUESIZE(4096);

I see that the code in TCP is quite recent: dfa2f0483360 ("tcp: get rid
of sysctl_tcp_adv_win_scale").

Do we want to add a new function in include/net/tcp.h which returns:

  (1200 << TCP_RMEM_TO_WIN_SCALE) / SKB_TRUESIZE(4096);

and use it here and in tcp_scaling_ratio_init()?

>  
>  	WRITE_ONCE(msk->first, NULL);
>  	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash-to: "mptcp: give rcvlowat some love"

Posted by Paolo Abeni 7 months ago

On Thu, 2023-09-28 at 19:11 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 28/09/2023 17:52, Paolo Abeni wrote:
> > Christoph reported a couple of serious splat caused by
> > the mentioned patch.
> > 
> > mptcp_set_rcvlowat() can use msk->scaling_ratio, before
> > such field is initialized, causing a divide by zero: we
> > need to init it in the sock constructor.
> > 
> > Additionally the same function bogusly cast an msk to a
> > tcp_sock, causing memory corruption. The reproducer likely
> > clears the sk refcount for the next msk allocated into the
> > same slab.
> > 
> > The intent was to properly propagate the rcvbuf changes to
> > the subflows. Let's do that explicitly.
> 
> Thank you for fixing all that!
> 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > --
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/442
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/443
> > 
> > since the above issues are introduced by the squash-to patch, I think
> > we can't have the tag in the final patch.
> > ---
> >  net/mptcp/protocol.c |  2 ++
> >  net/mptcp/sockopt.c  | 17 ++++++++++++++---
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 6f9e116598ed..3ef6368e26f6 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2758,6 +2758,8 @@ static void __mptcp_init_sock(struct sock *sk)
> >  	msk->rmem_fwd_alloc = 0;
> >  	WRITE_ONCE(msk->rmem_released, 0);
> >  	msk->timer_ival = TCP_RTO_MIN;
> > +	msk->scaling_ratio = (1200 << TCP_RMEM_TO_WIN_SCALE) /
> > +			     SKB_TRUESIZE(4096);
> 
> I see that the code in TCP is quite recent: dfa2f0483360 ("tcp: get rid
> of sysctl_tcp_adv_win_scale").
> 
> Do we want to add a new function in include/net/tcp.h which returns:
> 
>   (1200 << TCP_RMEM_TO_WIN_SCALE) / SKB_TRUESIZE(4096);
> 
> and use it here and in tcp_scaling_ratio_init()?

Perhaps just a define? 

#define TCP_INITIAL_SCALING_RATIO ((1200 << TCP_RMEM_TO_WIN_SCALE) / SKB_TRUESIZE(4096))

Note: we want to avoid any argument, to avoid 'different sock' struct
issues.

/P