[Qemu-devel] [PATCH 0/2] virtio: error report fixes in 9P and PCI

[Qemu-devel] [PATCH 0/2] virtio: error report fixes in 9P and PCI

[Greg Kurz]

This series brings error reporting to virtio 9P in case a malformed
element is returned by virtqueue_pop().

While here, I also re-post a patch for virtio PCI that fell through the
cracks and missed the 2.7 merge window.

---

Greg Kurz (2):
      virtio-9p: print error message and exit instead of BUG_ON()
      virtio-pci: error out when both legacy and modern modes are disabled


 hw/9pfs/virtio-9p-device.c |   20 ++++++++++++++++++--
 hw/virtio/virtio-pci.c     |    8 ++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

--
Greg

[Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

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);
+        }
+        if (elem->in_num == 0) {
+            virtio_9p_error("missing VirtFS reply's header");
+            exit(1);
+        }
         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);
+        }
 
         pdu->size = le32_to_cpu(out.size_le);
 

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Markus Armbruster]

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);
>  

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Thu, 08 Sep 2016 09:14:05 +0200
Markus Armbruster <armbru@redhat.com> wrote:

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

Yes it can in theory if it pushes an empty buffer... but this "recent"
commit changed the outcome:

commit 1e7aed70144b4673fc26e73062064b6724795e5f
Author: Prasad J Pandit <pjp@fedoraproject.org>
Date:   Wed Jul 27 21:07:56 2016 +0530

    virtio: check vring descriptor buffer length

And now, the error is caught in virtqueue_map_desc():

    if (!sz) {
        error_report("virtio: zero sized buffers are not allowed");
        exit(1);
    }

So I guess we should keep the BUG_ON() then.

BTW, there are similar checks in virtio-blk and virtio-net leading to a QEMU
exit... which seem to be obsoleted by the above commit. I'll have a closer
look.

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

Same answer. :)

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

Here this is different: the guest can put a bogus len in the vring_desc
structure, and this doesn't get checked earlier.

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

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Cornelia Huck]

On Wed, 07 Sep 2016 19:19:24 +0200
Greg Kurz <groug@kaod.org> wrote:

> 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(-)

While this is an improvement over the current state, I don't think the
guest should be able to kill qemu just by doing something stupid.

The right way to go is to mark the virtio device as broken and stop
doing any processing until the guest resets it. I think Stefan had a
patch series doing that for some base virtio errors, but I'd have to
search for it.

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Thu, 8 Sep 2016 10:59:26 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 07 Sep 2016 19:19:24 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > 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(-)  
> 
> While this is an improvement over the current state, I don't think the
> guest should be able to kill qemu just by doing something stupid.
> 

Hi Connie,

I'm glad you're pointing this out... this was also my impression, but
since there are a bunch of sanity checks in the virtio code that cause
QEMU to exit (even recently added like 1e7aed70144b), I did not dare
stand up :)

> The right way to go is to mark the virtio device as broken and stop
> doing any processing until the guest resets it. I think Stefan had a
> patch series doing that for some base virtio errors, but I'd have to
> search for it.
> 

I'd be glad to have a look and try to address this issue.

Thanks !

--
Greg

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Michael S. Tsirkin]

On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:
> On Thu, 8 Sep 2016 10:59:26 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Wed, 07 Sep 2016 19:19:24 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > 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(-)  
> > 
> > While this is an improvement over the current state, I don't think the
> > guest should be able to kill qemu just by doing something stupid.
> > 
> 
> Hi Connie,
> 
> I'm glad you're pointing this out... this was also my impression, but
> since there are a bunch of sanity checks in the virtio code that cause
> QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> stand up :)

It's true that it's broken in many places but we should just
fix them all.


A separate question is how to log such hardware/guest bugs generally.
People already complained about disk filling up because of us printing
errors on each such bug.  Maybe print each message only N times, and
then set a flag to skip the log until management tells us to restart
logging again.


> > The right way to go is to mark the virtio device as broken and stop
> > doing any processing until the guest resets it. I think Stefan had a
> > patch series doing that for some base virtio errors, but I'd have to
> > search for it.
> > 
> 
> I'd be glad to have a look and try to address this issue.
> 
> Thanks !
> 
> --
> Greg

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Cornelia Huck]

