From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36983)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1bhtXK-0006iF-0d
	for qemu-devel@nongnu.org; Thu, 08 Sep 2016 03:14:15 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1bhtXF-0002Bx-1Y
	for qemu-devel@nongnu.org; Thu, 08 Sep 2016 03:14:13 -0400
Received: from mx1.redhat.com ([209.132.183.28]:52824)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1bhtXE-0002Bn-SK
	for qemu-devel@nongnu.org; Thu, 08 Sep 2016 03:14:08 -0400
From: Markus Armbruster <armbru@redhat.com>
References: <147326875705.8546.11347276277137015855.stgit@bahia.lan>
	<147326876478.8546.16045138068342092499.stgit@bahia.lan>
Date: Thu, 08 Sep 2016 09:14:05 +0200
In-Reply-To: <147326876478.8546.16045138068342092499.stgit@bahia.lan> (Greg
	Kurz's message of "Wed, 07 Sep 2016 19:19:24 +0200")
Message-ID: <87eg4uyibm.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and
 exit instead of BUG_ON()
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Greg Kurz <groug@kaod.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, qemu-devel@nongnu.org, "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

Greg Kurz <groug@kaod.org> writes:

> Calling assert() really makes sense when hitting a genuine bug, which calls
> for a fix in QEMU. However, when something goes wrong because the guest
> sends a malformed message, it is better to write down a more meaningul
> error message and exit.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/9pfs/virtio-9p-device.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 009b43f6d045..67059182645a 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -19,6 +19,7 @@
>  #include "coth.h"
>  #include "hw/virtio/virtio-access.h"
>  #include "qemu/iov.h"
> +#include "qemu/error-report.h"
>  
>  void virtio_9p_push_and_notify(V9fsPDU *pdu)
>  {
> @@ -35,6 +36,11 @@ void virtio_9p_push_and_notify(V9fsPDU *pdu)
>      virtio_notify(VIRTIO_DEVICE(v), v->vq);
>  }
>  
> +static void virtio_9p_error(const char *msg)
> +{
> +    error_report("The virtio-9p driver in the guest has an issue: %s", msg);
> +}
> +
>  static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      V9fsVirtioState *v = (V9fsVirtioState *)vdev;
> @@ -56,13 +62,23 @@ static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
>              break;
>          }
>  
> -        BUG_ON(elem->out_num == 0 || elem->in_num == 0);
> +        if (elem->out_num == 0) {
> +            virtio_9p_error("missing VirtFS request's header");
> +            exit(1);
> +        }

Can the guest trigger this?

> +        if (elem->in_num == 0) {
> +            virtio_9p_error("missing VirtFS reply's header");
> +            exit(1);
> +        }

Same question.

>          QEMU_BUILD_BUG_ON(sizeof out != 7);
>  
>          v->elems[pdu->idx] = elem;
>          len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>                           &out, sizeof out);
> -        BUG_ON(len != sizeof out);
> +        if (len != sizeof out) {
> +            virtio_9p_error("malformed VirtFS request");
> +            exit(1);
> +        }

Same question.

>  
>          pdu->size = le32_to_cpu(out.size_le);
>