Messages in this thread | | | From | Jason Wang <> | Date | Mon, 30 May 2022 11:48:08 +0800 | Subject | Re: [PATCH V6 9/9] virtio: use WARN_ON() to warning illegal status value |
| |
On Fri, May 27, 2022 at 6:50 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > At a minimum, I don't see why it's part of the series. Host can always > crash the guest if it wants to ...
Probably not with some recent technology. In those cases, a fault will be generated if the hypervisor tries to access the memory that is private to the guest.
> The point of BUG_ON is device or driver is already corrupted so we > should not try to drive it. If you still want this in pls come up with > a better commit log explaining the why.
A question here, should we always use BUG_ON for the buggy/malicious hypervisor?
The interrupt hardening logic in this series tries to make guest survive, so did this patch.
> > On Fri, May 27, 2022 at 02:01:20PM +0800, Jason Wang wrote: > > We used to use BUG_ON() in virtio_device_ready() to detect illegal > > not really, BUG_ON just crashes the kernel. we detect by checking > status.
We need a kind of notification otherwise there's no way for the user to know about this expected value.
> > > status value, this seems sub-optimal since the value is under the > > control of the device. Switch to use WARN_ON() instead. > > some people use crash on warn so ...
Yes, but the policy is under the control of the user.
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: "Paul E. McKenney" <paulmck@kernel.org> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Halil Pasic <pasic@linux.ibm.com> > > Cc: Cornelia Huck <cohuck@redhat.com> > > Cc: Vineeth Vijayan <vneethv@linux.ibm.com> > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com> > > Cc: linux-s390@vger.kernel.org > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > --- > > include/linux/virtio_config.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > > index d4edfd7d91bb..9a36051ceb76 100644 > > --- a/include/linux/virtio_config.h > > +++ b/include/linux/virtio_config.h > > @@ -255,7 +255,7 @@ void virtio_device_ready(struct virtio_device *dev) > > { > > unsigned status = dev->config->get_status(dev); > > > > - BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); > > + WARN_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); > > > > we lose debuggability as guest will try to continue. > if we are doing this let us print a helpful message and dump a lot of > state right here.
I'm ok with dropping this patch from the series. And revisit it in the future.
Thanks
> > > /* > > * The virtio_synchronize_cbs() makes sure vring_interrupt() > > -- > > 2.25.1 >
| |