On Thu, 8 Sep 2016 18:00:28 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:
> > On Thu, 8 Sep 2016 10:59:26 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > 
> > > On Wed, 07 Sep 2016 19:19:24 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > > 
> > > > 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(-)  
> > > 
> > > While this is an improvement over the current state, I don't think the
> > > guest should be able to kill qemu just by doing something stupid.
> > > 
> > 
> > Hi Connie,
> > 
> > I'm glad you're pointing this out... this was also my impression, but
> > since there are a bunch of sanity checks in the virtio code that cause
> > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> > stand up :)
> 
> It's true that it's broken in many places but we should just
> fix them all.
> 
> 
> A separate question is how to log such hardware/guest bugs generally.
> People already complained about disk filling up because of us printing
> errors on each such bug.  Maybe print each message only N times, and
> then set a flag to skip the log until management tells us to restart
> logging again.

I'd expect to get the message just once per device if we set the device
to broken (unless the guess continuously resets it again...) Do we have
a generic print/log ratelimit infrastructure in qemu?

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Michael S. Tsirkin]

On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:
> On Thu, 8 Sep 2016 18:00:28 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:
> > > On Thu, 8 Sep 2016 10:59:26 +0200
> > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > 
> > > > On Wed, 07 Sep 2016 19:19:24 +0200
> > > > Greg Kurz <groug@kaod.org> wrote:
> > > > 
> > > > > 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(-)  
> > > > 
> > > > While this is an improvement over the current state, I don't think the
> > > > guest should be able to kill qemu just by doing something stupid.
> > > > 
> > > 
> > > Hi Connie,
> > > 
> > > I'm glad you're pointing this out... this was also my impression, but
> > > since there are a bunch of sanity checks in the virtio code that cause
> > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> > > stand up :)
> > 
> > It's true that it's broken in many places but we should just
> > fix them all.
> > 
> > 
> > A separate question is how to log such hardware/guest bugs generally.
> > People already complained about disk filling up because of us printing
> > errors on each such bug.  Maybe print each message only N times, and
> > then set a flag to skip the log until management tells us to restart
> > logging again.
> 
> I'd expect to get the message just once per device if we set the device
> to broken (unless the guess continuously resets it again...)

Which it can do, so we should limit that anyway.

> Do we have
> a generic print/log ratelimit infrastructure in qemu?

There are actually two kinds of errors
host side ones and ones triggered by guests.

We should distinguish between them API-wise, then
we will be able to limit the logging of those
that guest can trigger.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Thu, 8 Sep 2016 18:19:27 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:
> > On Thu, 8 Sep 2016 18:00:28 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:  
> > > > On Thu, 8 Sep 2016 10:59:26 +0200
> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > >   
> > > > > On Wed, 07 Sep 2016 19:19:24 +0200
> > > > > Greg Kurz <groug@kaod.org> wrote:
> > > > >   
> > > > > > 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(-)    
> > > > > 
> > > > > While this is an improvement over the current state, I don't think the
> > > > > guest should be able to kill qemu just by doing something stupid.
> > > > >   
> > > > 
> > > > Hi Connie,
> > > > 
> > > > I'm glad you're pointing this out... this was also my impression, but
> > > > since there are a bunch of sanity checks in the virtio code that cause
> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> > > > stand up :)  
> > > 
> > > It's true that it's broken in many places but we should just
> > > fix them all.
> > > 
> > > 
> > > A separate question is how to log such hardware/guest bugs generally.
> > > People already complained about disk filling up because of us printing
> > > errors on each such bug.  Maybe print each message only N times, and
> > > then set a flag to skip the log until management tells us to restart
> > > logging again.  
> > 
> > I'd expect to get the message just once per device if we set the device
> > to broken (unless the guess continuously resets it again...)  
> 
> Which it can do, so we should limit that anyway.
> 
> > Do we have
> > a generic print/log ratelimit infrastructure in qemu?  
> 
> There are actually two kinds of errors
> host side ones and ones triggered by guests.
> 
> We should distinguish between them API-wise, then
> we will be able to limit the logging of those
> that guest can trigger.
> 

FWIW it makes sense to use error_report() if QEMU exits. If it continues
execution, this means we're expecting the guest or the host to do something
to fix the error condition. This requires QEMU to emit an event of some
sort, but not necessarily to log an error message in a file. I guess this
depends if QEMU is run by some tooling, or by a human.

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Michael S. Tsirkin]

