lkml.org 
[lkml]   [2018]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed
Date
Hi Alex,
After days of intensive test, the bug did not show up any more.
So I think the new patch does solve this problem.

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Friday, April 20, 2018 3:55 AM
> To: xuyandong <xuyandong2@huawei.com>
> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; wangxin (U)
> <wangxinxin.wang@huawei.com>; Kirti Wankhede
> <kwankhede@nvidia.com>
> Subject: Re: [PATCH] vfio iommu type1: no need to check task->mm if task
> has been destroyed
>
> On Thu, 19 Apr 2018 10:19:26 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
> > [cc +Kirti]
> >
> > On Wed, 18 Apr 2018 18:55:45 +0800
> > Xu Yandong <xuyandong2@huawei.com> wrote:
> >
> > > The task structure in vfio_dma struct used to identify the same task
> > > who map it or other task who shares same adress space is allowed to
> > > unmap. But if the task who map it has exited, mm of the task has
> > > been set to null, we should unmap the vfio dma directly.
> > >
> > > Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> > > ---
> > > Hi all,
> > > When I unplug a vcpu from a VM lanched with a VFIO hostdev device, I
> > > found that the *vfio_dma* mapped by this vcpu task could not be
> > > unmaped in the future, so I send this patch to unmap vfio_dma
> > > directly if the task who mapped it has exited.
> > >
> > > Howerver this patch may introduce a new security risk because any task
> can
> > > unmap the *vfio_dma* if the mapper task has exited.
> >
> > Well that's unexpected, but adding some debugging code I can clearly
> > see that the map and unmap ioctls are typically called by the various
> > processor threads, which all share the same mm_struct (so accounting
> > is correct regardless of which CPU does the unmap). I don't think the
> > fix below is correct though, it's not for a security risk, but for
> > accounting issue and correctness issues. The pages are mapped and
> > accounted against the users locked memory limits, if we simply bail
> > out, both the IOMMU mappings and the limit accounting are wrong.
> > Perhaps rather than referencing the calling task_struct in the
> > vfio_dma on mapping, we should traverse to the highest parent task
> > sharing the same mm_struct. Kirti, any thoughts since this code
> > originated for mdev support? Thanks,
>
> I think something like below is a better solution. More research required on
> group_leader and if it needs to be sanity tested or if testing mm_struct is
> redundant, but I think it should resolve the failing test case, all mappings
> reference the same task regardless of which vCPU triggers it. Thanks,
>
> Alex
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c index 5c212bf29640..3a1d3695c3fb
> 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1093,6 +1093,7 @@ static int vfio_dma_do_map(struct vfio_iommu
> *iommu,
> int ret = 0, prot = 0;
> uint64_t mask;
> struct vfio_dma *dma;
> + struct task_struct *task;
>
> /* Verify that none of our __u64 fields overflow */
> if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> @@ -1131,8 +1132,12 @@ static int vfio_dma_do_map(struct vfio_iommu
> *iommu,
> dma->iova = iova;
> dma->vaddr = vaddr;
> dma->prot = prot;
> - get_task_struct(current);
> - dma->task = current;
> +
> + task = (current->mm == current->group_leader->mm ?
> + current->group_leader : current);
> + get_task_struct(task);
> + dma->task = task;
> +
> dma->pfn_list = RB_ROOT;
>
> /* Insert zero-sized and grow as we map chunks of it */
>
>
\
 
 \ /
  Last update: 2018-04-20 13:53    [W:0.040 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site