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

[John Fastabend <john.fastabend@gmail.com>]

On Wed, Jul 08, 2026 at 05:16:59PM +0000, sashiko-bot@kernel.org wrote:
>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.

This is fine lets ignore this for now. Every system we have adds sockets
to the sockmap on connect so this condition is not seen. And above 
solves common case.

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

Sure but doesn't need to hold up this patch. Mattia would you want to
fix this as well? Otherwise I'll put it on my list and get to it at
some point.

>
>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)?

yes we should do something here.

>
>[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?

yep pre-existing needs another patch.

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