On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote:
> On Thu, 8 Sep 2016 18:19:27 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:
> > > On Thu, 8 Sep 2016 18:00:28 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:  
> > > > > On Thu, 8 Sep 2016 10:59:26 +0200
> > > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > > >   
> > > > > > On Wed, 07 Sep 2016 19:19:24 +0200
> > > > > > Greg Kurz <groug@kaod.org> wrote:
> > > > > >   
> > > > > > > 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(-)    
> > > > > > 
> > > > > > While this is an improvement over the current state, I don't think the
> > > > > > guest should be able to kill qemu just by doing something stupid.
> > > > > >   
> > > > > 
> > > > > Hi Connie,
> > > > > 
> > > > > I'm glad you're pointing this out... this was also my impression, but
> > > > > since there are a bunch of sanity checks in the virtio code that cause
> > > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> > > > > stand up :)  
> > > > 
> > > > It's true that it's broken in many places but we should just
> > > > fix them all.
> > > > 
> > > > 
> > > > A separate question is how to log such hardware/guest bugs generally.
> > > > People already complained about disk filling up because of us printing
> > > > errors on each such bug.  Maybe print each message only N times, and
> > > > then set a flag to skip the log until management tells us to restart
> > > > logging again.  
> > > 
> > > I'd expect to get the message just once per device if we set the device
> > > to broken (unless the guess continuously resets it again...)  
> > 
> > Which it can do, so we should limit that anyway.
> > 
> > > Do we have
> > > a generic print/log ratelimit infrastructure in qemu?  
> > 
> > There are actually two kinds of errors
> > host side ones and ones triggered by guests.
> > 
> > We should distinguish between them API-wise, then
> > we will be able to limit the logging of those
> > that guest can trigger.
> > 
> 
> FWIW it makes sense to use error_report() if QEMU exits.

Not necessarily e.g. hotplug errors trigger error_report too.
Generally it should be for host misconfiguration or similar
management errors.

> If it continues
> execution, this means we're expecting the guest or the host to do something
> to fix the error condition. This requires QEMU to emit an event of some
> sort, but not necessarily to log an error message in a file. I guess this
> depends if QEMU is run by some tooling, or by a human.

I'm not sure we need an event if tools are not expected to
do anything with it. If we limit # of times error
is printed, tools will need to reset this counter,
so we will need an event on overflow.


-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Cornelia Huck]

On Thu, 8 Sep 2016 19:55:16 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote:
> > On Thu, 8 Sep 2016 18:19:27 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:
> > > > On Thu, 8 Sep 2016 18:00:28 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >   
> > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:  

> > If it continues
> > execution, this means we're expecting the guest or the host to do something
> > to fix the error condition. This requires QEMU to emit an event of some
> > sort, but not necessarily to log an error message in a file. I guess this
> > depends if QEMU is run by some tooling, or by a human.
> 
> I'm not sure we need an event if tools are not expected to
> do anything with it. If we limit # of times error
> is printed, tools will need to reset this counter,
> so we will need an event on overflow.

If the device goes into a broken state, it should be discoverable from
outside. I'm not sure we need an actual event signalling this if this
happens due to the guest doing something wrong: That would be a task
for tools monitoring _inside_ the guest. For tools monitoring the
health of the machine (from the host perspective), the discovery
interface would probably be enough?

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Fri, 9 Sep 2016 10:30:53 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Thu, 8 Sep 2016 19:55:16 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote:  
> > > On Thu, 8 Sep 2016 18:19:27 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:  
> > > > > On Thu, 8 Sep 2016 18:00:28 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:    
> 
> > > If it continues
> > > execution, this means we're expecting the guest or the host to do something
> > > to fix the error condition. This requires QEMU to emit an event of some
> > > sort, but not necessarily to log an error message in a file. I guess this
> > > depends if QEMU is run by some tooling, or by a human.  
> > 
> > I'm not sure we need an event if tools are not expected to
> > do anything with it. If we limit # of times error
> > is printed, tools will need to reset this counter,
> > so we will need an event on overflow.  
> 
> If the device goes into a broken state, it should be discoverable from
> outside. I'm not sure we need an actual event signalling this if this
> happens due to the guest doing something wrong: That would be a task
> for tools monitoring _inside_ the guest. 

Well, in case of a virtio device being broken, section 2.1.2 in the spec
suggests to set the status to DEVICE_NEEDS_RESET and to notify it to
the guest (aka. event signalling). I'll send a patch shortly.

> For tools monitoring the
> health of the machine (from the host perspective), the discovery
> interface would probably be enough?
> 

Yeah, probably.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Cornelia Huck]

