Re: [PATCH mptcp-next v4 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 1 potential issue(s) to consider:
- [Medium] The `test_avoid` selftest contains fundamental logic flaws that cause it to silently pass without ever exercising the `avoid` functionality, resulting in a false positive test.
--

commit 23085ecdc8efb175d47c716d935708a8072a4cd5
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

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

This commit adds a BPF struct_ops scheduler selftest for 
mptcp_subflow_set_avoid(). It attempts to mark subflows as avoided during the
first scheduling round and verifies they are skipped in subsequent rounds.

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..359b515800875
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_avoid.c

[ ... ]

> +SEC("struct_ops")
> +int BPF_PROG(bpf_avoid_get_send, struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_avoid_storage *ptr;
> +
> +	ptr = bpf_sk_storage_get(&mptcp_avoid_map, msk, 0,
> +				 BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	if (!ptr)
> +		return -1;
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (!ptr->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);
> +		}
> +		ptr->avoid_marked = true;
> +	}

[Severity: Medium]
Since bpf_avoid_get_send() is called immediately when the first subflow is 
established (and before the second subflow finishes its MP_JOIN), is `i++ > 0` 
ever true here?

If there is only one subflow when this initialization block runs, `avoid` will 
never be set for any subflow, but `avoid_marked` becomes true, which permanently 
locks out this block.

> +
> +	/*
> +	 * every round -- including this one -- just reads back state set
> +	 * at most once above. no re-assertion, ever.
> +	 */
> +	bpf_for_each(mptcp_subflow, subflow, (struct sock *)msk) {
> +		if (subflow->avoid)
> +			continue;
> +		if (mptcp_subflow_active(subflow)) {
> +			mptcp_subflow_set_scheduled(subflow, true);
> +			break;
> +		}
> +	}
> +	return 0;
> +}

[Severity: Medium]
Because the first subflow is never marked to be avoided, does this loop always 
break on the first active subflow?

Since the first subflow naturally handles the entire data transfer, it is always 
selected, and the second subflow is never evaluated. Does this cause the test 
to artificially pass and provide a false positive result, regardless of whether 
the `avoid` feature actually works?

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