linux-kernel - Re: [PATCH] vdpa_sim: fix vringh initialization in vdpasim_queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJaqyWfYx+63-hOp0K8fznkyjkScKu6-r8CUPd3eD96oKCHu9A@mail.gmail.com>
Date:   Mon, 14 Nov 2022 10:13:51 +0100
From:   Eugenio Perez Martin <eperezma@...hat.com>
To:     Stefano Garzarella <sgarzare@...hat.com>
Cc:     virtualization@...ts.linux-foundation.org,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vdpa_sim: fix vringh initialization in vdpasim_queue_ready()

On Fri, Nov 11, 2022 at 5:30 PM Stefano Garzarella <sgarzare@...hat.com> wrote:
>
> On Fri, Nov 11, 2022 at 04:40:33PM +0100, Eugenio Perez Martin wrote:
> >On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@...hat.com> wrote:
> >>
> >> When we initialize vringh, we should pass the features and the
> >> number of elements in the virtqueue negotiated with the driver,
> >> otherwise operations with vringh may fail.
> >>
> >> This was discovered in a case where the driver sets a number of
> >> elements in the virtqueue different from the value returned by
> >> .get_vq_num_max().
> >>
> >> In vdpasim_vq_reset() is safe to initialize the vringh with
> >> default values, since the virtqueue will not be used until
> >> vdpasim_queue_ready() is called again.
> >>
> >> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> >> Signed-off-by: Stefano Garzarella <sgarzare@...hat.com>
> >> ---
> >>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> index b071f0d842fb..b20689f8fe89 100644
> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> >>  {
> >>         struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> >>
> >> -       vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
> >> -                         VDPASIM_QUEUE_MAX, false,
> >> +       vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
> >>                           (struct vring_desc *)(uintptr_t)vq->desc_addr,
> >>                           (struct vring_avail *)
> >>                           (uintptr_t)vq->driver_addr,
> >> --
> >> 2.38.1
> >>
> >
> >I think this is definitely an improvement, but I'd say we should go a
> >step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As
> >you point out in the patch message it is not a max anymore.
>
> I'm not sure about renaming since it is the value returned by
> vdpasim_get_vq_num_max, so IMHO the _MAX suffix is fine.

Oh that's a very good point. But then I guess a conformant driver
should never set more descriptors than that.

Would it be convenient to make the default queue size of 32768 and let
the guest specify less descriptors than that? Default configuration
will consume more memory then.

> But I admit that initially I didn't understand whether it's the maximum
> number of queues or elements, so maybe VDPASIM_VQ_NUM_MAX is better.
>
> >
> >Another thing to note is that we don't have a way to report that
> >userspace indicated a bad value for queue length. With the current
> >code vringh will not initialize at all if I'm not wrong, so we should
> >prevent userspace to put a bad num.
>
> Right!
>
> >
> >Ideally, we should repeat the tests of vring_init_kern at
> >vdpasim_set_vq_num. We could either call it with NULL vring addresses
> >to check for -EINVAL, or simply repeat the conditional (!num || num >
> >0xffff || (num & (num - 1))). I'd say the first one is better to not
> >go out of sync.
>
> Or we could do the check in vdpasim_set_vq_ready() and set it not ready
> if the vq_num is wrong.
>

Maybe it is the right place to do it, but the device is initiated at
that point so the driver needs to perform a full reset.

As a reference, qemu will retain the last valid size set to a vq, or
the default. This is because it ignores the bad values systematically.
Not sure what is more conformant actually :).


> >
> >All of that can be done on top anyway, so for this patch:
> >
> >Acked-by: Eugenio Pérez <eperezma@...hat.com>
> >
>
> Thanks for the review,
> Stefano
>