On Fri, 9 Sep 2016 10:46:25 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 9 Sep 2016 10:30:53 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Thu, 8 Sep 2016 19:55:16 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote:  
> > > > On Thu, 8 Sep 2016 18:19:27 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >   
> > > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:  
> > > > > > On Thu, 8 Sep 2016 18:00:28 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >     
> > > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:    
> > 
> > > > If it continues
> > > > execution, this means we're expecting the guest or the host to do something
> > > > to fix the error condition. This requires QEMU to emit an event of some
> > > > sort, but not necessarily to log an error message in a file. I guess this
> > > > depends if QEMU is run by some tooling, or by a human.  
> > > 
> > > I'm not sure we need an event if tools are not expected to
> > > do anything with it. If we limit # of times error
> > > is printed, tools will need to reset this counter,
> > > so we will need an event on overflow.  
> > 
> > If the device goes into a broken state, it should be discoverable from
> > outside. I'm not sure we need an actual event signalling this if this
> > happens due to the guest doing something wrong: That would be a task
> > for tools monitoring _inside_ the guest. 
> 
> Well, in case of a virtio device being broken, section 2.1.2 in the spec
> suggests to set the status to DEVICE_NEEDS_RESET and to notify it to
> the guest (aka. event signalling). I'll send a patch shortly.

Stefan had already sent
<1460467534-29147-4-git-send-email-stefanha@redhat.com> ages ago, but
it has not yet made it anywhere...

Anyhow, I was concerned with host signalling (sorry for being unclear),
and I still do not think we need to alert host monitoring software to
guest stupidity.

> 
> > For tools monitoring the
> > health of the machine (from the host perspective), the discovery
> > interface would probably be enough?
> > 
> 
> Yeah, probably.
> 
> Cheers.
> 
> --
> Greg
> 

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Fri, 9 Sep 2016 10:53:05 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 9 Sep 2016 10:46:25 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Fri, 9 Sep 2016 10:30:53 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >   
> > > On Thu, 8 Sep 2016 19:55:16 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote:    
> > > > > On Thu, 8 Sep 2016 18:19:27 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >     
> > > > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:    
> > > > > > > On Thu, 8 Sep 2016 18:00:28 +0300
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > >       
> > > > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:      
> > >   
> > > > > If it continues
> > > > > execution, this means we're expecting the guest or the host to do something
> > > > > to fix the error condition. This requires QEMU to emit an event of some
> > > > > sort, but not necessarily to log an error message in a file. I guess this
> > > > > depends if QEMU is run by some tooling, or by a human.    
> > > > 
> > > > I'm not sure we need an event if tools are not expected to
> > > > do anything with it. If we limit # of times error
> > > > is printed, tools will need to reset this counter,
> > > > so we will need an event on overflow.    
> > > 
> > > If the device goes into a broken state, it should be discoverable from
> > > outside. I'm not sure we need an actual event signalling this if this
> > > happens due to the guest doing something wrong: That would be a task
> > > for tools monitoring _inside_ the guest.   
> > 
> > Well, in case of a virtio device being broken, section 2.1.2 in the spec
> > suggests to set the status to DEVICE_NEEDS_RESET and to notify it to
> > the guest (aka. event signalling). I'll send a patch shortly.  
> 
> Stefan had already sent
> <1460467534-29147-4-git-send-email-stefanha@redhat.com> ages ago, but
> it has not yet made it anywhere...
> 

I don't know what to do with this message-id :\

> Anyhow, I was concerned with host signalling (sorry for being unclear),
> and I still do not think we need to alert host monitoring software to
> guest stupidity.
> 

I agree. Sorry if my poor wording made you (and others) think I was
suggesting that :) My point was that if QEMU exits because of guest
stupidity, you are forced to error_report() something to the host,
but this is really suboptimal (even if BUG_ON is worse)... then
there was that discussion about log files getting to big, but I don't
even know how we came there, as it does not really make sense when QEMU
exits.

> >   
> > > For tools monitoring the
> > > health of the machine (from the host perspective), the discovery
> > > interface would probably be enough?
> > >   
> > 
> > Yeah, probably.
> > 
> > Cheers.
> > 
> > --
> > Greg
> >   
> 

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Fri, 9 Sep 2016 11:26:17 +0200
Greg Kurz <groug@kaod.org> wrote:

> > 
> > Stefan had already sent
> > <1460467534-29147-4-git-send-email-stefanha@redhat.com> ages ago, but
> > it has not yet made it anywhere...
> >   
> 
> I don't know what to do with this message-id :\

I finally found :)

