linux-kernel - Re: [PATCH] virtio: Remove virtio devices on device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <pnpkqz6keacwz4myydc6aak6gkdmomqn2lo376przlhb3mscsq@tdsfxwb2ujpk>
Date: Thu, 6 Feb 2025 12:04:34 +0200
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: Eric Auger <eauger@...hat.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>, 
	Jason Wang <jasowang@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, 
	Eugenio Pérez <eperezma@...hat.com>, Zhenzhong Duan <zhenzhong.duan@...el.com>, 
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	Hongyu Ning <hongyu.ning@...ux.intel.com>
Subject: Re: [PATCH] virtio: Remove virtio devices on device_shutdown()

On Thu, Feb 06, 2025 at 09:59:58AM +0100, Eric Auger wrote:
> Hi,
> 
> On 2/4/25 12:46 PM, Eric Auger wrote:
> > Hi,
> > 
> > On 2/3/25 3:48 PM, Michael S. Tsirkin wrote:
> >> On Fri, Jan 31, 2025 at 10:53:15AM +0100, Eric Auger wrote:
> >>> Hi Kirill, Michael
> >>>
> >>> On 8/8/24 9:51 AM, Kirill A. Shutemov wrote:
> >>>> Hongyu reported a hang on kexec in a VM. QEMU reported invalid memory
> >>>> accesses during the hang.
> >>>>
> >>>> 	Invalid read at addr 0x102877002, size 2, region '(null)', reason: rejected
> >>>> 	Invalid write at addr 0x102877A44, size 2, region '(null)', reason: rejected
> >>>> 	...
> >>>>
> >>>> It was traced down to virtio-console. Kexec works fine if virtio-console
> >>>> is not in use.
> >>>>
> >>>> Looks like virtio-console continues to write to the MMIO even after
> >>>> underlying virtio-pci device is removed.
> >>>>
> >>>> The problem can be mitigated by removing all virtio devices on virtio
> >>>> bus shutdown.
> >>>>
> >>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> >>>> Reported-by: Hongyu Ning <hongyu.ning@...ux.intel.com>
> >>>
> >>> Gentle ping on that patch that seems to have fallen though the cracks.
> >>>
> >>> I think this fix is really needed. I have another test case with a
> >>> rebooting guest exposed with virtio-net (backed by vhost-net) and
> >>> viommu. Since there is currently no shutdown for the virtio-net, on
> >>> reboot, the IOMMU is disabled through the native_machine_shutdown()/
> >>> x86_platform.iommu_shutdown() while the virtio-net is still alive.
> >>>
> >>> Normally device_shutdown() should call virtio-net shutdown before the
> >>> IOMMU tear down and we wouldn't see any spurious transactions after
> >>> iommu shutdown.
> >>>
> >>> With that fix, the above test case is fixed and I do not see spurious
> >>> vhost IOTLB miss spurious requests.
> >>>
> >>> For more details, see qemu thread ([PATCH] hw/virtio/vhost: Disable
> >>> IOTLB callbacks when IOMMU gets disabled,
> >>> https://lore.kernel.org/all/20250120173339.865681-1-eric.auger@redhat.com/)
> >>>
> >>>
> >>> Reviewed-by: Eric Auger <eric.auger@...hat.com>
> >>> Tested-by: Eric Auger <eric.auger@...hat.com>
> >>>
> >>> Thanks
> >>>
> >>> Eric
> >>>
> >>>> ---
> >>>>  drivers/virtio/virtio.c | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> >>>> index a9b93e99c23a..6c2f908eb22c 100644
> >>>> --- a/drivers/virtio/virtio.c
> >>>> +++ b/drivers/virtio/virtio.c
> >>>> @@ -356,6 +356,15 @@ static void virtio_dev_remove(struct device *_d)
> >>>>  	of_node_put(dev->dev.of_node);
> >>>>  }
> >>>>  
> >>>> +static void virtio_dev_shutdown(struct device *_d)
> >>>> +{
> >>>> +	struct virtio_device *dev = dev_to_virtio(_d);
> >>>> +	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> >>>> +
> >>>> +	if (drv && drv->remove)
> >>>> +		drv->remove(dev);
> >>
> >>
> >> I am concerned that full remove is a heavyweight operation.
> >> Do not want to slow down reboots even more.
> >> How about just doing a reset, instead?
> > 
> > I tested with
> > 
> > static void virtio_dev_shutdown(struct device *_d)
> > {
> >         struct virtio_device *dev = dev_to_virtio(_d);
> > 
> >         virtio_reset_device(dev);
> > }
> > 
> > 
> > and it fixes my issue.
> > 
> > Kirill, would that fix you issue too?
> gentle ping.

I am on vacation this week. Will try to get around to it next week.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov