Re: [PATCH v3 7/9] vhost: Add new UAPI to support change to task mode

[Stefano Garzarella]

On Thu, Nov 07, 2024 at 03:12:49PM +0800, Cindy Lu wrote:
> On Tue, Nov 5, 2024 at 6:32 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
> > On Tue, Nov 05, 2024 at 03:25:26PM +0800, Cindy Lu wrote:
> > >Add a new UAPI to enable setting the vhost device to task mode.
> > >The userspace application can use VHOST_SET_INHERIT_FROM_OWNER
> > >to configure the mode if necessary.
> > >This setting must be applied before VHOST_SET_OWNER, as the worker
> > >will be created in the VHOST_SET_OWNER function
> > >
> > >Signed-off-by: Cindy Lu <lulu@xxxxxxxxxx>
> > >---
> > > drivers/vhost/vhost.c      | 15 ++++++++++++++-
> > > include/uapi/linux/vhost.h |  2 ++
> > > 2 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > >index c17dc01febcc..70c793b63905 100644
> > >--- a/drivers/vhost/vhost.c
> > >+++ b/drivers/vhost/vhost.c
> > >@@ -2274,8 +2274,9 @@ 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;
> >
> > I don't know if something is missing in this patch, but I am confused:
> >
> > `r` is set few lines below...
> >
> > >       int i, fd;
> > >+      bool inherit_owner;
> > >
> > >       /* If you are not the owner, you can become one */
> > >       if (ioctl == VHOST_SET_OWNER) {
> > ...
> >
> >         /* You must be the owner to do anything else */
> >         r = vhost_dev_check_owner(d);
> >         if (r)
> >                 goto done;
> >
> > So, why we are now initializing it to 0?
> r = 0 mean return successfully here.
> Therefore, in the case VHOST_SET_INHERIT_FROM_OWNER function, I don't
> need to set it again and can simply return.
> ....
>    if (vhost_dev_has_owner(d))
>       break;
> .....

Okay, but vhost_dev_check_owner() already set it to 0, so we can avoid 
that, no?

> > >@@ -2332,6 +2333,18 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > >               if (ctx)
> > >                       eventfd_ctx_put(ctx);
> > >               break;
> > >+      case VHOST_SET_INHERIT_FROM_OWNER:
> > >+              /*inherit_owner can only be modified before owner is set*/
> > >+              if (vhost_dev_has_owner(d))
> >
> > And here, how this check can be false, if at the beginning of the
> > function we call vhost_dev_check_owner()?
> >
> > Maybe your intention was to add this code before the
> > `vhost_dev_check_owner()` call, so this should explain why initialize
> > `r` to 0, but I'm not sure.
> Yes, in the function beginning, the code is
> if (ioctl == VHOST_SET_OWNER) {
> r = vhost_dev_set_owner(d);
> goto done;
> }
> if the ioctl is not VHOST_SET_OWNER,  then the  code will not run the
> function vhost_dev_set_owner.

Sorry, I meant vhost_dev_check_owner(), not vhost_dev_set_owner().

I'll try to explain again.

After applying this series we have this code:

long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
{
	struct eventfd_ctx *ctx;
	u64 p;
	long r = 0;
	int i, fd;
	bool inherit_owner;

	/* If you are not the owner, you can become one */
	if (ioctl == VHOST_SET_OWNER) {
		r = vhost_dev_set_owner(d);
		goto done;
	}

	/* You must be the owner to do anything else */
	r = vhost_dev_check_owner(d);
	if (r)
		goto done;

	switch (ioctl) {
	...
    	case VHOST_SET_INHERIT_FROM_OWNER:
		/*inherit_owner can only be modified before owner is 
		 * set*/
		if (vhost_dev_has_owner(d))
			break;

IIUC this check is always true, so we always call `break` because at
the beginning of this function we call vhost_dev_check_owner() which
if `dev->mm != current->mm` (so it can't be null I guess) jumps directly
into `done`, returning an error.

So I still don't understand in which condition we can run the code after
this check.

Thanks,
Stefano

		if (copy_from_user(&inherit_owner, argp,
				   sizeof(inherit_owner))) {
			r = -EFAULT;
			break;
		}
		d->inherit_owner = inherit_owner;
		break;


> This ioctl is used by userspace applications, so we cannot be certain
> of the type and sequence of their calls; therefore, I added this
> check.
>
> > >+                      break;
> >
> > Should we return an error (e.g. -EPERM) in this case?
> sure，will add this back
> thanks
> Cindy
> > >+
> > >+              if (copy_from_user(&inherit_owner, argp,
> > >+                                 sizeof(inherit_owner))) {
> > >+                      r = -EFAULT;
> > >+                      break;
> > >+              }
> > >+              d->inherit_owner = inherit_owner;
> > >+              break;
> > >       default:
> > >               r = -ENOIOCTLCMD;
> > >               break;
> > >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 a documentation here, this is UAPI, so the user should
> > know what this ioctl does based on the parameter.
> >
> > Thanks,
> > Stefano
> >
> > >+#define VHOST_SET_INHERIT_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, bool)
> > > #endif
> > >--
> > >2.45.0
> > >