+message-id:<1460467534-29147-4-git-send-email-stefanha@redhat.com> in the
search box at https://lists.nongnu.org/archive/html/qemu-devel/

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Thu, 8 Sep 2016 18:19:27 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:
>> > On Thu, 8 Sep 2016 18:00:28 +0300
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >   
>> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:  
>> > > > On Thu, 8 Sep 2016 10:59:26 +0200
>> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> > > >   
>> > > > > On Wed, 07 Sep 2016 19:19:24 +0200
>> > > > > Greg Kurz <groug@kaod.org> wrote:
>> > > > >   
>> > > > > > 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(-)    
>> > > > > 
>> > > > > While this is an improvement over the current state, I don't think the
>> > > > > guest should be able to kill qemu just by doing something stupid.
>> > > > >   
>> > > > 
>> > > > Hi Connie,
>> > > > 
>> > > > I'm glad you're pointing this out... this was also my impression, but
>> > > > since there are a bunch of sanity checks in the virtio code that cause
>> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
>> > > > stand up :)  
>> > > 
>> > > It's true that it's broken in many places but we should just
>> > > fix them all.
>> > > 
>> > > 
>> > > A separate question is how to log such hardware/guest bugs generally.
>> > > People already complained about disk filling up because of us printing
>> > > errors on each such bug.  Maybe print each message only N times, and
>> > > then set a flag to skip the log until management tells us to restart
>> > > logging again.  
>> > 
>> > I'd expect to get the message just once per device if we set the device
>> > to broken (unless the guess continuously resets it again...)  
>> 
>> Which it can do, so we should limit that anyway.
>> 
>> > Do we have
>> > a generic print/log ratelimit infrastructure in qemu?  
>> 
>> There are actually two kinds of errors
>> host side ones and ones triggered by guests.
>> 
>> We should distinguish between them API-wise, then
>> we will be able to limit the logging of those
>> that guest can trigger.
>> 
>
> FWIW it makes sense to use error_report() if QEMU exits.

exit(STATUS) with STATUS != 0 without printing a message is always
wrong.

>                                                          If it continues
> execution, this means we're expecting the guest or the host to do something
> to fix the error condition. This requires QEMU to emit an event of some
> sort, but not necessarily to log an error message in a file. I guess this
> depends if QEMU is run by some tooling, or by a human.

error_report() normally goes to stderr.  Tooling or humans can of course
make it go to a file instead.

error_report() is indeed a sub-par way to send an "attention" signal to
the host, because recognizing such a signal reliably is unnecessary hard
for management applications.  QMP events are much easier.

Both are useless when the signal needs to go to the guest.  Signalling
the guest is a device model job.

error_report() without exit() has its uses.  Error conditions in need of
fixing aren't the only reason to call error_report().  But when you add
a call, ask yourself whether management application or guest would like
to respond to it.

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Fri, 09 Sep 2016 08:38:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > On Thu, 8 Sep 2016 18:19:27 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >  
> >> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:  
> >> > On Thu, 8 Sep 2016 18:00:28 +0300
> >> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >     
> >> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:    
> >> > > > On Thu, 8 Sep 2016 10:59:26 +0200
> >> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >> > > >     
> >> > > > > On Wed, 07 Sep 2016 19:19:24 +0200
> >> > > > > Greg Kurz <groug@kaod.org> wrote:
> >> > > > >     
> >> > > > > > 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(-)      
> >> > > > > 
> >> > > > > While this is an improvement over the current state, I don't think the
> >> > > > > guest should be able to kill qemu just by doing something stupid.
> >> > > > >     
> >> > > > 
> >> > > > Hi Connie,
> >> > > > 
> >> > > > I'm glad you're pointing this out... this was also my impression, but
> >> > > > since there are a bunch of sanity checks in the virtio code that cause
> >> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> >> > > > stand up :)    
> >> > > 
> >> > > It's true that it's broken in many places but we should just
> >> > > fix them all.
> >> > > 
> >> > > 
> >> > > A separate question is how to log such hardware/guest bugs generally.
> >> > > People already complained about disk filling up because of us printing
> >> > > errors on each such bug.  Maybe print each message only N times, and
> >> > > then set a flag to skip the log until management tells us to restart
> >> > > logging again.    
> >> > 
> >> > I'd expect to get the message just once per device if we set the device
> >> > to broken (unless the guess continuously resets it again...)    
> >> 
> >> Which it can do, so we should limit that anyway.
> >>   
> >> > Do we have
> >> > a generic print/log ratelimit infrastructure in qemu?    
> >> 
> >> There are actually two kinds of errors
> >> host side ones and ones triggered by guests.
> >> 
> >> We should distinguish between them API-wise, then
> >> we will be able to limit the logging of those
> >> that guest can trigger.
> >>   
> >
> > FWIW it makes sense to use error_report() if QEMU exits.  
> 
> exit(STATUS) with STATUS != 0 without printing a message is always
> wrong.
> 

I fully agree.

