All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, tglx@linutronix.de,
	peterz@infradead.org, paulmck@kernel.org, maz@kernel.org,
	pasic@linux.ibm.com, cohuck@redhat.com, eperezma@redhat.com,
	lulu@redhat.com, sgarzare@redhat.com, xuanzhuo@linux.alibaba.com,
	Vineeth Vijayan <vneethv@linux.ibm.com>,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	linux-s390@vger.kernel.org
Subject: Re: [PATCH V6 9/9] virtio: use WARN_ON() to warning illegal status value
Date: Fri, 27 May 2022 06:50:00 -0400	[thread overview]
Message-ID: <20220527064118-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20220527060120.20964-10-jasowang@redhat.com>

At a minimum, I don't see why it's part of the series. Host can always
crash the guest if it wants to ...
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.

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.

> 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 ...

> 
> 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.

>  	/*
>  	 * The virtio_synchronize_cbs() makes sure vring_interrupt()
> -- 
> 2.25.1


WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: Peter Oberparleiter <oberpar@linux.ibm.com>,
	lulu@redhat.com, paulmck@kernel.org, linux-s390@vger.kernel.org,
	peterz@infradead.org, maz@kernel.org, cohuck@redhat.com,
	linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org, pasic@linux.ibm.com,
	eperezma@redhat.com, Vineeth Vijayan <vneethv@linux.ibm.com>,
	tglx@linutronix.de
Subject: Re: [PATCH V6 9/9] virtio: use WARN_ON() to warning illegal status value
Date: Fri, 27 May 2022 06:50:00 -0400	[thread overview]
Message-ID: <20220527064118-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20220527060120.20964-10-jasowang@redhat.com>

At a minimum, I don't see why it's part of the series. Host can always
crash the guest if it wants to ...
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.

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.

> 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 ...

> 
> 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.

>  	/*
>  	 * The virtio_synchronize_cbs() makes sure vring_interrupt()
> -- 
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

  parent reply	other threads:[~2022-05-27 10:50 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-27  6:01 [PATCH V6 0/9] rework on the IRQ hardening of virtio Jason Wang
2022-05-27  6:01 ` Jason Wang
2022-05-27  6:01 ` [PATCH V6 1/9] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:29   ` Xuan Zhuo
2022-05-27  7:29     ` Xuan Zhuo
2022-05-27  6:01 ` [PATCH V6 2/9] virtio: use virtio_reset_device() when possible Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:30   ` Xuan Zhuo
2022-05-27  7:30     ` Xuan Zhuo
2022-05-27  8:52   ` Eugenio Perez Martin
2022-05-27 10:34   ` Stefano Garzarella
2022-05-27 10:34     ` Stefano Garzarella
2022-05-27  6:01 ` [PATCH V6 3/9] virtio: introduce config op to synchronize vring callbacks Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:30   ` Xuan Zhuo
2022-05-27  7:30     ` Xuan Zhuo
2022-05-27 10:36   ` Stefano Garzarella
2022-05-27 10:36     ` Stefano Garzarella
2022-05-27  6:01 ` [PATCH V6 4/9] virtio-pci: implement synchronize_cbs() Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:31   ` Xuan Zhuo
2022-05-27  7:31     ` Xuan Zhuo
2022-05-27  6:01 ` [PATCH V6 5/9] virtio-mmio: " Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:32   ` Xuan Zhuo
2022-05-27  7:32     ` Xuan Zhuo
2022-05-27  6:01 ` [PATCH V6 6/9] virtio-ccw: " Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-30 15:12   ` Cornelia Huck
2022-05-30 15:12     ` Cornelia Huck
2022-05-27  6:01 ` [PATCH V6 7/9] virtio: allow to unbreak virtqueue Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:33   ` Xuan Zhuo
2022-05-27  7:33     ` Xuan Zhuo
2022-05-30 15:15   ` Cornelia Huck
2022-05-30 15:15     ` Cornelia Huck
2022-05-27  6:01 ` [PATCH V6 8/9] virtio: harden vring IRQ Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:49   ` Xuan Zhuo
2022-05-27  7:49     ` Xuan Zhuo
2022-05-30 15:18   ` Cornelia Huck
2022-05-30 15:18     ` Cornelia Huck
2022-06-11  5:12   ` Michael S. Tsirkin
2022-06-11  5:12     ` Michael S. Tsirkin
2022-06-13  5:26     ` Jason Wang
2022-06-13  5:26       ` Jason Wang
2022-06-13  7:23       ` Michael S. Tsirkin
2022-06-13  7:23         ` Michael S. Tsirkin
2022-06-13  8:07         ` Jason Wang
2022-06-13  8:07           ` Jason Wang
2022-06-13  8:19           ` Michael S. Tsirkin
2022-06-13  8:19             ` Michael S. Tsirkin
2022-06-13  8:51             ` Jason Wang
2022-06-13  8:51               ` Jason Wang
2022-06-13  8:59               ` Michael S. Tsirkin
2022-06-13  8:59                 ` Michael S. Tsirkin
2022-06-13  9:08                 ` Jason Wang
2022-06-13  9:08                   ` Jason Wang
2022-06-13  9:14                   ` Jason Wang
2022-06-13  9:14                     ` Jason Wang
2022-06-13  9:27                     ` Michael S. Tsirkin
2022-06-13  9:27                       ` Michael S. Tsirkin
2022-06-14  7:40                       ` Jason Wang
2022-06-14  7:40                         ` Jason Wang
2022-06-14 13:50                         ` Michael S. Tsirkin
2022-06-14 13:50                           ` Michael S. Tsirkin
2022-06-15  1:32                           ` Jason Wang
2022-06-15  1:32                             ` Jason Wang
2022-06-14 15:49                         ` Michael S. Tsirkin
2022-06-14 15:49                           ` Michael S. Tsirkin
2022-06-15  1:38                           ` Jason Wang
2022-06-15  1:38                             ` Jason Wang
2022-06-16 17:11                             ` Michael S. Tsirkin
2022-06-16 17:11                               ` Michael S. Tsirkin
2022-06-17  1:24                               ` Jason Wang
2022-06-17  1:24                                 ` Jason Wang
2022-06-17  5:36                                 ` Michael S. Tsirkin
2022-06-17  5:36                                   ` Michael S. Tsirkin
2022-06-17  7:26                                   ` Jason Wang
2022-06-17  7:26                                     ` Jason Wang
2022-06-17 14:33                                     ` Peter Zijlstra
2022-06-17 14:33                                       ` Peter Zijlstra
2022-06-14 16:46                         ` Cristian Marussi
2022-06-15  1:41                           ` Jason Wang
2022-06-15  1:41                             ` Jason Wang
2022-06-15 18:24                             ` Cristian Marussi
2022-06-17  3:14                               ` Jason Wang
2022-06-17  3:14                                 ` Jason Wang
2022-06-13  9:26                   ` Michael S. Tsirkin
2022-06-13  9:26                     ` Michael S. Tsirkin
2022-06-14  7:19                     ` Jason Wang
2022-06-14  7:19                       ` Jason Wang
2022-07-05 11:06   ` chenxiang (M)
2022-07-05 12:56     ` Jason Wang
2022-07-05 12:56       ` Jason Wang
2022-05-27  6:01 ` [PATCH V6 9/9] virtio: use WARN_ON() to warning illegal status value Jason Wang
2022-05-27  6:01   ` Jason Wang
2022-05-27  7:49   ` Xuan Zhuo
2022-05-27  7:49     ` Xuan Zhuo
2022-05-27 10:35   ` Stefano Garzarella
2022-05-27 10:35     ` Stefano Garzarella
2022-05-27 10:50   ` Michael S. Tsirkin [this message]
2022-05-27 10:50     ` Michael S. Tsirkin
2022-05-30  3:48     ` Jason Wang
2022-05-30  3:48       ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220527064118-mutt-send-email-mst@kernel.org \
    --to=mst@redhat.com \
    --cc=cohuck@redhat.com \
    --cc=eperezma@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=lulu@redhat.com \
    --cc=maz@kernel.org \
    --cc=oberpar@linux.ibm.com \
    --cc=pasic@linux.ibm.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=sgarzare@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=vneethv@linux.ibm.com \
    --cc=xuanzhuo@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.