Re: [PATCH mptcp-next 6/6] mptcp: defer sk_data_ready to the worker

On 7/30/26 2:57 AM, Geliang Tang wrote:
> On Wed, 2026-07-29 at 10:55 +0200, Paolo Abeni wrote:
>> On 7/27/26 1:29 PM, Geliang Tang wrote:
>>> From: Geliang Tang <[email protected]>
>>>
>>> When MPTCP carries TLS, the data path runs under mptcp_data_lock().
>>> Reaching sk->sk_data_ready(sk) synchronously ends up at
>>> tls_strp_check_rcv() -> mptcp_recv_skb() -> mptcp_move_skbs(),
>>> which
>>> calls mptcp_data_lock() on the same sk and recurses on
>>> sk_lock.slock.
>>
>> Is mptcp_move_skbs() really needed in mptcp_recv_skb()? Anyway it
>> looks 
>> like that is not the only constraint: AFAICS, before the
>> mptcp_recv_skb() 
>> calls, the TLS code would call __mptcp_read_sock() which in turns
>> calls  
>> mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() that requires
>> holding 
>> the msk socket lock in process context, while the mptcp/TLS caller is
>> in 
>> BH scope.
>>
>>> Fix this by deferring sk->sk_data_ready(sk) to mptcp_worker() via a
>>> new MPTCP_WORK_DATA_READY bit, re-using the existing
>>> mptcp_schedule_work()/mptcp_cancel_work() infrastructure. The
>>> wakeup
>>> bit is consumed after the SOCK_DEAD && TCP_CLOSE destroy branch, so
>>> a
>>> socket that reaches the destroy path drops the pending wakeup
>>> rather
>>> than running it post-free.
>>
>> I think that unconditionally adding the work latency for non-TLS
>> application 
>> is a no-go.
>>
>> Instead I *think* that the constraints in __mptcp_read_sock() could
>> be relaxed
>> with something alike the following (completely untested):
> 
> Thank you for your patch. It is very useful, but when running TLS
> tests, it deadlocks with the mptcp_data_lock in mptcp_inq_hint(). 

This looks like an unrelated problem.

AFAICS tls calls tcp_in() under the sk socket lock, and the helper is
implemented accordingly (in fact is separated from tcp_inq_hint()). Even
the mptcp helper must use the same assumption. No need to call
additional locking, nor (AFAICS) ONCE annoation, as `ack_seq` and
`copied_seq` should be modified only under the msk socket lock.

> Currently, the implementation of mptcp_read_done() is as follows:
> 
> static void mptcp_read_done(struct sock *sk, size_t len)
> {
>         struct mptcp_sock *msk = mptcp_sk(sk);
>         struct sk_buff *skb;
>         size_t left;
>         u32 offset;
> 
>         msk_owned_by_me(msk);
> 
>         if (sk->sk_state == TCP_LISTEN)
>                 return;
> 
>         left = len;
>         while (left && (skb = mptcp_recv_skb(sk, &offset)) != NULL) {
>                 int used;
>                 
>                 used = min_t(size_t, skb->len - offset, left);
>                 msk->bytes_consumed += used;
>                 msk->copied_seq += used;
>                 left -= used;
>            
>                 if (skb->len > offset + used)
>                         break;
> 
>                 mptcp_eat_recv_skb(sk, skb);
>         }
>         
>         /* Clean up data we have read: This will do ACK frames. */
>         if (left != len) {
>                 msk->read_copied = len - left;

Possibly both here and in __mptcp_read_sock() should be:

		msk->read_copied += len - left

(note the '+=' operator instead of '=')

/P