[PATCH mptcp-next v3 0/2] mptcp: sched: add subflow avoid flag

[PATCH mptcp-next v3 0/2] mptcp: sched: add subflow avoid flag

[Kalpan Jani]

The core benches a subflow via the "stale" bit after
net.mptcp.stale_loss_cnt retransmission intervals without progress.
That bit is core-owned and RTO-coupled -- cleared as soon as a packet
is acked on the subflow. A scheduler has no way to bench a subflow for
its own reasons (high latency, instability) independently of ack
traffic.

"scheduled" doesn't help either: the core clears it after every
send/retrans pass, so it can't hold a standing "leave this alone"
decision, and nothing outside the scheduler that set it can see it.

Changes since v2:

v2 was generated against a stale local snapshot that predated several
unrelated upstream commits touching the same bitfield region in
mptcp_subflow_context; applying v2 against the real export tree
produced a merge conflict in protocol.h. This series is a straight
rebase onto the current export tree -- no functional change from v2.
Only two things differ as a result of the rebase itself:

- avoid now leaves __unused:8 instead of __unused:7, since the real
  tree has 9 unused bits available at this point rather than 8.
- line offsets throughout protocol.c/protocol.h shifted to match
  current export.

Changes since v1:

- avoid now lives in the existing __unused bitfield padding instead
  of its own bool. No struct growth (Paolo).
- the selftest scheduler sets avoid once, on the first get_send()
  call, and only reads it back afterward -- proving persistence
  scheduled can't do, instead of just claiming it. A get_send_calls
  counter, asserted >1, makes sure multiple rounds actually ran
  (Paolo).
- mptcp_subflow_get_send() and mptcp_subflow_get_retrans() -- the
  default subflow picker, active with or without a custom scheduler
  -- now skip an avoided subflow next to the existing active-subflow
  check. avoid is also exposed via MPTCP diag (Paolo).
- set_avoid() marked __bpf_kfunc: it has no C callers, unlike
  set_scheduled(), and could be dropped under LTO before
  resolve_btfids finds it in BTF (review bot).
- added the missing extern __ksym for mptcp_subflow_active() in the
  selftest, matching mptcp_bpf_burst.c (review bot).
- moved the set_avoid extern out of mptcp_bpf.h into
  mptcp_bpf_avoid.c: checkpatch flags new externs in shared headers,
  and it has only the one caller.

