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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Li,

25 Mar 2026 03:39:13 Li Xiasong <lixiasong1@huawei.com>:

> 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.

Cheers,
Matt