* [PATCH v2] vsock: keep SOCK_SEQPACKET message boundaries on interrupted send
@ 2026-09-19 12:29 Bartłomiej Dmitruk
2026-09-20 9:08 ` David Laight
2026-09-23 12:46 ` netdev-bot+sashiko
0 siblings, 2 replies; 3+ messages in thread
From: Bartłomiej Dmitruk @ 2026-09-19 12:29 UTC (permalink / raw)
To: Stefan Hajnoczi, Stefano Garzarella, Michael S . Tsirkin,
Jason Wang, Eugenio Pérez
Cc: Xuan Zhuo, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, kvm, virtualization, netdev,
linux-kernel
A credit-limited SOCK_SEQPACKET send transmits fragments as credit becomes
available, and the VIRTIO_VSOCK_SEQ_EOM flag is set only on the fragment
where msg_data_left() reaches 0. If vsock_connectible_sendmsg() exits via
out_err after a partial send -- notably the non-terminal -EINTR path
(signal_pending while blocked for credit), but also sk_err / peer
RCV_SHUTDOWN -- the already-transmitted fragments carry no EOM. The receiver
only advances msg_count / sets msg_ready on an EOM skb, so the orphaned
fragments are silently merged into the next message, violating SOCK_SEQPACKET
atomicity.
Wait until the whole remaining SEQPACKET message fits before enqueuing, so a
message is committed atomically (with its EOM) or not started; an error while
waiting then leaves nothing on the wire. SOCK_STREAM behaviour is unchanged
(min_space == 1 reproduces the old "wait while space == 0").
To avoid blocking forever on a message that can never fit -- a peer can
advertise a small buf_alloc, or the message can simply exceed the transmit
buffer -- reject such a message up front with -EMSGSIZE (the same error the
transport already returns for an oversized message) instead of waiting. This
needs the transport's maximum message size, added as a new optional
seqpacket_max_size() transport op implemented by the virtio/loopback
transports.
Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
Assisted-by: Claude (Anthropic)
---
Changes in v2:
- Fix an indefinite wait the v1 approach introduced for oversized
SOCK_SEQPACKET messages (reported by the Sashiko AI review on v1): the
atomic-wait could never be satisfied when len > the transport's max
message size (e.g. a peer advertising a small peer_buf_alloc), so the
sender blocked instead of returning -EMSGSIZE. v2 checks this up front via
the new seqpacket_max_size() op and returns -EMSGSIZE without waiting.
- v1: https://lore.kernel.org/netdev/20260917220101.55744-1-bartlomiej.dmitruk@isec.pl/
Testing (vsock_loopback, unprivileged):
- Interrupted partial send (-EINTR) no longer leaks orphan bytes into the
next message (recv() returns only the next message).
- Oversized message and small-peer_buf_alloc cases return -EMSGSIZE promptly
instead of blocking.
- Normal SEQPACKET send/recv unaffected.
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index f91704731..e96790c03 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -219,6 +219,7 @@ virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
s64 virtio_transport_stream_has_data(struct vsock_sock *vsk);
s64 virtio_transport_stream_has_space(struct vsock_sock *vsk);
u32 virtio_transport_seqpacket_has_data(struct vsock_sock *vsk);
+u32 virtio_transport_seqpacket_max_size(struct vsock_sock *vsk);
ssize_t virtio_transport_unsent_bytes(struct vsock_sock *vsk);
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 5549298c1..d94c613ef 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -143,6 +143,7 @@ struct vsock_transport {
size_t len);
bool (*seqpacket_allow)(struct vsock_sock *vsk, u32 remote_cid);
u32 (*seqpacket_has_data)(struct vsock_sock *vsk);
+ u32 (*seqpacket_max_size)(struct vsock_sock *vsk);
/* Notification. */
int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index f840498b5..ac64cb957 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2250,9 +2250,32 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
while (total_written < len) {
ssize_t written;
+ s64 min_space;
+
+ if (sk->sk_type == SOCK_SEQPACKET) {
+ /* A SEQPACKET message must be delivered atomically, so
+ * wait until the whole remaining message fits before
+ * enqueuing. Otherwise a credit-limited partial send that
+ * later errors out (e.g. -EINTR) leaves EOM-less fragments
+ * that the peer merges into the next message.
+ *
+ * Reject a message that can never fit up front so the wait
+ * below cannot block forever (a peer may advertise a small
+ * buf_alloc); this mirrors the -EMSGSIZE the transport
+ * returns for an oversized message.
+ */
+ if (transport->seqpacket_max_size &&
+ len > transport->seqpacket_max_size(vsk)) {
+ err = -EMSGSIZE;
+ goto out_err;
+ }
+ min_space = len - total_written;
+ } else {
+ min_space = 1;
+ }
add_wait_queue(sk_sleep(sk), &wait);
- while (vsock_stream_has_space(vsk) == 0 &&
+ while (vsock_stream_has_space(vsk) < min_space &&
sk->sk_err == 0 &&
!(sk->sk_shutdown & SEND_SHUTDOWN) &&
!(vsk->peer_shutdown & RCV_SHUTDOWN)) {
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 4f9aa9c4c..b7d587a20 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -585,6 +585,7 @@ static struct virtio_transport virtio_transport = {
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = virtio_transport_seqpacket_allow,
.seqpacket_has_data = virtio_transport_seqpacket_has_data,
+ .seqpacket_max_size = virtio_transport_seqpacket_max_size,
.msgzerocopy_allow = virtio_transport_msgzerocopy_allow,
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index f225f53ed..e10e7b958 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -994,6 +994,19 @@ virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
}
EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_enqueue);
+u32 virtio_transport_seqpacket_max_size(struct vsock_sock *vsk)
+{
+ struct virtio_vsock_sock *vvs = vsk->trans;
+ u32 max_size;
+
+ spin_lock_bh(&vvs->tx_lock);
+ max_size = virtio_transport_tx_buf_size(vvs);
+ spin_unlock_bh(&vvs->tx_lock);
+
+ return max_size;
+}
+EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_max_size);
+
int
virtio_transport_dgram_dequeue(struct vsock_sock *vsk,
struct msghdr *msg,
diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
index 8068d1b6e..b9a0cc861 100644
--- a/net/vmw_vsock/vsock_loopback.c
+++ b/net/vmw_vsock/vsock_loopback.c
@@ -90,6 +90,7 @@ static struct virtio_transport loopback_transport = {
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = vsock_loopback_seqpacket_allow,
.seqpacket_has_data = virtio_transport_seqpacket_has_data,
+ .seqpacket_max_size = virtio_transport_seqpacket_max_size,
.msgzerocopy_allow = vsock_loopback_msgzerocopy_allow,
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] vsock: keep SOCK_SEQPACKET message boundaries on interrupted send
2026-09-19 12:29 [PATCH v2] vsock: keep SOCK_SEQPACKET message boundaries on interrupted send Bartłomiej Dmitruk
@ 2026-09-20 9:08 ` David Laight
2026-09-23 12:46 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: David Laight @ 2026-09-20 9:08 UTC (permalink / raw)
To: Bartłomiej Dmitruk
Cc: Stefan Hajnoczi, Stefano Garzarella, Michael S . Tsirkin,
Jason Wang, Eugenio Pérez, Xuan Zhuo, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, kvm,
virtualization, netdev, linux-kernel
On Sat, 19 Sep 2026 14:29:15 +0200
Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl> wrote:
> A credit-limited SOCK_SEQPACKET send transmits fragments as credit becomes
> available, and the VIRTIO_VSOCK_SEQ_EOM flag is set only on the fragment
> where msg_data_left() reaches 0. If vsock_connectible_sendmsg() exits via
> out_err after a partial send -- notably the non-terminal -EINTR path
> (signal_pending while blocked for credit), but also sk_err / peer
> RCV_SHUTDOWN -- the already-transmitted fragments carry no EOM. The receiver
> only advances msg_count / sets msg_ready on an EOM skb, so the orphaned
> fragments are silently merged into the next message, violating SOCK_SEQPACKET
> atomicity.
Should the unsent fragments just get requeued with an EOM flag?
The receiving system will have a partial message (and should be allowed to
pass that to an application) and will append the fragments to it when
they arrive.
>
> Wait until the whole remaining SEQPACKET message fits before enqueuing, so a
> message is committed atomically (with its EOM) or not started; an error while
> waiting then leaves nothing on the wire. SOCK_STREAM behaviour is unchanged
> (min_space == 1 reproduces the old "wait while space == 0").
Why should the message ever fit?
ISO transport (which no one really uses any more) maps to SEQPACKET.
It is perfectly valid for a file transfer program to send the entire file
as one 'message' by not sending EOM until the end of file fragment.
David
>
> To avoid blocking forever on a message that can never fit -- a peer can
> advertise a small buf_alloc, or the message can simply exceed the transmit
> buffer -- reject such a message up front with -EMSGSIZE (the same error the
> transport already returns for an oversized message) instead of waiting. This
> needs the transport's maximum message size, added as a new optional
> seqpacket_max_size() transport op implemented by the virtio/loopback
> transports.
>
> Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
> Assisted-by: Claude (Anthropic)
> ---
> Changes in v2:
> - Fix an indefinite wait the v1 approach introduced for oversized
> SOCK_SEQPACKET messages (reported by the Sashiko AI review on v1): the
> atomic-wait could never be satisfied when len > the transport's max
> message size (e.g. a peer advertising a small peer_buf_alloc), so the
> sender blocked instead of returning -EMSGSIZE. v2 checks this up front via
> the new seqpacket_max_size() op and returns -EMSGSIZE without waiting.
> - v1: https://lore.kernel.org/netdev/20260917220101.55744-1-bartlomiej.dmitruk@isec.pl/
>
> Testing (vsock_loopback, unprivileged):
> - Interrupted partial send (-EINTR) no longer leaks orphan bytes into the
> next message (recv() returns only the next message).
> - Oversized message and small-peer_buf_alloc cases return -EMSGSIZE promptly
> instead of blocking.
> - Normal SEQPACKET send/recv unaffected.
>
> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> index f91704731..e96790c03 100644
> --- a/include/linux/virtio_vsock.h
> +++ b/include/linux/virtio_vsock.h
> @@ -219,6 +219,7 @@ virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
> s64 virtio_transport_stream_has_data(struct vsock_sock *vsk);
> s64 virtio_transport_stream_has_space(struct vsock_sock *vsk);
> u32 virtio_transport_seqpacket_has_data(struct vsock_sock *vsk);
> +u32 virtio_transport_seqpacket_max_size(struct vsock_sock *vsk);
>
> ssize_t virtio_transport_unsent_bytes(struct vsock_sock *vsk);
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index 5549298c1..d94c613ef 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -143,6 +143,7 @@ struct vsock_transport {
> size_t len);
> bool (*seqpacket_allow)(struct vsock_sock *vsk, u32 remote_cid);
> u32 (*seqpacket_has_data)(struct vsock_sock *vsk);
> + u32 (*seqpacket_max_size)(struct vsock_sock *vsk);
>
> /* Notification. */
> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index f840498b5..ac64cb957 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -2250,9 +2250,32 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
>
> while (total_written < len) {
> ssize_t written;
> + s64 min_space;
> +
> + if (sk->sk_type == SOCK_SEQPACKET) {
> + /* A SEQPACKET message must be delivered atomically, so
> + * wait until the whole remaining message fits before
> + * enqueuing. Otherwise a credit-limited partial send that
> + * later errors out (e.g. -EINTR) leaves EOM-less fragments
> + * that the peer merges into the next message.
> + *
> + * Reject a message that can never fit up front so the wait
> + * below cannot block forever (a peer may advertise a small
> + * buf_alloc); this mirrors the -EMSGSIZE the transport
> + * returns for an oversized message.
> + */
> + if (transport->seqpacket_max_size &&
> + len > transport->seqpacket_max_size(vsk)) {
> + err = -EMSGSIZE;
> + goto out_err;
> + }
> + min_space = len - total_written;
> + } else {
> + min_space = 1;
> + }
>
> add_wait_queue(sk_sleep(sk), &wait);
> - while (vsock_stream_has_space(vsk) == 0 &&
> + while (vsock_stream_has_space(vsk) < min_space &&
> sk->sk_err == 0 &&
> !(sk->sk_shutdown & SEND_SHUTDOWN) &&
> !(vsk->peer_shutdown & RCV_SHUTDOWN)) {
> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 4f9aa9c4c..b7d587a20 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -585,6 +585,7 @@ static struct virtio_transport virtio_transport = {
> .seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
> .seqpacket_allow = virtio_transport_seqpacket_allow,
> .seqpacket_has_data = virtio_transport_seqpacket_has_data,
> + .seqpacket_max_size = virtio_transport_seqpacket_max_size,
>
> .msgzerocopy_allow = virtio_transport_msgzerocopy_allow,
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index f225f53ed..e10e7b958 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -994,6 +994,19 @@ virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
> }
> EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_enqueue);
>
> +u32 virtio_transport_seqpacket_max_size(struct vsock_sock *vsk)
> +{
> + struct virtio_vsock_sock *vvs = vsk->trans;
> + u32 max_size;
> +
> + spin_lock_bh(&vvs->tx_lock);
> + max_size = virtio_transport_tx_buf_size(vvs);
> + spin_unlock_bh(&vvs->tx_lock);
> +
> + return max_size;
> +}
> +EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_max_size);
> +
> int
> virtio_transport_dgram_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
> diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
> index 8068d1b6e..b9a0cc861 100644
> --- a/net/vmw_vsock/vsock_loopback.c
> +++ b/net/vmw_vsock/vsock_loopback.c
> @@ -90,6 +90,7 @@ static struct virtio_transport loopback_transport = {
> .seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
> .seqpacket_allow = vsock_loopback_seqpacket_allow,
> .seqpacket_has_data = virtio_transport_seqpacket_has_data,
> + .seqpacket_max_size = virtio_transport_seqpacket_max_size,
>
> .msgzerocopy_allow = vsock_loopback_msgzerocopy_allow,
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] vsock: keep SOCK_SEQPACKET message boundaries on interrupted send
2026-09-19 12:29 [PATCH v2] vsock: keep SOCK_SEQPACKET message boundaries on interrupted send Bartłomiej Dmitruk
2026-09-20 9:08 ` David Laight
@ 2026-09-23 12:46 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-09-23 12:46 UTC (permalink / raw)
To: bartlomiej.dmitruk
Cc: stefanha, sgarzare, mst, jasowangio, eperezma, xuanzhuo, davem,
edumazet, kuba, pabeni, horms, kvm, virtualization, netdev,
linux-kernel
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 3 · Medium: 2 · 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).
- [High] vhost-vsock is left with an unsatisfiable atomic wait: the new
whole-message wait (`min_space = len - total_written`) in…
- [High] Waiting for whole-message credit does not make the SEQPACKET
enqueue atomic, so EOM-less orphan fragments — the exact corruption…
- [High] The `len > transport->seqpacket_max_size(vsk)` feasibility test
is evaluated once at the top of the outer `while (total_written <…
- [Medium] The commit message claims "SOCK_STREAM behaviour is unchanged
(min_space == 1 reproduces the old 'wait while space == 0')", but that…
- [Medium] Raising the wake-up threshold from 1 byte to the full message
length makes SEQPACKET send liveness depend on the peer proactively…
- [Low] The patch fixes a user-visible SOCK_SEQPACKET framing bug
(interrupted credit-limited sends leaving EOM-less fragments that the…
--
vsock: keep SOCK_SEQPACKET message boundaries on interrupted send
The patch makes vsock_connectible_sendmsg() wait until a whole remaining
SOCK_SEQPACKET message fits in the transmit window before enqueuing it, so
an error during the wait leaves no EOM-less fragments behind. A new
optional seqpacket_max_size() transport op, implemented by the virtio and
loopback transports, is used to reject up front a message that could never
fit, with -EMSGSIZE.
A few questions below.
[Severity: Low]
This fixes a user-visible SEQPACKET framing problem but carries no Fixes:
tag and no Cc: stable. Should it point at the commit that added SEQPACKET
handling to the generic send loop ("af_vsock: implement send logic for
SEQPACKET"), so backport tooling can pick the affected stable trees?
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 9b71479a2b29..01fc9ac6fa2b 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -2250,9 +2250,32 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
>
> while (total_written < len) {
> ssize_t written;
> + s64 min_space;
> +
> + if (sk->sk_type == SOCK_SEQPACKET) {
> + /* A SEQPACKET message must be delivered atomically, so
> + * wait until the whole remaining message fits before
> + * enqueuing. Otherwise a credit-limited partial send that
> + * later errors out (e.g. -EINTR) leaves EOM-less fragments
> + * that the peer merges into the next message.
> + *
> + * Reject a message that can never fit up front so the wait
> + * below cannot block forever (a peer may advertise a small
> + * buf_alloc); this mirrors the -EMSGSIZE the transport
> + * returns for an oversized message.
> + */
> + if (transport->seqpacket_max_size &&
> + len > transport->seqpacket_max_size(vsk)) {
> + err = -EMSGSIZE;
> + goto out_err;
> + }
[Severity: High]
The new wait threshold below applies to every SOCK_SEQPACKET socket, but
this -EMSGSIZE escape is gated on the new optional op. vhost_transport in
drivers/vhost/vsock.c does not register it:
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = vhost_transport_seqpacket_allow,
.seqpacket_has_data = virtio_transport_seqpacket_has_data,
so transport->seqpacket_max_size is NULL there and this branch is skipped,
while min_space still becomes len - total_written.
What happens on a host-side SEQPACKET sendmsg() whose len exceeds the
window? vsock_stream_has_space() -> virtio_transport_stream_has_space() ->
virtio_transport_has_space() is capped by virtio_transport_tx_buf_size() =
min(peer_buf_alloc, buf_alloc):
net/vmw_vsock/virtio_transport_common.c:virtio_transport_has_space() {
bytes = (s64)virtio_transport_tx_buf_size(vvs) -
(vvs->tx_cnt - vvs->peer_fwd_cnt);
...
}
Can the loop condition vsock_stream_has_space(vsk) < min_space ever become
false in that case? A blocking sender would sleep in wait_woken() until
SO_SNDTIMEO or a signal, and a non-blocking one would keep getting -EAGAIN,
where before this change virtio_transport_seqpacket_enqueue() returned
-EMSGSIZE immediately.
Since peer_buf_alloc is taken verbatim from peer headers in
virtio_transport_space_update(), can a guest advertising a small buf_alloc
hold a host process inside sendmsg()?
Does tools/testing/vsock/vsock_test.c:test_seqpacket_bigmsg_client() still
pass on vhost-vsock? It expects the immediate error:
send_buf(fd, data, buf_size, 0, -EMSGSIZE);
> + min_space = len - total_written;
> + } else {
> + min_space = 1;
> + }
[Severity: High]
Does waiting for whole-message credit actually make the enqueue atomic?
virtio_transport_send_pkt_info() still splits the payload into
VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (64K) chunks and hands each skb to send_pkt()
as it goes, and EOM is stamped only on the chunk where the iterator drains:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_alloc_skb() {
if (msg_data_left(info->msg) == 0 &&
info->type == VIRTIO_VSOCK_TYPE_SEQPACKET) {
hdr->flags |= cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
...
}
If a later chunk fails, the loop breaks but the error is replaced by a
positive partial count:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_send_pkt_info() {
skb = virtio_transport_alloc_skb(...);
if (!skb) {
ret = -ENOMEM;
break;
}
...
/* Return number of bytes, if any data has been sent. */
if (rest_len != pkt_len)
ret = pkt_len - rest_len;
}
So for a message larger than 64K whose second fragment hits -ENOMEM, or a
copy failure on an unmapped part of the user iovec, doesn't this loop see
written > 0, add it to total_written, recompute min_space and re-enter the
wait with EOM-less fragments already at the peer? Any later out_err exit
(-EINTR, -EAGAIN, sk_err, peer RCV_SHUTDOWN) then abandons them, and
virtio_transport_seqpacket_do_dequeue() only advances msg_count on an EOM
skb:
if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SEQ_EOM) {
msg_ready = true;
vvs->msg_count--;
virtio_transport_get_credit() can also grant less than requested if the
window shrank after the wait:
ret = min_t(u32, credit, virtio_transport_has_space(vvs));
which produces the same short, EOM-less write.
Separately, the size test above compares the full len rather than
len - total_written, so on a second iteration a shrunken maximum can return
-EMSGSIZE in the middle of a partially transmitted message. Should the
transport enqueue itself be made transactional (emit EOM or discard on
abort) instead of, or in addition to, extending the wait here?
>
> add_wait_queue(sk_sleep(sk), &wait);
> - while (vsock_stream_has_space(vsk) == 0 &&
> + while (vsock_stream_has_space(vsk) < min_space &&
> sk->sk_err == 0 &&
> !(sk->sk_shutdown & SEND_SHUTDOWN) &&
> !(vsk->peer_shutdown & RCV_SHUTDOWN)) {
[Severity: Medium]
The commit message says:
"SOCK_STREAM behaviour is unchanged (min_space == 1 reproduces the old
"wait while space == 0")"
Is that accurate for transports whose stream_has_space() can return a
negative error? The comparison is signed, so < 1 is also true for negative
values, while the old == 0 test was not. vsock_stream_has_space() passes
the raw s64 through, and VMCI does not clamp:
net/vmw_vsock/vmci_transport.c:vmci_transport_stream_has_space() {
return vmci_qpair_produce_free_space(vmci_trans(vsk)->qpair);
}
vmci_q_header_free_space() returns a negative error when the shared
queue-pair indices are out of range:
include/linux/vmw_vmci_defs.h:vmci_q_header_free_space() {
if (tail >= produce_q_size || head >= produce_q_size)
return VMCI_ERROR_INVALID_SIZE;
}
Before this change that negative value skipped the wait and
vmci_transport_stream_enqueue() reported -ENOMEM to userspace. With the
new test, doesn't a blocking STREAM sender sleep instead (sk_err is 0 and
no shutdown bit is set), and a non-blocking one get -EAGAIN?
[Severity: Medium]
Raising the wake threshold from one byte to the full message length also
makes SEQPACKET send liveness depend on the peer advertising enough credit
on its own. Linux never sends VIRTIO_VSOCK_OP_CREDIT_REQUEST:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_notify_send_pre_block() {
return 0;
}
So a sender blocked on vsock_stream_has_space(vsk) < len cannot solicit a
credit refresh. For a peer whose credit-update policy uses hysteresis
(the same heuristic Linux uses on its stream receive path, updating only
when free_space < VIRTIO_VSOCK_MAX_PKT_BUF_SIZE || low_rx_bytes), can the
cached peer_fwd_cnt/peer_buf_alloc stay below len with no further packets
arriving, now that the partial data that used to keep the exchange moving
is no longer sent? Linux peers mask this because
virtio_transport_seqpacket_do_dequeue() calls
virtio_transport_send_credit_update() unconditionally, so the exposure
looks limited to non-Linux peers.
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index f225f53ed4ba..e10e7b95807e 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -994,6 +994,19 @@ virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
> }
> EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_enqueue);
>
> +u32 virtio_transport_seqpacket_max_size(struct vsock_sock *vsk)
> +{
> + struct virtio_vsock_sock *vvs = vsk->trans;
> + u32 max_size;
> +
> + spin_lock_bh(&vvs->tx_lock);
> + max_size = virtio_transport_tx_buf_size(vvs);
> + spin_unlock_bh(&vvs->tx_lock);
> +
> + return max_size;
> +}
> +EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_max_size);
[Severity: High]
This returns a snapshot of min(peer_buf_alloc, buf_alloc) taken under
tx_lock, and the lock is dropped before the value is used. The feasibility
test in vsock_connectible_sendmsg() runs once per outer iteration, before
add_wait_queue(), and the inner predicate only re-reads
vsock_stream_has_space(), sk_err and the shutdown bits. Meanwhile the wait
releases the socket lock:
release_sock(sk);
timeout = wait_woken(&wait, TASK_INTERRUPTIBLE, timeout);
lock_sock(sk);
During that window the bound can shrink from the peer side:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_space_update() {
vvs->peer_buf_alloc = le32_to_cpu(hdr->buf_alloc);
vvs->peer_fwd_cnt = le32_to_cpu(hdr->fwd_cnt);
}
or locally via setsockopt(SO_VM_SOCKETS_BUFFER_SIZE):
net/vmw_vsock/virtio_transport_common.c:virtio_transport_notify_buffer_size() {
vvs->buf_alloc = *val;
}
Consider sendmsg(len = 128K) passing the check with a 256K window, then
sleeping; a peer credit update lowers peer_buf_alloc to 64K and wakes the
sender. The sender re-tests only vsock_stream_has_space(vsk) < min_space,
which virtio_transport_has_space() can no longer satisfy, sk_err is 0 and
no shutdown bit is set, so doesn't it sleep again indefinitely with the
default infinite SO_SNDTIMEO? Before the patch the sender either made
partial progress or got -EMSGSIZE from
virtio_transport_seqpacket_enqueue().
Would it work to fold the maximum-size test into the wait predicate, so it
is re-evaluated after each wakeup under the same lock that computes the
available space?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919122916.28226-1-bartlomiej.dmitruk%40isec.pl
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-23 12:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-19 12:29 [PATCH v2] vsock: keep SOCK_SEQPACKET message boundaries on interrupted send Bartłomiej Dmitruk
2026-09-20 9:08 ` David Laight
2026-09-23 12:46 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®