RE: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

RE: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

[Cornelia Huck]

On Thu, Aug 28 2025, Parav Pandit <parav@nvidia.com> wrote:

>> From: Cornelia Huck <cohuck@redhat.com>
>> Sent: 27 August 2025 05:04 PM
>> 
>> On Wed, Aug 27 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Tue, Aug 26, 2025 at 06:52:03PM +0000, Parav Pandit wrote:
>> >> > What I do not understand, is what good does the revert do. Sorry.
>> >> >
>> >> Let me explain.
>> >> It prevents the issue of vblk requests being stuck due to broken VQ.
>> >> It prevents the vnet driver start_xmit() to be not stuck on skb completions.
>> >
>> > This is the part I don't get.  In what scenario, before 43bb40c5b9265
>> > start_xmit is not stuck, but after 43bb40c5b9265 it is stuck?
>> >
>> > Once the device is gone, it is not using any buffers at all.
>> 
>> What I also don't understand: virtio-ccw does exactly the same thing
>> (virtio_break_device(), added in 2014), and it supports surprise removal
>> _only_, yet I don't remember seeing bug reports?
>
> I suspect that stress testing may not have happened for ccw with active vblk Ios and outstanding transmit pkt and cvq commands.
> Hard to say as we don't have ccw hw or systems.

cc:ing linux-s390 list. I'd be surprised if nobody ever tested surprise
removal on a loaded system in the last 11 years.

Re: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

[Michael S. Tsirkin]

On Thu, Aug 28, 2025 at 02:16:28PM +0200, Cornelia Huck wrote:
> On Thu, Aug 28 2025, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: 27 August 2025 05:04 PM
> >> 
> >> On Wed, Aug 27 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > On Tue, Aug 26, 2025 at 06:52:03PM +0000, Parav Pandit wrote:
> >> >> > What I do not understand, is what good does the revert do. Sorry.
> >> >> >
> >> >> Let me explain.
> >> >> It prevents the issue of vblk requests being stuck due to broken VQ.
> >> >> It prevents the vnet driver start_xmit() to be not stuck on skb completions.
> >> >
> >> > This is the part I don't get.  In what scenario, before 43bb40c5b9265
> >> > start_xmit is not stuck, but after 43bb40c5b9265 it is stuck?
> >> >
> >> > Once the device is gone, it is not using any buffers at all.
> >> 
> >> What I also don't understand: virtio-ccw does exactly the same thing
> >> (virtio_break_device(), added in 2014), and it supports surprise removal
> >> _only_, yet I don't remember seeing bug reports?
> >
> > I suspect that stress testing may not have happened for ccw with active vblk Ios and outstanding transmit pkt and cvq commands.
> > Hard to say as we don't have ccw hw or systems.
> 
> cc:ing linux-s390 list. I'd be surprised if nobody ever tested surprise
> removal on a loaded system in the last 11 years.


As it became very clear from follow up discussion, the issue is nothing
to do with virtio, it is with a broken hypervisor that allows device to
DMA into guest memory while also telling the guest that the device has
been removed.

I guess s390 is just not broken like this.

-- 
MST

Re: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

[Cornelia Huck]

