Re: [PATCH bpf] bpf, sockmap: Account for receive queue in FIONREAD without a verdict program

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Missing READ_ONCE() when accessing `psock->progs.stream_verdict` and `psock->progs.skb_verdict` concurrently.

Pre-existing issues:
- [High] Missing equivalent FIONREAD fix for UDP sockets in udp_bpf_ioctl().
--

--- Patch [1]: [PATCH bpf] bpf, sockmap: Account for receive queue in FIONREAD without a verdict program ---
commit 9131d0861f04fc0f5fba703ea6f2e057df4b0975
Author: Mattia Meleleo <mattia.meleleo@coralogix.com>

bpf, sockmap: Account for receive queue in FIONREAD without a verdict program

This commit fixes tcp_bpf_ioctl() to account for data in the sk_receive_queue
(via tcp_inq()) when answering SIOCINQ for a psock without a verdict program.
Previously, it only counted bytes diverted to ingress_msg.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..a001b1fff578a 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -344,7 +345,20 @@ static int tcp_bpf_ioctl(struct sock *sk, int cmd, int *karg)
>  		return -EINVAL;
>  
>  	slow = lock_sock_fast(sk);
> -	*karg = sk_psock_msg_inq(sk);
> +	psock = sk_psock_get(sk);
> +	if (unlikely(!psock)) {
> +		unlock_sock_fast(sk, slow);
> +		return tcp_ioctl(sk, cmd, karg);
> +	}
> +	*karg = sk_psock_get_msg_len_nolock(psock);
> +	/* Without a verdict program, ingress data is never diverted to
> +	 * ingress_msg: it stays in sk_receive_queue and is read through
> +	 * the fallback to tcp_recvmsg(), so account for it like
> +	 * tcp_ioctl() does.
> +	 */
> +	if (!psock->progs.stream_verdict && !psock->progs.skb_verdict)

[Severity: Medium]
Are READ_ONCE() wrappers needed when accessing these verdict program pointers?

These pointers can be updated concurrently by other threads via xchg() (for
example, when sk_psock_stop_verdict() is called during map detachment under
sk_callback_lock).

Since tcp_bpf_ioctl() reads these pointers holding only lock_sock_fast(),
reading them without READ_ONCE() might constitute a data race and lead to
compiler optimization issues.

[Severity: High]
This isn't a bug introduced by this patch, but does udp_bpf_ioctl() in
net/ipv4/udp_bpf.c need an equivalent fix for UDP sockets?

Looking at udp_bpf_ioctl():

net/ipv4/udp_bpf.c:udp_bpf_ioctl() {
    ...
    if (cmd == SIOCINQ) {
        slow = lock_sock_fast(sk);
        *karg = sk_msg_first_len(sk);
        unlock_sock_fast(sk, slow);
        return 0;
    }
    ...
}

It unconditionally answers SIOCINQ from sk_msg_first_len() and ignores
the standard socket receive queue where data resides when no verdict
program is attached.

FIONREAD returning 0 for a pending UDP datagram causes applications
to allocate insufficient buffer space. Since UDP datagrams are truncated
if the provided buffer is too small, could this lead to silent data loss?

> +		*karg += tcp_inq(sk);
> +	sk_psock_put(sk, psock);
>  	unlock_sock_fast(sk, slow);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-fionread-no-verdict-v1-1-ce94a72357ec@coralogix.com?part=1