* [PATCH mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure"
@ 2021-05-20 13:46 Paolo Abeni
2021-05-20 23:14 ` Mat Martineau
2021-05-25 10:36 ` Matthieu Baerts
0 siblings, 2 replies; 3+ messages in thread
From: Paolo Abeni @ 2021-05-20 13:46 UTC (permalink / raw)
To: mptcp; +Cc: Matthieu Baerts
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure"
2021-05-20 13:46 [PATCH mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure" Paolo Abeni
@ 2021-05-20 23:14 ` Mat Martineau
2021-05-25 10:36 ` Matthieu Baerts
1 sibling, 0 replies; 3+ messages in thread
From: Mat Martineau @ 2021-05-20 23:14 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Matthieu Baerts
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure"
2021-05-20 13:46 [PATCH mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure" Paolo Abeni
2021-05-20 23:14 ` Mat Martineau
@ 2021-05-25 10:36 ` Matthieu Baerts
1 sibling, 0 replies; 3+ messages in thread
From: Matthieu Baerts @ 2021-05-25 10:36 UTC (permalink / raw)
To: Paolo Abeni, mptcp
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
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-25 10:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20 13:46 [PATCH mptcp-net] Squash-to: "mptcp: fix sk_forward_memory corruption under memory pressure" Paolo Abeni
2021-05-20 23:14 ` Mat Martineau
2021-05-25 10:36 ` Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.