Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
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 new early returns in vsock_bpf_recvmsg()
(net/vmw_vsock/vsock_bpf.c:125-129) skip the transport and sk_state…
- [Low] The new `if (!len) return 0;` in vsock_bpf_recvmsg()
(net/vmw_vsock/vsock_bpf.c:128-129) returns before the native stream…
--
vsock/bpf: fix zero-length, MSG_OOB and MSG_ERRQUEUE reads in recvmsg
This commit adds checks at the top of vsock_bpf_recvmsg() for three cases.
MSG_ERRQUEUE goes to the native path, MSG_OOB returns -EOPNOTSUPP, and
zero-length reads return 0. Before this, these reads went through
sk_msg_recvmsg() and the sockmap wait, which could spin with the socket
lock held or consume stream bytes.
> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> index 6d96ff03f28c4..58b32fbc1f4b2 100644
> --- a/net/vmw_vsock/vsock_bpf.c
> +++ b/net/vmw_vsock/vsock_bpf.c
> @@ -119,6 +119,15 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct
> msghdr *msg,
> struct vsock_sock *vsk;
> int copied;
>
> + if (unlikely(flags & MSG_ERRQUEUE))
> + return __vsock_recvmsg(sk, msg, len, flags);
> +
> + if (flags & MSG_OOB)
> + return -EOPNOTSUPP;
> +
> + if (!len)
> + return 0;
[Severity: Medium]
Do these two early returns give the right result on a sockmap'd socket that
is no longer established?
The native __vsock_connectible_recvmsg() checks the transport and socket
state before it looks at MSG_OOB or len:
net/vmw_vsock/af_vsock.c:__vsock_connectible_recvmsg() {
...
if (!transport || sk->sk_state != TCP_ESTABLISHED) {
...
if (sock_flag(sk, SOCK_DONE))
err = 0;
else
err = -ENOTCONN;
goto out;
}
if (flags & MSG_OOB) {
err = -EOPNOTSUPP;
goto out;
}
...
if (!len) {
err = 0;
goto out;
}
...
}
sock_map_sk_state_allowed() requires TCP_ESTABLISHED only when a vsock
STREAM or SEQPACKET socket is inserted. After that, the state can change
while the socket stays in the map. One example is a transport reset:
virtio_vsock_event_handle()
vsock_for_each_connected_socket()
virtio_vsock_reset_sock()
sk->sk_state = TCP_CLOSE;
sk->sk_err = ECONNRESET;
Another is a peer close through virtio_transport_do_close(), which sets
SOCK_DONE and moves the socket to TCP_CLOSING.
With this patch, the BPF path then returns:
- recv(fd, buf, n, MSG_OOB) after a transport reset: -EOPNOTSUPP, where
the native path returns -ENOTCONN.
- recv(fd, NULL, 0, 0) after a transport reset: 0, where the native path
returns -ENOTCONN.
- recv(fd, buf, n, MSG_OOB) after a peer close: -EOPNOTSUPP, where the
native path returns 0.
The zero-length case also looks like a regression from the behaviour
before this patch. With empty queues, the old code reached
vsock_msg_wait_data(), which returned -ECONNRESET from sk_err. If SO_ERROR
had already cleared sk_err, it returned -ENOTCONN from the state check:
/* virtio_vsock_reset_sock() sets no shutdown bits, and
* SO_ERROR clears sk_err: check the state the way
* __vsock_connectible_recvmsg() does.
*/
if (sk->sk_state != TCP_ESTABLISHED) {
ret = sock_flag(sk, SOCK_DONE) ? 0 : -ENOTCONN;
The new early return now reports a successful 0 instead of that error.
unix_bpf can use these returns because its native path has no state check
before them. The vsock native path does have one, and
__vsock_connectible_recvmsg() already handles MSG_OOB and !len in the
right order without touching the psock queues.
Would it work to send both cases to __vsock_recvmsg(), the way MSG_ERRQUEUE
is already handled here?
[Severity: Low]
Does the !len early return also drop the SCM_INQ cmsg and msg_inq hint for
zero-length stream reads?
In the native path, the !len case jumps to out:, which still reports the
hint for SOCK_STREAM:
net/vmw_vsock/af_vsock.c:__vsock_connectible_recvmsg() {
...
out:
if ((vsk->recvmsg_inq || msg->msg_get_inq) && err >= 0 &&
sk->sk_type == SOCK_STREAM) {
msg->msg_inq = vsock_stream_inq_hint(sk);
...
do_cmsg = vsk->recvmsg_inq && msg->msg_inq >= 0;
}
...
}
Before this patch, take a zero-length read with SO_INQ set, an empty psock
queue and data in the transport. It matched vsock_has_data() &&
sk_psock_queue_empty() and went to __vsock_recvmsg(), so it got SCM_INQ
and msg_inq. Now it returns 0 with neither.
io_uring sets msg_inq to -1 before most of its recv calls, so there the
hint is missing rather than wrong. Sending !len to __vsock_recvmsg(), as
suggested above, would fix this too.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-kbh3-1-022-fix-v2-v4-0-c7d36703814d%40eldare.de
