* [PATCH mptcp 0/2] mptcp: a couple of fix - almost
@ 2021-08-20 17:44 Paolo Abeni
2021-08-20 17:44 ` [PATCH mptcp 1/2] mptcp: fix possible divide by zero Paolo Abeni
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Paolo Abeni @ 2021-08-20 17:44 UTC (permalink / raw)
To: mptcp
This are couple of changes poped-up while investigating
issues/219. They does not address the mentioned issue,
but the first one should fix an actual bug, the other
is more a cleanup, so the target tree is undefined ;)
Paolo Abeni (2):
mptcp: fix possible divide by zero
mptcp: make the locking tx schema more readable
net/mptcp/protocol.c | 73 +++++++++++++++++++++-----------------------
1 file changed, 35 insertions(+), 38 deletions(-)
--
2.26.3
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH mptcp 1/2] mptcp: fix possible divide by zero 2021-08-20 17:44 [PATCH mptcp 0/2] mptcp: a couple of fix - almost Paolo Abeni @ 2021-08-20 17:44 ` Paolo Abeni 2021-08-20 17:53 ` Mat Martineau 2021-08-24 7:28 ` Matthieu Baerts 2021-08-20 17:44 ` [PATCH mptcp 2/2] mptcp: make the locking tx schema more readable Paolo Abeni 2021-08-20 23:02 ` [PATCH mptcp 0/2] mptcp: a couple of fix - almost Mat Martineau 2 siblings, 2 replies; 7+ messages in thread From: Paolo Abeni @ 2021-08-20 17:44 UTC (permalink / raw) To: mptcp Florian noted that if mptcp_alloc_tx_skb() allocation fails in __mptcp_push_pending(), we can end-up invoking mptcp_push_release()/tcp_push() with a zero mss, causing a divide by 0 error. This change addresses the issue refactoring the skb allocation code so that skb allocation always happens after the call to tcp_send_mss() correctly initialize mss_now. As side bonuses we now fill the skb tx cache only when needed, and this also clean-up a bit the output path. Reported-by: Florian Westphal <fw@strlen.de> Fixes: 724cfd2ee8aa ("mptcp: allocate TX skbs in msk context") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 63 ++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index b5f8ad634571..84aa11a0b2d5 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1003,6 +1003,15 @@ static void mptcp_wmem_uncharge(struct sock *sk, int size) msk->wmem_reserved += size; } +static void __mptcp_mem_reclaim_partial(struct sock *sk) +{ +#ifdef CONFIG_LOCKDEP + WARN_ON_ONCE(!lockdep_is_held(&sk->sk_lock.slock)); +#endif + __mptcp_update_wmem(sk); + sk_mem_reclaim_partial(sk); +} + static void mptcp_mem_reclaim_partial(struct sock *sk) { struct mptcp_sock *msk = mptcp_sk(sk); @@ -1094,12 +1103,8 @@ static void __mptcp_clean_una(struct sock *sk) msk->recovery = false; out: - if (cleaned) { - if (tcp_under_memory_pressure(sk)) { - __mptcp_update_wmem(sk); - sk_mem_reclaim_partial(sk); - } - } + if (cleaned && tcp_under_memory_pressure(sk)) + __mptcp_mem_reclaim_partial(sk); if (snd_una == READ_ONCE(msk->snd_nxt) && !msk->recovery) { if (mptcp_timer_pending(sk) && !mptcp_data_fin_enabled(msk)) @@ -1179,6 +1184,7 @@ struct mptcp_sendmsg_info { u16 limit; u16 sent; unsigned int flags; + bool data_lock_held; }; static int mptcp_check_allowed_size(struct mptcp_sock *msk, u64 data_seq, @@ -1250,17 +1256,17 @@ static bool __mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, gfp_t gfp) return false; } -static bool mptcp_must_reclaim_memory(struct sock *sk, struct sock *ssk) +static bool mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk, bool data_lock_held) { - return !ssk->sk_tx_skb_cache && - tcp_under_memory_pressure(sk); -} + gfp_t gfp = data_lock_held ? GFP_ATOMIC : sk->sk_allocation; -static bool mptcp_alloc_tx_skb(struct sock *sk, struct sock *ssk) -{ - if (unlikely(mptcp_must_reclaim_memory(sk, ssk))) - mptcp_mem_reclaim_partial(sk); - return __mptcp_alloc_tx_skb(sk, ssk, sk->sk_allocation); + if (unlikely(tcp_under_memory_pressure(sk))) { + if (data_lock_held) + __mptcp_mem_reclaim_partial(sk); + else + mptcp_mem_reclaim_partial(sk); + } + return __mptcp_alloc_tx_skb(sk, ssk, gfp); } /* note: this always recompute the csum on the whole skb, even @@ -1314,6 +1320,10 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk, } } + if (!can_collapse && !ssk->sk_tx_skb_cache && + !mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held)) + return 0; + /* Zero window and all data acked? Probe. */ avail_size = mptcp_check_allowed_size(msk, data_seq, avail_size); if (avail_size == 0) { @@ -1526,15 +1536,6 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) if (ssk != prev_ssk || !prev_ssk) lock_sock(ssk); - /* keep it simple and always provide a new skb for the - * subflow, even if we will not use it when collapsing - * on the pending one - */ - if (!mptcp_alloc_tx_skb(sk, ssk)) { - mptcp_push_release(sk, ssk, &info); - goto out; - } - ret = mptcp_sendmsg_frag(sk, ssk, dfrag, &info); if (ret <= 0) { mptcp_push_release(sk, ssk, &info); @@ -1567,7 +1568,9 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk) { struct mptcp_sock *msk = mptcp_sk(sk); - struct mptcp_sendmsg_info info; + struct mptcp_sendmsg_info info = { + .data_lock_held = true, + }; struct mptcp_data_frag *dfrag; struct sock *xmit_ssk; int len, copied = 0; @@ -1593,13 +1596,6 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk) goto out; } - if (unlikely(mptcp_must_reclaim_memory(sk, ssk))) { - __mptcp_update_wmem(sk); - sk_mem_reclaim_partial(sk); - } - if (!__mptcp_alloc_tx_skb(sk, ssk, GFP_ATOMIC)) - goto out; - ret = mptcp_sendmsg_frag(sk, ssk, dfrag, &info); if (ret <= 0) goto out; @@ -2405,9 +2401,6 @@ static void __mptcp_retrans(struct sock *sk) info.sent = 0; info.limit = READ_ONCE(msk->csum_enabled) ? dfrag->data_len : dfrag->already_sent; while (info.sent < info.limit) { - if (!mptcp_alloc_tx_skb(sk, ssk)) - break; - ret = mptcp_sendmsg_frag(sk, ssk, dfrag, &info); if (ret <= 0) break; -- 2.26.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp 1/2] mptcp: fix possible divide by zero 2021-08-20 17:44 ` [PATCH mptcp 1/2] mptcp: fix possible divide by zero Paolo Abeni @ 2021-08-20 17:53 ` Mat Martineau 2021-08-24 7:28 ` Matthieu Baerts 1 sibling, 0 replies; 7+ messages in thread From: Mat Martineau @ 2021-08-20 17:53 UTC (permalink / raw) To: Paolo Abeni; +Cc: mptcp On Fri, 20 Aug 2021, Paolo Abeni wrote: > Florian noted that if mptcp_alloc_tx_skb() allocation fails > in __mptcp_push_pending(), we can end-up invoking > mptcp_push_release()/tcp_push() with a zero mss, causing > a divide by 0 error. > I'll start running this in syzkaller now and then follow up later with review comments - thanks! - Mat > This change addresses the issue refactoring the skb allocation > code so that skb allocation always happens after the call > to tcp_send_mss() correctly initialize mss_now. > > As side bonuses we now fill the skb tx cache only when needed, > and this also clean-up a bit the output path. > > Reported-by: Florian Westphal <fw@strlen.de> > Fixes: 724cfd2ee8aa ("mptcp: allocate TX skbs in msk context") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/protocol.c | 63 ++++++++++++++++++++------------------------ > 1 file changed, 28 insertions(+), 35 deletions(-) -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp 1/2] mptcp: fix possible divide by zero 2021-08-20 17:44 ` [PATCH mptcp 1/2] mptcp: fix possible divide by zero Paolo Abeni 2021-08-20 17:53 ` Mat Martineau @ 2021-08-24 7:28 ` Matthieu Baerts 1 sibling, 0 replies; 7+ messages in thread From: Matthieu Baerts @ 2021-08-24 7:28 UTC (permalink / raw) To: mptcp; +Cc: Paolo Abeni Hello On 20/08/2021 19:44, Paolo Abeni wrote: > Florian noted that if mptcp_alloc_tx_skb() allocation fails > in __mptcp_push_pending(), we can end-up invoking > mptcp_push_release()/tcp_push() with a zero mss, causing > a divide by 0 error. > > This change addresses the issue refactoring the skb allocation > code so that skb allocation always happens after the call > to tcp_send_mss() correctly initialize mss_now. > > As side bonuses we now fill the skb tx cache only when needed, > and this also clean-up a bit the output path. For those who might have missed the info, a regression has been introduced by this patch. This is tracked on Github: https://github.com/multipath-tcp/mptcp_net-next/issues/227 (Paolo already shared a patch that seems fixing the issue) Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH mptcp 2/2] mptcp: make the locking tx schema more readable 2021-08-20 17:44 [PATCH mptcp 0/2] mptcp: a couple of fix - almost Paolo Abeni 2021-08-20 17:44 ` [PATCH mptcp 1/2] mptcp: fix possible divide by zero Paolo Abeni @ 2021-08-20 17:44 ` Paolo Abeni 2021-08-20 23:02 ` [PATCH mptcp 0/2] mptcp: a couple of fix - almost Mat Martineau 2 siblings, 0 replies; 7+ messages in thread From: Paolo Abeni @ 2021-08-20 17:44 UTC (permalink / raw) To: mptcp Florian noted the locking schema used by __mptcp_push_pending() is hard to follow, let's add some more descriptive comments and drop an unneeded and confusing check. Suggested-by: Florian Westphal <fw@strlen.de> Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 84aa11a0b2d5..cd690794e22b 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1525,15 +1525,19 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) mptcp_flush_join_list(msk); ssk = mptcp_subflow_get_send(msk); - /* try to keep the subflow socket lock across - * consecutive xmit on the same socket + /* First check. If the ssk has changed since + * the last round, release prev_ssk */ if (ssk != prev_ssk && prev_ssk) mptcp_push_release(sk, prev_ssk, &info); if (!ssk) goto out; - if (ssk != prev_ssk || !prev_ssk) + /* Need to lock the new subflow only if different + * from the previous one, otherwise we are still + * helding the relevant lock + */ + if (ssk != prev_ssk) lock_sock(ssk); ret = mptcp_sendmsg_frag(sk, ssk, dfrag, &info); -- 2.26.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp 0/2] mptcp: a couple of fix - almost 2021-08-20 17:44 [PATCH mptcp 0/2] mptcp: a couple of fix - almost Paolo Abeni 2021-08-20 17:44 ` [PATCH mptcp 1/2] mptcp: fix possible divide by zero Paolo Abeni 2021-08-20 17:44 ` [PATCH mptcp 2/2] mptcp: make the locking tx schema more readable Paolo Abeni @ 2021-08-20 23:02 ` Mat Martineau 2021-08-23 7:16 ` Matthieu Baerts 2 siblings, 1 reply; 7+ messages in thread From: Mat Martineau @ 2021-08-20 23:02 UTC (permalink / raw) To: Paolo Abeni; +Cc: mptcp On Fri, 20 Aug 2021, Paolo Abeni wrote: > This are couple of changes poped-up while investigating > issues/219. They does not address the mentioned issue, > but the first one should fix an actual bug, the other > is more a cleanup, so the target tree is undefined ;) > > Paolo Abeni (2): > mptcp: fix possible divide by zero > mptcp: make the locking tx schema more readable > > net/mptcp/protocol.c | 73 +++++++++++++++++++++----------------------- > 1 file changed, 35 insertions(+), 38 deletions(-) > > -- > 2.26.3 So far my syzkaller setup hasn't seen the div-by-0 again, but it was typically taking days to show up. I'll leave that running over the weekend. Changes look good to me - mptcp_push_release() is now only called from places where info->mss_now is non-zero. Patch 1 for -net, patch 2 for net-next seems appropriate. Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> -- Mat Martineau Intel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp 0/2] mptcp: a couple of fix - almost 2021-08-20 23:02 ` [PATCH mptcp 0/2] mptcp: a couple of fix - almost Mat Martineau @ 2021-08-23 7:16 ` Matthieu Baerts 0 siblings, 0 replies; 7+ messages in thread From: Matthieu Baerts @ 2021-08-23 7:16 UTC (permalink / raw) To: Mat Martineau, Paolo Abeni; +Cc: mptcp Hi Paolo, Florian, Mat, On 21/08/2021 01:02, Mat Martineau wrote: > On Fri, 20 Aug 2021, Paolo Abeni wrote: > >> This are couple of changes poped-up while investigating >> issues/219. They does not address the mentioned issue, >> but the first one should fix an actual bug, the other >> is more a cleanup, so the target tree is undefined ;) > > So far my syzkaller setup hasn't seen the div-by-0 again, but it was > typically taking days to show up. I'll leave that running over the weekend. > > Changes look good to me - mptcp_push_release() is now only called from > places where info->mss_now is non-zero. > > Patch 1 for -net, patch 2 for net-next seems appropriate. Thank you for the patches, the suggestions and the reviews! These two patches are now in our tree (net and net-next) with Mat's RvB tag: - 5d8855f6a71f: mptcp: fix possible divide by zero - Results: 84fc11b83a29..653135eedf64 - 33708631c1fd: mptcp: make the locking tx schema more readable - Results: 653135eedf64..ebac4c6745a4 Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210823T071618 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210823T071618 Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-24 7:28 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-20 17:44 [PATCH mptcp 0/2] mptcp: a couple of fix - almost Paolo Abeni 2021-08-20 17:44 ` [PATCH mptcp 1/2] mptcp: fix possible divide by zero Paolo Abeni 2021-08-20 17:53 ` Mat Martineau 2021-08-24 7:28 ` Matthieu Baerts 2021-08-20 17:44 ` [PATCH mptcp 2/2] mptcp: make the locking tx schema more readable Paolo Abeni 2021-08-20 23:02 ` [PATCH mptcp 0/2] mptcp: a couple of fix - almost Mat Martineau 2021-08-23 7:16 ` Matthieu Baerts
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).