| Message ID | 03e3ba49f6d847686dced53e915ace73928fd008.1621517601.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 8ab3ae44f1192f30a89bf0e58f4e29173307b982 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure" | expand |
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
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)
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(-)