Probing an avoided subflow to decide when to clear it (issue #348) is
left out on purpose -- that's scheduler policy, not core mechanism.

Patch 1 adds the flag, the kfunc, default-scheduler enforcement, and
diag support. Patch 2 reworks the selftest to prove persistence and
picks up the missing extern.

Ran the full suite through mptcp-upstream-virtme-docker (auto-normal
and auto-btf) -- selftests, packetdrill, and BPF tests pass, avoid
subtest included.

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

Kalpan Jani (2):
  mptcp: sched: add subflow avoid flag and enforce it in core
  selftests: mptcp: bpf: exercise the subflow avoid flag across rounds

 include/uapi/linux/mptcp.h                    |  1 +
 net/mptcp/bpf.c                               |  1 +
 net/mptcp/diag.c                              |  2 +
 net/mptcp/protocol.c                          |  4 +-
 net/mptcp/protocol.h                          |  5 +-
 net/mptcp/sched.c                             |  6 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 16 +++++
 .../selftests/bpf/progs/mptcp_bpf_avoid.c     | 70 +++++++++++++++++++
 8 files changed, 102 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_avoid.c

base-commit: 21f4dd6665f74afe8fe39591e4abd5971828cf08
-- 
2.43.0

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

[Kalpan Jani]

The core benches a subflow via the "stale" bit after
net.mptcp.stale_loss_cnt retransmission intervals without progress.
That bit belongs to the core and is tightly coupled to the RTO: it
gets cleared as soon as a packet is acked on the subflow
(mptcp_subflow_active() calls mptcp_subflow_set_active() once
rcv_tstamp moves forward).

"scheduled" doesn't cover this either. The core clears it right after
every send/retrans pass the scheduler set it in, so there's no way to
express "leave this subflow alone across future rounds" with it, and
nothing outside the scheduler that set it ever sees the value.

Add a scheduler-owned "avoid" bit to mptcp_subflow_context:

 - only mptcp_subflow_set_avoid() touches it, never the core. It's
   exposed to BPF struct_ops schedulers as a kfunc with the same
   registration and filter as mptcp_subflow_set_scheduled(), so only
   a scheduler can call it;

 - it sits in the existing __unused:9 padding of the flag bitfield
   rather than as its own bool, so the struct doesn't grow;

 - it lives in the struct_group(reset) block, so it clears on
   subflow reset and a re-established path starts fresh;

 - 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;

 - it's surfaced through MPTCP diag as MPTCP_SUBFLOW_FLAG_AVOID,
   alongside the existing BKUP/FULLY_ESTABLISHED flags.

set_avoid() has no C callers in-tree, unlike set_scheduled() which
the core send/retrans/close paths call directly -- mark it
__bpf_kfunc so it survives LTO/dead-code-elimination before
resolve_btfids looks for it in BTF.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/349
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

Changes since v2:
- rebased onto the current export tree. v2 was generated against a
  stale local snapshot that predated several unrelated commits
  touching the same bitfield region in mptcp_subflow_context; applying
  v2 there produced a merge conflict in protocol.h. No functional
  change from v2 -- only line offsets and the __unused padding count
  differ (was 8 unused bits available in the stale snapshot, is 9 in
  current export, so avoid now leaves __unused:8 instead of
  __unused:7).

Changes since v1:
- moved avoid from a standalone bool into the existing __unused
  bitfield padding: the extra bool pushed lent_mem_frag onto a
  4-byte-aligned boundary, adding a 4-byte hole. The bitfield slot was
  already reserved and unused.
- marked mptcp_subflow_set_avoid() __bpf_kfunc: no C callers, unlike
  set_scheduled(), so it could be dropped under LTO before
  resolve_btfids finds it in BTF.
- switched subflow->avoid access from WRITE_ONCE()/READ_ONCE() to
  plain assignment/read: it's a bitfield member now, and
  {READ,WRITE}_ONCE() require taking its address, which bitfields
  don't allow.
- mptcp_subflow_get_send() and mptcp_subflow_get_retrans() now skip a
  subflow with avoid set, next to the existing active-subflow check:
  previously nothing in the core consulted the flag.
- exposed avoid via MPTCP diag as MPTCP_SUBFLOW_FLAG_AVOID, next to
  the existing BKUP_LOC/FULLY_ESTABLISHED flags.

v2: https://lore.kernel.org/all/20260824102741.347492-2-kalpan.jani@mpiricsoftware.com/
v1: https://lore.kernel.org/all/20260715101148.2601045-2-kalpan.jani@mpiricsoftware.com/

---
 include/uapi/linux/mptcp.h | 1 +
 net/mptcp/bpf.c            | 1 +
 net/mptcp/diag.c           | 2 ++
 net/mptcp/protocol.c       | 4 ++--
 net/mptcp/protocol.h       | 5 ++++-
 net/mptcp/sched.c          | 6 ++++++
 6 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 72a5d030154e..b4ca97e07445 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -22,6 +22,7 @@
 #define MPTCP_SUBFLOW_FLAG_FULLY_ESTABLISHED	_BITUL(6)
 #define MPTCP_SUBFLOW_FLAG_CONNECTED		_BITUL(7)
 #define MPTCP_SUBFLOW_FLAG_MAPVALID		_BITUL(8)
+#define MPTCP_SUBFLOW_FLAG_AVOID		_BITUL(9)
 
 #define MPTCP_PM_CMD_GRP_NAME	"mptcp_pm_cmds"
 #define MPTCP_PM_EV_GRP_NAME	"mptcp_pm_events"
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 82b0ad25f700..3a229e28960d 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -318,6 +318,7 @@ BTF_KFUNCS_START(bpf_mptcp_common_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx, KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock, KF_RET_NULL)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
+BTF_ID_FLAGS(func, mptcp_subflow_set_avoid)
 BTF_ID_FLAGS(func, mptcp_subflow_active)
 BTF_ID_FLAGS(func, mptcp_set_timeout)
 BTF_ID_FLAGS(func, mptcp_wnd_end)
diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c
index 70cf9ebce833..3e98125a9c0c 100644
--- a/net/mptcp/diag.c
+++ b/net/mptcp/diag.c
@@ -49,6 +49,8 @@ static int subflow_get_info(struct sock *sk, struct sk_buff *skb, bool net_admin
 		flags |= MPTCP_SUBFLOW_FLAG_BKUP_LOC;
 	if (READ_ONCE(sf->fully_established))
 		flags |= MPTCP_SUBFLOW_FLAG_FULLY_ESTABLISHED;
+	if (sf->avoid)
+		flags |= MPTCP_SUBFLOW_FLAG_AVOID;
 	if (sf->conn_finished)
 		flags |= MPTCP_SUBFLOW_FLAG_CONNECTED;
 	if (sf->map_valid)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f22d64ab1c53..69ab6583f1df 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1645,7 +1645,7 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 
 		trace_mptcp_subflow_get_send(subflow);
 		ssk =  mptcp_subflow_tcp_sock(subflow);
-		if (!mptcp_subflow_active(subflow))
+		if (!mptcp_subflow_active(subflow) || subflow->avoid)
 			continue;
 
 		tout = max(tout, mptcp_timeout_from_subflow(subflow));
@@ -2563,7 +2563,7 @@ struct sock *mptcp_subflow_get_retrans(struct mptcp_sock *msk)
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 
-		if (!__mptcp_subflow_active(subflow))
+		if (!__mptcp_subflow_active(subflow) || subflow->avoid)
 			continue;
 
 		/* still data outstanding at TCP level? skip this */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3d250e8204d5..7768a464ff4e 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;
 	bool	scheduled;
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
@@ -902,6 +903,8 @@ static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow)
 
 void mptcp_subflow_set_active(struct mptcp_subflow_context *subflow);
 
+void mptcp_subflow_set_avoid(struct mptcp_subflow_context *subflow, bool avoid);
+
 bool mptcp_subflow_active(struct mptcp_subflow_context *subflow);
 
 void mptcp_subflow_drop_ctx(struct sock *ssk);
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 1e59072d478c..8a448814a11d 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;
+}
+
 int mptcp_sched_get_send(struct mptcp_sock *msk)
 {
 	struct mptcp_subflow_context *subflow;
-- 
2.43.0

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

[Kalpan Jani]

Add a BPF struct_ops scheduler selftest for mptcp_subflow_set_avoid(),
and add the required extern declarations for it and for
mptcp_subflow_active(), both declared locally in this file rather
than in the shared mptcp_bpf.h. mptcp_subflow_active()'s extern was
missing here even though the other BPF scheduler progs already have
it (see mptcp_bpf_burst.c).

"scheduled" gets cleared by the core after every single send/retrans
pass, so a scheduler has to set it again every round. This test's
scheduler does the opposite on purpose: it marks a subflow avoided
exactly once, the first time get_send() runs, and never calls
set_avoid() again after that. Every later round skips the marked
subflow purely by reading back state set earlier. A counter of
get_send() invocations, asserted greater than one once the transfer
is done, makes sure the test actually ran more than one round instead
of just assuming it from how much data got sent.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/349
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

Changes since v2:
- rebased onto the current export tree, no functional change from v2.

Changes since v1:
- reworked the scheduler to set avoid once, on the first get_send()
  call, and rely on reading it back on every later call rather than
  re-asserting it: the previous version set and read the flag within
  the same call, which doesn't exercise standing state across
  scheduling rounds. Added a get_send_calls counter, asserted >1, so
  the test enforces multiple rounds actually occurred.
- added the missing extern __ksym declaration for
  mptcp_subflow_active(), matching the existing pattern in
  mptcp_bpf_burst.c.
- declared the mptcp_subflow_set_avoid() extern in this file instead
  of mptcp_bpf.h: checkpatch flags new externs in shared headers, and
  this kfunc has only one caller.

v2: https://lore.kernel.org/all/20260824102741.347492-3-kalpan.jani@mpiricsoftware.com/
v1: https://lore.kernel.org/all/20260715101148.2601045-3-kalpan.jani@mpiricsoftware.com/

---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 16 +++++
 .../selftests/bpf/progs/mptcp_bpf_avoid.c     | 70 +++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_avoid.c

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index d77c9f8c53c7..1c903c121b21 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -18,6 +18,7 @@
 #include "mptcp_bpf_rr.skel.h"
 #include "mptcp_bpf_red.skel.h"
 #include "mptcp_bpf_burst.skel.h"
+#include "mptcp_bpf_avoid.skel.h"
 
 #define NS_TEST "mptcp_ns"
 #define ADDR_1	"10.0.1.1"
@@ -820,6 +821,19 @@ static void test_burst(void)
 	mptcp_bpf_burst__destroy(skel);
 }
 