> >                                                          If it continues
> > execution, this means we're expecting the guest or the host to do something
> > to fix the error condition. This requires QEMU to emit an event of some
> > sort, but not necessarily to log an error message in a file. I guess this
> > depends if QEMU is run by some tooling, or by a human.  
> 
> error_report() normally goes to stderr.  Tooling or humans can of course
> make it go to a file instead.
> 
> error_report() is indeed a sub-par way to send an "attention" signal to
> the host, because recognizing such a signal reliably is unnecessary hard
> for management applications.  QMP events are much easier.
> 

My wording was poor but yes, that was my point. :)

> Both are useless when the signal needs to go to the guest.  Signalling
> the guest is a device model job.
> 

I also agree with that. In the case of virtio, this is explained in section
2.1.2 of the spec.

> error_report() without exit() has its uses.  Error conditions in need of
> fixing aren't the only reason to call error_report().  But when you add
> a call, ask yourself whether management application or guest would like
> to respond to it.

In the case of the present patch, we currently have BUG_ON() which generates
a cryptic and unusable message.

It turns out that the first one (elem->out_num == 0 || elem->in_num == 0) is
correct since it is now [1] impossible to hit this according to the code (see
virtqueue_pop() and virtqueue_map_desc()).

The second one (len != sizeof out) though matches a potential guest originated
error. If I do as suggested by Connie, then the error_report() isn't needed
anymore.

Cheers.

--
Greg

[1] sending an empty buffer was sufficient before commit 1e7aed70144b4 as said
    in my previous answer

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Fri, 09 Sep 2016 08:38:13 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Greg Kurz <groug@kaod.org> writes:
>> 
>> > On Thu, 8 Sep 2016 18:19:27 +0300
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >  
>> >> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:  
>> >> > On Thu, 8 Sep 2016 18:00:28 +0300
>> >> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> >     
>> >> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:    
>> >> > > > On Thu, 8 Sep 2016 10:59:26 +0200
>> >> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> >> > > >     
>> >> > > > > On Wed, 07 Sep 2016 19:19:24 +0200
>> >> > > > > Greg Kurz <groug@kaod.org> wrote:
>> >> > > > >     
>> >> > > > > > 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(-)      
>> >> > > > > 
>> >> > > > > While this is an improvement over the current state, I don't think the
>> >> > > > > guest should be able to kill qemu just by doing something stupid.
>> >> > > > >     
>> >> > > > 
>> >> > > > Hi Connie,
>> >> > > > 
>> >> > > > I'm glad you're pointing this out... this was also my impression, but
>> >> > > > since there are a bunch of sanity checks in the virtio code that cause
>> >> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
>> >> > > > stand up :)    
>> >> > > 
>> >> > > It's true that it's broken in many places but we should just
>> >> > > fix them all.
>> >> > > 
>> >> > > 
>> >> > > A separate question is how to log such hardware/guest bugs generally.
>> >> > > People already complained about disk filling up because of us printing
>> >> > > errors on each such bug.  Maybe print each message only N times, and
>> >> > > then set a flag to skip the log until management tells us to restart
>> >> > > logging again.    
>> >> > 
>> >> > I'd expect to get the message just once per device if we set the device
>> >> > to broken (unless the guess continuously resets it again...)    
>> >> 
>> >> Which it can do, so we should limit that anyway.
>> >>   
>> >> > Do we have
>> >> > a generic print/log ratelimit infrastructure in qemu?    
>> >> 
>> >> There are actually two kinds of errors
>> >> host side ones and ones triggered by guests.
>> >> 
>> >> We should distinguish between them API-wise, then
>> >> we will be able to limit the logging of those
>> >> that guest can trigger.
>> >>   
>> >
>> > FWIW it makes sense to use error_report() if QEMU exits.  
>> 
>> exit(STATUS) with STATUS != 0 without printing a message is always
>> wrong.
>> 
>
> I fully agree.
>
>> >                                                          If it continues
>> > execution, this means we're expecting the guest or the host to do something
>> > to fix the error condition. This requires QEMU to emit an event of some
>> > sort, but not necessarily to log an error message in a file. I guess this
>> > depends if QEMU is run by some tooling, or by a human.  
>> 
>> error_report() normally goes to stderr.  Tooling or humans can of course
>> make it go to a file instead.
>> 
>> error_report() is indeed a sub-par way to send an "attention" signal to
>> the host, because recognizing such a signal reliably is unnecessary hard
>> for management applications.  QMP events are much easier.
>> 
>
> My wording was poor but yes, that was my point. :)
>
>> Both are useless when the signal needs to go to the guest.  Signalling
>> the guest is a device model job.
>> 
>
> I also agree with that. In the case of virtio, this is explained in section
> 2.1.2 of the spec.
>
>> error_report() without exit() has its uses.  Error conditions in need of
>> fixing aren't the only reason to call error_report().  But when you add
>> a call, ask yourself whether management application or guest would like
>> to respond to it.
>
> In the case of the present patch, we currently have BUG_ON() which generates
> a cryptic and unusable message.
>
> It turns out that the first one (elem->out_num == 0 || elem->in_num == 0) is
> correct since it is now [1] impossible to hit this according to the code (see
> virtqueue_pop() and virtqueue_map_desc()).
>
> The second one (len != sizeof out) though matches a potential guest originated
> error. If I do as suggested by Connie, then the error_report() isn't needed
> anymore.

