Re: [PATCH V2 3/7] vhost-vdpa: VHOST_NEW_OWNER

[Steven Sistare <steven.sistare@oracle.com>]

On 7/16/2024 1:16 AM, Jason Wang wrote:
> On Mon, Jul 15, 2024 at 10:27 PM Steven Sistare
> <steven.sistare@oracle.com> wrote:
>>
>> On 7/14/2024 10:26 PM, Jason Wang wrote:
>>> On Fri, Jul 12, 2024 at 9:19 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>
>>>> Add an ioctl to transfer file descriptor ownership and pinned memory
>>>> accounting from one process to another.
>>>>
>>>> This is more efficient than VHOST_RESET_OWNER followed by VHOST_SET_OWNER,
>>>> as that would unpin all physical pages, requiring them to be repinned in
>>>> the new process.  That would cost multiple seconds for large memories, and
>>>> be incurred during a virtual machine's pause time during live update.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>    drivers/vhost/vdpa.c       | 41 ++++++++++++++++++++++++++++++++++++++
>>>>    drivers/vhost/vhost.c      | 15 ++++++++++++++
>>>>    drivers/vhost/vhost.h      |  1 +
>>>>    include/uapi/linux/vhost.h | 10 ++++++++++
>>>>    4 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>> index b49e5831b3f0..5cf55ca4ec02 100644
>>>> --- a/drivers/vhost/vdpa.c
>>>> +++ b/drivers/vhost/vdpa.c
>>>> @@ -632,6 +632,44 @@ static long vhost_vdpa_resume(struct vhost_vdpa *v)
>>>>           return ret;
>>>>    }
>>>>
>>>> +static long vhost_vdpa_new_owner(struct vhost_vdpa *v)
>>>> +{
>>>> +       int r;
>>>> +       struct vhost_dev *vdev = &v->vdev;
>>>> +       struct mm_struct *mm_old = vdev->mm;
>>>> +       struct mm_struct *mm_new = current->mm;
>>>> +       long pinned_vm = v->pinned_vm;
>>>> +       unsigned long lock_limit = PFN_DOWN(rlimit(RLIMIT_MEMLOCK));
>>>> +
>>>> +       if (!mm_old)
>>>> +               return -EINVAL;
>>>> +       mmgrab(mm_old);
>>>> +
>>>> +       if (!v->vdpa->use_va &&
>>>> +           pinned_vm + atomic64_read(&mm_new->pinned_vm) > lock_limit) {
>>>> +               r = -ENOMEM;
>>>> +               goto out;
>>>> +       }
>>>
>>> So this seems to allow an arbitrary process to execute this. Seems to be unsafe.
>>>
>>> I wonder if we need to add some checks here, maybe PID or other stuff
>>> to only allow the owner process to do this.
>>
>> The original owner must send the file descriptor to the new owner.
> 
> This seems not to be in the steps you put in the cover letter.

It's there:
   "The vdpa device descriptor, fd, remains open across the exec."

But, I can say more about how fd visibility constitutes permission to changer
owner in this commit message.

>> That constitutes permission to take ownership.
> 
> This seems like a relaxed version of the reset_owner:
> 
> Currently, reset_owner have the following check:

Not relaxed, just different.  A process cannot do anything with fd if it
is not the owner, *except* for becoming the new owner.  Holding the fd is
like holding a key.

- Steve

> /* Caller should have device mutex */
> long vhost_dev_check_owner(struct vhost_dev *dev)
> {
>          /* Are you the owner? If not, I don't think you mean to do that */
>          return dev->mm == current->mm ? 0 : -EPERM;
> }
> EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
> 
> It means even if the fd is passed to some other process, the reset
> owner won't work there.
> 
> Thanks
> 
>>
>>>> +       r = vhost_vdpa_bind_mm(v, mm_new);
>>>> +       if (r)
>>>> +               goto out;
>>>> +
>>>> +       r = vhost_dev_new_owner(vdev);
>>>> +       if (r) {
>>>> +               vhost_vdpa_bind_mm(v, mm_old);
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       if (!v->vdpa->use_va) {
>>>> +               atomic64_sub(pinned_vm, &mm_old->pinned_vm);
>>>> +               atomic64_add(pinned_vm, &mm_new->pinned_vm);
>>>> +       }
>>>> +
>>>> +out:
>>>> +       mmdrop(mm_old);
>>>> +       return r;
>>>> +}
>>>> +
>>>>    static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>>>>                                      void __user *argp)
>>>>    {
>>>> @@ -876,6 +914,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>>>>           case VHOST_VDPA_RESUME:
>>>>                   r = vhost_vdpa_resume(v);
>>>>                   break;
>>>> +       case VHOST_NEW_OWNER:
>>>> +               r = vhost_vdpa_new_owner(v);
>>>> +               break;
>>>>           default:
>>>>                   r = vhost_dev_ioctl(&v->vdev, cmd, argp);
>>>>                   if (r == -ENOIOCTLCMD)
>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>> index b60955682474..ab40ae50552f 100644
>>>> --- a/drivers/vhost/vhost.c
>>>> +++ b/drivers/vhost/vhost.c
>>>> @@ -963,6 +963,21 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(vhost_dev_set_owner);
>>>>
>>>> +/* Caller should have device mutex */
>>>> +long vhost_dev_new_owner(struct vhost_dev *dev)
>>>> +{
>>>> +       if (dev->mm == current->mm)
>>>> +               return -EBUSY;
>>>> +
>>>> +       if (!vhost_dev_has_owner(dev))
>>>> +               return -EINVAL;
>>>> +
>>>> +       vhost_detach_mm(dev);
>>>> +       vhost_attach_mm(dev);
>>>
>>> This seems to do nothing unless I miss something.
>>
>> vhost_detach mm drops dev->mm.
>> vhost_attach_mm grabs current->mm.
>>
>> - Steve
>>
>