Re: [PATCH v1 1/4] virtio: protect non-modern devices from too big virtqueue size setting

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 07:46:31AM +0000, Denis Plotnikov wrote:
> 
> On 05.11.2019 23:56, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:02PM +0300, Denis Plotnikov wrote:
> >> The patch protects from creating illegal virtio device configuration
> >> via direct virtqueue size property setting.
> >>
> >> Signed-off-by: Denis Plotnikov <address@hidden>
> >> ---
> >>   hw/virtio/virtio-blk-pci.c  |  9 +++++++++
> >>   hw/virtio/virtio-scsi-pci.c | 10 ++++++++++
> >>   2 files changed, 19 insertions(+)
> >>
> >> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> >> index 60c9185c39..6177ff1df8 100644
> >> --- a/hw/virtio/virtio-blk-pci.c
> >> +++ b/hw/virtio/virtio-blk-pci.c
> >> @@ -48,6 +48,15 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy 
> >> *vpci_dev, Error **errp)
> >>   {
> >>       VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
> >>       DeviceState *vdev = DEVICE(&dev->vdev);
> >> +    bool modern = virtio_pci_modern(vpci_dev);
> >> +    uint32_t queue_size = dev->vdev.conf.queue_size;
> >> +
> >> +    if (!modern && queue_size > 128) {
> >> +        error_setg(errp,
> >> +                   "too big queue size (%u, max: 128) "
> >> +                   "for non-modern virtio device", queue_size);
> >> +        return;
> >> +    }
> >
> > this enables for transitional so still visible to legacy
> > interface. I am guessing you want to check whether
> > device is accessed through the modern interface instead.
> 
> My goal is to not break something when I'm setting the queue size > 128 
> (taking into account the current seabios queue size restriction to 128). 
> I'm not quite sure what to check. Could I ask why one want to the check 
> whether accessing through the modern interface

Well now that you say that I don't really know why did you put this test
in here.  I was guessing you wanted modern because with modern queue
size is not forced by the host, guest can always use a smaller queue.
So it's safe to have a large queue.  But if not maybe you can comment on
why this is limited like this, and add a code comment here.

> and how it could be checked?


As Stefan said, you can look at the features.
But you can't do it from realize, you need to do it after guest
from the set features or validate features or set status callback.
I think validate features is the easiest to use of the three.

This calls for an API to resize queues which we
do not have now, but it's not hard to add.


> Thanks!
> 
> Denis
> 
> >>       if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> >>           vpci_dev->nvectors = dev->vdev.conf.num_queues + 1;
> >> diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> >> index 2830849729..6e6790fda5 100644
> >> --- a/hw/virtio/virtio-scsi-pci.c
> >> +++ b/hw/virtio/virtio-scsi-pci.c
> >> @@ -17,6 +17,7 @@
> >>   
> >>   #include "hw/virtio/virtio-scsi.h"
> >>   #include "virtio-pci.h"
> >> +#include "qapi/error.h"
> >>   
> >>   typedef struct VirtIOSCSIPCI VirtIOSCSIPCI;
> >>   
> >> @@ -47,6 +48,15 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy 
> >> *vpci_dev, Error **errp)
> >>       VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
> >>       DeviceState *proxy = DEVICE(vpci_dev);
> >>       char *bus_name;
> >> +    bool modern = virtio_pci_modern(vpci_dev);
> >> +    uint32_t virtqueue_size = vs->conf.virtqueue_size;
> >> +
> >> +    if (!modern && virtqueue_size > 128) {
> >> +        error_setg(errp,
> >> +                   "too big virtqueue size (%u, max: 128) "
> >> +                   "for non-modern virtio device", virtqueue_size);
> >> +        return;
> >> +    }
> > why? what is illegal about 256 for legacy?
> >
> >>   
> >>       if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> >>           vpci_dev->nvectors = vs->conf.num_queues + 3;
> >> -- 
> >> 2.17.0