1. Patchew
  2. linux
  3. [PATCH v2 0/7] vhost: Add support of kthread API
  4. View patch

[PATCH v2 7/7] vhost: Add new UAPI to support change to task mode

Cindy Lu posted 7 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v2 7/7] vhost: Add new UAPI to support change to task mode
Posted by Cindy Lu 1 month, 3 weeks ago 
Add a new UAPI to support setting the vhost device to
use task mode. The user space application needs to use
VHOST_SET_INHERIT_FROM_OWNER to set the mode.
This setting must be set before VHOST_SET_OWNER is set.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c      | 18 +++++++++++++++++-
 include/uapi/linux/vhost.h |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 08c9e77916ca..0e5c81026acd 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2341,8 +2341,24 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 {
 	struct eventfd_ctx *ctx;
 	u64 p;
-	long r;
+	long r = 0;
 	int i, fd;
+	bool inherit_owner;
+
+	if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
+		/* Is there an owner already? */
+		if (vhost_dev_has_owner(d)) {
+			r = -EBUSY;
+			goto done;
+		}
+		if (copy_from_user(&inherit_owner, argp,
+				   sizeof(inherit_owner))) {
+			r = -EFAULT;
+			goto done;
+		}
+		enforce_inherit_owner = inherit_owner;
+		goto done;
+	}
 
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index b95dd84eef2d..1e192038633d 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,6 @@
  */
 #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
 					      struct vhost_vring_state)
+
+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
 #endif