+static void test_avoid(void)
+{
+	struct mptcp_bpf_avoid *skel;
+
+	skel = mptcp_bpf_avoid__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load: avoid"))
+		return;
+
+	test_bpf_sched(skel->maps.avoid, "avoid", WITH_DATA, WITHOUT_DATA);
+	ASSERT_GT(skel->bss->get_send_calls, 1, "get_send_calls");
+	mptcp_bpf_avoid__destroy(skel);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
@@ -842,4 +856,6 @@ void test_mptcp(void)
 		test_red();
 	if (test__start_subtest("burst"))
 		test_burst();
+	if (test__start_subtest("avoid"))
+		test_avoid();
 }
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 000000000000..8b22c5967a7a
--- /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.
+ */
+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;
+}
+
+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++;
+
+	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);
+		}
+		avoid_marked = true;
+	}
+
+	/* 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;
+}
+
+SEC(".struct_ops.link")
+struct mptcp_sched_ops avoid = {
+	.init		= (void *)mptcp_sched_avoid_init,
+	.release	= (void *)mptcp_sched_avoid_release,
+	.get_send	= (void *)bpf_avoid_get_send,
+	.name		= "bpf_avoid",
+};
-- 
2.43.0

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

[sashiko-bot]

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

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

[sashiko-bot]

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

Re: [PATCH mptcp-next v3 0/2] mptcp: sched: add subflow avoid flag

[MPTCP CI]

Hi Kalpan,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Notice: Call Traces at boot time, rebooted and continued ⚠️ 
- KVM Validation: debug (only selftest_mptcp_join): Notice: Call Traces at boot time, rebooted and continued ⚠️ 
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Notice: Call Traces at boot time, rebooted and continued - Notice: Call Traces at shutdown time, ignored and continued ⚠️ 
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/33381492140

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/95677b25f96d
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1154369


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)