Skip to content

Commit

Permalink
mptcp: fix sk_forward_memory corruption under memory pressure
Browse files Browse the repository at this point in the history
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: 64b9cea ("mptcp: fix spurious retransmissions")
Closes: #172
Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
  • Loading branch information
Paolo Abeni authored and matttbe committed May 17, 2021
1 parent 53eec25 commit e31f807
Showing 1 changed file with 28 additions and 8 deletions.
36 changes: 28 additions & 8 deletions net/mptcp/protocol.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1068,19 +1072,22 @@ 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))
mptcp_stop_timer(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)
Expand All @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down

0 comments on commit e31f807

Please sign in to comment.