On Tue, Jun 9, 2026 at 1:48 AM Stefano Garzarella <[email protected]> wrote:
>
> On Tue, Jun 09, 2026 at 12:48:05AM +0000, Octavian Purdila wrote:
> >When transmission fails in virtio_transport_send_pkt_info, the msg_iter
> >might have been partially advanced. If we don't restore it, the next
> >attempt to send data will use an incorrect iterator state, leading to
> >desync and warnings like "send_pkt() returns 0, but X expected".
>
> Thanks for the fix! I have some comments.
Thank you for the quick review!
> >diff --git a/net/vmw_vsock/virtio_transport_common.c
> >b/net/vmw_vsock/virtio_transport_common.c
> >index b10666937c490..588623a3e2bbc 100644
> >--- a/net/vmw_vsock/virtio_transport_common.c
> >+++ b/net/vmw_vsock/virtio_transport_common.c
> >@@ -367,6 +367,10 @@ static int virtio_transport_send_pkt_info(struct
> >vsock_sock *vsk,
> > do {
> > struct sk_buff *skb;
> > size_t skb_len;
> >+ struct iov_iter saved_iter;
>
> trivial: reverse xmas tree:
> https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs
>
I'll fix it in v2, sorry for missing this.
> >+
> >+ if (info->msg)
> >+ saved_iter = info->msg->msg_iter;
>
> What about using iov_iter_save_state()/iov_iter_restore() ?
>
> IIUC we may need to export iov_iter_restore(), so not a strong opinion,
> but it looks better to use those API IMHO.
>
I agree, I'll add the export as a separate patch in v2 and move to
using these APIs.
> >
> > skb_len = min(max_skb_len, rest_len);
> >
> >@@ -375,6 +379,8 @@ static int virtio_transport_send_pkt_info(struct
> >vsock_sock *vsk,
> > src_cid, src_port,
> > dst_cid, dst_port);
>
> What about adding a comment on top of virtio_transport_alloc_skb() call
> (or when we save the state) to explain that in specific cases it can
> advance the msg_iter ?
>
Good point, I will add a comment explaining this in v2.
> > if (!skb) {
> >+ if (info->msg)
> >+ info->msg->msg_iter = saved_iter;
> > ret = -ENOMEM;
> > break;
> > }
> >@@ -382,8 +388,11 @@ static int virtio_transport_send_pkt_info(struct
> >vsock_sock *vsk,
> > virtio_transport_inc_tx_pkt(vvs, skb);
> >
> > ret = t_ops->send_pkt(skb, info->net);
> >- if (ret < 0)
> >+ if (ret < 0) {
> >+ if (info->msg)
> >+ info->msg->msg_iter = saved_iter;
>
> Also, what about having a single restore point after the loop?
>
> I mean something like this (untested):
>
Yes, that looks much better. I tested it locally and it works fine. Thanks!
I'll follow-up with v2 in a day or two.