I dive into the details of your analysis right now, only make high-level
recommendations:

* Issues common to all virtio devices should be addressed in the virtio
  core.  If that's not feasible, they should be addressed in all devices
  consistently.

* Guest misbehavior should put the device in a guest-observable error
  state.  It should not crash QEMU, it should not spam stderr.  Code
  handling it in other ways should be marked FIXME.

* Nobody expects you to get things perfectly right in one step.  Just
  try to move towards the goal.

>
> Cheers.
>
> --
> Greg
>
> [1] sending an empty buffer was sufficient before commit 1e7aed70144b4 as said
>     in my previous answer

Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()

[Greg Kurz]

On Fri, 09 Sep 2016 11:08:56 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > On Fri, 09 Sep 2016 08:38:13 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >  
> >> Greg Kurz <groug@kaod.org> writes:
> >>   
> >> > On Thu, 8 Sep 2016 18:19:27 +0300
> >> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >    
> >> >> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote:    
> >> >> > On Thu, 8 Sep 2016 18:00:28 +0300
> >> >> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> >       
> >> >> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote:      
> >> >> > > > On Thu, 8 Sep 2016 10:59:26 +0200
> >> >> > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >> >> > > >       
> >> >> > > > > On Wed, 07 Sep 2016 19:19:24 +0200
> >> >> > > > > Greg Kurz <groug@kaod.org> wrote:
> >> >> > > > >       
> >> >> > > > > > 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(-)        
> >> >> > > > > 
> >> >> > > > > While this is an improvement over the current state, I don't think the
> >> >> > > > > guest should be able to kill qemu just by doing something stupid.
> >> >> > > > >       
> >> >> > > > 
> >> >> > > > Hi Connie,
> >> >> > > > 
> >> >> > > > I'm glad you're pointing this out... this was also my impression, but
> >> >> > > > since there are a bunch of sanity checks in the virtio code that cause
> >> >> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare
> >> >> > > > stand up :)      
> >> >> > > 
> >> >> > > It's true that it's broken in many places but we should just
> >> >> > > fix them all.
> >> >> > > 
> >> >> > > 
> >> >> > > A separate question is how to log such hardware/guest bugs generally.
> >> >> > > People already complained about disk filling up because of us printing
> >> >> > > errors on each such bug.  Maybe print each message only N times, and
> >> >> > > then set a flag to skip the log until management tells us to restart
> >> >> > > logging again.      
> >> >> > 
> >> >> > I'd expect to get the message just once per device if we set the device
> >> >> > to broken (unless the guess continuously resets it again...)      
> >> >> 
> >> >> Which it can do, so we should limit that anyway.
> >> >>     
> >> >> > Do we have
> >> >> > a generic print/log ratelimit infrastructure in qemu?      
> >> >> 
> >> >> There are actually two kinds of errors
> >> >> host side ones and ones triggered by guests.
> >> >> 
> >> >> We should distinguish between them API-wise, then
> >> >> we will be able to limit the logging of those
> >> >> that guest can trigger.
> >> >>     
> >> >
> >> > FWIW it makes sense to use error_report() if QEMU exits.    
> >> 
> >> exit(STATUS) with STATUS != 0 without printing a message is always
> >> wrong.
> >>   
> >
> > I fully agree.
> >  
> >> >                                                          If it continues
> >> > execution, this means we're expecting the guest or the host to do something
> >> > to fix the error condition. This requires QEMU to emit an event of some
> >> > sort, but not necessarily to log an error message in a file. I guess this
> >> > depends if QEMU is run by some tooling, or by a human.    
> >> 
> >> error_report() normally goes to stderr.  Tooling or humans can of course
> >> make it go to a file instead.
> >> 
> >> error_report() is indeed a sub-par way to send an "attention" signal to
> >> the host, because recognizing such a signal reliably is unnecessary hard
> >> for management applications.  QMP events are much easier.
> >>   
> >
> > My wording was poor but yes, that was my point. :)
> >  
> >> Both are useless when the signal needs to go to the guest.  Signalling
> >> the guest is a device model job.
> >>   
> >
> > I also agree with that. In the case of virtio, this is explained in section
> > 2.1.2 of the spec.
> >  
> >> error_report() without exit() has its uses.  Error conditions in need of
> >> fixing aren't the only reason to call error_report().  But when you add
> >> a call, ask yourself whether management application or guest would like
> >> to respond to it.  
> >
> > In the case of the present patch, we currently have BUG_ON() which generates
> > a cryptic and unusable message.
> >
> > It turns out that the first one (elem->out_num == 0 || elem->in_num == 0) is
> > correct since it is now [1] impossible to hit this according to the code (see
> > virtqueue_pop() and virtqueue_map_desc()).
> >
> > The second one (len != sizeof out) though matches a potential guest originated
> > error. If I do as suggested by Connie, then the error_report() isn't needed
> > anymore.  
> 
> I dive into the details of your analysis right now, only make high-level
> recommendations:
> 
> * Issues common to all virtio devices should be addressed in the virtio
>   core.  If that's not feasible, they should be addressed in all devices
>   consistently.
> 