-- 
2.45.0
Re: [PATCH v2 7/7] vhost: Add new UAPI to support change to task mode
Posted by Mike Christie 1 month, 2 weeks ago 
On 10/3/24 8:58 PM, Cindy Lu wrote:
> Add a new UAPI to support setting the vhost device to
> use task mode. The user space application needs to use
> VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> This setting must be set before VHOST_SET_OWNER is set.
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
>  include/uapi/linux/vhost.h |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 08c9e77916ca..0e5c81026acd 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2341,8 +2341,24 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  {
>  	struct eventfd_ctx *ctx;
>  	u64 p;
> -	long r;
> +	long r = 0;
>  	int i, fd;
> +	bool inherit_owner;
> +
> +	if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {

Maybe instead of a modparam and this ioctl we just want a new ioctl:

/*
 * This will setup the owner based on the calling thread instead of
 * using kthread.
 */
#define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)

It would initially be used by vhost-scsi when worker_per_virtqueue=true
since that is a new use case and there will be no regressions.

For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
tool XYZ can use the new ioctl when they are ready.
Re: [PATCH v2 7/7] vhost: Add new UAPI to support change to task mode
Posted by Michael S. Tsirkin 1 month, 1 week ago 
On Mon, Oct 14, 2024 at 03:56:33PM -0500, Mike Christie wrote:
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > Add a new UAPI to support setting the vhost device to
> > use task mode. The user space application needs to use
> > VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> > This setting must be set before VHOST_SET_OWNER is set.
> > 
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> >  include/uapi/linux/vhost.h |  2 ++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 08c9e77916ca..0e5c81026acd 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2341,8 +2341,24 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >  {
> >  	struct eventfd_ctx *ctx;
> >  	u64 p;
> > -	long r;
> > +	long r = 0;
> >  	int i, fd;
> > +	bool inherit_owner;
> > +
> > +	if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
> 
> Maybe instead of a modparam and this ioctl we just want a new ioctl:
> 
> /*
>  * This will setup the owner based on the calling thread instead of
>  * using kthread.
>  */
> #define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)

I feel this is not good because it is insecure -
process should not normally have a say in whether
namespaces work correctly.
So we want the system admin to be able to block the
old mode.

> It would initially be used by vhost-scsi when worker_per_virtqueue=true
> since that is a new use case and there will be no regressions.
> 
> For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
> tool XYZ can use the new ioctl when they are ready.

I do not like it that we switched default now we apparently will be
switching it back. Will break some userspace whatever we do.

-- 
MST
Re: [PATCH v2 7/7] vhost: Add new UAPI to support change to task mode
Posted by Jason Wang 1 month, 1 week ago 
On Tue, Oct 15, 2024 at 6:19 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Oct 14, 2024 at 03:56:33PM -0500, Mike Christie wrote:
> > On 10/3/24 8:58 PM, Cindy Lu wrote:
> > > Add a new UAPI to support setting the vhost device to
> > > use task mode. The user space application needs to use
> > > VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> > > This setting must be set before VHOST_SET_OWNER is set.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> > >  include/uapi/linux/vhost.h |  2 ++
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 08c9e77916ca..0e5c81026acd 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -2341,8 +2341,24 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > >  {
> > >     struct eventfd_ctx *ctx;
> > >     u64 p;
> > > -   long r;
> > > +   long r = 0;
> > >     int i, fd;
> > > +   bool inherit_owner;
> > > +
> > > +   if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
> >
> > Maybe instead of a modparam and this ioctl we just want a new ioctl:
> >
> > /*
> >  * This will setup the owner based on the calling thread instead of
> >  * using kthread.
> >  */
> > #define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)
>
> I feel this is not good because it is insecure -
> process should not normally have a say in whether
> namespaces work correctly.

Note there's still a lot of kthread users, so the "problem" is not
specific to vhost.

> So we want the system admin to be able to block the
> old mode.

Then we will break the userspace silently which seems not good.

>
> > It would initially be used by vhost-scsi when worker_per_virtqueue=true
> > since that is a new use case and there will be no regressions.
> >
> > For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
> > tool XYZ can use the new ioctl when they are ready.
>
> I do not like it that we switched default now we apparently will be
> switching it back. Will break some userspace whatever we do.
>
> --
> MST
>

Thanks
Re: [PATCH v2 7/7] vhost: Add new UAPI to support change to task mode
Posted by Cindy Lu 1 month, 2 weeks ago 
On Tue, 15 Oct 2024 at 04:56, Mike Christie <michael.christie@oracle.com> wrote:
>
> On 10/3/24 8:58 PM, Cindy Lu wrote:
> > Add a new UAPI to support setting the vhost device to
> > use task mode. The user space application needs to use
> > VHOST_SET_INHERIT_FROM_OWNER to set the mode.
> > This setting must be set before VHOST_SET_OWNER is set.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> >  include/uapi/linux/vhost.h |  2 ++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 08c9e77916ca..0e5c81026acd 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2341,8 +2341,24 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >  {
> >       struct eventfd_ctx *ctx;
> >       u64 p;
> > -     long r;
> > +     long r = 0;
> >       int i, fd;
> > +     bool inherit_owner;
> > +
> > +     if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {
>
> Maybe instead of a modparam and this ioctl we just want a new ioctl:
>
> /*
>  * This will setup the owner based on the calling thread instead of
>  * using kthread.
>  */
> #define VHOST_INHERIT_OWNER _IO(VHOST_VIRTIO, 0x83)
>
> It would initially be used by vhost-scsi when worker_per_virtqueue=true
> since that is a new use case and there will be no regressions.
>
> For the other cases we default to VHOST_SET_OWNER. Other QEMU cases or
> tool XYZ can use the new ioctl when they are ready.
>
If I understand correctly, this means the default vhost function is
using kthread?
Thanks
Cindy
Re: [PATCH v2 7/7] vhost: Add new UAPI to support change to task mode
Posted by Stefano Garzarella 1 month, 3 weeks ago 
On Fri, Oct 04, 2024 at 09:58:21AM GMT, Cindy Lu wrote:
>Add a new UAPI to support setting the vhost device to
>use task mode. The user space application needs to use
>VHOST_SET_INHERIT_FROM_OWNER to set the mode.
>This setting must be set before VHOST_SET_OWNER is set.

Why?

>
>Signed-off-by: Cindy Lu <lulu@redhat.com>
>---
> drivers/vhost/vhost.c      | 18 +++++++++++++++++-
> include/uapi/linux/vhost.h |  2 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 08c9e77916ca..0e5c81026acd 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -2341,8 +2341,24 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> {
> 	struct eventfd_ctx *ctx;
> 	u64 p;
>-	long r;
>+	long r = 0;
> 	int i, fd;
>+	bool inherit_owner;
              ^
              This can be moved ...
>+
>+	if (ioctl == VHOST_SET_INHERIT_FROM_OWNER) {

                 ^
                 ... here.

>+		/* Is there an owner already? */

This comment is superfluous, I would instead explain why there has to be 
an owner, what problem would we have if not?

>+		if (vhost_dev_has_owner(d)) {
>+			r = -EBUSY;
>+			goto done;
>+		}
>+		if (copy_from_user(&inherit_owner, argp,
>+				   sizeof(inherit_owner))) {
>+			r = -EFAULT;
>+			goto done;
>+		}
>+		enforce_inherit_owner = inherit_owner;

mmmm, I'm confused.

IIUC with this change, an user, for example, can call 
VHOST_SET_INHERIT_FROM_OWNER on /dev/vhost-vsock and change the 
behaviour of /dev/vhost-net.

Is that really what we want?

Maybe it's better if the module parameter controls the default of any 
device, but the ioctl changes the behavior only for that device.

>+		goto done;
>+	}
>
> 	/* If you are not the owner, you can become one */
> 	if (ioctl == VHOST_SET_OWNER) {
>diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>index b95dd84eef2d..1e192038633d 100644
>--- a/include/uapi/linux/vhost.h
>+++ b/include/uapi/linux/vhost.h
>@@ -235,4 +235,6 @@
>  */
> #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
> 					      struct vhost_vring_state)
>+

Please, add documentation here.

>+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
                                        ^
                                        Please, add a tab like we did
                                        for all previous definitions.

Thanks,
Stefano

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.