* Re: [PATCH] subflow: relax WARN in subflow_data_ready() on teardown races
2025-12-12 9:59 [PATCH] subflow: relax WARN in subflow_data_ready() on teardown races evan.li
@ 2025-12-12 10:34 ` Matthieu Baerts
2025-12-12 11:15 ` MPTCP CI
1 sibling, 0 replies; 3+ messages in thread
From: Matthieu Baerts @ 2025-12-12 10:34 UTC (permalink / raw)
To: evan.li
Cc: netdev, mptcp, linux-kernel, kitta, martineau, davem, edumazet,
kuba, pabeni, horms
Hi Evan, Kitta,
Thank you for sharing this patch.
On 12/12/2025 10:59, evan.li@linux.alibaba.com wrote:
> From: Evan Li <evan.li@linux.alibaba.com>
>
> A WARN splat in subflow_data_ready() can be triggered when a subflow
> enters an unexpected state during connection teardown or cleanup:
>
> WARNING: net/mptcp/subflow.c:1527 at subflow_data_ready+0x38a/0x670
Please always share the full stacktrace, not just the warning: it helps
reviewers, devs later, and other people who found the same issue on
their side.
> This comes from the following check:
>
> WARN_ON_ONCE(!__mptcp_check_fallback(msk) &&
> !subflow->mp_capable &&
> !subflow->mp_join &&
> !(state & TCPF_CLOSE));
>
> Under fuzzing and other stress scenarios, there are legitimate windows
> where this condition can become true without indicating a real bug, for
> example:
>
> during connection teardown / fastclose handling
> races with subflow destruction
> packets arriving after subflow cleanup
> when the parent MPTCP socket is being destroyed
> After commit ae155060247b ("mptcp: fix duplicate reset on fastclose"),
> these edge cases became easier to trigger and the WARN started firing
> spuriously, causing noisy reports but no functional issues.
>
> Refine the state check in subflow_data_ready() so that:
>
> if the socket is in a known teardown/cleanup situation
> (SOCK_DEAD, zero parent refcnt, or repair/recv-queue handling),
> the function simply returns without emitting a warning; and
>
> for other unexpected states, we emit a ratelimited pr_debug() to
> aid debugging, instead of a WARN_ON_ONCE() that can panic
> fuzzing/CI kernels or flood logs in production.
>
> This suppresses the bogus warning while preserving diagnostics for any
> real state machine bugs.
>
> Fixes: ae155060247b ("mptcp: fix duplicate reset on fastclose")
> Reported-by: kitta <kitta@linux.alibaba.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220856
I don't think you should continue using Bugzilla: it is no longer used
for the Networking subsystem, and it might be shutdown soon. Instead,
please check the instructions in the MAINTAINERS file: for general
Networking issues, send them by email. For MPTCP, there is a bug tracker
on GitHub.
Note that you don't need to open an issue somewhere if you already have
the patch: simply put all the details in the commit message, e.g.
stacktrace, reproducer, conditions, etc. You only need to open an issue
somewhere if you need to include large files, or if you need help to fix
it. I know Checkpatch will ask you to add a link after a "Reported-by"
but that's only valid if the initial report was done publicly: in your
case, you can then ignore this warning.
> Co-developed-by: kitta <kitta@linux.alibaba.com>
(Note that a Co-developed-by should be followed by a Signed-off-by from
the same person.)
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
While at it, please next time use the 'net' prefix, see:
https://docs.kernel.org/process/maintainer-netdev.html
> Signed-off-by: Evan Li <evan.li@linux.alibaba.com>
> ---
> net/mptcp/subflow.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 86ce58ae5..01d30679c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1524,9 +1524,27 @@ static void subflow_data_ready(struct sock *sk)
> return;
> }
>
> - WARN_ON_ONCE(!__mptcp_check_fallback(msk) && !subflow->mp_capable &&
> - !subflow->mp_join && !(state & TCPF_CLOSE));
> -
> + /* Check if subflow is in a valid state. Skip warning for legitimate edge cases
> + * such as connection teardown, race conditions, or when parent is being destroyed.
> + */
> + if (!__mptcp_check_fallback(msk) && !subflow->mp_capable &&
> + !subflow->mp_join && !(state & TCPF_CLOSE)) {
> + /* Legitimate cases where this can happen:
> + * 1. During connection teardown
> + * 2. Race conditions with subflow destruction
> + * 3. Packets arriving after subflow cleanup
> + * Log debug info but don't warn loudly in production.
> + */
> + if (unlikely(tcp_sk(sk)->repair_queue == TCP_RECV_QUEUE ||
> + sock_flag(sk, SOCK_DEAD) || !refcount_read(&parent->sk_refcnt))) {
> + /* Expected during cleanup, silently return */
> + return;
> + }
> + /* For other cases, still log for debugging but don't WARN */
> + if (net_ratelimit())
> + pr_debug("MPTCP: subflow in unexpected state sk=%p parent=%p state=%u\n",
> + sk, parent, state);
> + }
The warning was there to catch issues: it is *not* normal to have to
process data in this state. I think it is better to prevent
subflow_data_ready() to be called in this state than ignoring the
warning, even if the warnings you saw didn't cause visible functional
issues. Please also note that a pr_debug() will very likely not be
caught in case of real issues: a WARN_ON_ONCE should have been kept.
Regarding this issue, it was already tracked in our bug tracker, see:
https://github.com/multipath-tcp/mptcp_net-next/issues/586
There are two patches from Paolo in our tree addressing it:
https://lore.kernel.org/mptcp/cover.1764928598.git.pabeni@redhat.com
I was waiting to upstream them because they are not urgent and net
maintainers are busy at the LPC for the moment, but I guess I should
still send them to avoid syzkaller instances to complain about these
issues. I will try to do that today.
Do you mind checking if they fix the issues on your side please?
pw-bot: cr
> if (mptcp_subflow_data_available(sk)) {
> mptcp_data_ready(parent, sk);
>
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
^ permalink raw reply [flat|nested] 3+ messages in thread