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

[sashiko-bot@kernel.org]

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?

> +
> +			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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611123538.156005-1-jiayuan.chen@linux.dev?part=5