mptcp_pm_subflow_chk_stale() is exposed to BPF MPTCP packet schedulers as
a kfunc taking a generic "struct sock *ssk", but it treats ssk as a
subflow TCP socket: it derives the subflow context with
mptcp_subflow_ctx(), an unchecked cast of inet_csk(ssk)->icsk_ulp_data,
then reads and writes through it. The verifier only proves ssk is a
trusted struct sock, not that it is one of msk's subflows, so a mistyped
or foreign socket would make the helper operate on a bogus context.
Register a bpf_mptcp_pm_subflow_chk_stale() wrapper that validates ssk is
a full MPTCP subflow TCP socket belonging to the passed msk before calling
the helper, which assumes both but checks neither. This mirrors
bpf_mptcp_subflow_ctx(). A scheduler passing one of its own subflows is
unaffected; the in-tree burst scheduler selftest is updated to the wrapper
name in a separate squash-to.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
net/mptcp/bpf.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 9355fb53e89d5..e2d2c3394ad71 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -306,6 +306,18 @@ __bpf_kfunc static void bpf_mptcp_set_timeout(struct mptcp_sock *msk)
mptcp_set_timeout((struct sock *)msk);
}
+__bpf_kfunc static void
+bpf_mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
+{
+ if (ssk && sk_fullsock(ssk) && ssk->sk_type == SOCK_STREAM &&
+ ssk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(ssk)) {
+ struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+
+ if (subflow && subflow->conn == (const struct sock *)msk)
+ mptcp_pm_subflow_chk_stale(msk, ssk);
+ }
+}
+
__bpf_kfunc_end_defs();
BTF_KFUNCS_START(bpf_mptcp_iter_kfunc_ids)
@@ -327,7 +339,7 @@ BTF_ID_FLAGS(func, mptcp_subflow_active)
BTF_ID_FLAGS(func, bpf_mptcp_set_timeout)
BTF_ID_FLAGS(func, mptcp_wnd_end)
BTF_ID_FLAGS(func, bpf_sk_stream_memory_free)
-BTF_ID_FLAGS(func, mptcp_pm_subflow_chk_stale, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_mptcp_pm_subflow_chk_stale, KF_SLEEPABLE)
BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
static int bpf_mptcp_common_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
--
2.34.1Hi Shardul, On 03/07/2026 18:57, Shardul Bankar wrote: > mptcp_pm_subflow_chk_stale() is exposed to BPF MPTCP packet schedulers as > a kfunc taking a generic "struct sock *ssk", but it treats ssk as a > subflow TCP socket: it derives the subflow context with > mptcp_subflow_ctx(), an unchecked cast of inet_csk(ssk)->icsk_ulp_data, > then reads and writes through it. The verifier only proves ssk is a > trusted struct sock, not that it is one of msk's subflows, so a mistyped > or foreign socket would make the helper operate on a bogus context. > > Register a bpf_mptcp_pm_subflow_chk_stale() wrapper that validates ssk is > a full MPTCP subflow TCP socket belonging to the passed msk before calling > the helper, which assumes both but checks neither. This mirrors > bpf_mptcp_subflow_ctx(). A scheduler passing one of its own subflows is > unaffected; the in-tree burst scheduler selftest is updated to the wrapper > name in a separate squash-to. Thank you for this! FYI, this modification should no longer be needed after: https://lore.kernel.org/0a81a4bf60008faefdd1688012b8fcebc291a4d6.1786026372.git.pabeni@redhat.com (Being applied now) Cheers, Matt -- Sponsored by the NGI0 Core fund.
Hi Shardul,
Thanks for this v2. It's much better now.
On Fri, 2026-07-03 at 22:27 +0530, Shardul Bankar wrote:
> mptcp_pm_subflow_chk_stale() is exposed to BPF MPTCP packet
> schedulers as
> a kfunc taking a generic "struct sock *ssk", but it treats ssk as a
> subflow TCP socket: it derives the subflow context with
> mptcp_subflow_ctx(), an unchecked cast of inet_csk(ssk)-
> >icsk_ulp_data,
> then reads and writes through it. The verifier only proves ssk is a
> trusted struct sock, not that it is one of msk's subflows, so a
> mistyped
> or foreign socket would make the helper operate on a bogus context.
>
> Register a bpf_mptcp_pm_subflow_chk_stale() wrapper that validates
> ssk is
> a full MPTCP subflow TCP socket belonging to the passed msk before
> calling
> the helper, which assumes both but checks neither. This mirrors
> bpf_mptcp_subflow_ctx(). A scheduler passing one of its own subflows
> is
> unaffected; the in-tree burst scheduler selftest is updated to the
> wrapper
> name in a separate squash-to.
>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
> net/mptcp/bpf.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 9355fb53e89d5..e2d2c3394ad71 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -306,6 +306,18 @@ __bpf_kfunc static void
> bpf_mptcp_set_timeout(struct mptcp_sock *msk)
> mptcp_set_timeout((struct sock *)msk);
> }
>
> +__bpf_kfunc static void
> +bpf_mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct
> sock *ssk)
> +{
> + if (ssk && sk_fullsock(ssk) && ssk->sk_type == SOCK_STREAM
> &&
> + ssk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(ssk)) {
> + struct mptcp_subflow_context *subflow =
> mptcp_subflow_ctx(ssk);
> +
> + if (subflow && subflow->conn == (const struct sock
> *)msk)
Sashiko complained that the rcu lock is not held here [1]. It seems we
can add the lock, something like:
rcu_read_lock();
subflow = mptcp_subflow_ctx(ssk);
if (subflow && subflow->conn == (const struct sock *)msk)
mptcp_pm_subflow_chk_stale(msk, ssk);
rcu_read_unlock();
WDYT?
Thanks,
-Geliang
[1]
https://sashiko.dev/#/patchset/20260703-mptcp_bpf_kfunc_fixes-v2-0-87ae3c64dc7e@mpiricsoftware.com?part=2
> + mptcp_pm_subflow_chk_stale(msk, ssk);
> + }
> +}
> +
> __bpf_kfunc_end_defs();
>
> BTF_KFUNCS_START(bpf_mptcp_iter_kfunc_ids)
> @@ -327,7 +339,7 @@ BTF_ID_FLAGS(func, mptcp_subflow_active)
> BTF_ID_FLAGS(func, bpf_mptcp_set_timeout)
> BTF_ID_FLAGS(func, mptcp_wnd_end)
> BTF_ID_FLAGS(func, bpf_sk_stream_memory_free)
> -BTF_ID_FLAGS(func, mptcp_pm_subflow_chk_stale, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_mptcp_pm_subflow_chk_stale, KF_SLEEPABLE)
> BTF_KFUNCS_END(bpf_mptcp_common_kfunc_ids)
>
> static int bpf_mptcp_common_kfunc_filter(const struct bpf_prog
> *prog, u32 kfunc_id)
Hi Geliang, Thanks for taking a look. On Mon, 2026-07-06 at 14:34 +0800, Geliang Tang wrote: > Hi Shardul, > > Thanks for this v2. It's much better now. > > On Fri, 2026-07-03 at 22:27 +0530, Shardul Bankar wrote: > > ... > > ... > > Sashiko complained that the rcu lock is not held here [1]. It seems > we > can add the lock, something like: > > rcu_read_lock(); > subflow = mptcp_subflow_ctx(ssk); > if (subflow && subflow->conn == (const struct sock *)msk) > mptcp_pm_subflow_chk_stale(msk, ssk); > rcu_read_unlock(); > > > WDYT? > > Thanks, > -Geliang > > [1] > https://sashiko.dev/#/patchset/20260703-mptcp_bpf_kfunc_fixes-v2-0-87ae3c64dc7e@mpiricsoftware.com?part=2 > > I don't think we need the rcu_read_lock() here, and I think it would actually be unsafe. The subflow context is freed with kfree_rcu(), but in this path the msk socket lock prevents that free, so rcu_read_lock() is not needed. get_retrans() runs with the msk lock held (msk_owned_by_me() in mptcp_sched_get_retrans()). A subflow is removed from msk->conn_list by __mptcp_close_ssk() under that same lock, and only then can it be torn down and its context freed, so it cannot be freed while get_retrans() is running. The in-kernel mptcp_subflow_get_retrans() relies on exactly this: it dereferences the subflow and calls mptcp_pm_subflow_chk_stale() under the msk lock, without rcu_read_lock(). rcu_read_lock() would also not be the right call here: mptcp_pm_subflow_chk_stale() is not a pure reader (it updates subflow- >stale_count and can trigger a retransmit), so it needs the msk lock for serialization, which rcu_read_lock() would not provide. It is also KF_SLEEPABLE and can sleep (its stale path takes lock_sock_fast() and calls __mptcp_push_pending()), so it cannot run inside an RCU read-side critical section anyway. So I'd prefer to keep the code as is. If it helps, I can add a short comment noting that the msk lock keeps the subflow context alive. Does this address the concern? Thanks, Shardul
Hi Shardul, On Mon, 2026-07-06 at 16:33 +0530, Shardul Bankar wrote: > Hi Geliang, > > Thanks for taking a look. > > On Mon, 2026-07-06 at 14:34 +0800, Geliang Tang wrote: > > Hi Shardul, > > > > Thanks for this v2. It's much better now. > > > > On Fri, 2026-07-03 at 22:27 +0530, Shardul Bankar wrote: > > > ... > > > ... > > > > Sashiko complained that the rcu lock is not held here [1]. It seems > > we > > can add the lock, something like: > > > > rcu_read_lock(); > > subflow = mptcp_subflow_ctx(ssk); > > if (subflow && subflow->conn == (const struct sock *)msk) > > mptcp_pm_subflow_chk_stale(msk, ssk); > > rcu_read_unlock(); > > > > > > WDYT? > > > > Thanks, > > -Geliang > > > > [1] > > https://sashiko.dev/#/patchset/20260703-mptcp_bpf_kfunc_fixes-v2-0-87ae3c64dc7e@mpiricsoftware.com?part=2 > > > > > > I don't think we need the rcu_read_lock() here, and I think it would > actually be unsafe. > > The subflow context is freed with kfree_rcu(), but in this path the > msk > socket lock prevents that free, so rcu_read_lock() is not needed. > get_retrans() runs with the msk lock held (msk_owned_by_me() in > mptcp_sched_get_retrans()). A subflow is removed from msk->conn_list > by > __mptcp_close_ssk() under that same lock, and only then can it be > torn > down and its context freed, so it cannot be freed while get_retrans() > is running. The in-kernel mptcp_subflow_get_retrans() relies on > exactly > this: it dereferences the subflow and calls > mptcp_pm_subflow_chk_stale() under the msk lock, without > rcu_read_lock(). If that is the case, would it be reasonable to add a msk_owned_by_me(msk) check inside bpf_mptcp_pm_subflow_chk_stale()? Thanks, -Geliang > > rcu_read_lock() would also not be the right call here: > mptcp_pm_subflow_chk_stale() is not a pure reader (it updates > subflow- > > stale_count and can trigger a retransmit), so it needs the msk lock > for serialization, which rcu_read_lock() would not provide. It is > also > KF_SLEEPABLE and can sleep (its stale path takes lock_sock_fast() and > calls __mptcp_push_pending()), so it cannot run inside an RCU > read-side critical section anyway. > > So I'd prefer to keep the code as is. If it helps, I can add a short > comment noting that the msk lock keeps the subflow context alive. > Does > this address the concern? > > Thanks, > Shardul
© 2016 - 2026 Red Hat, Inc.