Re: [PATCH mptcp-next v2 2/5] Squash to "bpf: Export mptcp packet scheduler helpers"

[Geliang Tang <geliang@kernel.org>]

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