Agreed.

> * Guest misbehavior should put the device in a guest-observable error
>   state.  It should not crash QEMU, it should not spam stderr.  Code
>   handling it in other ways should be marked FIXME.
> 

Agreed. FWIW a bunch of FIXMEs are missing in the virtio code then :)

> * Nobody expects you to get things perfectly right in one step.  Just
>   try to move towards the goal.
> 

Sure ! I'm now reading through Stefan's series to address the issue:

https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg01978.html

Cheers.

--
Greg

> >
> > Cheers.
> >
> > --
> > Greg
> >
> > [1] sending an empty buffer was sufficient before commit 1e7aed70144b4 as said
> >     in my previous answer  

[Qemu-devel] [PATCH 2/2] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

From: Greg Kurz <gkurz@linux.vnet.ibm.com>

Without presuming if we got there because of a user mistake or some
more subtle bug in the tooling, it really does not make sense to
implement a non-functional device.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/virtio/virtio-pci.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 268fd8ebb219..4b6a8a356621 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
     VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
     PCIDevice *pci_dev = &proxy->pci_dev;
 
+    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
+        error_setg(errp, "device cannot work as neither modern nor legacy mode"
+                   " is enabled");
+        error_append_hint(errp, "Set either disable-modern or disable-legacy"
+                          " to off\n");
+        return;
+    }
+
     if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
         virtio_pci_modern(proxy)) {
         pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

Re: [Qemu-devel] [PATCH 2/2] virtio-pci: error out when both legacy and modern modes are disabled

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> From: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
> Without presuming if we got there because of a user mistake or some
> more subtle bug in the tooling, it really does not make sense to
> implement a non-functional device.
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/virtio-pci.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 268fd8ebb219..4b6a8a356621 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>      PCIDevice *pci_dev = &proxy->pci_dev;
>  
> +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
> +        error_setg(errp, "device cannot work as neither modern nor legacy mode"
> +                   " is enabled");
> +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> +                          " to off\n");
> +        return;
> +    }
> +
>      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
>          virtio_pci_modern(proxy)) {
>          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

Pardon my ignorance... is this a device-specific restriction, or is it
the same for more (all?) virtio devices?

Re: [Qemu-devel] [PATCH 2/2] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

On Thu, 08 Sep 2016 09:15:28 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> writes:
> 
> > From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >
> > Without presuming if we got there because of a user mistake or some
> > more subtle bug in the tooling, it really does not make sense to
> > implement a non-functional device.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/virtio/virtio-pci.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 268fd8ebb219..4b6a8a356621 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> >      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> >      PCIDevice *pci_dev = &proxy->pci_dev;
> >  
> > +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
> > +        error_setg(errp, "device cannot work as neither modern nor legacy mode"
> > +                   " is enabled");
> > +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> > +                          " to off\n");
> > +        return;
> > +    }
> > +
> >      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> >          virtio_pci_modern(proxy)) {
> >          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;  
> 
> Pardon my ignorance... is this a device-specific restriction, or is it
> the same for more (all?) virtio devices?

Yes this for all virtio devices.

But wait, I now realize that the check isn't done in the right place because
if we don't pass an explicit disable-legacy=on then proxy->disable_legacy gets
its final value in virtio_pci_realize() which gets called after virtio_pci_dc_realize()...

I'll fix that.

Thanks !

--
Greg