Re: [PATCH net-next v2 1/2] vsock: support SO_INQ on stream sockets

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch puts a different meaning into the generic
  SOL_SOCKET/SCM_INQ cmsg than the one already in the tree.

--

vsock: support SO_INQ on stream sockets

This adds SO_INQ support to AF_VSOCK SOCK_STREAM sockets. recvmsg() then
returns an SCM_INQ cmsg filled from vsock_stream_inq_hint(). Accepted
sockets inherit the setting from the listener, and SOCK_SEQPACKET rejects
the option with ENOPROTOOPT.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 1c2d1cea67f4c..186ed57fd7978 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c

[ ... ]

> @@ -2637,13 +2679,26 @@ __vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  		err = __vsock_seqpacket_recvmsg(sk, msg, len, flags);
>  
>  out:
> -	/* Seqpacket has_data counts messages, while io_uring treats msg_inq as
> -	 * a byte length when sizing retries, so only streams report a hint.
> +	/* Seqpacket has_data counts messages, while SCM_INQ reports bytes and
> +	 * io_uring treats msg_inq as a byte length when sizing retries, so
> +	 * only streams report it.
>  	 */
> -	if (msg->msg_get_inq && err >= 0 && sk->sk_type == SOCK_STREAM)
> +	if ((vsk->recvmsg_inq || msg->msg_get_inq) && err >= 0 &&
> +	    sk->sk_type == SOCK_STREAM) {
>  		msg->msg_inq = vsock_stream_inq_hint(sk);
>  
> +		/* A negative hint means the transport could not tell. SCM_INQ
> +		 * has no encoding for that, so report nothing.
> +		 */
> +		do_cmsg = vsk->recvmsg_inq && msg->msg_inq >= 0;
> +	}
> +
>  	release_sock(sk);
> +
> +	if (do_cmsg)
> +		put_cmsg(msg, SOL_SOCKET, SCM_INQ, sizeof(msg->msg_inq),
> +			 &msg->msg_inq);

[Severity: Medium]

Does this give SOL_SOCKET/SCM_INQ a different meaning on vsock than it has
on AF_UNIX?

SO_INQ/SCM_INQ was added in commit df30285b3670 ("af_unix: Introduce
SO_INQ.") as the generic version of TCP_INQ. That commit says the cmsg
holds the exact value of ioctl(SIOCINQ). AF_UNIX follows this:

net/unix/af_unix.c:unix_stream_read_generic() {
    ...
			msg->msg_inq = READ_ONCE(u->inq_len);
			if (do_cmsg)
				put_cmsg(msg, SOL_SOCKET, SCM_INQ,
					 sizeof(msg->msg_inq), &msg->msg_inq);
    ...
}

tools/testing/selftests/net/af_unix/scm_inq.c also checks that the cmsg
payload equals the SIOCINQ result.

Here the value comes from vsock_stream_inq_hint(). In several states it
returns 1 instead of a byte count:

net/vmw_vsock/af_vsock.c:vsock_stream_inq_hint() {
	if ((sk->sk_shutdown & RCV_SHUTDOWN) || !vsk->transport ||
	    (sock_flag(sk, SOCK_DONE) && sk->sk_state != TCP_ESTABLISHED))
		return 1;
    ...
	if (sock_flag(sk, SOCK_DONE) ||
	    (READ_ONCE(vsk->peer_shutdown) & SEND_SHUTDOWN))
		return 1;
    ...
}

The first branch returns 1 even when bytes are still queued. SIOCINQ in
vsock_do_ioctl() still returns the raw vsock_stream_has_data() value,
which is 0 at EOF:

net/vmw_vsock/af_vsock.c:vsock_do_ioctl() {
    ...
		n_bytes = vsock_stream_has_data(vsk);
		if (n_bytes < 0) {
			ret = n_bytes;
			break;
		}
		ret = put_user(n_bytes, arg);
    ...
}

After the peer closes or shuts down, SCM_INQ reports 1 while SIOCINQ
reports 0. After SHUT_RD, SCM_INQ reports 1 no matter how many bytes are
queued.

The commit message says "As with TCP_INQ, 1 is reported once the stream
is finished". TCP_CM_INQ is a separate SOL_TCP cmsg, though. Suppose a
portable program treats SCM_INQ as an exact byte count. Would it see a
byte on vsock that isn't there?

The vsock selftest in the same series ("vsock/test: cover SO_INQ")
expects 1 at EOF. Once this is merged, that per-family meaning becomes
part of the uAPI.

The updated comment above gives "SCM_INQ reports bytes" as the reason to
refuse SEQPACKET, but the stream path does not report bytes at EOF.
Should the comment or the reported value change so they match?

>  	return err;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015154334.3217358-1-devnexen%40gmail.com