Re: [PATCH bpf v2 5/7] sockmap: Fix use-after-free in udp_bpf_recvmsg()

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Thu Jun 11, 2026 at 8:53 AM EDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Critical] The addition of `lock_sock(sk)` creates a D-state deadlock and an unkillable spin loop if a packet is queued to `psock->ingress_skb` but cannot be converted due to memory limits.
>
> Pre-existing issues:
> - [High] Delegating to `sk_udp_recvmsg` on a blocking socket can cause a permanent hang if BPF programs redirect incoming packets to `psock`.
> --
>
> commit 8b4a0fe8904c1ee85231827ded90d15006435b9c
> Author: Kuniyuki Iwashima <kuniyu@google.com>
>
> sockmap: Fix use-after-free in udp_bpf_recvmsg()
>
> This patch aims to fix a use-after-free in sk_msg_recvmsg reported by syzbot.
> It adds lock_sock serialization around sk_msg_recvmsg in udp_bpf_recvmsg
> to prevent multiple threads from concurrently touching the same sk_msg.
>
> Link: https://lore.kernel.org/netdev/69922ac9.a70a0220.2c38d7.00e0.GAE@google.com/
>
>> diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
>> index 9f33b07b14813..ad57c4c9eaab6 100644
>> --- a/net/ipv4/udp_bpf.c
>> +++ b/net/ipv4/udp_bpf.c
>
> [ ... ]
>
>> @@ -79,6 +81,7 @@ static int udp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>>  		goto out;
>>  	}
>>  
>> +	lock_sock(sk);
>>  msg_bytes_ready:
>>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>>  	if (!copied) {
>> @@ -90,11 +93,17 @@ static int udp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>>  		if (data) {
>>  			if (psock_has_data(psock))
>>  				goto msg_bytes_ready;
>
> [Severity: Critical]
> Does this new locking introduce a potential unkillable spin loop under memory
> pressure?
>
> If sk_receive_queue fills up and hits the sk_rcvbuf limit, and a BPF program
> redirects a packet to psock (placing it in ingress_skb), the backlog work
> might fail to move it to ingress_msg with -EAGAIN due to the memory limits.
>
> When recvmsg is called, it acquires lock_sock here, and sk_msg_recvmsg
> returns 0 since ingress_msg is empty.
>
> Then udp_msg_wait_data checks udp_msg_has_data. Since there is data
> in ingress_skb, it immediately returns 1 without releasing lock_sock:
>
> 	ret = udp_msg_has_data(sk, psock);
> 	if (!ret) {
> 		release_sock(sk);
> 		...
>
> Then back in udp_bpf_recvmsg, we hit the code above. Because
> psock_has_data is true, it jumps back to msg_bytes_ready.
>
> The lock is never released, and we never read from sk_receive_queue, so the
> memory limit is never resolved. Could the syscall spin indefinitely holding
> lock_sock, causing any other threads attempting to operate on the socket
> to hang in D-state?

I think this is slightly confusing in that it implies the system can
fall into a deadlock permanently. In reality the call is only stuck
for as long as memory pressure is preventing the deferred skb -> sk_msg
conversion. The lock itself isn't preventing the deferred work AFAICT, so
once memory pressure goes down the call will progress. The "unkillable loop"
could be avoided by changing some of the udp_msg_has_data/psock_has_data calls
to checks that ignore ingress_skb, but I'm not sure if there would be
significant benefits.

>
>> +
>> +			release_sock(sk);
>> +
>>  			ret = sk_udp_recvmsg(sk, msg, len, flags);
>>  			goto out;
>>  		}
>
> [Severity: High]
> This isn't a bug introduced by this patch, but could delegating to
> sk_udp_recvmsg here result in a permanent hang for blocking sockets?
>
> When psock_has_data is false, this code (and similar code at the start of
> udp_bpf_recvmsg) delegates to sk_udp_recvmsg. For a blocking socket,
> sk_udp_recvmsg will sleep waiting for data exclusively on sk_receive_queue.
>
> If a packet arrives later and a BPF verdict program redirects it to the
> socket's psock (meaning it goes to psock->ingress_skb or ingress_msg
> instead of sk_receive_queue), the socket wait queue is awoken.
>
> However, sk_udp_recvmsg will only check sk_receive_queue, find it empty,
> and immediately go back to sleep.
>
> Will this result in the syscall ignoring valid data successfully queued in
> psock and hanging indefinitely?
>
>>  		copied = -EAGAIN;
>>  	}
>> +
>> +	release_sock(sk);
>> +
>>  	ret = copied;
>>  out:
>>  	sk_psock_put(sk, psock);