From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Subject: Re: [RFC PATCH 30/30] vfio: Allow to bind foreign task To: Jean-Philippe Brucker References: <20170227195441.5170-1-jean-philippe.brucker@arm.com> <20170227195441.5170-31-jean-philippe.brucker@arm.com> From: Tomasz Nowicki Message-ID: Date: Wed, 26 Apr 2017 09:25:38 +0200 MIME-Version: 1.0 In-Reply-To: <20170227195441.5170-31-jean-philippe.brucker@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Lorenzo Pieralisi , Shanker Donthineni , kvm@vger.kernel.org, Catalin Marinas , Joerg Roedel , Sinan Kaya , Will Deacon , Alex Williamson , Harv Abdulhamid , iommu@lists.linux-foundation.org, linux-pci@vger.kernel.org, Bjorn Helgaas , Robin Murphy , David Woodhouse , linux-arm-kernel@lists.infradead.org, Nate Watterson Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: On 27.02.2017 20:54, Jean-Philippe Brucker wrote: > Let the process that owns the device create an address space bond on > behalf of another process. We add a pid argument to the BIND_TASK ioctl, > allowing the caller to bind a foreign task. The expected program flow in > this case is: > > * Process A creates the VFIO context and initializes the device. > * Process B asks A to bind its address space. > * Process A issues an ioctl to the VFIO device fd with BIND_TASK(pid). > It may communicate the given PASID back to process B or keep track of it > internally. > * Process B asks A to perform transactions on its virtual address. > * Process A launches transaction tagged with the given PASID. > > Signed-off-by: Jean-Philippe Brucker > --- > drivers/vfio/vfio.c | 35 +++++++++++++++++++++++++++++++++-- > include/uapi/linux/vfio.h | 15 +++++++++++++++ > 2 files changed, 48 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index c4505d8f4c61..ecc5d07e3dbb 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1660,7 +1661,7 @@ static long vfio_svm_ioctl(struct vfio_device *device, unsigned int cmd, > struct vfio_device_svm svm; > struct vfio_task *vfio_task; > > - minsz = offsetofend(struct vfio_device_svm, pasid); > + minsz = offsetofend(struct vfio_device_svm, pid); > > if (copy_from_user(&svm, (void __user *)arg, minsz)) > return -EFAULT; > @@ -1669,9 +1670,39 @@ static long vfio_svm_ioctl(struct vfio_device *device, unsigned int cmd, > return -EINVAL; > > if (cmd == VFIO_DEVICE_BIND_TASK) { > - struct task_struct *task = current; > + struct mm_struct *mm; > + struct task_struct *task; > + > + if (svm.flags & ~VFIO_SVM_PID) > + return -EINVAL; > + > + if (svm.flags & VFIO_SVM_PID) { > + rcu_read_lock(); > + task = find_task_by_vpid(svm.pid); > + if (task) > + get_task_struct(task); > + rcu_read_unlock(); > + if (!task) > + return -ESRCH; > + > + /* > + * Ensure process has RW access on the task's mm > + * FIXME: > + * - I think this ought to be in the IOMMU API > + * - I'm assuming permission is never revoked during the > + * task's lifetime. Might be mistaken. > + */ > + mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS); > + if (!mm || IS_ERR(mm)) I know this is RFC patch but considering we will keep this as is, we need here: +put_task_struct(task); > + return IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; > + mmput(mm); > + } else { > + get_task_struct(current); > + task = current; > + } > > ret = iommu_bind_task(device->dev, task, &svm.pasid, 0, NULL); > + put_task_struct(task); > if (ret) > return ret; > Thanks, Tomasz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Nowicki Subject: Re: [RFC PATCH 30/30] vfio: Allow to bind foreign task Date: Wed, 26 Apr 2017 09:25:38 +0200 Message-ID: References: <20170227195441.5170-1-jean-philippe.brucker@arm.com> <20170227195441.5170-31-jean-philippe.brucker@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: Lorenzo Pieralisi , Shanker Donthineni , kvm@vger.kernel.org, Catalin Marinas , Joerg Roedel , Sinan Kaya , Will Deacon , iommu@lists.linux-foundation.org, Harv Abdulhamid , Alex Williamson , linux-pci@vger.kernel.org, Bjorn Helgaas , Robin Murphy , David Woodhouse , linux-arm-kernel@lists.infradead.org, Nate Watterson To: Jean-Philippe Brucker Return-path: In-Reply-To: <20170227195441.5170-31-jean-philippe.brucker@arm.com> Sender: linux-pci-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 27.02.2017 20:54, Jean-Philippe Brucker wrote: > Let the process that owns the device create an address space bond on > behalf of another process. We add a pid argument to the BIND_TASK ioctl, > allowing the caller to bind a foreign task. The expected program flow in > this case is: > > * Process A creates the VFIO context and initializes the device. > * Process B asks A to bind its address space. > * Process A issues an ioctl to the VFIO device fd with BIND_TASK(pid). > It may communicate the given PASID back to process B or keep track of it > internally. > * Process B asks A to perform transactions on its virtual address. > * Process A launches transaction tagged with the given PASID. > > Signed-off-by: Jean-Philippe Brucker > --- > drivers/vfio/vfio.c | 35 +++++++++++++++++++++++++++++++++-- > include/uapi/linux/vfio.h | 15 +++++++++++++++ > 2 files changed, 48 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index c4505d8f4c61..ecc5d07e3dbb 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1660,7 +1661,7 @@ static long vfio_svm_ioctl(struct vfio_device *device, unsigned int cmd, > struct vfio_device_svm svm; > struct vfio_task *vfio_task; > > - minsz = offsetofend(struct vfio_device_svm, pasid); > + minsz = offsetofend(struct vfio_device_svm, pid); > > if (copy_from_user(&svm, (void __user *)arg, minsz)) > return -EFAULT; > @@ -1669,9 +1670,39 @@ static long vfio_svm_ioctl(struct vfio_device *device, unsigned int cmd, > return -EINVAL; > > if (cmd == VFIO_DEVICE_BIND_TASK) { > - struct task_struct *task = current; > + struct mm_struct *mm; > + struct task_struct *task; > + > + if (svm.flags & ~VFIO_SVM_PID) > + return -EINVAL; > + > + if (svm.flags & VFIO_SVM_PID) { > + rcu_read_lock(); > + task = find_task_by_vpid(svm.pid); > + if (task) > + get_task_struct(task); > + rcu_read_unlock(); > + if (!task) > + return -ESRCH; > + > + /* > + * Ensure process has RW access on the task's mm > + * FIXME: > + * - I think this ought to be in the IOMMU API > + * - I'm assuming permission is never revoked during the > + * task's lifetime. Might be mistaken. > + */ > + mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS); > + if (!mm || IS_ERR(mm)) I know this is RFC patch but considering we will keep this as is, we need here: +put_task_struct(task); > + return IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; > + mmput(mm); > + } else { > + get_task_struct(current); > + task = current; > + } > > ret = iommu_bind_task(device->dev, task, &svm.pasid, 0, NULL); > + put_task_struct(task); > if (ret) > return ret; > Thanks, Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 From: tn@semihalf.com (Tomasz Nowicki) Date: Wed, 26 Apr 2017 09:25:38 +0200 Subject: [RFC PATCH 30/30] vfio: Allow to bind foreign task In-Reply-To: <20170227195441.5170-31-jean-philippe.brucker@arm.com> References: <20170227195441.5170-1-jean-philippe.brucker@arm.com> <20170227195441.5170-31-jean-philippe.brucker@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 27.02.2017 20:54, Jean-Philippe Brucker wrote: > Let the process that owns the device create an address space bond on > behalf of another process. We add a pid argument to the BIND_TASK ioctl, > allowing the caller to bind a foreign task. The expected program flow in > this case is: > > * Process A creates the VFIO context and initializes the device. > * Process B asks A to bind its address space. > * Process A issues an ioctl to the VFIO device fd with BIND_TASK(pid). > It may communicate the given PASID back to process B or keep track of it > internally. > * Process B asks A to perform transactions on its virtual address. > * Process A launches transaction tagged with the given PASID. > > Signed-off-by: Jean-Philippe Brucker > --- > drivers/vfio/vfio.c | 35 +++++++++++++++++++++++++++++++++-- > include/uapi/linux/vfio.h | 15 +++++++++++++++ > 2 files changed, 48 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index c4505d8f4c61..ecc5d07e3dbb 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1660,7 +1661,7 @@ static long vfio_svm_ioctl(struct vfio_device *device, unsigned int cmd, > struct vfio_device_svm svm; > struct vfio_task *vfio_task; > > - minsz = offsetofend(struct vfio_device_svm, pasid); > + minsz = offsetofend(struct vfio_device_svm, pid); > > if (copy_from_user(&svm, (void __user *)arg, minsz)) > return -EFAULT; > @@ -1669,9 +1670,39 @@ static long vfio_svm_ioctl(struct vfio_device *device, unsigned int cmd, > return -EINVAL; > > if (cmd == VFIO_DEVICE_BIND_TASK) { > - struct task_struct *task = current; > + struct mm_struct *mm; > + struct task_struct *task; > + > + if (svm.flags & ~VFIO_SVM_PID) > + return -EINVAL; > + > + if (svm.flags & VFIO_SVM_PID) { > + rcu_read_lock(); > + task = find_task_by_vpid(svm.pid); > + if (task) > + get_task_struct(task); > + rcu_read_unlock(); > + if (!task) > + return -ESRCH; > + > + /* > + * Ensure process has RW access on the task's mm > + * FIXME: > + * - I think this ought to be in the IOMMU API > + * - I'm assuming permission is never revoked during the > + * task's lifetime. Might be mistaken. > + */ > + mm = mm_access(task, PTRACE_MODE_ATTACH_REALCREDS); > + if (!mm || IS_ERR(mm)) I know this is RFC patch but considering we will keep this as is, we need here: +put_task_struct(task); > + return IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; > + mmput(mm); > + } else { > + get_task_struct(current); > + task = current; > + } > > ret = iommu_bind_task(device->dev, task, &svm.pasid, 0, NULL); > + put_task_struct(task); > if (ret) > return ret; > Thanks, Tomasz