From ccbdcf78bca37a489fedb29f5fd7266bf2f018c9 Mon Sep 17 00:00:00 2001 From: Paolo Abeni Date: Thu, 20 May 2021 20:38:58 +0200 Subject: [PATCH] mptcp: fix sk_forward_memory corruption under memory pressure 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() -> __mptcp_update_wmem() We can hit the above only under memory pressure. When that happen, forward memory accounting could be corrupted, as reported by Matt. Address the issue creating a new variant of __mptcp_clean_una_wakeup() which handle fwd memory updates with the proper care and invoking such new helper in the relevant code path. Fixes: 64b9cea7a0af ("mptcp: fix spurious retransmissions") Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/172 Reported-by: Matthieu Baerts Tested-by: Matthieu Baerts Reviewed-by: Mat Martineau Signed-off-by: Paolo Abeni --- net/mptcp/protocol.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 2bc199549a887..703a98f3e9676 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -947,6 +947,10 @@ static void __mptcp_update_wmem(struct sock *sk) { struct mptcp_sock *msk = mptcp_sk(sk); +#ifdef CONFIG_LOCKDEP + WARN_ON_ONCE(!lockdep_is_held(&sk->sk_lock.slock)); +#endif + if (!msk->wmem_reserved) return; @@ -1027,7 +1031,7 @@ static void dfrag_clear(struct sock *sk, struct mptcp_data_frag *dfrag) put_page(dfrag->page); } -static void __mptcp_clean_una(struct sock *sk) +static bool __mptcp_do_clean_una(struct sock *sk) { struct mptcp_sock *msk = mptcp_sk(sk); struct mptcp_data_frag *dtmp, *dfrag; @@ -1068,12 +1072,6 @@ static void __mptcp_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)) @@ -1081,6 +1079,15 @@ static void __mptcp_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) @@ -1089,6 +1096,19 @@ static void __mptcp_clean_una_wakeup(struct sock *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); +} + static void mptcp_enter_memory_pressure(struct sock *sk) { struct mptcp_subflow_context *subflow; @@ -2299,7 +2319,7 @@ static void __mptcp_retrans(struct sock *sk) struct sock *ssk; int ret; - __mptcp_clean_una_wakeup(sk); + mptcp_clean_una_wakeup(sk); dfrag = mptcp_rtx_head(sk); if (!dfrag) { if (mptcp_data_fin_enabled(msk)) {