On Thu, Aug 28 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Aug 28, 2025 at 02:16:28PM +0200, Cornelia Huck wrote:
>> On Thu, Aug 28 2025, Parav Pandit <parav@nvidia.com> wrote:
>> 
>> >> From: Cornelia Huck <cohuck@redhat.com>
>> >> Sent: 27 August 2025 05:04 PM
>> >> 
>> >> On Wed, Aug 27 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> 
>> >> > On Tue, Aug 26, 2025 at 06:52:03PM +0000, Parav Pandit wrote:
>> >> >> > What I do not understand, is what good does the revert do. Sorry.
>> >> >> >
>> >> >> Let me explain.
>> >> >> It prevents the issue of vblk requests being stuck due to broken VQ.
>> >> >> It prevents the vnet driver start_xmit() to be not stuck on skb completions.
>> >> >
>> >> > This is the part I don't get.  In what scenario, before 43bb40c5b9265
>> >> > start_xmit is not stuck, but after 43bb40c5b9265 it is stuck?
>> >> >
>> >> > Once the device is gone, it is not using any buffers at all.
>> >> 
>> >> What I also don't understand: virtio-ccw does exactly the same thing
>> >> (virtio_break_device(), added in 2014), and it supports surprise removal
>> >> _only_, yet I don't remember seeing bug reports?
>> >
>> > I suspect that stress testing may not have happened for ccw with active vblk Ios and outstanding transmit pkt and cvq commands.
>> > Hard to say as we don't have ccw hw or systems.
>> 
>> cc:ing linux-s390 list. I'd be surprised if nobody ever tested surprise
>> removal on a loaded system in the last 11 years.
>
>
> As it became very clear from follow up discussion, the issue is nothing
> to do with virtio, it is with a broken hypervisor that allows device to
> DMA into guest memory while also telling the guest that the device has
> been removed.
>
> I guess s390 is just not broken like this.

Ah good, I missed that -- that indeed sounds broken, and needs to be
fixed there.

RE: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: 28 August 2025 05:52 PM
> 
> On Thu, Aug 28 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Aug 28, 2025 at 02:16:28PM +0200, Cornelia Huck wrote:
> >> On Thu, Aug 28 2025, Parav Pandit <parav@nvidia.com> wrote:
> >>
> >> >> From: Cornelia Huck <cohuck@redhat.com>
> >> >> Sent: 27 August 2025 05:04 PM
> >> >>
> >> >> On Wed, Aug 27 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >>
> >> >> > On Tue, Aug 26, 2025 at 06:52:03PM +0000, Parav Pandit wrote:
> >> >> >> > What I do not understand, is what good does the revert do. Sorry.
> >> >> >> >
> >> >> >> Let me explain.
> >> >> >> It prevents the issue of vblk requests being stuck due to broken VQ.
> >> >> >> It prevents the vnet driver start_xmit() to be not stuck on skb
> completions.
> >> >> >
> >> >> > This is the part I don't get.  In what scenario, before
> >> >> > 43bb40c5b9265 start_xmit is not stuck, but after 43bb40c5b9265 it is
> stuck?
> >> >> >
> >> >> > Once the device is gone, it is not using any buffers at all.
> >> >>
> >> >> What I also don't understand: virtio-ccw does exactly the same
> >> >> thing (virtio_break_device(), added in 2014), and it supports
> >> >> surprise removal _only_, yet I don't remember seeing bug reports?
> >> >
> >> > I suspect that stress testing may not have happened for ccw with active
> vblk Ios and outstanding transmit pkt and cvq commands.
> >> > Hard to say as we don't have ccw hw or systems.
> >>
> >> cc:ing linux-s390 list. I'd be surprised if nobody ever tested
> >> surprise removal on a loaded system in the last 11 years.
> >
> >
> > As it became very clear from follow up discussion, the issue is
> > nothing to do with virtio, it is with a broken hypervisor that allows
> > device to DMA into guest memory while also telling the guest that the
> > device has been removed.
> >
> > I guess s390 is just not broken like this.
> 
> Ah good, I missed that -- that indeed sounds broken, and needs to be fixed
> there.
Nop. This is not the issue. You missed this focused on fixing the device.

The fact is: the driver is expecting the IOs and CVQ commands and DMA to succeed even after device is removed.
The driver is expecting the device reset to also succeed.
Stefan already pointed out this in the vblk driver patches.

This is why you see call traces on del_gendisk(), CVQ commands.

Again, it is the broken drivers not the device.
Device can stop the DMA and stop responding to the requests and kernel 6.X will continue to hang as long as it has cited commit.

Re: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

[Michael S. Tsirkin]

