Re: [PATCH mptcp-next v3 1/2] mptcp: sched: add subflow avoid flag and enforce it in core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent modification of the `mptcp_subflow_context` bitfield causes a read-modify-write data race, leading to corrupted critical subflow state.
- [Medium] The `avoid` flag is not enforced by the core when a custom BPF scheduler is loaded, directly contradicting the commit message's claims.
--

commit 99e88cae3546011d9eecb7fbb324ce6a3cff75f9
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

mptcp: sched: add subflow avoid flag and enforce it in core

This commit adds a new avoid flag to the MPTCP subflow context to allow
BPF schedulers to bench subflows. It also modifies the core send and
retransmit paths to skip subflows marked with this flag.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/349

>  - mptcp_subflow_get_send() and mptcp_subflow_get_retrans() -- the
>    default in-kernel subflow picker -- now skip an avoided subflow
>    next to the existing active-subflow check. This runs regardless of
>    whether a custom scheduler is loaded, so the flag has real weight
>    on core selection and isn't only meaningful to whatever BPF
>    program happens to read it back. avoid defaults to false and only
>    an explicit set_avoid() call flips it, so this is a no-op for
>    anyone who doesn't use it;

[Severity: Medium]
Is this description accurate regarding custom BPF schedulers?

Looking at mptcp_sched_get_send() in net/mptcp/sched.c, if a custom
BPF scheduler is active (msk->sched != &mptcp_sched_default), the core
directly returns the result of the custom scheduler's callback:

net/mptcp/sched.c:mptcp_sched_get_send() {
    ...
    if (msk->sched == &mptcp_sched_default || !msk->sched)
        return mptcp_sched_default_get_send(msk);
    return msk->sched->get_send(msk);
}

This appears to bypass mptcp_subflow_get_send() entirely (which is
where the avoid check lives) when a custom scheduler is used. Does the
core provide fallback enforcement elsewhere, or does the flag become a
no-op unless the custom BPF program manually checks it?

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3d250e8204d52..7768a464ff4e8 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -586,7 +586,8 @@ struct mptcp_subflow_context {
>  		is_mptfo : 1,	    /* subflow is doing TFO */
>  		close_event_done : 1,       /* has done the post-closed part */
>  		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
> -		__unused : 9;
> +		avoid : 1,	    /* pkt scheduler: skip subflow if possible */
> +		__unused : 8;
>  	bool	data_avail;

[Severity: High]
Can placing avoid in this shared bitfield lead to data corruption via
a read-modify-write (RMW) race?

The core network stack modifies other flags in this same 32-bit bitfield
from the RX softirq path while holding only the TCP subflow socket lock
(ssk). For example, it clears subflow->map_valid = 0 in
net/mptcp/subflow.c:mptcp_subflow_data_available().

Because bitfield assignments compile to non-atomic RMW operations on the
containing 32-bit word, modifying this new flag from the BPF kfunc under
the MPTCP socket lock (msk) could race with those softirq updates.

[ ... ]

> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index 1e59072d478c9..8a448814a11d4 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -165,6 +165,12 @@ void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
>  	WRITE_ONCE(subflow->scheduled, scheduled);
>  }
>  
> +__bpf_kfunc void mptcp_subflow_set_avoid(struct mptcp_subflow_context *subflow,
> +					 bool avoid)
> +{
> +	subflow->avoid = avoid;
> +}

[Severity: High]
Does this bitfield assignment risk overwriting concurrent softirq updates?

Since this function executes under the msk lock and not the ssk lock,
this assignment races with the ssk-locked updates to the same bitfield word
mentioned above, potentially causing flags like map_valid or mpc_drop
to be silently dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831094651.2682660-1-kalpan.jani@mpiricsoftware.com?part=1