Re: [PATCH bpf v3 1/2] 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 3 potential issue(s) to consider:

New issues:
- [High] FIONREAD (SIOCINQ) double-counts available bytes when a verdict program is detached, exposes negative values, and under-reports bytes when a verdict program is active.

Pre-existing issues:
- [High] udp_bpf_ioctl() suffers from the exact same bug that this patch fixes for tcp_bpf_ioctl(). Without a verdict program, data stays in sk_receive_queue and UDP FIONREAD returns 0.
- [High] Data race and sequence number corruption in psock->ingress_bytes during SK_REDIRECT.
--

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

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

This commit modifies tcp_bpf_ioctl() to add the output of tcp_inq() to the
reported FIONREAD value when no stream or skb verdict program is attached.
This correctly accounts for data resting in sk_receive_queue that has not
been diverted to the ingress_msg list.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..8e905b50deadb 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
[ ... ]
> @@ -344,7 +345,21 @@ 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 (!READ_ONCE(psock->progs.stream_verdict) &&
> +	    !READ_ONCE(psock->progs.skb_verdict))
> +		*karg += tcp_inq(sk);

[Severity: High]
Does this conditional logic double-count available bytes or under-report them
in some situations?

If a stream verdict program is attached, data is queued (which delays
tp->copied_seq), and then the program is detached. The unread bytes sit in
both msg_tot_len (ingress_msg) and are also reflected in tcp_inq(sk) because
copied_seq was held back. Adding them together here would double-count the
available data.

Conversely, when a verdict program is active, tcp_inq(sk) is ignored. Could
this fail to account for unparsed data in sk_receive_queue, leading to an
under-reported value?

> +	sk_psock_put(sk, psock);
>  	unlock_sock_fast(sk, slow);
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but udp_bpf_ioctl() in
net/ipv4/udp_bpf.c appears to suffer from the exact same bug that this patch
fixes for tcp_bpf_ioctl().

net/ipv4/udp_bpf.c:udp_bpf_ioctl() {
    ...
    *karg = sk_msg_first_len(sk);
    ...
}

Without a verdict program, data stays in sk_receive_queue, and UDP FIONREAD
constantly returns 0. Could a similar fallback to the native receive queue be
applied there to prevent user-space applications from hanging or dropping
packets when relying on ioctl(FIONREAD)?

[Severity: High]
This is a pre-existing issue, but is there a data race and sequence number
corruption in psock->ingress_bytes during SK_REDIRECT?

In sk_psock_skb_ingress_enqueue(), psock->ingress_bytes is incremented without
locks:

net/core/skmsg.c:sk_psock_skb_ingress_enqueue() {
    ...
    psock->ingress_bytes += len;
    ...
}

Concurrently, tcp_bpf_strp_read_sock() locklessly resets psock->ingress_bytes
to 0 and uses it to adjust tp->copied_seq:

net/ipv4/tcp_bpf.c:tcp_bpf_strp_read_sock() {
    ...
    psock->ingress_bytes = 0;
    ...
    tp->copied_seq = psock->copied_seq - psock->ingress_bytes;
    ...
}

If another socket redirects data to this socket, the target socket's
psock->ingress_bytes is incremented by the workqueue or softirq. Does the race
with tcp_bpf_strp_read_sock() cause redirected bytes to be incorrectly
subtracted from the target socket's tp->copied_seq, artificially holding back
the ACK sequence and corrupting the TCP state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-fionread-no-verdict-v3-0-b4ee31b3af53@coralogix.com?part=1