On Thu, Aug 28, 2025 at 12:33:58PM +0000, Parav Pandit wrote:
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: 28 August 2025 05:52 PM
> > 
> > On Thu, Aug 28 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Aug 28, 2025 at 02:16:28PM +0200, Cornelia Huck wrote:
> > >> On Thu, Aug 28 2025, Parav Pandit <parav@nvidia.com> wrote:
> > >>
> > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > >> >> Sent: 27 August 2025 05:04 PM
> > >> >>
> > >> >> On Wed, Aug 27 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >> >>
> > >> >> > On Tue, Aug 26, 2025 at 06:52:03PM +0000, Parav Pandit wrote:
> > >> >> >> > What I do not understand, is what good does the revert do. Sorry.
> > >> >> >> >
> > >> >> >> Let me explain.
> > >> >> >> It prevents the issue of vblk requests being stuck due to broken VQ.
> > >> >> >> It prevents the vnet driver start_xmit() to be not stuck on skb
> > completions.
> > >> >> >
> > >> >> > This is the part I don't get.  In what scenario, before
> > >> >> > 43bb40c5b9265 start_xmit is not stuck, but after 43bb40c5b9265 it is
> > stuck?
> > >> >> >
> > >> >> > Once the device is gone, it is not using any buffers at all.
> > >> >>
> > >> >> What I also don't understand: virtio-ccw does exactly the same
> > >> >> thing (virtio_break_device(), added in 2014), and it supports
> > >> >> surprise removal _only_, yet I don't remember seeing bug reports?
> > >> >
> > >> > I suspect that stress testing may not have happened for ccw with active
> > vblk Ios and outstanding transmit pkt and cvq commands.
> > >> > Hard to say as we don't have ccw hw or systems.
> > >>
> > >> cc:ing linux-s390 list. I'd be surprised if nobody ever tested
> > >> surprise removal on a loaded system in the last 11 years.
> > >
> > >
> > > As it became very clear from follow up discussion, the issue is
> > > nothing to do with virtio, it is with a broken hypervisor that allows
> > > device to DMA into guest memory while also telling the guest that the
> > > device has been removed.
> > >
> > > I guess s390 is just not broken like this.
> > 
> > Ah good, I missed that -- that indeed sounds broken, and needs to be fixed
> > there.
> Nop. This is not the issue. You missed this focused on fixing the device.
> 
> The fact is: the driver is expecting the IOs and CVQ commands and DMA to succeed even after device is removed.
> The driver is expecting the device reset to also succeed.
> Stefan already pointed out this in the vblk driver patches.
> This is why you see call traces on del_gendisk(), CVQ commands.
> 
> Again, it is the broken drivers not the device.
> Device can stop the DMA and stop responding to the requests and kernel 6.X will continue to hang as long as it has cited commit.


Parav, the issues you cite are real but unrelated and will hang anyway
with or without the commit.

All you have to do is pull out the device while e.g. a command is in the
process of being submitted.

All the commit you want to revert does, is in some instances instead of
just hanging it will make queue as broken and release memory. Since you
device is not really gone and keeps DMAing into memory, guest memory
gets corrupted.


But your argument that the issue is that the fix is "incomplete" is
bogus - when we make the fix complete it will become even worse for
this broken devices.





-- 
MST

