Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Halil Pasic]

On 05/15/2018 10:32 AM, Cornelia Huck wrote:
> On Mon, 14 May 2018 19:12:27 +0100
> Peter Maydell <address@hidden> wrote:
>
> > Hi; Coverity has I think enabled a new warning recently, which
> > is triggering on virtio_ccw_notify() in hw/s390x/virtio-ccw.c
> > (CID 1390619).
> >
> > This function does
> >      indicators |= 1ULL << vector;
> > but the code is guarded only by
> >      if (vector < VIRTIO_QUEUE_MAX) {
> >
> > That used to be OK when VIRTIO_QUEUE_MAX was 64, but in
> > commit b829c2a98f1 it was raised to 1024, and this is no longer
> > a useful guard. The commit message for b829c2a98f1 suggests that
> > this is a "can't happen" case -- is that so?

The logic behind this condition is the following. Vector either
a) stands for an used buffer notification and holds a virtqueue index, or
b) it stands for configuration change notification and holds VIRTIO_QUEUE_MAX.

The valid values for a virtqueue index are [0 .. VIRTIO_QUEUE_MAX -1]. For
a particular device only a subset of this set may be valid, but anything
outside is an invalid virtqueue index QEMU-wide.

> That is outdated; we bumped max virtqueues for ccw in b1914b824ade.

Right. With two stage indicators we can support VIRTIO_QUEUE_MAX queues,
but with classic indicators however we are stuck at 64 vritqueues at most.
We have  check for that (if interested look for
'if (virtio_get_num_queues(vdev) > NR_CLASSIC_INDICATOR_BITS) {').

> > If so then the
> > else {} part of the code and an earlier check on
> > "if (vector >= VIRTIO_QUEUE_MAX + 64)" are dead code.
> > However it looks like the device_plugged method is also
> > checking VIRTIO_QUEUE_MAX, rather than 64.
> >
> > If this is a false positive, then an assert() in
> > virtio_ccw_notify() and cleaning up the dead code would
> > help placate coverity.

The else is definitely not dead code, as I've explained above.

But vector >= VIRTIO_QUEUE_MAX + 64 should never be observed. Although
blame blames me, all I did was get rid of the transport specific limit:
-    if (vector >= VIRTIO_CCW_QUEUE_MAX + 64) {
+    if (vector >= VIRTIO_QUEUE_MAX + 64) {

The check however does not make much sense IMHO (and it did not back
then when I touched the code).

The vector values we can observe are AFAIU determined by:
git grep -e 'vdev->config_vector = ' -e 'virtio_queue_set_vector' -n  -- 
hw/s390x/virtio-ccw.c
and the possible values are [0..VIRTIO_QUEUE_MAX].

So we should really check for that. If it's better to do the check
via assert or log a warning and carry on without notifying, I'm
not sure.


> It is a false positive as far as I can see; this is the notification
> code for classical interrupts, and we fence off more than 64 virtqueues
> already when the guest tries to set it up (introduced in 797b608638c5).
> As that code flow is basically impossible to deduce by a static code
> checker, adding an assert() seems like a good idea. Halil, what do you
> think?

See all over the place ;)

> > (Other odd code in that function:
> >      vector = 0;
> >      [...]
> >      indicators |= 1ULL << vector;
> > is that really supposed to ignore the input vector number?)

This is why the vector >= VIRTIO_QUEUE_MAX + 64 does not make sense. I
think this should be simplified. Overwriting the vector with zero and
doing the shift is just nonsensical.

To sum it up, my take on the whole is the diff below. I can convert
it to a proper patch if we agree that's the way to go.

Regards,
Halil

> Yes; this as a configuration change notification done via secondary
> indicators (different from either the classical indicators or the
> adapter interrupt indicators). We always set the same bit, and it is
> always triggered by doing a notification with a number one above the
> maximum possible virtqueue number. (I agree that it does look odd, we
> should maybe add a comment.)

----------------------------8<---------------------------------------

From: Halil Pasic <address@hidden>
Date: Tue, 15 May 2018 13:57:44 +0200
Subject: [PATCH] WIP: cleanup virtio notify

Signed-off-by: Halil Pasic <address@hidden>
---
 hw/s390x/virtio-ccw.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e51fbefd23..22078605d1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t 
vector)
     SubchDev *sch = ccw_dev->sch;
     uint64_t indicators;

-    /* queue indicators + secondary indicators */
-    if (vector >= VIRTIO_QUEUE_MAX + 64) {
-        return;
-    }
+    /* vector == VIRTIO_QUEUE_MAX means configuration change */
+    assert(vector <= VIRTIO_QUEUE_MAX);

     if (vector < VIRTIO_QUEUE_MAX) {
         if (!dev->indicators) {
@@ -1042,12 +1040,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t 
vector)
         if (!dev->indicators2) {
             return;
         }
-        vector = 0;
         indicators = address_space_ldq(&address_space_memory,
                                        dev->indicators2->addr,
                                        MEMTXATTRS_UNSPECIFIED,
                                        NULL);
-        indicators |= 1ULL << vector;
+        indicators |= 1ULL;
         address_space_stq(&address_space_memory, dev->indicators2->addr,
                           indicators, MEMTXATTRS_UNSPECIFIED, NULL);
         css_conditional_io_interrupt(sch);
--