[PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device

Abhin Parekadan Jose posted 2 patches 2 months ago
drivers/virtio/virtio_pci_modern.c     |  6 +++++-
drivers/virtio/virtio_pci_modern_dev.c |  8 +++++++-
include/uapi/linux/virtio_config.h     | 16 ++++++++++++++++
3 files changed, 28 insertions(+), 2 deletions(-)
[PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device
Posted by Abhin Parekadan Jose 2 months ago
While investigating a syzbot report of a WARN_ON_ONCE firing in
virtio_dev_remove() [1], I found a related but more serious issue:
vp_reset() in the modern virtio-pci transport can hang indefinitely
if PCI_COMMAND memory-space decode is disabled while the device is
bound (e.g. surprise removal, hardware fault, or -- as reproduced
here -- a direct write to the PCI_COMMAND register). The status
register poll loop has no way to distinguish "device still resetting"
from "device unreachable," so it never terminates.

Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
all-ones status read as invalid (per spec, bits 4-5 are reserved and
can never legitimately be set) and warns once at the point the bad
read actually happens.

Patch 2 uses that check to break out of vp_reset()'s poll loop
instead of spinning forever.

Reproduced on a modern-transport virtio-blk-pci device:

  # printf '\x00\x00' | dd of=/sys/bus/pci/devices/0000:01:00.0/config \
    bs=1 seek=4 count=2 conv=notrunc
  # echo 1 > /sys/bus/pci/devices/0000:01:00.0/remove

The second command hangs indefinitely without this series; gdb
confirms the CPU is stuck in vp_reset()'s status-polling loop, with
vp_modern_get_status() consistently returning 0xff.

[1] https://syzbot.org/bug?extid=a1c8effc62c569d4bd25

Abhin Parekadan Jose (2):
  virtio_pci_modern_dev: warn once on invalid status
  virtio_pci_modern: avoid infinite loop in vp_reset() on invalid status

 drivers/virtio/virtio_pci_modern.c     |  6 +++++-
 drivers/virtio/virtio_pci_modern_dev.c |  8 +++++++-
 include/uapi/linux/virtio_config.h     | 16 ++++++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

--
2.51.1
Re: [PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device
Posted by Michael S. Tsirkin 2 months ago
On Sun, Aug 02, 2026 at 05:40:57PM +0000, Abhin Parekadan Jose wrote:
> While investigating a syzbot report of a WARN_ON_ONCE firing in
> virtio_dev_remove() [1],


And I responded to that syzbot report, and I quote:

So it writes 0 into pci command, effectively killing the device,
and then is unhappy that the driver prints warnings?
Who thought it's a good idea? Why?


> I found a related but more serious issue:
> vp_reset() in the modern virtio-pci transport can hang indefinitely
> if PCI_COMMAND memory-space decode is disabled while the device is
> bound (e.g. surprise removal, hardware fault, or -- as reproduced
> here -- a direct write to the PCI_COMMAND register). The status
> register poll loop has no way to distinguish "device still resetting"
> from "device unreachable," so it never terminates.
> 
> Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
> all-ones status read as invalid (per spec, bits 4-5 are reserved and
> can never legitimately be set) and warns once at the point the bad
> read actually happens.
> 
> Patch 2 uses that check to break out of vp_reset()'s poll loop
> instead of spinning forever.

Was all this including the cover letter written with ai assistance?
if yes pls disclose this.

> Reproduced on a modern-transport virtio-blk-pci device:
> 
>   # printf '\x00\x00' | dd of=/sys/bus/pci/devices/0000:01:00.0/config \
>     bs=1 seek=4 count=2 conv=notrunc
>   # echo 1 > /sys/bus/pci/devices/0000:01:00.0/remove
> 
> The second command hangs indefinitely without this series; gdb
> confirms the CPU is stuck in vp_reset()'s status-polling loop, with
> vp_modern_get_status() consistently returning 0xff.
> 
> [1] https://syzbot.org/bug?extid=a1c8effc62c569d4bd25
> 
> Abhin Parekadan Jose (2):
>   virtio_pci_modern_dev: warn once on invalid status
>   virtio_pci_modern: avoid infinite loop in vp_reset() on invalid status
> 
>  drivers/virtio/virtio_pci_modern.c     |  6 +++++-
>  drivers/virtio/virtio_pci_modern_dev.c |  8 +++++++-
>  include/uapi/linux/virtio_config.h     | 16 ++++++++++++++++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> --
> 2.51.1
Re: [PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device
Posted by Abhin Parekadan Jose 2 months ago
On Sun, Aug 02, 2026 at 01:47:01PM -0400, Michael S. Tsirkin wrote:
> On Sun, Aug 02, 2026 at 05:40:57PM +0000, Abhin Parekadan Jose wrote:
> > While investigating a syzbot report of a WARN_ON_ONCE firing in
> > virtio_dev_remove() [1],
>
>
> And I responded to that syzbot report, and I quote:
>
> So it writes 0 into pci command, effectively killing the device,
> and then is unhappy that the driver prints warnings?
> Who thought it's a good idea? Why?

I was learning how to reproduce syzbot bugs when I found this
issue by writing 0 to PCI_COMMAND to simulate an unresponsive
device. While doing that I noticed that echo 1 > /sys/../remove
hung completely rather than just printing the warning. Since the
device_status register lives in the virtio common config MMIO
space and has defined values(based on the bits set) in the spec.
I thought it made sense for virtio to detect this and handle it
gracefully rather than spin forever, so I wrote up a small fix
for that.

> > I found a related but more serious issue:
> > vp_reset() in the modern virtio-pci transport can hang indefinitely
> > if PCI_COMMAND memory-space decode is disabled while the device is
> > bound (e.g. surprise removal, hardware fault, or -- as reproduced
> > here -- a direct write to the PCI_COMMAND register). The status
> > register poll loop has no way to distinguish "device still resetting"
> > from "device unreachable," so it never terminates.
> >
> > Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
> > all-ones status read as invalid (per spec, bits 4-5 are reserved and
> > can never legitimately be set) and warns once at the point the bad
> > read actually happens.
> >
> > Patch 2 uses that check to break out of vp_reset()'s poll loop
> > instead of spinning forever.
>
> Was all this including the cover letter written with ai assistance?
> if yes pls disclose this.

Yes, I used AI assistance (Claude). The commit messages were written
by me and then refined with AI for spelling and grammar; the cover
letter was generated by Claude and reviewed by me. The code, testing,
and debugging were done by me -- I reproduced the hang in QEMU,
debugged to reach the hanging loop, and wrote the actual fix.

I should have disclosed this upfront. I'll do so in future
submissions.

Do I need to add Assisted-by: Claude <claude-4-6-sonnet> to the
commit messages?

P.S. This is my first kernel patch set.
Re: [PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device
Posted by Michael S. Tsirkin 2 months ago
On Sun, Aug 02, 2026 at 06:28:03PM +0000, Abhin Parekadan Jose wrote:
> On Sun, Aug 02, 2026 at 01:47:01PM -0400, Michael S. Tsirkin wrote:
> > On Sun, Aug 02, 2026 at 05:40:57PM +0000, Abhin Parekadan Jose wrote:
> > > While investigating a syzbot report of a WARN_ON_ONCE firing in
> > > virtio_dev_remove() [1],
> >
> >
> > And I responded to that syzbot report, and I quote:
> >
> > So it writes 0 into pci command, effectively killing the device,
> > and then is unhappy that the driver prints warnings?
> > Who thought it's a good idea? Why?
> 
> I was learning how to reproduce syzbot bugs when I found this
> issue by writing 0 to PCI_COMMAND to simulate an unresponsive
> device.

Yea I have no idea where does this syzbot "bug report"
come from. Poking at random at device registers is ... not
a very good idea.

> While doing that I noticed that echo 1 > /sys/../remove
> hung completely rather than just printing the warning. Since the
> device_status register lives in the virtio common config MMIO
> space and has defined values(based on the bits set) in the spec.
> I thought it made sense for virtio to detect this and handle it
> gracefully rather than spin forever, so I wrote up a small fix
> for that.
> 
> > > I found a related but more serious issue:
> > > vp_reset() in the modern virtio-pci transport can hang indefinitely
> > > if PCI_COMMAND memory-space decode is disabled while the device is
> > > bound (e.g. surprise removal, hardware fault, or -- as reproduced
> > > here -- a direct write to the PCI_COMMAND register). The status
> > > register poll loop has no way to distinguish "device still resetting"
> > > from "device unreachable," so it never terminates.
> > >
> > > Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
> > > all-ones status read as invalid (per spec, bits 4-5 are reserved and
> > > can never legitimately be set) and warns once at the point the bad
> > > read actually happens.
> > >
> > > Patch 2 uses that check to break out of vp_reset()'s poll loop
> > > instead of spinning forever.
> >
> > Was all this including the cover letter written with ai assistance?
> > if yes pls disclose this.
> 
> Yes, I used AI assistance (Claude). The commit messages were written
> by me and then refined with AI for spelling and grammar; the cover
> letter was generated by Claude and reviewed by me.

I suggest limiting it to fixing spelling and grammar exclusively.  It
tends to do things like dramatize, e.g. "more serious issue", like it
did here.

> The code, testing,
> and debugging were done by me -- I reproduced the hang in QEMU,
> debugged to reach the hanging loop, and wrote the actual fix.
> 
> I should have disclosed this upfront. I'll do so in future
> submissions.
> 
> Do I need to add Assisted-by: Claude <claude-4-6-sonnet> to the
> commit messages?

    Assisted-by: Claude:claude-sonnet-4-6


> 
> P.S. This is my first kernel patch set.


Thanks, keep at it. Bonus points if you find a real fix for
issues raised in thread about surprise removal, see e.g. here
cover.1752094439.git.mst@redhat.com
but don't expect it to be easy.


-- 
MST
Re: [PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device
Posted by Abhin Parekadan Jose 2 months ago
On Sun, Aug 02, 2026 at 03:08:12PM -0400, Michael S. Tsirkin wrote:
> On Sun, Aug 02, 2026 at 06:28:03PM +0000, Abhin Parekadan Jose wrote:
> > On Sun, Aug 02, 2026 at 01:47:01PM -0400, Michael S. Tsirkin wrote:
> > > On Sun, Aug 02, 2026 at 05:40:57PM +0000, Abhin Parekadan Jose wrote:
> > > > While investigating a syzbot report of a WARN_ON_ONCE firing in
> > > > virtio_dev_remove() [1],
> > >
> > >
> > > And I responded to that syzbot report, and I quote:
> > >
> > > So it writes 0 into pci command, effectively killing the device,
> > > and then is unhappy that the driver prints warnings?
> > > Who thought it's a good idea? Why?
> > 
> > I was learning how to reproduce syzbot bugs when I found this
> > issue by writing 0 to PCI_COMMAND to simulate an unresponsive
> > device.
> 
> Yea I have no idea where does this syzbot "bug report"
> come from. Poking at random at device registers is ... not
> a very good idea.
> 
> > While doing that I noticed that echo 1 > /sys/../remove
> > hung completely rather than just printing the warning. Since the
> > device_status register lives in the virtio common config MMIO
> > space and has defined values(based on the bits set) in the spec.
> > I thought it made sense for virtio to detect this and handle it
> > gracefully rather than spin forever, so I wrote up a small fix
> > for that.
> > 
> > > > I found a related but more serious issue:
> > > > vp_reset() in the modern virtio-pci transport can hang indefinitely
> > > > if PCI_COMMAND memory-space decode is disabled while the device is
> > > > bound (e.g. surprise removal, hardware fault, or -- as reproduced
> > > > here -- a direct write to the PCI_COMMAND register). The status
> > > > register poll loop has no way to distinguish "device still resetting"
> > > > from "device unreachable," so it never terminates.
> > > >
> > > > Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
> > > > all-ones status read as invalid (per spec, bits 4-5 are reserved and
> > > > can never legitimately be set) and warns once at the point the bad
> > > > read actually happens.
> > > >
> > > > Patch 2 uses that check to break out of vp_reset()'s poll loop
> > > > instead of spinning forever.
> > >
> > > Was all this including the cover letter written with ai assistance?
> > > if yes pls disclose this.
> > 
> > Yes, I used AI assistance (Claude). The commit messages were written
> > by me and then refined with AI for spelling and grammar; the cover
> > letter was generated by Claude and reviewed by me.
> 
> I suggest limiting it to fixing spelling and grammar exclusively.  It
> tends to do things like dramatize, e.g. "more serious issue", like it
> did here.
> 
> > The code, testing,
> > and debugging were done by me -- I reproduced the hang in QEMU,
> > debugged to reach the hanging loop, and wrote the actual fix.
> > 
> > I should have disclosed this upfront. I'll do so in future
> > submissions.
> > 
> > Do I need to add Assisted-by: Claude <claude-4-6-sonnet> to the
> > commit messages?
> 
>     Assisted-by: Claude:claude-sonnet-4-6
> 
> 
> > 
> > P.S. This is my first kernel patch set.
> 
> 
> Thanks, keep at it. Bonus points if you find a real fix for
> issues raised in thread about surprise removal, see e.g. here
> cover.1752094439.git.mst@redhat.com
> but don't expect it to be easy.
> 

That looks interesting (haven't gone through in detail but got a gist of it).
I'll try it out and make suggestions if I find a good solution.

As for the current patch set, does it make sense to drop macro
`VIRTIO_STATUS_ERROR` and use `PCI_POSSIBLE_ERROR` to break out of the loop,
I could test it by actually doing a surprise removal on qemu via the monitor
or just drop this patch set and try on cover.1752094439.git.mst@redhat.com patch set?

> -- 
> MST
>
Re: [PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device
Posted by Michael S. Tsirkin 2 months ago
On Sun, Aug 02, 2026 at 09:48:26PM +0200, Abhin Parekadan Jose wrote:
> On Sun, Aug 02, 2026 at 03:08:12PM -0400, Michael S. Tsirkin wrote:
> > On Sun, Aug 02, 2026 at 06:28:03PM +0000, Abhin Parekadan Jose wrote:
> > > On Sun, Aug 02, 2026 at 01:47:01PM -0400, Michael S. Tsirkin wrote:
> > > > On Sun, Aug 02, 2026 at 05:40:57PM +0000, Abhin Parekadan Jose wrote:
> > > > > While investigating a syzbot report of a WARN_ON_ONCE firing in
> > > > > virtio_dev_remove() [1],
> > > >
> > > >
> > > > And I responded to that syzbot report, and I quote:
> > > >
> > > > So it writes 0 into pci command, effectively killing the device,
> > > > and then is unhappy that the driver prints warnings?
> > > > Who thought it's a good idea? Why?
> > > 
> > > I was learning how to reproduce syzbot bugs when I found this
> > > issue by writing 0 to PCI_COMMAND to simulate an unresponsive
> > > device.
> > 
> > Yea I have no idea where does this syzbot "bug report"
> > come from. Poking at random at device registers is ... not
> > a very good idea.
> > 
> > > While doing that I noticed that echo 1 > /sys/../remove
> > > hung completely rather than just printing the warning. Since the
> > > device_status register lives in the virtio common config MMIO
> > > space and has defined values(based on the bits set) in the spec.
> > > I thought it made sense for virtio to detect this and handle it
> > > gracefully rather than spin forever, so I wrote up a small fix
> > > for that.
> > > 
> > > > > I found a related but more serious issue:
> > > > > vp_reset() in the modern virtio-pci transport can hang indefinitely
> > > > > if PCI_COMMAND memory-space decode is disabled while the device is
> > > > > bound (e.g. surprise removal, hardware fault, or -- as reproduced
> > > > > here -- a direct write to the PCI_COMMAND register). The status
> > > > > register poll loop has no way to distinguish "device still resetting"
> > > > > from "device unreachable," so it never terminates.
> > > > >
> > > > > Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
> > > > > all-ones status read as invalid (per spec, bits 4-5 are reserved and
> > > > > can never legitimately be set) and warns once at the point the bad
> > > > > read actually happens.
> > > > >
> > > > > Patch 2 uses that check to break out of vp_reset()'s poll loop
> > > > > instead of spinning forever.
> > > >
> > > > Was all this including the cover letter written with ai assistance?
> > > > if yes pls disclose this.
> > > 
> > > Yes, I used AI assistance (Claude). The commit messages were written
> > > by me and then refined with AI for spelling and grammar; the cover
> > > letter was generated by Claude and reviewed by me.
> > 
> > I suggest limiting it to fixing spelling and grammar exclusively.  It
> > tends to do things like dramatize, e.g. "more serious issue", like it
> > did here.
> > 
> > > The code, testing,
> > > and debugging were done by me -- I reproduced the hang in QEMU,
> > > debugged to reach the hanging loop, and wrote the actual fix.
> > > 
> > > I should have disclosed this upfront. I'll do so in future
> > > submissions.
> > > 
> > > Do I need to add Assisted-by: Claude <claude-4-6-sonnet> to the
> > > commit messages?
> > 
> >     Assisted-by: Claude:claude-sonnet-4-6
> > 
> > 
> > > 
> > > P.S. This is my first kernel patch set.
> > 
> > 
> > Thanks, keep at it. Bonus points if you find a real fix for
> > issues raised in thread about surprise removal, see e.g. here
> > cover.1752094439.git.mst@redhat.com
> > but don't expect it to be easy.
> > 
> 
> That looks interesting (haven't gone through in detail but got a gist of it).
> I'll try it out and make suggestions if I find a good solution.
> 
> As for the current patch set, does it make sense to drop macro
> `VIRTIO_STATUS_ERROR` and use `PCI_POSSIBLE_ERROR` to break out of the loop,
> I could test it by actually doing a surprise removal on qemu via the monitor
> or just drop this patch set

I'd drop this, I'm not interested in working around one source of hangs
if others in the same exact path remain unfixable.

> and try on cover.1752094439.git.mst@redhat.com patch set?

it's not a question of "trying it on" it's a question of the fact that
the pci core serializes probe/removal events so a driver inside
probe/remove never sees the removal event. in this instance it
is polling so it can check (at the cost of adding cpu overhead,
mostly for nothing) but in most places it can't, we need the event
to reach it.

> > -- 
> > MST
> >