RE: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: 28 August 2025 06:31 PM
> 
> On Thu, Aug 28, 2025 at 12:33:58PM +0000, Parav Pandit wrote:
> >
> > > From: Cornelia Huck <cohuck@redhat.com>
> > > Sent: 28 August 2025 05:52 PM
> > >
> > > On Thu, Aug 28 2025, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Thu, Aug 28, 2025 at 02:16:28PM +0200, Cornelia Huck wrote:
> > > >> On Thu, Aug 28 2025, Parav Pandit <parav@nvidia.com> wrote:
> > > >>
> > > >> >> From: Cornelia Huck <cohuck@redhat.com>
> > > >> >> Sent: 27 August 2025 05:04 PM
> > > >> >>
> > > >> >> On Wed, Aug 27 2025, "Michael S. Tsirkin" <mst@redhat.com>
> wrote:
> > > >> >>
> > > >> >> > On Tue, Aug 26, 2025 at 06:52:03PM +0000, Parav Pandit wrote:
> > > >> >> >> > What I do not understand, is what good does the revert do.
> Sorry.
> > > >> >> >> >
> > > >> >> >> Let me explain.
> > > >> >> >> It prevents the issue of vblk requests being stuck due to broken
> VQ.
> > > >> >> >> It prevents the vnet driver start_xmit() to be not stuck on
> > > >> >> >> skb
> > > completions.
> > > >> >> >
> > > >> >> > This is the part I don't get.  In what scenario, before
> > > >> >> > 43bb40c5b9265 start_xmit is not stuck, but after
> > > >> >> > 43bb40c5b9265 it is
> > > stuck?
> > > >> >> >
> > > >> >> > Once the device is gone, it is not using any buffers at all.
> > > >> >>
> > > >> >> What I also don't understand: virtio-ccw does exactly the same
> > > >> >> thing (virtio_break_device(), added in 2014), and it supports
> > > >> >> surprise removal _only_, yet I don't remember seeing bug reports?
> > > >> >
> > > >> > I suspect that stress testing may not have happened for ccw
> > > >> > with active
> > > vblk Ios and outstanding transmit pkt and cvq commands.
> > > >> > Hard to say as we don't have ccw hw or systems.
> > > >>
> > > >> cc:ing linux-s390 list. I'd be surprised if nobody ever tested
> > > >> surprise removal on a loaded system in the last 11 years.
> > > >
> > > >
> > > > As it became very clear from follow up discussion, the issue is
> > > > nothing to do with virtio, it is with a broken hypervisor that
> > > > allows device to DMA into guest memory while also telling the
> > > > guest that the device has been removed.
> > > >
> > > > I guess s390 is just not broken like this.
> > >
> > > Ah good, I missed that -- that indeed sounds broken, and needs to be
> > > fixed there.
> > Nop. This is not the issue. You missed this focused on fixing the device.
> >
> > The fact is: the driver is expecting the IOs and CVQ commands and DMA to
> succeed even after device is removed.
> > The driver is expecting the device reset to also succeed.
> > Stefan already pointed out this in the vblk driver patches.
> > This is why you see call traces on del_gendisk(), CVQ commands.
> >
> > Again, it is the broken drivers not the device.
> > Device can stop the DMA and stop responding to the requests and kernel
> 6.X will continue to hang as long as it has cited commit.
> 
> 
> Parav, the issues you cite are real but unrelated and will hang anyway with or
> without the commit.
> 
How is it unrelated?

If it is going to hang anyway (in your view), and you proposed different callback etc as brand-new feature to Linux kernel, what is the objection to revert it?
As you pointed out it will be in multiple subsystems (net, block, pci) etc, why not do the proper work?

Reverting at least helps those stable kernels to operate smoothly as before.

> All you have to do is pull out the device while e.g. a command is in the
> process of being submitted.
> 

> All the commit you want to revert does, is in some instances instead of just
> hanging it will make queue as broken and release memory. Since you device is
> not really gone and keeps DMAing into memory, guest memory gets
> corrupted.
Nop. This is not the case.
What is "some instance"?
The virtio block driver is expecting the IOs to be completed without the cited commit.
As you listed cross subsystem callbacks, such infrastructure != fix.

So to make things clear, as discussed.
1. have proper kernel infrastructure in placed as you outlined the design using callback
2. have the spec update to make sure drivers negotiate its readiness for surprise removal and do not expect to access the device.

Until that point, restore the stability of stable kernels.

> 
> But your argument that the issue is that the fix is "incomplete" is bogus -

> when we make the fix complete it will become even worse for this broken
> devices.
I explained you that the device was doing the right thing and that is why exactly the call trace in the cited patch showed up.

Again quoting "broken device" is wrong.
The drivers are broken trying to reset the removed device.
And the ask is to do proper feature negotiation to get to that point.

When the reasonable workaround is suggested in previous email, you opted to not respond to it?
This is not how broken user experience is restored for stable kernel.

Are you waiting for the test results if it works?
If so, then yes, it makes sense.
I will of course test and submit proper v2.