Re: [PATCH mptcp-next v3 2/2] selftests: mptcp: bpf: exercise the subflow avoid flag across rounds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The kfunc mptcp_subflow_set_avoid() modifies the 'avoid' bitfield without holding the TCP socket lock, causing a data race with the TCP receive path.
- [Medium] Global .bss variables are incorrectly used for per-socket state, causing race conditions and test flakiness.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem modern kernel comment style.
--

commit 1f082f0309b048cf33dbb4e85b2d29227d5feef3
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

selftests: mptcp: bpf: exercise the subflow avoid flag across rounds

This commit introduces a new BPF struct_ops scheduler selftest in
mptcp_bpf_avoid.c to test mptcp_subflow_set_avoid(). It sets the avoid flag
once on the first scheduling round and verifies that the core preserves this
state across multiple get_send() invocations.

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

> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_avoid.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_avoid.c
> new file mode 100644
> index 0000000000000..8b22c5967a7a5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_avoid.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026, MPTCP. */
> +
> +#include "mptcp_bpf.h"
> +#include <bpf/bpf_tracing.h>
> +
> +extern bool mptcp_subflow_active(struct mptcp_subflow_context *subflow) __ksym;
> +extern void mptcp_subflow_set_avoid(struct mptcp_subflow_context *subflow,
> +				    bool avoid) __ksym;
> +
> +char _license[] SEC("license") = "GPL";
> +
> +/* set once, on the first scheduling round, never touched again -- this
> + * is what makes "avoid" different from "scheduled": nothing here
> + * re-asserts the decision, and the core doesn't clear it either.
> + */

[Severity: Low]
Is this multi-line comment following the mandatory BPF subsystem modern kernel
comment style?

The BPF subsystem requires multi-line comments to have the opening /* on its
own line.

> +bool avoid_marked;
> +__u32 get_send_calls;
> +
> +SEC("struct_ops")
> +void BPF_PROG(mptcp_sched_avoid_init, struct mptcp_sock *msk)
> +{
> +	avoid_marked = false;
> +	get_send_calls = 0;
> +}

[Severity: Medium]
Are these global .bss variables safe to use for per-socket state?

When client and server sockets run concurrently in the test namespace, the
server's accept() triggers mptcp_sched_avoid_init(). This will zero out the
shared .bss variables (avoid_marked and get_send_calls) while the client might
be concurrently executing bpf_avoid_get_send().

Would it be better to utilize bpf_sk_storage for isolated per-socket state
instead of global variables to prevent intermittent test failures?

> +
> +SEC("struct_ops")
> +void BPF_PROG(mptcp_sched_avoid_release, struct mptcp_sock *msk)
> +{
> +}
> +
> +SEC("struct_ops")
> +int BPF_PROG(bpf_avoid_get_send, struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	get_send_calls++;

[Severity: Medium]
Will this non-atomic increment race with both the server's init reset and any
concurrent bpf_avoid_get_send() calls from other sockets?

> +
> +	if (!avoid_marked) {
> +		int i = 0;
> +
> +		/* bench every subflow but the first, once */
> +		bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
> +			if (i++ > 0)
> +				mptcp_subflow_set_avoid(subflow, true);

[Severity: High]
Can calling mptcp_subflow_set_avoid() safely update the bitfield?

The BPF scheduler runs under the MPTCP socket lock, but the underlying kfunc in
net/mptcp/sched.c executes a non-atomic read-modify-write on the 32-bit
bitfield word in struct mptcp_subflow_context:

net/mptcp/sched.c:mptcp_subflow_set_avoid() {
	subflow->avoid = avoid;
}

Concurrently, the TCP receive path (mptcp_subflow_data_available()) modifies
map_valid (which is in the same bitfield word) under the TCP socket lock.

Could this data race silently corrupt adjacent bitfields and lead to stalled
connections? Would it be possible to extract avoid into a separate bool byte
(like scheduled) to prevent bitfield word aliasing?

> +		}
> +		avoid_marked = true;
> +	}
> +
> +	/* every round -- including this one -- just reads back state set
> +	 * at most once above. no re-assertion, ever.
> +	 */

[Severity: Low]
Should this multi-line comment also be updated to place the opening /* on its
own line, conforming to the BPF subsystem comment style?

> +	bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
> +		if (subflow->avoid)
> +			continue;

[ ... ]

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