Re: [PATCH net v2] mptcp: fix soft lockup in mptcp_recvmsg()

[Matthieu Baerts]

On 25/03/2026 08:16, Matthieu Baerts wrote:
> Hi Li,
> 
> 25 Mar 2026 03:39:13 Li Xiasong <lixiasong1@xxxxxxxxxx>:
> 
>> Hi Matt,
>>
>> On 3/25/2026 3:23 AM, Matthieu Baerts wrote:
>>> Hi Li,
>>>
>>> On 24/03/2026 09:51, Li Xiasong wrote:
>>>> syzbot reported a soft lockup in mptcp_recvmsg() [0].
>>>>
>>>> When receiving data with MSG_PEEK | MSG_WAITALL flags, the skb is not
>>>> removed from the sk_receive_queue. This causes sk_wait_data() to always
>>>> find available data and never perform actual waiting, leading to a soft
>>>> lockup.
>>>>
>>>> Fix this by adding a 'last' parameter to track the last peeked skb.
>>>> This allows sk_wait_data() to make informed waiting decisions and prevent
>>>> infinite loops when MSG_PEEK is used.
>>>
>>> (...)
>>>
>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>> index cf1852b99963..401fb2b17685 100644
>>>> --- a/net/mptcp/protocol.c
>>>> +++ b/net/mptcp/protocol.c
>>>> @@ -2006,7 +2006,7 @@ static void mptcp_eat_recv_skb(struct sock *sk, struct sk_buff *skb)
>>>> static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
>>>>                 size_t len, int flags, int copied_total,
>>>>                 struct scm_timestamping_internal *tss,
>>>> -               int *cmsg_flags)
>>>> +               int *cmsg_flags, struct sk_buff **last)
>>>> {
>>>>     struct mptcp_sock *msk = mptcp_sk(sk);
>>>>     struct sk_buff *skb, *tmp;
>>>> @@ -2048,6 +2048,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
>>>>         }
>>>>
>>>>         copied += count;
>>>> +       *last = skb;
>>>
>>> My bad, my recommendation to move this assignment here was not a good
>>> idea, because 'skb' can be freed at some points after mptcp_eat_recv_skb
>>> that can be called here below.
>>>
>>> If I'm not mistaken, when MSG_PEEK is not used, sk_wait_data() can
>>> always be called with NULL for the last skb, because the queue is
>>> supposed to be empty. If not, no need to wait for new packets, so NULL
>>> can be used. Is that correct?
>>>
>>> If yes, then I guess 'last' can be initialised to NULL before calling
>>> __mptcp_recvmsg_mskq (limit scope), and only set to 'skb' here below,
>>> when MSG_PEEK is not used (what you had in v1).
>>>
>>> Would that work for you?
>>>
>>> Cheers,
>>> Matt
>>
>> Thanks for the review. I analyzed the concern about the 'last' pointer.
>>
>> When writing the v2 patch, I did consider this case - in non-MSG_PEEK
>> scenario, the skb is freed by mptcp_eat_recv_skb() after setting *last =
>> skb, making 'last' point to freed memory. However, in sk_wait_data(), the
>> 'last' parameter is only used for pointer comparison:
>>
>>     skb_peek_tail(&sk->sk_receive_queue) != skb
>>
>> It only compares the pointer value without dereferencing, so there's no
>> actual UAF issue.
> 
> Indeed, but the skb could be (unlikely) reused at that stage.
> 
> For me, the main point is that mptcp_eat_recv_skb() will remove the
> skb from the queue. Then NULL can be passed instead, safer.
One last thing: as noticed by the AI review, this Fixes tag could be
used instead, as it is more tied to MSG_PEEK:

  Fixes: 8e04ce45a8db ("mptcp: fix MSG_PEEK stream corruption")

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.