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: <CAJaqyWf2F_yBLBjj1RiPeJ92_zfq8BSMz8Pak2Vg6QinN8jS1Q@mail.gmail.com>
Date: Tue, 4 Jul 2023 12:25:32 +0200
From: Eugenio Perez Martin <eperezma@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Shannon Nelson <shannon.nelson@....com>, 
	virtualization@...ts.linux-foundation.org, kvm@...r.kernel.org
Subject: Re: [PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not
 support it

On Mon, Jul 3, 2023 at 4:52 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> On Mon, Jul 03, 2023 at 04:22:18PM +0200, Eugenio Pérez wrote:
> > With the current code it is accepted as long as userland send it.
> >
> > Although userland should not set a feature flag that has not been
> > offered to it with VHOST_GET_BACKEND_FEATURES, the current code will not
> > complain for it.
> >
> > Since there is no specific reason for any parent to reject that backend
> > feature bit when it has been proposed, let's control it at vdpa frontend
> > level. Future patches may move this control to the parent driver.
> >
> > Fixes: 967800d2d52e ("vdpa: accept VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend feature")
> > Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
>
> Please do send v3. And again, I don't want to send "after driver ok" hack
> upstream at all, I merged it in next just to give it some testing.
> We want RING_ACCESS_AFTER_KICK or some such.
>

Current devices do not support that semantic. My plan was to convert
it in vp_vdpa if needed, and reuse the current vdpa ops. Sorry if I
was not explicit enough.

The only solution I can see to that is to trap & emulate in the vdpa
(parent?) driver, as talked in virtio-comment. But that complicates
the architecture:
* Offer VHOST_BACKEND_F_RING_ACCESS_AFTER_KICK
* Store vq enable state separately, at
vdpa->config->set_vq_ready(true), but not transmit that enable to hw
* Store the doorbell state separately, but do not configure it to the
device directly.

But how to recover if the device cannot configure them at kick time,
for example?

Maybe we can just fail if the parent driver does not support enabling
the vq after DRIVER_OK? That way no new feature flag is needed.

Thanks!

>
> > ---
> > Sent with Fixes: tag pointing to git.kernel.org/pub/scm/linux/kernel/git/mst
> > commit. Please let me know if I should send a v3 of [1] instead.
> >
> > [1] https://lore.kernel.org/lkml/20230609121244-mutt-send-email-mst@kernel.org/T/
> > ---
> >  drivers/vhost/vdpa.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index e1abf29fed5b..a7e554352351 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> >  {
> >       struct vhost_vdpa *v = filep->private_data;
> >       struct vhost_dev *d = &v->vdev;
> > +     const struct vdpa_config_ops *ops = v->vdpa->config;
> >       void __user *argp = (void __user *)arg;
> >       u64 __user *featurep = argp;
> > -     u64 features;
> > +     u64 features, parent_features = 0;
> >       long r = 0;
> >
> >       if (cmd == VHOST_SET_BACKEND_FEATURES) {
> >               if (copy_from_user(&features, featurep, sizeof(features)))
> >                       return -EFAULT;
> > +             if (ops->get_backend_features)
> > +                     parent_features = ops->get_backend_features(v->vdpa);
> >               if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> >                                BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
> >                                BIT_ULL(VHOST_BACKEND_F_RESUME) |
> > -                              BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
> > +                              parent_features))
> >                       return -EOPNOTSUPP;
> >               if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
> >                    !vhost_vdpa_can_suspend(v))
> > --
> > 2.39.3
>


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