On 7/16/2024 1:16 AM, Jason Wang wrote:
On Mon, Jul 15, 2024 at 10:27 PM Steven Sistare
<steven.sistare@xxxxxxxxxx> wrote:
On 7/14/2024 10:26 PM, 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.
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
