[mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure"

Commit message to be replaced as follow:

"""
mptcp: fix sk_forward_memory corruption on retransmission

MPTCP sk_forward_memory handling is a bit special, as such field
is protected by the msk socket spin_lock, instead of the plain
socket lock.

Currently we have a code path updating such field without handling
the relevant lock:

__mptcp_retrans() -> __mptcp_clean_una_wakeup()

Several helpers in __mptcp_clean_una_wakeup() will update
sk_forward_alloc, possibly causing such field corruption, as reported
by Matthieu.

Address the issue providing and using a new variant of blamed function
which explicitly acquires the msk spin lock.
"""

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

On Thu, 20 May 2021, Paolo Abeni wrote:

> Commit message to be replaced as follow:
>
> """
> mptcp: fix sk_forward_memory corruption on retransmission
>
> MPTCP sk_forward_memory handling is a bit special, as such field
> is protected by the msk socket spin_lock, instead of the plain
> socket lock.
>
> Currently we have a code path updating such field without handling
> the relevant lock:
>
> __mptcp_retrans() -> __mptcp_clean_una_wakeup()
>
> Several helpers in __mptcp_clean_una_wakeup() will update
> sk_forward_alloc, possibly causing such field corruption, as reported
> by Matthieu.
>
> Address the issue providing and using a new variant of blamed function
> which explicitly acquires the msk spin lock.
> """
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)

This version of the fix looks good to me, thanks.

-Mat


>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0dcb9b753f80..446acfb85493 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1040,7 +1040,7 @@ static void dfrag_clear(struct sock *sk, struct mptcp_data_frag *dfrag)
> 	put_page(dfrag->page);
> }
>
> -static bool __mptcp_do_clean_una(struct sock *sk)
> +static void __mptcp_clean_una(struct sock *sk)
> {
> 	struct mptcp_sock *msk = mptcp_sk(sk);
> 	struct mptcp_data_frag *dtmp, *dfrag;
> @@ -1081,6 +1081,12 @@ static bool __mptcp_do_clean_una(struct sock *sk)
> 	}
>
> out:
> +	if (cleaned) {
> +		if (tcp_under_memory_pressure(sk)) {
> +			__mptcp_update_wmem(sk);
> +			sk_mem_reclaim_partial(sk);
> +		}
> +	}
>
> 	if (snd_una == READ_ONCE(msk->snd_nxt)) {
> 		if (msk->timer_ival && !mptcp_data_fin_enabled(msk))
> @@ -1088,34 +1094,22 @@ static bool __mptcp_do_clean_una(struct sock *sk)
> 	} else {
> 		mptcp_reset_timer(sk);
> 	}
> -	return cleaned;
> -}
> -
> -static void __mptcp_clean_una(struct sock *sk)
> -{
> -	if (__mptcp_do_clean_una(sk) && tcp_under_memory_pressure(sk)) {
> -		__mptcp_update_wmem(sk);
> -		sk_mem_reclaim_partial(sk);
> -	}
> }
>
> static void __mptcp_clean_una_wakeup(struct sock *sk)
> {
> +#ifdef CONFIG_LOCKDEP
> +	WARN_ON_ONCE(!lockdep_is_held(&sk->sk_lock.slock));
> +#endif
> 	__mptcp_clean_una(sk);
> 	mptcp_write_space(sk);
> }
>
> -/* variant __mptcp_clean_una_wakeup() for caller owning the msk socket lock,
> - * but not the msk_data_lock/msk socket spin lock
> - */
> static void mptcp_clean_una_wakeup(struct sock *sk)
> {
> -#ifdef CONFIG_LOCKDEP
> -	WARN_ON_ONCE(!lockdep_is_held(&sk->sk_lock));
> -#endif
> -	if (__mptcp_do_clean_una(sk) && tcp_under_memory_pressure(sk))
> -		mptcp_mem_reclaim_partial(sk);
> -	mptcp_write_space(sk);
> +	mptcp_data_lock(sk);
> +	__mptcp_clean_una_wakeup(sk);
> +	mptcp_data_unlock(sk);
> }
>
> static void mptcp_enter_memory_pressure(struct sock *sk)
> -- 
> 2.26.3
>
>
>

--
Mat Martineau
Intel

Hi Paolo, Mat,

On 20/05/2021 15:46, Paolo Abeni wrote:
> Commit message to be replaced as follow:
> 
> """
> mptcp: fix sk_forward_memory corruption on retransmission
> 
> MPTCP sk_forward_memory handling is a bit special, as such field
> is protected by the msk socket spin_lock, instead of the plain
> socket lock.
> 
> Currently we have a code path updating such field without handling
> the relevant lock:
> 
> __mptcp_retrans() -> __mptcp_clean_una_wakeup()
> 
> Several helpers in __mptcp_clean_una_wakeup() will update
> sk_forward_alloc, possibly causing such field corruption, as reported
> by Matthieu.
> 
> Address the issue providing and using a new variant of blamed function
> which explicitly acquires the msk spin lock.
> """

Thank you for the patch and the review!

- 8ab3ae44f119: "squashed" in "mptcp: fix sk_forward_memory corruption
under memory pressure"
- 48d3c38d7829: tg:msg: update commit message
- Results: f80805721af4..9d8cf0870447

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210525T103604
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210525T103604

Cheers,
Matt