linux-kernel - Re: [PATCH 2/2] vringh: fetch used_idx from vring at vringh_init

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: <CAJaqyWetovvndcU=pu_kPNUNYkgao=HsENnrKCzoHdK7RBjyAQ@mail.gmail.com>
Date:   Thu, 19 Jan 2023 09:10:45 +0100
From:   Eugenio Perez Martin <eperezma@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     mst@...hat.com, leiyang@...hat.com,
        Laurent Vivier <lvivier@...hat.com>, sgarzare@...hat.com,
        Zhu Lingshan <lingshan.zhu@...el.com>,
        virtualization@...ts.linux-foundation.org, si-wei.liu@...cle.com,
        linux-kernel@...r.kernel.org, lulu@...hat.com,
        Gautam Dawar <gdawar@...inx.com>, alvaro.karsz@...id-run.com
Subject: Re: [PATCH 2/2] vringh: fetch used_idx from vring at vringh_init_iotlb

On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@...hat.com> wrote:
>
> On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@...hat.com> wrote:
> >
> > Starting from an used_idx different than 0 is needed in use cases like
> > virtual machine migration.  Not doing so and letting the caller set an
> > avail idx different than 0 causes destination device to try to use old
> > buffers that source driver already recover and are not available
> > anymore.
> >
> > While callers like vdpa_sim set avail_idx directly it does not set
> > used_idx.  Instead of let the caller do the assignment, fetch it from
> > the guest at initialization like vhost-kernel do.
> >
> > To perform the same at vring_kernel_init and vring_user_init is left for
> > the future.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
> > ---
> >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 33eb941fcf15..0eed825197f2 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> >         return 0;
> >  }
> >
> > +/**
> > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > + * @vrh: The vring.
> > + *
> > + * Returns -errno or 0.
> > + */
> > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > +{
> > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > +}
> > +
> >  /**
> >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> >   * @vrh: the vringh to initialize.
> > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> >                       struct vring_avail *avail,
> >                       struct vring_used *used)
> >  {
>
> While at this, I wonder if it's better to have a dedicated parameter
> for last_avail_idx?
>

I also had that thought. To directly assign last_avail_idx is not a
specially elegant API IMO.

Maybe expose a way to fetch used_idx from device vring and pass
used_idx as parameter too?

> > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > -                               desc, avail, used);
> > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > +                                avail, used);
> > +
> > +       if (r != 0)
> > +               return r;
> > +
> > +       /* Consider the ring not initialized */
> > +       if ((void *)desc == used)
> > +               return 0;
>
> I don't understand when we can get this (actually it should be a bug
> of the caller).
>

You can see it in vdpasim_vq_reset.

Note that to consider desc == 0 to be an uninitialized ring is a bug
IMO. QEMU considers it that way also, but the standard does not forbid
any ring to be at address 0. Especially if we use vIOMMU.

So I think the best way to know if we can use the vringh is either
this way, or provide an explicit "initialized" boolean attribute.
Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
add new attributes.

Thanks!

> Thanks
>
> > +
> > +       return vringh_update_used_idx(vrh);
> > +
> >  }
> >  EXPORT_SYMBOL(vringh_init_iotlb);
> >
> > --
> > 2.31.1
> >
>