Re: [RFC mptcp-next v11 05/15] mptcp: avoid sleeping in read_sock path under softirq

[Geliang Tang <geliang@kernel.org>]

Hi Paolo,

Thanks for you help, I finally solved this deadlock issue as you
suggested.

On Wed, 2026-04-01 at 21:57 +0200, Paolo Abeni wrote:
> On 3/19/26 11:08 AM, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > When mptcp_read_sock() is called from softirq context via TLS
> > read_sock, lock_sock_fast() in mptcp_rcv_space_adjust() and
> > mptcp_cleanup_rbuf() may trigger might_sleep() warnings or
> > illegal sleeps, as softirq context cannot block.
> > 
> > Replace lock_sock_fast() with spin_trylock_bh() to make locking
> > non-blocking and context-safe. Skip operations if the lock cannot
> > be acquired.
> > 
> > Co-developed-by: Gang Yan <yangang@kylinos.cn>
> > Signed-off-by: Gang Yan <yangang@kylinos.cn>
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/protocol.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 5585f43cf879..e9e40dfab5ea 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -561,12 +561,11 @@ static void mptcp_send_ack(struct mptcp_sock
> > *msk)
> >  
> >  static void mptcp_subflow_cleanup_rbuf(struct sock *ssk, int
> > copied)
> >  {
> > -	bool slow;
> > -
> > -	slow = lock_sock_fast(ssk);
> > -	if (tcp_can_send_ack(ssk))
> > +	if (!spin_trylock_bh(&ssk->sk_lock.slock))
> > +		return;
> 
> I discussed this topic off-list with Geliang. Let me report the info
> here, too, for future memory.
> 
> This does not look like the correct solution: we risk not sending out
> the needed ack.
> 
> The exact, complete and decoded call trace leading to the problematic
> lock could be helpful.
> 
> I *guess* a better solution would be detecting the critical scenario
> from the mptcp/tls caller and in such a case queue the strp->work and

That's indeed the problem here.

/* Lower sock lock held */
void tls_strp_data_ready(struct tls_strparser *strp)
{
        struct tls_context *ctx = tls_get_ctx(strp->sk);

        /* This check is needed to synchronize with do_tls_strp_work.
         * do_tls_strp_work acquires a process lock (lock_sock) whereas
         * the lock held here is bh_lock_sock. The two locks can be
         * held by different threads at the same time, but bh_lock_sock
         * allows a thread in BH context to safely check if the process
         * lock is held. In this case, if the lock is held, queue work.
         */
        if (sock_owned_by_user_nocheck(strp->sk)) {
                queue_work(tls_strp_wq, &strp->work);
                return;
        }

        tls_strp_check_rcv(strp);
}

MPTCP should not only checks sock_owned_by_user_nocheck(sk) as TCP
does, but also needs to check whether the MPTCP data lock is held.

So I added a new .lock_is_held interface for struct tls_prot_ops in
v12. Also dropped this patch in v12.

Thanks,
-Geliang

> do
> not call into the tcp/mptcp code. The later strp worker invocation
> will
> happen from process context and could acquire the subflow lock
> without
> locking issues.
> 
> /P