* Re: [PATCH net] vsock: keep poll shutdown state consistent
From: Stefano Garzarella @ 2026-05-18 10:16 UTC (permalink / raw)
To: Ziyu Zhang
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andy King, George Zhang, Dmitry Torokhov,
virtualization, netdev, linux-kernel, baijiaju1990, r33s3n6,
gality369, zhenghaoran154, hanguidong02, zzzccc427
In-Reply-To: <20260516034745.260442-1-ziyuzhang201@gmail.com>
On Sat, May 16, 2026 at 11:47:45AM +0800, Ziyu Zhang wrote:
>vsock_poll() reads vsk->peer_shutdown before taking the socket
>lock to set EPOLLHUP and EPOLLRDHUP, then reads it again under the
>lock to report EOF readability. A shutdown packet can update
>peer_shutdown while poll is waiting for the lock, so one poll invocation
>can report EPOLLIN without the corresponding HUP/RDHUP bits.
>
>Keep non-connectible sockets on a single lockless READ_ONCE()
Should this be paired with WRITE_ONCE() on writers?
>snapshot. For connectible sockets, defer shutdown-derived poll bits
>until after lock_sock() and use one READ_ONCE() snapshot for both EOF
>readability and HUP/RDHUP. This preserves shutdowns that arrive before
>the lock is acquired and keeps all peer-shutdown-derived bits consistent
>for a poll pass.
>
>Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
>Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
>---
> net/vmw_vsock/af_vsock.c | 40 ++++++++++++++++++++++++++--------------
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index adcba1b7b..bed42347b 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1122,6 +1122,25 @@ static int vsock_shutdown(struct socket *sock, int mode)
> return err;
> }
>
>+static __poll_t vsock_poll_shutdown(struct sock *sk, u32 peer_shutdown)
>+{
>+ __poll_t mask = 0;
>+
>+ /* INET sockets treat local write shutdown and peer write shutdown as a
>+ * case of EPOLLHUP set.
>+ */
>+ if (sk->sk_shutdown == SHUTDOWN_MASK ||
>+ ((sk->sk_shutdown & SEND_SHUTDOWN) &&
>+ (peer_shutdown & SEND_SHUTDOWN)))
>+ mask |= EPOLLHUP;
>+
>+ if (sk->sk_shutdown & RCV_SHUTDOWN ||
>+ peer_shutdown & SEND_SHUTDOWN)
>+ mask |= EPOLLRDHUP;
>+
>+ return mask;
>+}
>+
> static __poll_t vsock_poll(struct file *file, struct socket *sock,
> poll_table *wait)
> {
>@@ -1139,19 +1158,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> /* Signify that there has been an error on this socket. */
> mask |= EPOLLERR;
>
>- /* INET sockets treat local write shutdown and peer write shutdown as a
>- * case of EPOLLHUP set.
>- */
>- if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
>- ((sk->sk_shutdown & SEND_SHUTDOWN) &&
>- (vsk->peer_shutdown & SEND_SHUTDOWN))) {
>- mask |= EPOLLHUP;
>- }
>-
>- if (sk->sk_shutdown & RCV_SHUTDOWN ||
>- vsk->peer_shutdown & SEND_SHUTDOWN) {
>- mask |= EPOLLRDHUP;
>- }
>+ if (!sock_type_connectible(sk->sk_type))
>+ mask |= vsock_poll_shutdown(sk,
>+ READ_ONCE(vsk->peer_shutdown));
Can we move this in the `if (sock->type == SOCK_DGRAM)` branch ?
Not a strong opinion about that, but in any case IMO we should add a
comment here to explain why we are doing only for not connectible
sockets.
That said, if we use WRITE_ONCE in the writers, do we really need to
move this after the lock_sock for the connectable ones?
>
> if (sk_is_readable(sk))
> mask |= EPOLLIN | EPOLLRDNORM;
>@@ -1171,6 +1180,7 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
>
> } else if (sock_type_connectible(sk->sk_type)) {
> const struct vsock_transport *transport;
>+ u32 peer_shutdown;
>
> lock_sock(sk);
>
>@@ -1203,10 +1213,12 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> * terminated should also be considered read, and we check the
> * shutdown flag for that.
> */
>+ peer_shutdown = READ_ONCE(vsk->peer_shutdown);
> if (sk->sk_shutdown & RCV_SHUTDOWN ||
>- vsk->peer_shutdown & SEND_SHUTDOWN) {
>+ peer_shutdown & SEND_SHUTDOWN) {
> mask |= EPOLLIN | EPOLLRDNORM;
> }
>+ mask |= vsock_poll_shutdown(sk, peer_shutdown);
nit: to keep the order the same as before, I would move this call just
before this `if` block, but I don't think it makes any difference in the
end.
>
> /* Connected sockets that can produce data can be written. */
> if (transport && sk->sk_state == TCP_ESTABLISHED) {
>--
>2.43.0
>
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix zerocopy completion for multi-skb sends
From: David Laight @ 2026-05-18 10:50 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Michael S. Tsirkin, netdev, Jakub Kicinski, Paolo Abeni,
Simon Horman, Arseniy Krasnov, Stefan Hajnoczi, kvm, Eric Dumazet,
Eugenio Pérez, Xuan Zhuo, virtualization, David S. Miller,
Jason Wang, linux-kernel, Maher Azzouzi
In-Reply-To: <agrg1GVzoJWtNuSd@sgarzare-redhat>
On Mon, 18 May 2026 11:54:19 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:
> On Mon, May 18, 2026 at 05:33:08AM -0400, Michael S. Tsirkin wrote:
> >On Mon, May 18, 2026 at 11:18:24AM +0200, Stefano Garzarella wrote:
> >> On Sat, May 16, 2026 at 12:53:29PM +0100, David Laight wrote:
> >> > On Thu, 14 May 2026 11:29:48 +0200
> >> > Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >
> >> > > From: Stefano Garzarella <sgarzare@redhat.com>
> >> > >
> >> > > When a large message is fragmented into multiple skbs, the zerocopy
> >> > > uarg is only allocated and attached to the last skb in the loop.
> >> > > Non-final skbs carry pinned user pages with no completion tracking,
> >> > > so the kernel has no way to notify userspace when those pages are safe
> >> > > to reuse. If the loop breaks early the uarg is never allocated at all,
> >> > > leaking pinned pages with no completion notification.
> >> > >
> >> > > Fix this by following the approach used by TCP: allocate the zerocopy
> >> > > uarg (if not provided by the caller) before the send loop and attach
> >> > > it to every skb via skb_zcopy_set(), which takes a reference per skb.
> >> > > Each skb's completion properly decrements the refcount, and the
> >> > > notification only fires after the last skb is freed.
> >> > > On failure, if no data was sent, the uarg is cleanly aborted via
> >> > > net_zcopy_put_abort().
> >> > >
> >> > > This issue was initially discovered by sashiko while reviewing commit
> >> > > 1cb36e252211 ("vsock/virtio: fix MSG_ZEROCOPY pinned-pages accounting")
> >> > > but was pre-existing.
> >> > >
> >> > > Fixes: 581512a6dc93 ("vsock/virtio: MSG_ZEROCOPY flag support")
> >> > > Cc: Arseniy Krasnov <avkrasnov@salutedevices.com>
> >> > > Closes: https://sashiko.dev/#/patchset/20260420132051.217589-1-sgarzare%40redhat.com
> >> > > Reported-by: Maher Azzouzi <maherazz04@gmail.com>
> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> > > ---
> >> > > net/vmw_vsock/virtio_transport_common.c | 83 ++++++++++---------------
> >> > > 1 file changed, 34 insertions(+), 49 deletions(-)
> >> > >
> >> > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> >> > > index 989cc252d3d3..1e3409d28164 100644
> >> > > --- a/net/vmw_vsock/virtio_transport_common.c
> >> > > +++ b/net/vmw_vsock/virtio_transport_common.c
> >> > > @@ -70,34 +70,6 @@ static bool virtio_transport_can_zcopy(const struct virtio_transport *t_ops,
> >> > > return true;
> >> > > }
> >> > >
> >> > > -static int virtio_transport_init_zcopy_skb(struct vsock_sock *vsk,
> >> > > - struct sk_buff *skb,
> >> > > - struct msghdr *msg,
> >> > > - size_t pkt_len,
> >> > > - bool zerocopy)
> >> > > -{
> >> > > - struct ubuf_info *uarg;
> >> > > -
> >> > > - if (msg->msg_ubuf) {
> >> > > - uarg = msg->msg_ubuf;
> >> > > - net_zcopy_get(uarg);
> >> > > - } else {
> >> > > - struct ubuf_info_msgzc *uarg_zc;
> >> > > -
> >> > > - uarg = msg_zerocopy_realloc(sk_vsock(vsk),
> >> > > - pkt_len, NULL, false);
> >> > > - if (!uarg)
> >> > > - return -1;
> >> > > -
> >> > > - uarg_zc = uarg_to_msgzc(uarg);
> >> > > - uarg_zc->zerocopy = zerocopy ? 1 : 0;
> >> > > - }
> >> > > -
> >> > > - skb_zcopy_init(skb, uarg);
> >> > > -
> >> > > - return 0;
> >> > > -}
> >> > > -
> >> > > static int virtio_transport_fill_skb(struct sk_buff *skb,
> >> > > struct virtio_vsock_pkt_info *info,
> >> > > size_t len,
> >> > > @@ -317,8 +289,10 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> >> > > u32 src_cid, src_port, dst_cid, dst_port;
> >> > > const struct virtio_transport *t_ops;
> >> > > struct virtio_vsock_sock *vvs;
> >> > > + struct ubuf_info *uarg = NULL;
> >> > > u32 pkt_len = info->pkt_len;
> >> > > bool can_zcopy = false;
> >> > > + bool have_uref = false;
> >> > > u32 rest_len;
> >> > > int ret;
> >> > >
> >> > > @@ -360,6 +334,25 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> >> > > if (can_zcopy)
> >> > > max_skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE,
> >> > > (MAX_SKB_FRAGS * PAGE_SIZE));
> >> > > +
> >> > > + if (info->msg->msg_flags & MSG_ZEROCOPY &&
> >> > > + info->op == VIRTIO_VSOCK_OP_RW) {
> >> > > + uarg = info->msg->msg_ubuf;
> >> > > +
> >> > > + if (!uarg) {
> >> > > + uarg = msg_zerocopy_realloc(sk_vsock(vsk),
> >> > > + pkt_len, NULL, false);
> >> > > + if (!uarg) {
> >> > > + virtio_transport_put_credit(vvs, pkt_len);
> >> > > + return -ENOMEM;
> >> > > + }
> >> > > +
> >> > > + if (!can_zcopy)
> >> > > + uarg_to_msgzc(uarg)->zerocopy = 0;
> >> > > +
> >> > > + have_uref = true;
> >> > > + }
> >> > > + }
> >> >
> >> > Surely that block should only be done if can_zcopy is true?
> >> > And shouldn't something unset it if info->op != VIRTIO_VSOCK_OP_RW ?
> >> > If the msg_zerocopy_realloc() fails then can't you just set can_zcopy to false.
> >> >
> >> > It info->msg->msg_buf is already set then I think you have to disable zero-copy.
> >> > The caller has already requested a callback - and you can't add another.
> >> >
> >> > In any case by the end of this can_zcopy and have_uref are really the same flag.
> >>
> >> I kept the same approach we had before, trying to make as few changes as
> >> possible.
> >>
> >> All these potential issues seem to be pre-existing and should be eventually
> >> addressed in other patches IMHO. This patch one only resolves the main issue
> >> of calling `skb_zcopy_set()` for every skb to avoid leaking pages, etc.
> >
> >the patch is upstream now, right? So pretty much have to be patches on
> >top.
>
> If those are actual issues, then yes. TBH, I didn’t look into that
> aspect and left it as it was before. We should take a closer look at how
> MSG_ZEROCOPY is handled.
>
> David, if you think it needs fixing and you have time, feel free to send
> patches on top.
I'm not fully sure how it all works.
Especially the paths where msg->msg_ubuf is non-NULL, I suspect it should
be added to all the skb even if the ZEROCOPY flag isn't set.
I was just reading the one function.
But there did look like some very dodgy conditionals.
-- David
>
> Thanks,
> Stefano
>
> >
> >> @Arseniy can you help on this?
> >>
> >> >
> >> > > }
> >> > >
> >> > > rest_len = pkt_len;
> >> > > @@ -378,27 +371,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> >> > > break;
> >> > > }
> >> > >
> >> > > - /* We process buffer part by part, allocating skb on
> >> > > - * each iteration. If this is last skb for this buffer
> >> > > - * and MSG_ZEROCOPY mode is in use - we must allocate
> >> > > - * completion for the current syscall.
> >> > > - *
> >> > > - * Pass pkt_len because msg iter is already consumed
> >> > > - * by virtio_transport_fill_skb(), so iter->count
> >> > > - * can not be used for RLIMIT_MEMLOCK pinned-pages
> >> > > - * accounting done by msg_zerocopy_realloc().
> >> > > - */
> >> > > - if (info->msg && info->msg->msg_flags & MSG_ZEROCOPY &&
> >> > > - skb_len == rest_len && info->op == VIRTIO_VSOCK_OP_RW) {
> >> > > - if (virtio_transport_init_zcopy_skb(vsk, skb,
> >> > > - info->msg,
> >> > > - pkt_len,
> >> > > - can_zcopy)) {
> >> > > - kfree_skb(skb);
> >> > > - ret = -ENOMEM;
> >> > > - break;
> >> > > - }
> >> > > - }
> >> > > + skb_zcopy_set(skb, uarg, NULL);
> >> > >
> >> > > virtio_transport_inc_tx_pkt(vvs, skb);
> >> > >
> >> > > @@ -422,6 +395,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> >> > >
> >> > > virtio_transport_put_credit(vvs, rest_len);
> >> > >
> >> > > + /* msg_zerocopy_realloc() initializes the ubuf_info refcnt to 1.
> >> > > + * skb_zcopy_set() increases it for each skb, so we can drop that
> >> > ^ must
> >> >
> >> > > + * initial reference to keep it balanced.
> >> > > + */
> >> > > + if (have_uref) {
> >> > > + if (rest_len == pkt_len)
> >> > > + /* No data sent, abort the notification. */
> >> > > + net_zcopy_put_abort(uarg, true);
> >> >
> >> > Is it worth optimising for the 'nothing sent' case ?
> >>
> >> What do you suggest doing?
> >>
> >> I followed what TCP does.
> >>
> >> Thanks,
> >> Stefano
> >>
> >> >
> >> > -- David
> >> >
> >> > > + else
> >> > > + net_zcopy_put(uarg);
> >> > > + }
> >> > > +
> >> > > /* Return number of bytes, if any data has been sent. */
> >> > > if (rest_len != pkt_len)
> >> > > ret = pkt_len - rest_len;
> >> >
> >
>
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix zerocopy completion for multi-skb sends
From: Stefano Garzarella @ 2026-05-18 11:08 UTC (permalink / raw)
To: David Laight
Cc: Michael S. Tsirkin, netdev, Jakub Kicinski, Paolo Abeni,
Simon Horman, Arseniy Krasnov, Stefan Hajnoczi, kvm, Eric Dumazet,
Eugenio Pérez, Xuan Zhuo, virtualization, David S. Miller,
Jason Wang, linux-kernel, Maher Azzouzi
In-Reply-To: <20260518115005.5f13bd2b@pumpkin>
On Mon, May 18, 2026 at 11:50:05AM +0100, David Laight wrote:
>On Mon, 18 May 2026 11:54:19 +0200
>Stefano Garzarella <sgarzare@redhat.com> wrote:
>
>> On Mon, May 18, 2026 at 05:33:08AM -0400, Michael S. Tsirkin wrote:
>> >On Mon, May 18, 2026 at 11:18:24AM +0200, Stefano Garzarella wrote:
>> >> On Sat, May 16, 2026 at 12:53:29PM +0100, David Laight wrote:
>> >> > On Thu, 14 May 2026 11:29:48 +0200
>> >> > Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >
>> >> > > From: Stefano Garzarella <sgarzare@redhat.com>
>> >> > >
>> >> > > When a large message is fragmented into multiple skbs, the zerocopy
>> >> > > uarg is only allocated and attached to the last skb in the loop.
>> >> > > Non-final skbs carry pinned user pages with no completion tracking,
>> >> > > so the kernel has no way to notify userspace when those pages are safe
>> >> > > to reuse. If the loop breaks early the uarg is never allocated at all,
>> >> > > leaking pinned pages with no completion notification.
>> >> > >
>> >> > > Fix this by following the approach used by TCP: allocate the zerocopy
>> >> > > uarg (if not provided by the caller) before the send loop and attach
>> >> > > it to every skb via skb_zcopy_set(), which takes a reference per skb.
>> >> > > Each skb's completion properly decrements the refcount, and the
>> >> > > notification only fires after the last skb is freed.
>> >> > > On failure, if no data was sent, the uarg is cleanly aborted via
>> >> > > net_zcopy_put_abort().
>> >> > >
>> >> > > This issue was initially discovered by sashiko while reviewing commit
>> >> > > 1cb36e252211 ("vsock/virtio: fix MSG_ZEROCOPY pinned-pages accounting")
>> >> > > but was pre-existing.
>> >> > >
>> >> > > Fixes: 581512a6dc93 ("vsock/virtio: MSG_ZEROCOPY flag support")
>> >> > > Cc: Arseniy Krasnov <avkrasnov@salutedevices.com>
>> >> > > Closes: https://sashiko.dev/#/patchset/20260420132051.217589-1-sgarzare%40redhat.com
>> >> > > Reported-by: Maher Azzouzi <maherazz04@gmail.com>
>> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> > > ---
>> >> > > net/vmw_vsock/virtio_transport_common.c | 83 ++++++++++---------------
>> >> > > 1 file changed, 34 insertions(+), 49 deletions(-)
>> >> > >
>> >> > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> >> > > index 989cc252d3d3..1e3409d28164 100644
>> >> > > --- a/net/vmw_vsock/virtio_transport_common.c
>> >> > > +++ b/net/vmw_vsock/virtio_transport_common.c
>> >> > > @@ -70,34 +70,6 @@ static bool virtio_transport_can_zcopy(const struct virtio_transport *t_ops,
>> >> > > return true;
>> >> > > }
>> >> > >
>> >> > > -static int virtio_transport_init_zcopy_skb(struct vsock_sock *vsk,
>> >> > > - struct sk_buff *skb,
>> >> > > - struct msghdr *msg,
>> >> > > - size_t pkt_len,
>> >> > > - bool zerocopy)
>> >> > > -{
>> >> > > - struct ubuf_info *uarg;
>> >> > > -
>> >> > > - if (msg->msg_ubuf) {
>> >> > > - uarg = msg->msg_ubuf;
>> >> > > - net_zcopy_get(uarg);
>> >> > > - } else {
>> >> > > - struct ubuf_info_msgzc *uarg_zc;
>> >> > > -
>> >> > > - uarg = msg_zerocopy_realloc(sk_vsock(vsk),
>> >> > > - pkt_len, NULL, false);
>> >> > > - if (!uarg)
>> >> > > - return -1;
>> >> > > -
>> >> > > - uarg_zc = uarg_to_msgzc(uarg);
>> >> > > - uarg_zc->zerocopy = zerocopy ? 1 : 0;
>> >> > > - }
>> >> > > -
>> >> > > - skb_zcopy_init(skb, uarg);
>> >> > > -
>> >> > > - return 0;
>> >> > > -}
>> >> > > -
>> >> > > static int virtio_transport_fill_skb(struct sk_buff *skb,
>> >> > > struct virtio_vsock_pkt_info *info,
>> >> > > size_t len,
>> >> > > @@ -317,8 +289,10 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>> >> > > u32 src_cid, src_port, dst_cid, dst_port;
>> >> > > const struct virtio_transport *t_ops;
>> >> > > struct virtio_vsock_sock *vvs;
>> >> > > + struct ubuf_info *uarg = NULL;
>> >> > > u32 pkt_len = info->pkt_len;
>> >> > > bool can_zcopy = false;
>> >> > > + bool have_uref = false;
>> >> > > u32 rest_len;
>> >> > > int ret;
>> >> > >
>> >> > > @@ -360,6 +334,25 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>> >> > > if (can_zcopy)
>> >> > > max_skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE,
>> >> > > (MAX_SKB_FRAGS * PAGE_SIZE));
>> >> > > +
>> >> > > + if (info->msg->msg_flags & MSG_ZEROCOPY &&
>> >> > > + info->op == VIRTIO_VSOCK_OP_RW) {
>> >> > > + uarg = info->msg->msg_ubuf;
>> >> > > +
>> >> > > + if (!uarg) {
>> >> > > + uarg = msg_zerocopy_realloc(sk_vsock(vsk),
>> >> > > + pkt_len, NULL, false);
>> >> > > + if (!uarg) {
>> >> > > + virtio_transport_put_credit(vvs, pkt_len);
>> >> > > + return -ENOMEM;
>> >> > > + }
>> >> > > +
>> >> > > + if (!can_zcopy)
>> >> > > + uarg_to_msgzc(uarg)->zerocopy = 0;
>> >> > > +
>> >> > > + have_uref = true;
>> >> > > + }
>> >> > > + }
>> >> >
>> >> > Surely that block should only be done if can_zcopy is true?
>> >> > And shouldn't something unset it if info->op != VIRTIO_VSOCK_OP_RW ?
>> >> > If the msg_zerocopy_realloc() fails then can't you just set can_zcopy to false.
>> >> >
>> >> > It info->msg->msg_buf is already set then I think you have to disable zero-copy.
>> >> > The caller has already requested a callback - and you can't add another.
>> >> >
>> >> > In any case by the end of this can_zcopy and have_uref are really the same flag.
>> >>
>> >> I kept the same approach we had before, trying to make as few changes as
>> >> possible.
>> >>
>> >> All these potential issues seem to be pre-existing and should be eventually
>> >> addressed in other patches IMHO. This patch one only resolves the main issue
>> >> of calling `skb_zcopy_set()` for every skb to avoid leaking pages, etc.
>> >
>> >the patch is upstream now, right? So pretty much have to be patches on
>> >top.
>>
>> If those are actual issues, then yes. TBH, I didn’t look into that
>> aspect and left it as it was before. We should take a closer look at how
>> MSG_ZEROCOPY is handled.
>>
>> David, if you think it needs fixing and you have time, feel free to send
>> patches on top.
>
>I'm not fully sure how it all works.
Same here, so I pinged Arseniy who worked on that, since it seemed
deliberate to have `can_zcopy` (and set `uarg->zerocopy` accordingly)
only when it was supported by the transport.
>Especially the paths where msg->msg_ubuf is non-NULL, I suspect it should
>be added to all the skb even if the ZEROCOPY flag isn't set.
>I was just reading the one function.
>But there did look like some very dodgy conditionals.
I see, let's wait for Arseniy's feedback; otherwise, I'll try to fix it
next week. As mentioned, this issue existed before this patch, so it
shouldn't be a regression.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH net] vsock: keep poll shutdown state consistent
From: ziyu zhang @ 2026-05-18 12:39 UTC (permalink / raw)
To: Stefano Garzarella
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andy King, George Zhang, Dmitry Torokhov,
virtualization, netdev, linux-kernel, baijiaju1990, r33s3n6,
gality369, zhenghaoran154, hanguidong02, zzzccc427
In-Reply-To: <agrldIdftDXvATYx@sgarzare-redhat>
On Mon, 18 May 2026 at 18:16, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Sat, May 16, 2026 at 11:47:45AM +0800, Ziyu Zhang wrote:
> >vsock_poll() reads vsk->peer_shutdown before taking the socket
> >lock to set EPOLLHUP and EPOLLRDHUP, then reads it again under the
> >lock to report EOF readability. A shutdown packet can update
> >peer_shutdown while poll is waiting for the lock, so one poll invocation
> >can report EPOLLIN without the corresponding HUP/RDHUP bits.
> >
> >Keep non-connectible sockets on a single lockless READ_ONCE()
>
> Should this be paired with WRITE_ONCE() on writers?
Yes, since the poll path uses lockless READ_ONCE() snapshots of
peer_shutdown, the writer side should be annotated with WRITE_ONCE() as
well. I will add that in v2.
>
> >snapshot. For connectible sockets, defer shutdown-derived poll bits
> >until after lock_sock() and use one READ_ONCE() snapshot for both EOF
> >readability and HUP/RDHUP. This preserves shutdowns that arrive before
> >the lock is acquired and keeps all peer-shutdown-derived bits consistent
> >for a poll pass.
> >
> >Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
> >Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
> >---
> > net/vmw_vsock/af_vsock.c | 40 ++++++++++++++++++++++++++--------------
> > 1 file changed, 26 insertions(+), 14 deletions(-)
> >
> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >index adcba1b7b..bed42347b 100644
> >--- a/net/vmw_vsock/af_vsock.c
> >+++ b/net/vmw_vsock/af_vsock.c
> >@@ -1122,6 +1122,25 @@ static int vsock_shutdown(struct socket *sock, int mode)
> > return err;
> > }
> >
> >+static __poll_t vsock_poll_shutdown(struct sock *sk, u32 peer_shutdown)
> >+{
> >+ __poll_t mask = 0;
> >+
> >+ /* INET sockets treat local write shutdown and peer write shutdown as a
> >+ * case of EPOLLHUP set.
> >+ */
> >+ if (sk->sk_shutdown == SHUTDOWN_MASK ||
> >+ ((sk->sk_shutdown & SEND_SHUTDOWN) &&
> >+ (peer_shutdown & SEND_SHUTDOWN)))
> >+ mask |= EPOLLHUP;
> >+
> >+ if (sk->sk_shutdown & RCV_SHUTDOWN ||
> >+ peer_shutdown & SEND_SHUTDOWN)
> >+ mask |= EPOLLRDHUP;
> >+
> >+ return mask;
> >+}
> >+
> > static __poll_t vsock_poll(struct file *file, struct socket *sock,
> > poll_table *wait)
> > {
> >@@ -1139,19 +1158,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> > /* Signify that there has been an error on this socket. */
> > mask |= EPOLLERR;
> >
> >- /* INET sockets treat local write shutdown and peer write shutdown as a
> >- * case of EPOLLHUP set.
> >- */
> >- if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
> >- ((sk->sk_shutdown & SEND_SHUTDOWN) &&
> >- (vsk->peer_shutdown & SEND_SHUTDOWN))) {
> >- mask |= EPOLLHUP;
> >- }
> >-
> >- if (sk->sk_shutdown & RCV_SHUTDOWN ||
> >- vsk->peer_shutdown & SEND_SHUTDOWN) {
> >- mask |= EPOLLRDHUP;
> >- }
> >+ if (!sock_type_connectible(sk->sk_type))
> >+ mask |= vsock_poll_shutdown(sk,
> >+ READ_ONCE(vsk->peer_shutdown));
>
> Can we move this in the `if (sock->type == SOCK_DGRAM)` branch ?
>
> Not a strong opinion about that, but in any case IMO we should add a
> comment here to explain why we are doing only for not connectible
> sockets.
>
> That said, if we use WRITE_ONCE in the writers, do we really need to
> move this after the lock_sock for the connectable ones?
Yes, I will move the non-connectible handling into the SOCK_DGRAM branch
and add a comment there.
For connectible sockets, my current understanding is that
READ_ONCE()/WRITE_ONCE() make the individual lockless accesses explicit, but
they do not make two separate reads in one vsock_poll() invocation observe the
same peer_shutdown value. So I still think using one peer_shutdown snapshot
after lock_sock() is useful for keeping the returned mask internally
consistent. Please let me know if you think WRITE_ONCE() is enough for this
case.
>
> >
> > if (sk_is_readable(sk))
> > mask |= EPOLLIN | EPOLLRDNORM;
> >@@ -1171,6 +1180,7 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> >
> > } else if (sock_type_connectible(sk->sk_type)) {
> > const struct vsock_transport *transport;
> >+ u32 peer_shutdown;
> >
> > lock_sock(sk);
> >
> >@@ -1203,10 +1213,12 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> > * terminated should also be considered read, and we check the
> > * shutdown flag for that.
> > */
> >+ peer_shutdown = READ_ONCE(vsk->peer_shutdown);
> > if (sk->sk_shutdown & RCV_SHUTDOWN ||
> >- vsk->peer_shutdown & SEND_SHUTDOWN) {
> >+ peer_shutdown & SEND_SHUTDOWN) {
> > mask |= EPOLLIN | EPOLLRDNORM;
> > }
> >+ mask |= vsock_poll_shutdown(sk, peer_shutdown);
>
> nit: to keep the order the same as before, I would move this call just
> before this `if` block, but I don't think it makes any difference in the
> end.
Sure, I will restore the previous ordering in v2.
Thanks,
Ziyu
>
> >
> > /* Connected sockets that can produce data can be written. */
> > if (transport && sk->sk_state == TCP_ESTABLISHED) {
> >--
> >2.43.0
> >
>
^ permalink raw reply
* iommu: iterator used after loop end in resv region insertion?
From: Maoyi Xie @ 2026-05-18 18:49 UTC (permalink / raw)
To: joro, will, robin.murphy, jpb; +Cc: iommu, linux-kernel, virtualization
Hi all,
While reading drivers/iommu/ I noticed two places that look
like a past the end iterator pattern. I would appreciate it
if you could take a look and let me know whether these are
real issues and whether they are worth fixing.
The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
(linux-7.1-rc1, around line 873):
list_for_each_entry(iter, regions, list) {
if (nr->start < iter->start ||
(nr->start == iter->start && nr->type <= iter->type))
break;
}
list_add_tail(&nr->list, &iter->list);
The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
(linux-7.1-rc1, around line 523):
list_for_each_entry(next, &vdev->resv_regions, list) {
if (next->start > region->start)
break;
}
list_add_tail(®ion->list, &next->list);
In both cases, when the loop walks all entries without break,
the iterator has gone one step past the last entry. &iter->list
then aliases the list head via container_of offset cancellation,
so the insert lands at the list tail. That is the intended
behaviour, but the access is undefined per C11.
Jakob Koschel cleaned up many such sites in 2022, for example
commits 99d8ae4ec8a (tracing: Remove usage of list iterator
variable after the loop), 2966a9918df (clockevents: Use dedicated
list iterator variable) and dc1acd5c946 (dlm: replace usage of
found with dedicated list iterator variable). These two sites
in drivers/iommu/ were not covered.
A candidate fix would track an explicit insert_before pointer
initialised to the list head and overwritten to &iter->list
only when the loop breaks early. The observable behaviour is
unchanged.
If this is intentional or already known, please disregard.
Otherwise, I am happy to send a [PATCH] or to leave the fix to
you. Thank you for your time, and sorry for the noise if this
is not actually worth fixing or has already been spotted.
Thanks,
Maoyi Xie
https://maoyixie.com/
^ permalink raw reply
* Re: [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC
From: Sean Christopherson @ 2026-05-18 21:11 UTC (permalink / raw)
To: Kiryl Shutsemau, Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Juergen Gross, Daniel Lezcano, Thomas Gleixner, John Stultz,
Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner, David Woodhouse
In-Reply-To: <20260515191942.1892718-1-seanjc@google.com>
On Fri, May 15, 2026, Sean Christopherson wrote:
> Dave/Thomas/Peter/Boris, what's the going rate for bribes to take something
> like this through the tip tree?
>
> The bulk of the changes are in kvmclock and TSC, but pretty much every
> hypervisor's guest-side code gets touched at some point. I am reaonsably
> confident in the correctness of the KVM changes. Michael tested Hyper-V in
> v2, and while there were conflicts when rebasing, they were largely
> superficial (and I've just jinxed myself). For all other hypervisors, assume
> the code is compile-tested only, but those changes are all quite small and
> straightforward.
>
> The only changes that are questionable/contentious are the last two patches,
> which have KVM-as-a-guest use CPUID 0x16 to get the CPU frequency, even on
> AMD (that's the dubious part). I very deliberately put them last, so that
> they can be dropped at will (I don't care terribly if those patches land).
> To merge them, I would want explicit Acks from Paolo and David W.
>
> So, except for the last two patches, to get the stuff I really care about
> landed, I think/hope it's just the TSC and guest-side CoCo changes that need
> reviews/acks?
FYI, don't bother reviewing this version. Sashiko found several glaring flaws,
but I just realized that sashiko-bot's emails are only being sent to myself and
linux-hyperv@vger.kernel.org. I'll make sure to highlight the changes in the
next version.
In the meantime, Sashiko's feedback is archived on lore if you want to see me
get torched by AI :-)
^ permalink raw reply
* Re: [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines
From: Woodhouse, David @ 2026-05-18 21:59 UTC (permalink / raw)
To: tglx@kernel.org, longli@microsoft.com, luto@kernel.org,
alexey.makhalov@broadcom.com, jstultz@google.com,
dave.hansen@linux.intel.com, ajay.kaher@broadcom.com,
jan.kiszka@siemens.com, haiyangz@microsoft.com, kas@kernel.org,
seanjc@google.com, pbonzini@redhat.com, kys@microsoft.com,
decui@microsoft.com, daniel.lezcano@kernel.org,
wei.liu@kernel.org, peterz@infradead.org, jgross@suse.com
Cc: boris.ostrovsky@oracle.com, linux-coco@lists.linux.dev,
kvm@vger.kernel.org, mhklinux@outlook.com,
thomas.lendacky@amd.com, linux-kernel@vger.kernel.org,
bcm-kernel-feedback-list@broadcom.com, tglx@linutronix.de,
nikunj@amd.com, xen-devel@lists.xenproject.org,
linux-hyperv@vger.kernel.org, vkuznets@redhat.com,
rick.p.edgecombe@intel.com, virtualization@lists.linux.dev,
sboyd@kernel.org, x86@kernel.org
In-Reply-To: <20260515191942.1892718-3-seanjc@google.com>
[-- Attachment #1.1: Type: text/plain, Size: 999 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
>
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -569,7 +569,7 @@ static void __init xen_init_time_common(void)
> static_call_update(pv_steal_clock, xen_steal_clock);
> paravirt_set_sched_clock(xen_sched_clock);
>
> - x86_platform.calibrate_tsc = xen_tsc_khz;
> + tsc_register_calibration_routines(xen_tsc_khz, NULL);
> x86_platform.get_wallclock = xen_get_wallclock;
> }
>
xen_tsc_khz() doesn't use CPUID but really *should*.
Care to pull in
https://lore.kernel.org/all/20260509224824.3264567-31-dwmw2@infradead.org/
to your next round please?
(Without the misplaced changes in kvm/x86.c that should have been in
two different prior commits, and are now folded into those correctly in
my kvmclock5 branch ready for the next posting of that).
I'll drop that patch, and the similar x86/kvm one which you *have*
already taken in this series, from my next posting.
Thanks.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5964 bytes --]
[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/4] x86/vmware: Hypercall refactoring and improved guest support
From: Alexey Makhalov @ 2026-05-18 22:52 UTC (permalink / raw)
To: x86, virtualization, bp, hpa, dave.hansen, mingo, tglx
Cc: ajay.kaher, brennan.lamoreaux, bo.gan, bcm-kernel-feedback-list,
linux-kernel, kas, rick.p.edgecombe, linux-coco
In-Reply-To: <20260309235250.2611115-1-alexey.makhalov@broadcom.com>
On 3/9/26 4:52 PM, Alexey Makhalov wrote:
> This series improves VMware guest support on x86 by refactoring the
> hypercall infrastructure and adding better crash diagnostics, along
> with encrypted guest support for the steal time clock.
>
> The first patch introduces a common vmware_hypercall() backend selected
> via static calls. It consolidates the existing hypercall mechanisms
> (backdoor, VMCALL/VMMCALL, and TDX) behind a single interface and
> selects the optimal implementation at boot. This reduces duplication
> and simplifies future extensions.
>
> Building on top of the new hypercall infrastructure, the next two
> patches improve post-mortem debugging of VMware guests. They export
> panic information to the hypervisor by dumping kernel messages to the
> VM vmware.log on the host and explicitly reporting guest crash event
> to the hypervisor.
>
> The final patch adds support for encrypted guests by ensuring that the
> shared memory used for the steal time clock is mapped as decrypted
> before being shared with the hypervisor. This enables steal time
> accounting to function correctly when guest memory encryption is
> enabled.
>
> Patch overview:
>
> 1. x86/vmware: Introduce common vmware_hypercall
>
> * Consolidate hypercall implementations behind a common API
> * Select backend via static_call at boot
>
> 2. x86/vmware: Log kmsg dump on panic
>
> * Register a kmsg dumper
> * Export panic logs to the host
>
> 3. x86/vmware: Report guest crash to the hypervisor
>
> * Register a panic notifier
> * Notify the hypervisor about guest crashes
>
> 4. x86/vmware: Support steal time clock for encrypted guests
>
> * Mark shared steal time memory as decrypted early in boot
>
>
> Changelog:
>
> V1 -> V2
> * Fix compilation warnings in patch 2 "x86/vmware: Log kmsg dump on panic"
> reported by kernel test robot <lkp@intel.com>
>
>
> Alexey Makhalov (4):
> x86/vmware: Introduce common vmware_hypercall()
> x86/vmware: Log kmsg dump on panic
> x86/vmware: Report guest crash to the hypervisor
> x86/vmware: Support steal time clock for encrypted guests
>
> arch/x86/include/asm/vmware.h | 276 ++++++++------------
> arch/x86/kernel/cpu/vmware.c | 470 +++++++++++++++++++++++++---------
> 2 files changed, 463 insertions(+), 283 deletions(-)
>
>
> base-commit: 7d08a6ad25f85c9bb7d0382142838cb54713f1a3
Gentle reminder to review this change. Thanks,
--Alexey
^ permalink raw reply
* Re: [PATCH v3 00/41] x86: Try to wrangle PV clocks vs. TSC
From: David Woodhouse @ 2026-05-18 23:38 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, Paolo Bonzini,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li,
Ajay Kaher, Alexey Makhalov, Jan Kiszka, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
Thomas Gleixner, John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner
In-Reply-To: <20260515191942.1892718-1-seanjc@google.com>
[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]
On Fri, 2026-05-15 at 12:19 -0700, Sean Christopherson wrote:
> Dave/Thomas/Peter/Boris, what's the going rate for bribes to take something
> like this through the tip tree?
>
> The bulk of the changes are in kvmclock and TSC, but pretty much every
> hypervisor's guest-side code gets touched at some point. I am reaonsably
> confident in the correctness of the KVM changes. Michael tested Hyper-V in
> v2, and while there were conflicts when rebasing, they were largely
> superficial (and I've just jinxed myself). For all other hypervisors, assume
> the code is compile-tested only, but those changes are all quite small and
> straightforward.
>
> The only changes that are questionable/contentious are the last two patches,
> which have KVM-as-a-guest use CPUID 0x16 to get the CPU frequency, even on
> AMD (that's the dubious part). I very deliberately put them last, so that
> they can be dropped at will (I don't care terribly if those patches land).
> To merge them, I would want explicit Acks from Paolo and David W.
>
> So, except for the last two patches, to get the stuff I really care about
> landed, I think/hope it's just the TSC and guest-side CoCo changes that need
> reviews/acks?
>
> The primary goal of this series is (or at least was, when I started) to
> fix flaws with SNP and TDX guests where a PV clock provided by the untrusted
> hypervisor is used instead of the secure/trusted TSC that is controlled by
> trusted firmware.
>
> The secondary goal is to draft off of the SNP and TDX changes to slightly
> modernize running under KVM. Currently, KVM guests will use TSC for
> clocksource, but not sched_clock. And they ignore Intel's CPUID-based TSC
> and CPU frequency enumeration, even when using the TSC instead of kvmclock.
> And if the host provides the core crystal frequency in CPUID.0x15, then KVM
> guests can use that for the APIC timer period instead of manually calibrating
> the frequency.
>
> The tertiary goal is to clean up all of the PV clock code to deduplicate logic
> across hypervisors, and to hopefully make it all easier to maintain going
> forward.
I booted this in qemu with -cpu host,+invtsc,+vmware-cpuid-freq
I was expecting to see it eschew the kvmclock and use *only* the TSC.
Is there even any need for 'tsc-early' given that it's *told* the TSC
frequency in CPUID? Shouldn't it have detected that the TSC is known
before init_tsc_clocksource() runs?
And then it even spent some time at boot actually using the kvmclock as
clocksource... when ideally I don't think it would even have *enabled*
it at all?
[ 0.000000] clocksource: kvm-clock: mask: 0xffffffffffffffff max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns
[ 0.000000] tsc: Detected 2400.000 MHz processor
[ 0.008205] TSC deadline timer available
[ 0.008270] clocksource: refined-jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1910969940391419 ns
[ 0.159085] clocksource: hpet: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604467 ns
[ 0.164074] clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles: 0x22983777dd9, max_idle_ns: 440795300422 ns
[ 0.229087] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275000 ns
[ 0.337095] clocksource: Switched to clocksource kvm-clock
[ 0.345246] clocksource: acpi_pm: mask: 0xffffff max_cycles: 0xffffff, max_idle_ns: 2085701024 ns
[ 0.356201] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x22983777dd9, max_idle_ns: 440795300422 ns
[ 0.360560] clocksource: Switched to clocksource tsc
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]
^ permalink raw reply
* [PATCH] virtio: add missing kernel-doc for map and vmap members
From: Christian Fontanez @ 2026-05-19 1:33 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez
Cc: skhan, luis.hernandez093, rbm, Christian Fontanez, virtualization,
linux-kernel
Commit bee8c7c24b73 ("virtio: introduce map ops in virtio core") and
commit b16060c5c7d5 ("virtio: introduce virtio_map container union")
added 'map' and 'vmap' members to struct virtio_device but did not
update the kernel-doc comment block. This caused 'make htmldocs' to
emit warnings:
./include/linux/virtio.h:188 struct member 'map' not described in 'virtio_device'
./include/linux/virtio.h:188 struct member 'vmap' not described in 'virtio_device'
Add the missing entries in struct-declaration order to match the
existing convention in the file. After this patch, 'make htmldocs'
no longer emits these warnings.
Fixes: bee8c7c24b73 ("virtio: introduce map ops in virtio core")
Fixes: b16060c5c7d5 ("virtio: introduce virtio_map container union")
Reported-by: Luis Felipe Hernandez <luis.hernandez093@gmail.com>
Signed-off-by: Christian Fontanez <christfontanez@gmail.com>
---
Notes for reviewers:
- Tested with 'make htmldocs' on linux-next (next-20260518); the two
warnings on include/linux/virtio.h:188 are gone, with no new warnings
introduced.
- Based on linux-next/master at commit 80dd246accce ("Add linux-next
specific files for 20260518").
include/linux/virtio.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3bbc4cb6a672..bf089e51970e 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -157,11 +157,13 @@ struct virtio_admin_cmd {
* @id: the device type identification (used to match it with a driver).
* @config: the configuration ops for this device.
* @vringh_config: configuration ops for host vrings.
+ * @map: the map operations for mapping virtio device memory.
* @vqs: the list of virtqueues for this device.
* @features: the 64 lower features supported by both driver and device.
* @features_array: the full features space supported by both driver and
* device.
* @priv: private pointer for the driver's use.
+ * @vmap: the map container with transport- or device-specific metadata.
* @debugfs_dir: debugfs directory entry.
* @debugfs_filter_features: features to be filtered set by debugfs.
*/
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] drm/virtio: add timeout to virtqueue wait to avoid hung task
From: Dmitry Osipenko @ 2026-05-19 6:11 UTC (permalink / raw)
To: Ryosuke Yasuoka, David Airlie, Gerd Hoffmann, Gurchetan Singh,
Chia-I Wu, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Simona Vetter
Cc: dri-devel, virtualization, linux-kernel
In-Reply-To: <18b095f06dafb7a0.57d57ad28d13539f.2240916b4a5459e2@ryasuoka-thinkpadx1carbongen9.tokyo.csb>
On 5/18/26 09:45, Ryosuke Yasuoka wrote:
>> It may be acceptable to have wait_event_timeout() in a loop, printing
>> warnings about unresponsive host.
> I considered this approach, but it does not solve the recovery problem
> described above. The guest would still be stuck in the loop with no way
> to remove the device or shut down gracefully.
Could `system_state != SYSTEM_RUNNING` be checked in the wait loop? This
may allow to handle system shutdown, aborting the timed out wait in the
special case.
Userspace may also get stuck in a zombie state. For that, code should
use wait_event_interruptible(). But driver has code unprepared to be
interrupted, it all needs to be reworked first [1].
Similarly, for unbind there could a driver flag that the wait loop will
check and handle specially.
WDYT?
[1]
https://lore.kernel.org/dri-devel/20260515084030.21986-1-kartikey406@gmail.com/
--
Best regards,
Dmitry
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix zerocopy completion for multi-skb sends
From: Arseniy Krasnov @ 2026-05-19 6:37 UTC (permalink / raw)
To: Stefano Garzarella, David Laight
Cc: Michael S. Tsirkin, netdev, Jakub Kicinski, Paolo Abeni,
Simon Horman, Stefan Hajnoczi, kvm, Eric Dumazet,
Eugenio Pérez, Xuan Zhuo, virtualization, David S. Miller,
Jason Wang, linux-kernel, Maher Azzouzi
In-Reply-To: <20260519053951.1C60440015@mx4.sberdevices.ru>
On 18/05/2026 14:08, Stefano Garzarella wrote:
> On Mon, May 18, 2026 at 11:50:05AM +0100, David Laight wrote:
>> On Mon, 18 May 2026 11:54:19 +0200
>> Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>>> On Mon, May 18, 2026 at 05:33:08AM -0400, Michael S. Tsirkin wrote:
>>> >On Mon, May 18, 2026 at 11:18:24AM +0200, Stefano Garzarella wrote:
>>> >> On Sat, May 16, 2026 at 12:53:29PM +0100, David Laight wrote:
>>> >> > On Thu, 14 May 2026 11:29:48 +0200
>>> >> > Stefano Garzarella <sgarzare@redhat.com> wrote:
>>> >> >
>>> >> > > From: Stefano Garzarella <sgarzare@redhat.com>
>>> >> > >
>>> >> > > When a large message is fragmented into multiple skbs, the zerocopy
>>> >> > > uarg is only allocated and attached to the last skb in the loop.
>>> >> > > Non-final skbs carry pinned user pages with no completion tracking,
>>> >> > > so the kernel has no way to notify userspace when those pages are safe
>>> >> > > to reuse. If the loop breaks early the uarg is never allocated at all,
>>> >> > > leaking pinned pages with no completion notification.
>>> >> > >
>>> >> > > Fix this by following the approach used by TCP: allocate the zerocopy
>>> >> > > uarg (if not provided by the caller) before the send loop and attach
>>> >> > > it to every skb via skb_zcopy_set(), which takes a reference per skb.
>>> >> > > Each skb's completion properly decrements the refcount, and the
>>> >> > > notification only fires after the last skb is freed.
>>> >> > > On failure, if no data was sent, the uarg is cleanly aborted via
>>> >> > > net_zcopy_put_abort().
>>> >> > >
>>> >> > > This issue was initially discovered by sashiko while reviewing commit
>>> >> > > 1cb36e252211 ("vsock/virtio: fix MSG_ZEROCOPY pinned-pages accounting")
>>> >> > > but was pre-existing.
>>> >> > >
>>> >> > > Fixes: 581512a6dc93 ("vsock/virtio: MSG_ZEROCOPY flag support")
>>> >> > > Cc: Arseniy Krasnov <avkrasnov@salutedevices.com>
>>> >> > > Closes: https://sashiko.dev/#/patchset/20260420132051.217589-1-sgarzare%40redhat.com
>>> >> > > Reported-by: Maher Azzouzi <maherazz04@gmail.com>
>>> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> >> > > ---
>>> >> > > net/vmw_vsock/virtio_transport_common.c | 83 ++++++++++---------------
>>> >> > > 1 file changed, 34 insertions(+), 49 deletions(-)
>>> >> > >
>>> >> > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> >> > > index 989cc252d3d3..1e3409d28164 100644
>>> >> > > --- a/net/vmw_vsock/virtio_transport_common.c
>>> >> > > +++ b/net/vmw_vsock/virtio_transport_common.c
>>> >> > > @@ -70,34 +70,6 @@ static bool virtio_transport_can_zcopy(const struct virtio_transport *t_ops,
>>> >> > > return true;
>>> >> > > }
>>> >> > >
>>> >> > > -static int virtio_transport_init_zcopy_skb(struct vsock_sock *vsk,
>>> >> > > - struct sk_buff *skb,
>>> >> > > - struct msghdr *msg,
>>> >> > > - size_t pkt_len,
>>> >> > > - bool zerocopy)
>>> >> > > -{
>>> >> > > - struct ubuf_info *uarg;
>>> >> > > -
>>> >> > > - if (msg->msg_ubuf) {
>>> >> > > - uarg = msg->msg_ubuf;
>>> >> > > - net_zcopy_get(uarg);
>>> >> > > - } else {
>>> >> > > - struct ubuf_info_msgzc *uarg_zc;
>>> >> > > -
>>> >> > > - uarg = msg_zerocopy_realloc(sk_vsock(vsk),
>>> >> > > - pkt_len, NULL, false);
>>> >> > > - if (!uarg)
>>> >> > > - return -1;
>>> >> > > -
>>> >> > > - uarg_zc = uarg_to_msgzc(uarg);
>>> >> > > - uarg_zc->zerocopy = zerocopy ? 1 : 0;
>>> >> > > - }
>>> >> > > -
>>> >> > > - skb_zcopy_init(skb, uarg);
>>> >> > > -
>>> >> > > - return 0;
>>> >> > > -}
>>> >> > > -
>>> >> > > static int virtio_transport_fill_skb(struct sk_buff *skb,
>>> >> > > struct virtio_vsock_pkt_info *info,
>>> >> > > size_t len,
>>> >> > > @@ -317,8 +289,10 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>>> >> > > u32 src_cid, src_port, dst_cid, dst_port;
>>> >> > > const struct virtio_transport *t_ops;
>>> >> > > struct virtio_vsock_sock *vvs;
>>> >> > > + struct ubuf_info *uarg = NULL;
>>> >> > > u32 pkt_len = info->pkt_len;
>>> >> > > bool can_zcopy = false;
>>> >> > > + bool have_uref = false;
>>> >> > > u32 rest_len;
>>> >> > > int ret;
>>> >> > >
>>> >> > > @@ -360,6 +334,25 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>>> >> > > if (can_zcopy)
>>> >> > > max_skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE,
>>> >> > > (MAX_SKB_FRAGS * PAGE_SIZE));
>>> >> > > +
>>> >> > > + if (info->msg->msg_flags & MSG_ZEROCOPY &&
>>> >> > > + info->op == VIRTIO_VSOCK_OP_RW) {
>>> >> > > + uarg = info->msg->msg_ubuf;
>>> >> > > +
>>> >> > > + if (!uarg) {
>>> >> > > + uarg = msg_zerocopy_realloc(sk_vsock(vsk),
>>> >> > > + pkt_len, NULL, false);
>>> >> > > + if (!uarg) {
>>> >> > > + virtio_transport_put_credit(vvs, pkt_len);
>>> >> > > + return -ENOMEM;
>>> >> > > + }
>>> >> > > +
>>> >> > > + if (!can_zcopy)
>>> >> > > + uarg_to_msgzc(uarg)->zerocopy = 0;
>>> >> > > +
>>> >> > > + have_uref = true;
>>> >> > > + }
>>> >> > > + }
>>> >> >
Hi guys! Just some replies after quick look:
>>> >> > Surely that block should only be done if can_zcopy is true?
I guess no, since TCP also allocates uarg even if zerocopy is impossible -
it just sets uarg_to_msgzc(uarg)->zerocopy = 0;
>>> >> > And shouldn't something unset it if info->op != VIRTIO_VSOCK_OP_RW ?
Hm, we can't enter block 'if (info->msg) {' when 'info->op' is not equal to 'VIRTIO_VSOCK_OP_RW',
because 'msg' is not NULL only for 'VIRTIO_VSOCK_OP_RW'. But anyway You point to right thing - check for
'VIRTIO_VSOCK_OP_RW' could be removed here. Just with comment why.
>>> >> > If the msg_zerocopy_realloc() fails then can't you just set can_zcopy to false.
Here I guess it is better to follow TCP way to make same behaviour, because exact
logic for MSG_ZEROCOPY is not documented. TCP also returns error and stops tx loop.
>>> >> >
>>> >> > It info->msg->msg_buf is already set then I think you have to disable zero-copy.
>>> >> > The caller has already requested a callback - and you can't add another.
In TCP implementation if 'msg_ubuf' is set they just use it and check for zerocopy in
the same way as 'msg_ubuf' is NULL.
>>> >> >
>>> >> > In any case by the end of this can_zcopy and have_uref are really the same flag.
>>> >>
Need to check it more. But 'can_zcopy' means that we fill frags in skb, have_uref means that we
allocated completion (but it could be reported with not set 'SO_EE_CODE_ZEROCOPY_COPIED' if
'can_zcopy' was false).
@Stefano, I guess current implementation differs from TCP in two cases (at least from first
view):
1) When 'msg_ubuf' is set: in TCP, already set 'msg_ubuf' is passed to 'skb_zerocopy_iter_stream()'
(if zerocopy is possible) where it is used as in vsock in call 'skb_zcopy_set()'. In vsock case if
'msg_ubuf' is not NULL we will pass just NULL to 'skb_zcopy_set()'. Yes this is will be
no-effect call today (due to checks in 'skb_zcopy_set()'), but anyway - in future may be not.
2) Also i see that 'skb_zerocopy_iter_stream()' in TCP version has some extra checks which are
missed in vsock - we only just call '__zerocopy_sg_from_iter()' to fill skb in zerocopy way.
But, I think, instead of trying to compare vsock and TCP versions best way is to just copy
current TCP flow as close as possible:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/net/ipv4/tcp.c#n1144
1) Use same flow of checks for 'msg_flags', 'msg_ubuf', SOCK_ZEROCOPY etc.
2) To copy data we can use 'skb_zerocopy_iter_stream()'.
3) The only thing that we don't need in vsock is dev mem related code from TCP implementation.
I can take this task, but pls need some time - may be two/three weeks due to another tasks.
What do You think?
Thanks
>>> >> I kept the same approach we had before, trying to make as few changes as
>>> >> possible.
>>> >>
>>> >> All these potential issues seem to be pre-existing and should be eventually
>>> >> addressed in other patches IMHO. This patch one only resolves the main issue
>>> >> of calling `skb_zcopy_set()` for every skb to avoid leaking pages, etc.
>>> >
>>> >the patch is upstream now, right? So pretty much have to be patches on
>>> >top.
>>>
>>> If those are actual issues, then yes. TBH, I didn’t look into that
>>> aspect and left it as it was before. We should take a closer look at how
>>> MSG_ZEROCOPY is handled.
>>>
>>> David, if you think it needs fixing and you have time, feel free to send
>>> patches on top.
>>
>> I'm not fully sure how it all works.
>
> Same here, so I pinged Arseniy who worked on that, since it seemed deliberate to have `can_zcopy` (and set `uarg->zerocopy` accordingly) only when it was supported by the transport.
>
>> Especially the paths where msg->msg_ubuf is non-NULL, I suspect it should
>> be added to all the skb even if the ZEROCOPY flag isn't set.
>> I was just reading the one function.
>> But there did look like some very dodgy conditionals.
>
> I see, let's wait for Arseniy's feedback; otherwise, I'll try to fix it next week. As mentioned, this issue existed before this patch, so it shouldn't be a regression.
>
> Thanks,
> Stefano
>
^ permalink raw reply
* Re: [PATCH] drm/virtio: add timeout to virtqueue wait to avoid hung task
From: Ryosuke Yasuoka @ 2026-05-19 8:00 UTC (permalink / raw)
To: Dmitry Osipenko, David Airlie, Gerd Hoffmann, Gurchetan Singh,
Chia-I Wu, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Simona Vetter
Cc: dri-devel, virtualization, linux-kernel
In-Reply-To: <70e966d1-ca74-4f5c-8527-9e34ffe19095@collabora.com>
Thank you for the suggestions.
On 19/05/2026 09:11, Dmitry Osipenko wrote:
> On 5/18/26 09:45, Ryosuke Yasuoka wrote:
>>> It may be acceptable to have wait_event_timeout() in a loop, printing
>>> warnings about unresponsive host.
>> I considered this approach, but it does not solve the recovery problem
>> described above. The guest would still be stuck in the loop with no way
>> to remove the device or shut down gracefully.
>
> Could `system_state != SYSTEM_RUNNING` be checked in the wait loop? This
> may allow to handle system shutdown, aborting the timed out wait in the
> special case.
Instead of checking system_state directly, I'm planning to add a driver
flag (e.g. vqs_released) to struct virtio_gpu_device. The shutdown path
(virtio_gpu_shutdown) will set this flag and wake up the wait queues
before calling drm_dev_unplug(). The wait loop checks the flag and
aborts if set. This may cover both the shutdown and the unbind cases
with a sinble check, so I believe a separate system_state check is not
needed. If my plan turns out to have issues, I will use system_state
check.
> Userspace may also get stuck in a zombie state. For that, code should
> use wait_event_interruptible(). But driver has code unprepared to be
> interrupted, it all needs to be reworked first [1].
Agreed. I will leave wait_event_interruptible() for follow-up once the
interruptible is prepared. For now, userspace stuck in D state remains a
pre-exsiting limitation.
> Similarly, for unbind there could a driver flag that the wait loop will
> check and handle specially.
Yes, the driver flag described above will handle this as well.
Best regards,
Ryosuke
> WDYT?
>
> [1]
> https://lore.kernel.org/dri-devel/20260515084030.21986-1-kartikey406@gmail.com/
^ permalink raw reply
* [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Deepanshu Kartikey @ 2026-05-19 8:22 UTC (permalink / raw)
To: airlied, kraxel, dmitry.osipenko, gurchetansingh, olvaffe,
maarten.lankhorst, mripard, tzimmermann, simona, sumit.semwal,
christian.koenig
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, Deepanshu Kartikey, syzbot+72bd3dd3a5d5f39a0271,
stable
virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
ignore its return value. The function can fail with -EINTR from
dma_resv_lock_interruptible() (signal during lock wait) or with
-ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
leaving the resv lock not held. The queue path then walks the object
array and calls dma_resv_add_fence(), which requires the lock held;
with lockdep enabled this trips dma_resv_assert_held():
WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
Call Trace:
virtio_gpu_array_add_fence
virtio_gpu_queue_ctrl_sgs
virtio_gpu_queue_fenced_ctrl_buffer
virtio_gpu_cursor_plane_update
drm_atomic_helper_commit_planes
drm_atomic_helper_commit_tail
commit_tail
drm_atomic_helper_commit
drm_atomic_commit
drm_atomic_helper_update_plane
__setplane_atomic
drm_mode_cursor_universal
drm_mode_cursor_common
drm_mode_cursor_ioctl
drm_ioctl
__x64_sys_ioctl
Beyond the WARN, mutating the dma_resv fence list without the lock
races with concurrent readers/writers and can corrupt the list.
Both call sites run inside the .atomic_update plane callback, which
DRM atomic helpers do not allow to fail (by the time it runs, the
commit has been signed off to userspace and there is no clean
rollback path). Moving the lock acquisition to .prepare_fb was
rejected because the broader lock scope deadlocks against other BO
locking paths in the same atomic commit.
Introduce virtio_gpu_lock_one_resv_uninterruptible() that uses
dma_resv_lock() instead of dma_resv_lock_interruptible(). This
eliminates the -EINTR failure mode -- the realistic syzbot trigger
-- without extending the lock hold across the commit. The helper
locks a single BO and rejects nents > 1 with -EINVAL; both fix
sites lock exactly one BO.
Use it from virtio_gpu_cursor_plane_update() and
virtio_gpu_resource_flush(); check the return value to handle the
remaining -ENOMEM case from dma_resv_reserve_fences() by freeing
the objs and skipping the plane update for that frame. The
framebuffer BOs touched here are not shared with other contexts
and lock contention is expected to be brief, so the loss of
signal-interruptibility is acceptable.
Other callers of virtio_gpu_array_lock_resv() (the ioctl paths)
continue to use the interruptible variant.
The bug was reported by syzbot, triggered via fault injection
(fail_nth) on the DRM_IOCTL_MODE_CURSOR path, which forces the
-ENOMEM branch in dma_resv_reserve_fences().
Reported-by: syzbot+72bd3dd3a5d5f39a0271@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=72bd3dd3a5d5f39a0271
Fixes: 5cfd31c5b3a3 ("drm/virtio: fix virtio_gpu_cursor_plane_update().")
Cc: stable@vger.kernel.org
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
v4: Rename the helper to virtio_gpu_lock_one_resv_uninterruptible()
and reject objs->nents > 1 with -EINVAL. The v3 helper's
multi-object branch used drm_gem_lock_reservations(), which is
interruptible, contradicting the "uninterruptible" name; both
fix sites lock a single BO so the multi-object path is dropped.
(Dmitry Osipenko)
v3: Drop the prepare_fb/cleanup_fb approach from v2 (it deadlocked
against virtio_gpu_resource_flush(), which also locks the BO in
the same atomic commit). Instead add an uninterruptible variant
of the resv lock helper and use it in both
virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush().
(Dmitry Osipenko)
v2: Move resv lock acquisition from .atomic_update (which must not
fail) to .prepare_fb (which may), per maintainer review of v1.
The v1 approach of silently skipping the cursor update on lock
failure violated the atomic-commit contract with userspace.
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
drivers/gpu/drm/virtio/virtgpu_gem.c | 17 +++++++++++++++++
drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++--
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index f17660a71a3e..2f3531950aa4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -317,6 +317,7 @@ virtio_gpu_array_from_handles(struct drm_file *drm_file, u32 *handles, u32 nents
void virtio_gpu_array_add_obj(struct virtio_gpu_object_array *objs,
struct drm_gem_object *obj);
int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs);
+int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs);
void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs);
void virtio_gpu_array_add_fence(struct virtio_gpu_object_array *objs,
struct dma_fence *fence);
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index f22dc5c21cd4..435d37d36034 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -238,6 +238,23 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
return ret;
}
+int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs)
+{
+ int ret;
+
+ if (objs->nents != 1)
+ return -EINVAL;
+
+ dma_resv_lock(objs->objs[0]->resv, NULL);
+
+ ret = dma_resv_reserve_fences(objs->objs[0]->resv, 1);
+ if (ret) {
+ virtio_gpu_array_unlock_resv(objs);
+ return ret;
+ }
+ return 0;
+}
+
void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs)
{
if (objs->nents == 1) {
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a126d1b25f46..652352424744 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -215,7 +215,10 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
if (!objs)
return;
virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
- virtio_gpu_array_lock_resv(objs);
+ if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
+ virtio_gpu_array_put_free(objs);
+ return;
+ }
virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
width, height, objs,
vgplane_st->fence);
@@ -459,7 +462,10 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
if (!objs)
return;
virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
- virtio_gpu_array_lock_resv(objs);
+ if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
+ virtio_gpu_array_put_free(objs);
+ return;
+ }
virtio_gpu_cmd_transfer_to_host_2d
(vgdev, 0,
plane->state->crtc_w,
--
2.43.0
^ permalink raw reply related
* Re: [Linaro-mm-sig] Re: [PATCH v3] drm/virtio: use uninterruptible resv lock for plane updates
From: Deepanshu Kartikey @ 2026-05-19 8:24 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: airlied, kraxel, gurchetansingh, olvaffe, maarten.lankhorst,
mripard, tzimmermann, simona, sumit.semwal, christian.koenig,
dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <afec1199-4889-4d35-964c-4432ec792fa3@collabora.com>
On Sun, May 17, 2026 at 9:44 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 5/15/26 11:40, Deepanshu Kartikey wrote:
> > +int virtio_gpu_array_lock_resv_uninterruptible(struct virtio_gpu_object_array *objs)
> > +{
> > + unsigned int i;
> > + int ret = 0;
> > +
> > + if (objs->nents == 1) {
> > + dma_resv_lock(objs->objs[0]->resv, NULL);
> > + } else {
> > + ret = drm_gem_lock_reservations(objs->objs, objs->nents,
> > + &objs->ticket);
>
> drm_gem_lock_reservations() is interruptible. Given that only one BO
> needs to be locked for the fix, make it
> virtio_gpu_lock_one_resv_uninterruptible() and fail with -EINVAL if
> objs->nents > 1
>
> --
> Best regards,
> Dmitry
>
I have sent patch v4.
Thanks
Deepanshu
^ permalink raw reply
* Re: [PATCH v4] drm/virtio: use uninterruptible resv lock for plane updates
From: Christian König @ 2026-05-19 8:27 UTC (permalink / raw)
To: Deepanshu Kartikey, airlied, kraxel, dmitry.osipenko,
gurchetansingh, olvaffe, maarten.lankhorst, mripard, tzimmermann,
simona, sumit.semwal
Cc: dri-devel, virtualization, linux-kernel, linux-media,
linaro-mm-sig, syzbot+72bd3dd3a5d5f39a0271, stable
In-Reply-To: <20260519082247.34470-1-kartikey406@gmail.com>
On 5/19/26 10:22, Deepanshu Kartikey wrote:
> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush() lock
> the framebuffer BO's dma_resv via virtio_gpu_array_lock_resv() and
> ignore its return value. The function can fail with -EINTR from
> dma_resv_lock_interruptible() (signal during lock wait) or with
> -ENOMEM from dma_resv_reserve_fences() (fence slot allocation),
> leaving the resv lock not held. The queue path then walks the object
> array and calls dma_resv_add_fence(), which requires the lock held;
> with lockdep enabled this trips dma_resv_assert_held():
>
> WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
> Call Trace:
> virtio_gpu_array_add_fence
> virtio_gpu_queue_ctrl_sgs
> virtio_gpu_queue_fenced_ctrl_buffer
> virtio_gpu_cursor_plane_update
> drm_atomic_helper_commit_planes
> drm_atomic_helper_commit_tail
> commit_tail
> drm_atomic_helper_commit
> drm_atomic_commit
> drm_atomic_helper_update_plane
> __setplane_atomic
> drm_mode_cursor_universal
> drm_mode_cursor_common
> drm_mode_cursor_ioctl
> drm_ioctl
> __x64_sys_ioctl
>
> Beyond the WARN, mutating the dma_resv fence list without the lock
> races with concurrent readers/writers and can corrupt the list.
Well why are you trying to add a fence on an atomic mode set in the first place?
That is usually an illegal operation here.
Regards,
Christian.
>
> Both call sites run inside the .atomic_update plane callback, which
> DRM atomic helpers do not allow to fail (by the time it runs, the
> commit has been signed off to userspace and there is no clean
> rollback path). Moving the lock acquisition to .prepare_fb was
> rejected because the broader lock scope deadlocks against other BO
> locking paths in the same atomic commit.
>
> Introduce virtio_gpu_lock_one_resv_uninterruptible() that uses
> dma_resv_lock() instead of dma_resv_lock_interruptible(). This
> eliminates the -EINTR failure mode -- the realistic syzbot trigger
> -- without extending the lock hold across the commit. The helper
> locks a single BO and rejects nents > 1 with -EINVAL; both fix
> sites lock exactly one BO.
>
> Use it from virtio_gpu_cursor_plane_update() and
> virtio_gpu_resource_flush(); check the return value to handle the
> remaining -ENOMEM case from dma_resv_reserve_fences() by freeing
> the objs and skipping the plane update for that frame. The
> framebuffer BOs touched here are not shared with other contexts
> and lock contention is expected to be brief, so the loss of
> signal-interruptibility is acceptable.
>
> Other callers of virtio_gpu_array_lock_resv() (the ioctl paths)
> continue to use the interruptible variant.
>
> The bug was reported by syzbot, triggered via fault injection
> (fail_nth) on the DRM_IOCTL_MODE_CURSOR path, which forces the
> -ENOMEM branch in dma_resv_reserve_fences().
>
> Reported-by: syzbot+72bd3dd3a5d5f39a0271@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=72bd3dd3a5d5f39a0271
> Fixes: 5cfd31c5b3a3 ("drm/virtio: fix virtio_gpu_cursor_plane_update().")
> Cc: stable@vger.kernel.org
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
> v4: Rename the helper to virtio_gpu_lock_one_resv_uninterruptible()
> and reject objs->nents > 1 with -EINVAL. The v3 helper's
> multi-object branch used drm_gem_lock_reservations(), which is
> interruptible, contradicting the "uninterruptible" name; both
> fix sites lock a single BO so the multi-object path is dropped.
> (Dmitry Osipenko)
> v3: Drop the prepare_fb/cleanup_fb approach from v2 (it deadlocked
> against virtio_gpu_resource_flush(), which also locks the BO in
> the same atomic commit). Instead add an uninterruptible variant
> of the resv lock helper and use it in both
> virtio_gpu_cursor_plane_update() and virtio_gpu_resource_flush().
> (Dmitry Osipenko)
> v2: Move resv lock acquisition from .atomic_update (which must not
> fail) to .prepare_fb (which may), per maintainer review of v1.
> The v1 approach of silently skipping the cursor update on lock
> failure violated the atomic-commit contract with userspace.
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
> drivers/gpu/drm/virtio/virtgpu_gem.c | 17 +++++++++++++++++
> drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++--
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index f17660a71a3e..2f3531950aa4 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -317,6 +317,7 @@ virtio_gpu_array_from_handles(struct drm_file *drm_file, u32 *handles, u32 nents
> void virtio_gpu_array_add_obj(struct virtio_gpu_object_array *objs,
> struct drm_gem_object *obj);
> int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs);
> +int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs);
> void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs);
> void virtio_gpu_array_add_fence(struct virtio_gpu_object_array *objs,
> struct dma_fence *fence);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index f22dc5c21cd4..435d37d36034 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -238,6 +238,23 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
> return ret;
> }
>
> +int virtio_gpu_lock_one_resv_uninterruptible(struct virtio_gpu_object_array *objs)
> +{
> + int ret;
> +
> + if (objs->nents != 1)
> + return -EINVAL;
> +
> + dma_resv_lock(objs->objs[0]->resv, NULL);
> +
> + ret = dma_resv_reserve_fences(objs->objs[0]->resv, 1);
> + if (ret) {
> + virtio_gpu_array_unlock_resv(objs);
> + return ret;
> + }
> + return 0;
> +}
> +
> void virtio_gpu_array_unlock_resv(struct virtio_gpu_object_array *objs)
> {
> if (objs->nents == 1) {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index a126d1b25f46..652352424744 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -215,7 +215,10 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
> if (!objs)
> return;
> virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
> - virtio_gpu_array_lock_resv(objs);
> + if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
> + virtio_gpu_array_put_free(objs);
> + return;
> + }
> virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
> width, height, objs,
> vgplane_st->fence);
> @@ -459,7 +462,10 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
> if (!objs)
> return;
> virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
> - virtio_gpu_array_lock_resv(objs);
> + if (virtio_gpu_lock_one_resv_uninterruptible(objs)) {
> + virtio_gpu_array_put_free(objs);
> + return;
> + }
> virtio_gpu_cmd_transfer_to_host_2d
> (vgdev, 0,
> plane->state->crtc_w,
^ permalink raw reply
* Re: [PATCH net v4 0/2] vsock/virtio: fix skb overhead accounting to preserve full buf_alloc
From: Stefano Garzarella @ 2026-05-19 9:27 UTC (permalink / raw)
To: netdev
Cc: Simon Horman, Jakub Kicinski, Michael S. Tsirkin, Paolo Abeni,
Jason Wang, David S. Miller, kvm, Stefan Hajnoczi, linux-kernel,
Eric Dumazet, Xuan Zhuo, virtualization, Eugenio Pérez
In-Reply-To: <20260518090656.134588-1-sgarzare@redhat.com>
On Mon, May 18, 2026 at 11:06:54AM +0200, Stefano Garzarella wrote:
>Patch 1 resets the connection when we can no longer queue packets,
>this prevents silent data loss, and both peers are notified.
>
>Patch 2 increases the total budget to `buf_alloc * 2` for payload
>plus skb overhead similar to how SO_RCVBUF is doubled to reserve
>space for sk_buff metadata. This preserves the full buf_alloc for
>payload under normal operation, while still bounding the skb queue
>growth.
>
>In the future, we plan to improve how we handle the merging of packets
>to minimize overhead and avoid closing connections.
>
>v4:
>- Split the buf_alloc check to be sure the credit is still respected and
> to avoid overflow of buf_used [sashiko]
>- call virtio_transport_do_close() and vsock_remove_sock() to properly
> close the connection and remove the socket from the connect table
> [sashiko]
sashiko reports 2 issues on the second patch but both IMO are
pre-existing:
https://sashiko.dev/#/patchset/20260518090656.134588-1-sgarzare%40redhat.com
The first one related 32-bit architectures should be fixed, but can be a
follow up since not introduced by this series.
The secong one related to a low value set by the user as buffer size,
it's also pre-existing and I don't think we can do much.
Please let me know if you prefer to add another patch to this series to
fix the first issue, or just send another patch.
Thanks,
Stefano
>
>v3: https://lore.kernel.org/netdev/20260513105417.56761-1-sgarzare@redhat.com/
>- Split in 2 patches [MST]
>
>v2: https://lore.kernel.org/netdev/20260512080737.36787-1-sgarzare@redhat.com/
>- Close the connection when we can no longer queue new packets instead
> of losing data.
>- No longer announce the reduced buf_alloc to avoid violating the
> spec. [MST]
>
>v1: https://lore.kernel.org/netdev/20260508092330.69690-1-sgarzare@redhat.com/
>
>Stefano Garzarella (2):
> vsock/virtio: reset connection on receiving queue overflow
> vsock/virtio: fix skb overhead accounting to preserve full buf_alloc
>
> net/vmw_vsock/virtio_transport_common.c | 29 ++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
>--
>2.54.0
>
^ permalink raw reply
* Re: [PATCH net] vsock: keep poll shutdown state consistent
From: Stefano Garzarella @ 2026-05-19 9:36 UTC (permalink / raw)
To: ziyu zhang
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andy King, George Zhang, Dmitry Torokhov,
virtualization, netdev, linux-kernel, baijiaju1990, r33s3n6,
gality369, zhenghaoran154, hanguidong02, zzzccc427
In-Reply-To: <CACPoYJx6szY6HcvBf+aNr5+BgrxNNJM0su-qKxzp0qW+cJWO9w@mail.gmail.com>
On Mon, May 18, 2026 at 08:39:37PM +0800, ziyu zhang wrote:
>On Mon, 18 May 2026 at 18:16, Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Sat, May 16, 2026 at 11:47:45AM +0800, Ziyu Zhang wrote:
>> >vsock_poll() reads vsk->peer_shutdown before taking the socket
>> >lock to set EPOLLHUP and EPOLLRDHUP, then reads it again under the
>> >lock to report EOF readability. A shutdown packet can update
>> >peer_shutdown while poll is waiting for the lock, so one poll invocation
>> >can report EPOLLIN without the corresponding HUP/RDHUP bits.
>> >
>> >Keep non-connectible sockets on a single lockless READ_ONCE()
>>
>> Should this be paired with WRITE_ONCE() on writers?
>
>Yes, since the poll path uses lockless READ_ONCE() snapshots of
>peer_shutdown, the writer side should be annotated with WRITE_ONCE() as
>well. I will add that in v2.
>
>>
>> >snapshot. For connectible sockets, defer shutdown-derived poll bits
>> >until after lock_sock() and use one READ_ONCE() snapshot for both EOF
>> >readability and HUP/RDHUP. This preserves shutdowns that arrive before
>> >the lock is acquired and keeps all peer-shutdown-derived bits consistent
>> >for a poll pass.
>> >
>> >Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
>> >Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
>> >---
>> > net/vmw_vsock/af_vsock.c | 40 ++++++++++++++++++++++++++--------------
>> > 1 file changed, 26 insertions(+), 14 deletions(-)
>> >
>> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> >index adcba1b7b..bed42347b 100644
>> >--- a/net/vmw_vsock/af_vsock.c
>> >+++ b/net/vmw_vsock/af_vsock.c
>> >@@ -1122,6 +1122,25 @@ static int vsock_shutdown(struct socket *sock, int mode)
>> > return err;
>> > }
>> >
>> >+static __poll_t vsock_poll_shutdown(struct sock *sk, u32 peer_shutdown)
>> >+{
>> >+ __poll_t mask = 0;
>> >+
>> >+ /* INET sockets treat local write shutdown and peer write shutdown as a
>> >+ * case of EPOLLHUP set.
>> >+ */
>> >+ if (sk->sk_shutdown == SHUTDOWN_MASK ||
>> >+ ((sk->sk_shutdown & SEND_SHUTDOWN) &&
>> >+ (peer_shutdown & SEND_SHUTDOWN)))
>> >+ mask |= EPOLLHUP;
>> >+
>> >+ if (sk->sk_shutdown & RCV_SHUTDOWN ||
>> >+ peer_shutdown & SEND_SHUTDOWN)
>> >+ mask |= EPOLLRDHUP;
>> >+
>> >+ return mask;
>> >+}
>> >+
>> > static __poll_t vsock_poll(struct file *file, struct socket *sock,
>> > poll_table *wait)
>> > {
>> >@@ -1139,19 +1158,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
>> > /* Signify that there has been an error on this socket. */
>> > mask |= EPOLLERR;
>> >
>> >- /* INET sockets treat local write shutdown and peer write shutdown as a
>> >- * case of EPOLLHUP set.
>> >- */
>> >- if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
>> >- ((sk->sk_shutdown & SEND_SHUTDOWN) &&
>> >- (vsk->peer_shutdown & SEND_SHUTDOWN))) {
>> >- mask |= EPOLLHUP;
>> >- }
>> >-
>> >- if (sk->sk_shutdown & RCV_SHUTDOWN ||
>> >- vsk->peer_shutdown & SEND_SHUTDOWN) {
>> >- mask |= EPOLLRDHUP;
>> >- }
>> >+ if (!sock_type_connectible(sk->sk_type))
>> >+ mask |= vsock_poll_shutdown(sk,
>> >+ READ_ONCE(vsk->peer_shutdown));
>>
>> Can we move this in the `if (sock->type == SOCK_DGRAM)` branch ?
>>
>> Not a strong opinion about that, but in any case IMO we should add a
>> comment here to explain why we are doing only for not connectible
>> sockets.
>>
>> That said, if we use WRITE_ONCE in the writers, do we really need to
>> move this after the lock_sock for the connectable ones?
>
>Yes, I will move the non-connectible handling into the SOCK_DGRAM branch
>and add a comment there.
>
>For connectible sockets, my current understanding is that
>READ_ONCE()/WRITE_ONCE() make the individual lockless accesses explicit, but
>they do not make two separate reads in one vsock_poll() invocation observe the
>same peer_shutdown value. So I still think using one peer_shutdown snapshot
>after lock_sock() is useful for keeping the returned mask internally
>consistent. Please let me know if you think WRITE_ONCE() is enough for this
>case.
What will be the issue of "do not make two separate reads in one
vsock_poll() invocation observe the same peer_shutdown value." ?
In any case, I'm not against it; I just want to understand the issue
better.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH net v2] vsock/vmci: fix UAF when peer resets connection during handshake
From: Stefano Garzarella @ 2026-05-19 9:41 UTC (permalink / raw)
To: Paolo Abeni, Minh Nguyen, Bryan Tan
Cc: Minh Nguyen, Bryan Tan, Vishnu Dasa, David S . Miller,
Eric Dumazet, Jakub Kicinski, Simon Horman,
bcm-kernel-feedback-list, netdev, virtualization, linux-kernel,
stable
In-Reply-To: <3518e2b5-b669-4aaa-82ca-bbf479a85889@redhat.com>
On Thu, May 14, 2026 at 03:26:28PM +0200, Paolo Abeni wrote:
>On 5/12/26 4:58 AM, Minh Nguyen wrote:
>> vmci_transport_recv_connecting_server() jumps to its destroy: label
>> and performs an unconditional sock_put(pending) to release the
>> explicit sock_hold() taken by vmci_transport_recv_listen() before
>> schedule_delayed_work(). The existing comment claimed this was safe
>> because the listen handler removes pending from the pending list on
>> the way out, which would prevent vsock_pending_work() from dropping
>> the same reference later.
>
[...]
>Sashiko says:
>
>---
>Could this change lead to a socket memory leak if another packet arrives
>before vsock_pending_work() executes?
>If a peer RST is received (err == 0), the socket stays on the
>pending_links list with its state set to TCP_CLOSE, and the base
>reference is kept.
>If the peer then sends another packet (such as another RST) within the
>delay window before vsock_pending_work() runs,
>vmci_transport_get_pending() might find this same socket.
>Since its state is TCP_CLOSE, vmci_transport_recv_listen() would hit the
>default switch case, set err = -EINVAL, and call vsock_remove_pending().
>This removes the socket from the list and drops the list reference, but
>it bypasses vmci_transport_recv_connecting_server(), meaning the base
>reference is never dropped.
>When vsock_pending_work() runs later, vsock_is_pending() evaluates to false.
>This sets cleanup = false and bypasses the sock_put(sk) call, leaking
>the pending socket.
>While not introduced by this patch, does this error path leak
>sk_ack_backlog slots on failed handshakes?
>If a handshake fails due to an error, vmci_transport_recv_listen()
>handles it by calling vsock_remove_pending(). This removes the socket
>from the pending_links list but does not call sk_acceptq_removed(sk).
>When vsock_pending_work() runs later, vsock_is_pending() evaluates to
>false because the socket is no longer in the list. This causes the work
>function to skip its own sk_acceptq_removed(listener) call, meaning the
>listener's sk_ack_backlog is never decremented.
>---
>
>it looks like the above is trading an UaF for a leak ?!?
>
@Minh @Bryan can you check this report?
It seems a real issue, so the patch was not applied.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH net] vsock/virtio: fix zerocopy completion for multi-skb sends
From: Stefano Garzarella @ 2026-05-19 9:49 UTC (permalink / raw)
To: Arseniy Krasnov
Cc: David Laight, Michael S. Tsirkin, netdev, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, kvm, Eric Dumazet,
Eugenio Pérez, Xuan Zhuo, virtualization, David S. Miller,
Jason Wang, linux-kernel, Maher Azzouzi
In-Reply-To: <aea2227c-3011-4457-b909-d16de1596a6d@salutedevices.com>
On Tue, May 19, 2026 at 09:37:23AM +0300, Arseniy Krasnov wrote:
>On 18/05/2026 14:08, Stefano Garzarella wrote:
>> On Mon, May 18, 2026 at 11:50:05AM +0100, David Laight wrote:
>>> On Mon, 18 May 2026 11:54:19 +0200
>>> Stefano Garzarella <sgarzare@redhat.com> wrote:
[...]
>
>Hi guys! Just some replies after quick look:
>
>>>> >> > Surely that block should only be done if can_zcopy is true?
>
>I guess no, since TCP also allocates uarg even if zerocopy is impossible -
>it just sets uarg_to_msgzc(uarg)->zerocopy = 0;
>
>>>> >> > And shouldn't something unset it if info->op != VIRTIO_VSOCK_OP_RW ?
>
>Hm, we can't enter block 'if (info->msg) {' when 'info->op' is not equal to 'VIRTIO_VSOCK_OP_RW',
>because 'msg' is not NULL only for 'VIRTIO_VSOCK_OP_RW'. But anyway You point to right thing - check for
>'VIRTIO_VSOCK_OP_RW' could be removed here. Just with comment why.
>
>>>> >> > If the msg_zerocopy_realloc() fails then can't you just set can_zcopy to false.
>
>Here I guess it is better to follow TCP way to make same behaviour, because exact
>logic for MSG_ZEROCOPY is not documented. TCP also returns error and stops tx loop.
>
>>>> >> >
>>>> >> > It info->msg->msg_buf is already set then I think you have to disable zero-copy.
>>>> >> > The caller has already requested a callback - and you can't add another.
>
>In TCP implementation if 'msg_ubuf' is set they just use it and check for zerocopy in
>the same way as 'msg_ubuf' is NULL.
>
>>>> >> >
>>>> >> > In any case by the end of this can_zcopy and have_uref are really the same flag.
>>>> >>
>
>Need to check it more. But 'can_zcopy' means that we fill frags in skb, have_uref means that we
>allocated completion (but it could be reported with not set 'SO_EE_CODE_ZEROCOPY_COPIED' if
>'can_zcopy' was false).
>
>
>@Stefano, I guess current implementation differs from TCP in two cases (at least from first
>view):
>1) When 'msg_ubuf' is set: in TCP, already set 'msg_ubuf' is passed to 'skb_zerocopy_iter_stream()'
> (if zerocopy is possible) where it is used as in vsock in call 'skb_zcopy_set()'. In vsock case if
> 'msg_ubuf' is not NULL we will pass just NULL to 'skb_zcopy_set()'. Yes this is will be
> no-effect call today (due to checks in 'skb_zcopy_set()'), but anyway - in future may be not.
>2) Also i see that 'skb_zerocopy_iter_stream()' in TCP version has some
>extra checks which are
> missed in vsock - we only just call '__zerocopy_sg_from_iter()' to fill skb in zerocopy way.
>
>But, I think, instead of trying to compare vsock and TCP versions best way is to just copy
>current TCP flow as close as possible:
>https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/net/ipv4/tcp.c#n1144
>1) Use same flow of checks for 'msg_flags', 'msg_ubuf', SOCK_ZEROCOPY etc.
>2) To copy data we can use 'skb_zerocopy_iter_stream()'.
>3) The only thing that we don't need in vsock is dev mem related code from TCP implementation.
>
>I can take this task, but pls need some time - may be two/three weeks due to another tasks.
>
>What do You think?
If you can work on it, please go head. They seems all pre-existing, so
2/3 weeks are fine. Please let me know if you can't and I'll try to
allocate some time.
Thanks for the help!
Stefano
^ permalink raw reply
* [PATCH net v3] vsock/vmci: fix UAF when peer resets connection during handshake
From: Minh Nguyen @ 2026-05-19 10:16 UTC (permalink / raw)
To: pabeni, bryan-bt.tan
Cc: sgarzare, vishnu.dasa, davem, edumazet, kuba, horms,
bcm-kernel-feedback-list, netdev, virtualization, linux-kernel,
stable
In-Reply-To: <agwv3YkxYIC7mvyj@sgarzare-redhat>
vmci_transport_recv_connecting_server() returned err = 0 for a peer
RST in its default switch arm:
err = pkt->type == VMCI_TRANSPORT_PACKET_TYPE_RST ? 0 : -EINVAL;
That made vmci_transport_recv_listen() skip vsock_remove_pending(),
leaving the pending socket on the listener's pending_links with
sk_state = TCP_CLOSE while destroy: still dropped the explicit
reference taken before schedule_delayed_work().
One second later vsock_pending_work() observed is_pending=true and
performed full cleanup: vsock_remove_pending() then the two trailing
sock_put(sk) calls -- the first reached refcount 0 and __sk_freed
the socket, and the second wrote into the freed object:
BUG: KASAN: slab-use-after-free in refcount_warn_saturate
Write of size 4 at addr ffff88800b1cac80 by task kworker
Workqueue: events vsock_pending_work
Treat peer RST like any other unexpected packet type (err = -EINVAL).
All destroy: arms now return err < 0, so vmci_transport_recv_listen()
removes pending from pending_links synchronously and
vsock_pending_work() takes the is_pending=false / !rejected branch,
dropping only its own work reference. This also closes the
multi-packet race Sashiko reported on v2: pending is removed from
the list before any subsequent packet can find it.
The pre-existing sk_acceptq_removed() gap on the err < 0 path of
vmci_transport_recv_listen() that Sashiko also noted is not
introduced or changed by this patch.
Tested on lts-6.12.79 with KASAN: 52/100 unpatched -> 0/100 patched.
Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Cc: stable@vger.kernel.org
Signed-off-by: Minh Nguyen <minhnguyen.080505@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
v3:
- Different approach to Sashiko/Paolo's "trading UAF for leak"
concern: normalize RST to err = -EINVAL so all destroy: arms
take the same err < 0 cleanup path -- no special case, no
multi-packet race.
- Sashiko's secondary observation ("while not introduced by this
patch, does this error path leak sk_ack_backlog slots on failed
handshakes?") is correct: the sk_acceptq_removed() gap on the
err < 0 branch of vmci_transport_recv_listen() is pre-existing
and is not introduced or changed by this patch. v3 stays
focused on the UAF; a separate fix for that gap is needed and
would be welcome from anyone closer to that area.
v2: https://lore.kernel.org/netdev/20260512025851.189140-1-minhnguyen.080505@gmail.com/
v1 was sent to security@kernel.org on 2026-05-10 (not on lore).
net/vmw_vsock/vmci_transport.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 4296ca1..ba3a66e 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1161,10 +1161,17 @@ vmci_transport_recv_connecting_server(struct sock *listener,
}
break;
default:
- /* Close and cleanup the connection. */
+ /* Close and cleanup the connection. Peer RST is treated like
+ * any other unexpected packet type in this state so that the
+ * pending socket follows the same cleanup path as other
+ * handshake failures, instead of being left on the pending
+ * list for vsock_pending_work() to find later (which races
+ * with subsequent packets and was the source of a UAF when
+ * the cleanup work observed an inconsistent ref count).
+ */
vmci_transport_send_reset(pending, pkt);
skerr = EPROTO;
- err = pkt->type == VMCI_TRANSPORT_PACKET_TYPE_RST ? 0 : -EINVAL;
+ err = -EINVAL;
goto destroy;
}
base-commit: be48e5fe51a5864566307998286a699d6b986934
--
2.54.0
^ permalink raw reply related
* [PATCH net v4] vsock/vmci: fix UAF when peer resets connection during handshake
From: Minh Nguyen @ 2026-05-19 10:23 UTC (permalink / raw)
To: pabeni, bryan-bt.tan
Cc: sgarzare, vishnu.dasa, davem, edumazet, kuba, horms,
bcm-kernel-feedback-list, netdev, virtualization, linux-kernel,
stable
vmci_transport_recv_connecting_server() returned err = 0 for a peer
RST in its default switch arm:
err = pkt->type == VMCI_TRANSPORT_PACKET_TYPE_RST ? 0 : -EINVAL;
That made vmci_transport_recv_listen() skip vsock_remove_pending(),
leaving the pending socket on the listener's pending_links with
sk_state = TCP_CLOSE while destroy: still dropped the explicit
reference taken before schedule_delayed_work().
One second later vsock_pending_work() observed is_pending=true and
performed full cleanup: vsock_remove_pending() then the two trailing
sock_put(sk) calls -- the first reached refcount 0 and __sk_freed
the socket, and the second wrote into the freed object:
BUG: KASAN: slab-use-after-free in refcount_warn_saturate
Write of size 4 at addr ffff88800b1cac80 by task kworker
Workqueue: events vsock_pending_work
Treat peer RST like any other unexpected packet type (err = -EINVAL).
All destroy: arms now return err < 0, so vmci_transport_recv_listen()
removes pending from pending_links synchronously and
vsock_pending_work() takes the is_pending=false / !rejected branch,
dropping only its own work reference. This also closes the
multi-packet race Sashiko reported on v2: pending is removed from
the list before any subsequent packet can find it.
The pre-existing sk_acceptq_removed() gap on the err < 0 path of
vmci_transport_recv_listen() that Sashiko also noted is not
introduced or changed by this patch.
Tested on lts-6.12.79 with KASAN: 52/100 unpatched -> 0/100 patched.
Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Cc: stable@vger.kernel.org
Signed-off-by: Minh Nguyen <minhnguyen.080505@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
v4:
- Resend as an independent thread per netdev workflow (v3 was
incorrectly posted in-reply-to the v2 thread).
- Drop the inline comment expansion; keep the original
/* Close and cleanup the connection. */. No functional change.
v3:
- Different approach to Sashiko/Paolo's "trading UAF for leak"
concern: normalize RST to err = -EINVAL so all destroy: arms
take the same err < 0 cleanup path -- no special case, no
multi-packet race.
- Sashiko's secondary observation ("while not introduced by this
patch, does this error path leak sk_ack_backlog slots on failed
handshakes?") is correct: the sk_acceptq_removed() gap on the
err < 0 branch of vmci_transport_recv_listen() is pre-existing
and is not introduced or changed by this patch. A separate fix
for that gap is needed and would be welcome.
v2: https://lore.kernel.org/netdev/20260512025851.189140-1-minhnguyen.080505@gmail.com/
v1 was sent to security@kernel.org on 2026-05-10 (not on lore).
net/vmw_vsock/vmci_transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 4296ca1..d257938 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1164,7 +1164,7 @@ vmci_transport_recv_connecting_server(struct sock *listener,
/* Close and cleanup the connection. */
vmci_transport_send_reset(pending, pkt);
skerr = EPROTO;
- err = pkt->type == VMCI_TRANSPORT_PACKET_TYPE_RST ? 0 : -EINVAL;
+ err = -EINVAL;
goto destroy;
}
base-commit: be48e5fe51a5864566307998286a699d6b986934
--
2.54.0
^ permalink raw reply related
* Re: [PATCH net] vsock/virtio: fix zerocopy completion for multi-skb sends
From: Arseniy Krasnov @ 2026-05-19 10:40 UTC (permalink / raw)
To: Stefano Garzarella
Cc: David Laight, Michael S. Tsirkin, netdev, Jakub Kicinski,
Paolo Abeni, Simon Horman, Stefan Hajnoczi, kvm, Eric Dumazet,
Eugenio Pérez, Xuan Zhuo, virtualization, David S. Miller,
Jason Wang, linux-kernel, Maher Azzouzi
In-Reply-To: <agwwxuQlIWjwXtMN@sgarzare-redhat>
On 19/05/2026 12:49, Stefano Garzarella wrote:
> On Tue, May 19, 2026 at 09:37:23AM +0300, Arseniy Krasnov wrote:
>> On 18/05/2026 14:08, Stefano Garzarella wrote:
>>> On Mon, May 18, 2026 at 11:50:05AM +0100, David Laight wrote:
>>>> On Mon, 18 May 2026 11:54:19 +0200
>>>> Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> [...]
>
>>
>> Hi guys! Just some replies after quick look:
>>
>>>>> >> > Surely that block should only be done if can_zcopy is true?
>>
>> I guess no, since TCP also allocates uarg even if zerocopy is impossible -
>> it just sets uarg_to_msgzc(uarg)->zerocopy = 0;
>>
>>>>> >> > And shouldn't something unset it if info->op != VIRTIO_VSOCK_OP_RW ?
>>
>> Hm, we can't enter block 'if (info->msg) {' when 'info->op' is not equal to 'VIRTIO_VSOCK_OP_RW',
>> because 'msg' is not NULL only for 'VIRTIO_VSOCK_OP_RW'. But anyway You point to right thing - check for
>> 'VIRTIO_VSOCK_OP_RW' could be removed here. Just with comment why.
>>
>>>>> >> > If the msg_zerocopy_realloc() fails then can't you just set can_zcopy to false.
>>
>> Here I guess it is better to follow TCP way to make same behaviour, because exact
>> logic for MSG_ZEROCOPY is not documented. TCP also returns error and stops tx loop.
>>
>>>>> >> >
>>>>> >> > It info->msg->msg_buf is already set then I think you have to disable zero-copy.
>>>>> >> > The caller has already requested a callback - and you can't add another.
>>
>> In TCP implementation if 'msg_ubuf' is set they just use it and check for zerocopy in
>> the same way as 'msg_ubuf' is NULL.
>>
>>>>> >> >
>>>>> >> > In any case by the end of this can_zcopy and have_uref are really the same flag.
>>>>> >>
>>
>> Need to check it more. But 'can_zcopy' means that we fill frags in skb, have_uref means that we
>> allocated completion (but it could be reported with not set 'SO_EE_CODE_ZEROCOPY_COPIED' if
>> 'can_zcopy' was false).
>>
>>
>> @Stefano, I guess current implementation differs from TCP in two cases (at least from first
>> view):
>> 1) When 'msg_ubuf' is set: in TCP, already set 'msg_ubuf' is passed to 'skb_zerocopy_iter_stream()'
>> (if zerocopy is possible) where it is used as in vsock in call 'skb_zcopy_set()'. In vsock case if
>> 'msg_ubuf' is not NULL we will pass just NULL to 'skb_zcopy_set()'. Yes this is will be
>> no-effect call today (due to checks in 'skb_zcopy_set()'), but anyway - in future may be not.
>> 2) Also i see that 'skb_zerocopy_iter_stream()' in TCP version has some extra checks which are
>> missed in vsock - we only just call '__zerocopy_sg_from_iter()' to fill skb in zerocopy way.
>>
>> But, I think, instead of trying to compare vsock and TCP versions best way is to just copy
>> current TCP flow as close as possible:
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/net/ipv4/tcp.c#n1144
>> 1) Use same flow of checks for 'msg_flags', 'msg_ubuf', SOCK_ZEROCOPY etc.
>> 2) To copy data we can use 'skb_zerocopy_iter_stream()'.
>> 3) The only thing that we don't need in vsock is dev mem related code from TCP implementation.
>>
>> I can take this task, but pls need some time - may be two/three weeks due to another tasks.
>>
>> What do You think?
>
> If you can work on it, please go head. They seems all pre-existing, so 2/3 weeks are fine. Please let me know if you can't and I'll try to allocate some time.
No problem I'll take it. If something goes wrong or stuck - i'll let you know
Thanks
>
> Thanks for the help!
>
> Stefano
>
^ permalink raw reply
* Re: [PATCH net] vsock: keep poll shutdown state consistent
From: ziyu zhang @ 2026-05-19 15:58 UTC (permalink / raw)
To: Stefano Garzarella
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andy King, George Zhang, Dmitry Torokhov,
virtualization, netdev, linux-kernel, baijiaju1990, r33s3n6,
gality369, zhenghaoran154, hanguidong02, zzzccc427
In-Reply-To: <agwugKMrTsNxQ2BD@sgarzare-redhat>
On Tue, 19 May 2026 at 17:36, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, May 18, 2026 at 08:39:37PM +0800, ziyu zhang wrote:
> >On Mon, 18 May 2026 at 18:16, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Sat, May 16, 2026 at 11:47:45AM +0800, Ziyu Zhang wrote:
> >> >vsock_poll() reads vsk->peer_shutdown before taking the socket
> >> >lock to set EPOLLHUP and EPOLLRDHUP, then reads it again under the
> >> >lock to report EOF readability. A shutdown packet can update
> >> >peer_shutdown while poll is waiting for the lock, so one poll invocation
> >> >can report EPOLLIN without the corresponding HUP/RDHUP bits.
> >> >
> >> >Keep non-connectible sockets on a single lockless READ_ONCE()
> >>
> >> Should this be paired with WRITE_ONCE() on writers?
> >
> >Yes, since the poll path uses lockless READ_ONCE() snapshots of
> >peer_shutdown, the writer side should be annotated with WRITE_ONCE() as
> >well. I will add that in v2.
> >
> >>
> >> >snapshot. For connectible sockets, defer shutdown-derived poll bits
> >> >until after lock_sock() and use one READ_ONCE() snapshot for both EOF
> >> >readability and HUP/RDHUP. This preserves shutdowns that arrive before
> >> >the lock is acquired and keeps all peer-shutdown-derived bits consistent
> >> >for a poll pass.
> >> >
> >> >Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
> >> >Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
> >> >---
> >> > net/vmw_vsock/af_vsock.c | 40 ++++++++++++++++++++++++++--------------
> >> > 1 file changed, 26 insertions(+), 14 deletions(-)
> >> >
> >> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >> >index adcba1b7b..bed42347b 100644
> >> >--- a/net/vmw_vsock/af_vsock.c
> >> >+++ b/net/vmw_vsock/af_vsock.c
> >> >@@ -1122,6 +1122,25 @@ static int vsock_shutdown(struct socket *sock, int mode)
> >> > return err;
> >> > }
> >> >
> >> >+static __poll_t vsock_poll_shutdown(struct sock *sk, u32 peer_shutdown)
> >> >+{
> >> >+ __poll_t mask = 0;
> >> >+
> >> >+ /* INET sockets treat local write shutdown and peer write shutdown as a
> >> >+ * case of EPOLLHUP set.
> >> >+ */
> >> >+ if (sk->sk_shutdown == SHUTDOWN_MASK ||
> >> >+ ((sk->sk_shutdown & SEND_SHUTDOWN) &&
> >> >+ (peer_shutdown & SEND_SHUTDOWN)))
> >> >+ mask |= EPOLLHUP;
> >> >+
> >> >+ if (sk->sk_shutdown & RCV_SHUTDOWN ||
> >> >+ peer_shutdown & SEND_SHUTDOWN)
> >> >+ mask |= EPOLLRDHUP;
> >> >+
> >> >+ return mask;
> >> >+}
> >> >+
> >> > static __poll_t vsock_poll(struct file *file, struct socket *sock,
> >> > poll_table *wait)
> >> > {
> >> >@@ -1139,19 +1158,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> >> > /* Signify that there has been an error on this socket. */
> >> > mask |= EPOLLERR;
> >> >
> >> >- /* INET sockets treat local write shutdown and peer write shutdown as a
> >> >- * case of EPOLLHUP set.
> >> >- */
> >> >- if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
> >> >- ((sk->sk_shutdown & SEND_SHUTDOWN) &&
> >> >- (vsk->peer_shutdown & SEND_SHUTDOWN))) {
> >> >- mask |= EPOLLHUP;
> >> >- }
> >> >-
> >> >- if (sk->sk_shutdown & RCV_SHUTDOWN ||
> >> >- vsk->peer_shutdown & SEND_SHUTDOWN) {
> >> >- mask |= EPOLLRDHUP;
> >> >- }
> >> >+ if (!sock_type_connectible(sk->sk_type))
> >> >+ mask |= vsock_poll_shutdown(sk,
> >> >+ READ_ONCE(vsk->peer_shutdown));
> >>
> >> Can we move this in the `if (sock->type == SOCK_DGRAM)` branch ?
> >>
> >> Not a strong opinion about that, but in any case IMO we should add a
> >> comment here to explain why we are doing only for not connectible
> >> sockets.
> >>
> >> That said, if we use WRITE_ONCE in the writers, do we really need to
> >> move this after the lock_sock for the connectable ones?
> >
> >Yes, I will move the non-connectible handling into the SOCK_DGRAM branch
> >and add a comment there.
> >
> >For connectible sockets, my current understanding is that
> >READ_ONCE()/WRITE_ONCE() make the individual lockless accesses explicit, but
> >they do not make two separate reads in one vsock_poll() invocation observe the
> >same peer_shutdown value. So I still think using one peer_shutdown snapshot
> >after lock_sock() is useful for keeping the returned mask internally
> >consistent. Please let me know if you think WRITE_ONCE() is enough for this
> >case.
>
> What will be the issue of "do not make two separate reads in one
> vsock_poll() invocation observe the same peer_shutdown value." ?
>
> In any case, I'm not against it; I just want to understand the issue
> better.
The issue I was trying to avoid is a transiently inconsistent poll mask
from one vsock_poll() pass.
For example, the early peer_shutdown read can see 0, so the
shutdown-derived bits are not set, especially EPOLLRDHUP, and EPOLLHUP
in the cases where the existing logic would set it. Then, before
lock_sock() succeeds, the peer shutdown can be processed. The later read
after lock_sock() can see SEND_SHUTDOWN and set EPOLLIN|EPOLLRDNORM for
EOF readability.
So the returned mask can say that EOF is readable, but miss the
shutdown-derived indication from the same peer_shutdown state. If
userspace is waiting specifically for EPOLLRDHUP, that notification can
be missed or delayed for that poll pass.
I agree this is small and transient. A following read() or another poll
pass will observe the shutdown state. My intention with the single
snapshot is only to avoid mixing old and new peer_shutdown values in one
returned mask.
I will send a v2 as a new thread with WRITE_ONCE() on the writers, the
DGRAM comment, and your ordering suggestion.
Thanks,
Ziyu
>
> Thanks,
> Stefano
>
^ permalink raw reply
* [PATCH v2] virtio_console: fix race between hvc put_chars and virtqueue teardown on freeze
From: Sungho Bae @ 2026-05-19 16:22 UTC (permalink / raw)
To: amit, arnd, gregkh; +Cc: virtualization, linux-kernel, Sungho Bae
From: Sungho Bae <baver.bae@lge.com>
With no_console_suspend enabled, hvc console output can continue while
virtio_console is freezing. In that window, put_chars can still enqueue
buffers to the output virtqueue while virtcons_freeze is tearing queues
down, triggering a BUG_ON in virtqueue_detach_unused_buf_split:
BUG_ON(vq->vq.num_free != vq->split.vring.num)
Add a pm_freezing flag to ports_device. Set it via smp_store_release()
at the start of virtcons_freeze(); put_chars() and __send_to_port() drop
output while the flag is set, checked via smp_load_acquire().
The check in __send_to_port() is placed under outvq_lock, making it
atomic with remove_port_data() which also acquires outvq_lock. Once
remove_port_data() returns for a given port, no concurrent
__send_to_port() can add buffers before remove_vqs() tears down the vq.
After setting pm_freezing, acquire and release outvq_lock for each port
before calling virtio_reset_device(). A TX thread that already passed
the pm_freezing check may still hold outvq_lock while spinning for host
acknowledgment (the nonblock=false hvc path); the drain loop ensures
all such threads have completed before the device is reset.
Clear pm_freezing in virtcons_restore() only after all port->out_vq
pointers have been reassigned to the newly allocated virtqueues,
preventing TX paths from dereferencing freed vqs during restore.
Also fix two races in __send_to_port() uncovered by this work: load
port->portdev via READ_ONCE() and check for NULL to guard against
concurrent hot-unplug, and move out_vq = port->out_vq inside
outvq_lock after the pm_freezing check to avoid a stale pointer.
Link: https://sashiko.dev/#/patchset/20260515225259.1054-1-baver.bae%40gmail.com
Signed-off-by: Sungho Bae <baver.bae@lge.com>
---
drivers/char/virtio_console.c | 80 ++++++++++++++++++++++++++++++++++-
1 file changed, 78 insertions(+), 2 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9a33217c68d9..fad673f733a8 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -157,6 +157,12 @@ struct ports_device {
/* Major number for this device. Ports will be created as minors. */
int chr_major;
+
+ /*
+ * Set to true during PM freeze to block TX paths that may race
+ * with virtqueue teardown (e.g. hvc put_chars with no_console_suspend).
+ */
+ bool pm_freezing;
};
struct port_stats {
@@ -601,11 +607,30 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int err;
unsigned long flags;
unsigned int len;
-
- out_vq = port->out_vq;
+ struct ports_device *portdev;
spin_lock_irqsave(&port->outvq_lock, flags);
+ portdev = READ_ONCE(port->portdev);
+
+ if (!portdev) {
+ in_count = 0;
+ goto done;
+ }
+
+ /*
+ * Check freeze flag under the lock so that the flag check and
+ * virtqueue_add_outbuf() are atomic with respect to
+ * remove_port_data() which also takes outvq_lock. This
+ * guarantees that once remove_port_data() returns, no new
+ * buffers can be added before remove_vqs() tears down the vq.
+ * Pairs with smp_store_release() in virtcons_freeze/restore.
+ */
+ if (smp_load_acquire(&portdev->pm_freezing)) /* pairs with freeze/restore */
+ goto done;
+
+ out_vq = port->out_vq;
+
reclaim_consumed_buffers(port);
err = virtqueue_add_outbuf(out_vq, sg, nents, data, GFP_ATOMIC);
@@ -1110,11 +1135,36 @@ static ssize_t put_chars(u32 vtermno, const u8 *buf, size_t count)
struct scatterlist sg[1];
void *data;
int ret;
+ struct ports_device *portdev;
port = find_port_by_vtermno(vtermno);
if (!port)
return -EPIPE;
+ /*
+ * Silently drop output in two cases, both by returning count so
+ * that the hvc layer does not spin-retry:
+ *
+ * 1. Device hot-unplug (!portdev): portdev was NULLed by
+ * unplug_port() after hvc_remove() was already called, so
+ * the hvc layer will stop invoking put_chars() very soon.
+ * Returning count avoids a pointless retry loop in the
+ * interim.
+ *
+ * 2. PM freeze (pm_freezing): the hvc console stays active
+ * under no_console_suspend but virtqueues are being torn
+ * down. Drop the output silently so the hvc layer does not
+ * stall suspend.
+ *
+ * This early check avoids a pointless GFP_ATOMIC allocation;
+ * __send_to_port() rechecks under outvq_lock for correctness.
+ * Pairs with smp_store_release() in virtcons_freeze/restore.
+ */
+ portdev = READ_ONCE(port->portdev);
+ if (!portdev ||
+ smp_load_acquire(&portdev->pm_freezing)) /* pairs with freeze/restore */
+ return count;
+
data = kmemdup(buf, count, GFP_ATOMIC);
if (!data)
return -ENOMEM;
@@ -1972,6 +2022,7 @@ static int virtcons_probe(struct virtio_device *vdev)
/* Attach this portdev to this virtio_device, and vice-versa. */
portdev->vdev = vdev;
vdev->priv = portdev;
+ portdev->pm_freezing = false;
portdev->chr_major = register_chrdev(0, "virtio-portsdev",
&portdev_fops);
@@ -2092,6 +2143,24 @@ static int virtcons_freeze(struct virtio_device *vdev)
portdev = vdev->priv;
+ /*
+ * Block TX paths (put_chars, __send_to_port) before resetting the
+ * device and tearing down virtqueues. This prevents races with
+ * hvc console writes that remain active under no_console_suspend.
+ */
+ smp_store_release(&portdev->pm_freezing, true);
+
+ /*
+ * Synchronize with any concurrent __send_to_port() that may have
+ * passed the pm_freezing check. By acquiring and releasing the
+ * outvq_lock for each port, we ensure all active TX paths have
+ * completed before we reset the device.
+ */
+ list_for_each_entry(port, &portdev->ports, list) {
+ spin_lock_irq(&port->outvq_lock);
+ spin_unlock_irq(&port->outvq_lock);
+ }
+
virtio_reset_device(vdev);
if (use_multiport(portdev))
@@ -2153,6 +2222,13 @@ static int virtcons_restore(struct virtio_device *vdev)
if (port->guest_connected)
send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
}
+
+ /*
+ * Allow TX paths only after all port->out_vq pointers have
+ * been reassigned to the newly allocated virtqueues.
+ */
+ smp_store_release(&portdev->pm_freezing, false);
+
return 0;
}
#endif
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox