On Mon, Jul 15, 2024 at 10:26:13AM +0800, Jason Wang wrote:
> On Fri, Jul 12, 2024 at 9:19 PM Steve Sistare <steven.sistare@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
Not pid pls.
> > + 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.
>
> Thanks
