lkml.org 
[lkml]   [2013]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] vfio: Limit group opens
From
Date
On Sat, 2013-06-22 at 12:44 +1000, Alexey Kardashevskiy wrote:
> On 06/22/2013 11:26 AM, Alex Williamson wrote:
> > On Sat, 2013-06-22 at 11:16 +1000, Alexey Kardashevskiy wrote:
> >> Cool, thanks!
> >>
> >> So we will need only this (to be called from KVM), and that will be it, right?
> >
> > For what? This is not the external lock you're looking for. As I've
> > mentioned, the file can only hold the group, but that doesn't give you
> > any guarantee that the group is protected by the IOMMU. Thanks,
>
>
> I am confused, sorry :) With this patch, a group fd cannot be reopened if
> already opened, and this is the only way for user space to take control
> over a group. If it is not an external lock, then what is it? And all I
> have to do now is to verify that the group fd passed to KVM is correct and
> I am happy. Who and how can break anything (group? KVM?) now?

By that logic all a user needs to do is open() a group and they they're
free to pass the fd to KVM, right? But the IOMMU protection isn't
enabled until the user calls SET_CONTAINER and SET_IOMMU, so you'd be
giving KVM access to the IOMMU that the user hasn't enabled. The group
may still have devices attached to host drivers. Likewise, a user need
only call UNSET_CONTAINER to teardown the IOMMU protection. At that
point a device could be re-bound to host drivers, thus making it unsafe
for KVM to be directly poking the IOMMU.

This patch is just a bug fix for inconsistent behavior. Thanks,

Alex

> >> int vfio_group_iommu_id_from_file(struct file *filep)
> >> ...
> >>
> >>
> >>
> >> On 06/22/2013 07:12 AM, Alex Williamson wrote:
> >>> vfio_group_fops_open attempts to limit concurrent sessions by
> >>> disallowing opens once group->container is set. This really doesn't
> >>> do what we want and allow for inconsistent behavior, for instance a
> >>> group can be opened twice, then a container set giving the user two
> >>> file descriptors to the group. But then it won't allow more to be
> >>> opened. There's not much reason to have the group opened multiple
> >>> times since most access is through devices or the container, so
> >>> complete what the original code intended and only allow a single
> >>> instance.
> >>>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>> ---
> >>> drivers/vfio/vfio.c | 14 ++++++++++++++
> >>> 1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >>> index 6d78736..d30f44d 100644
> >>> --- a/drivers/vfio/vfio.c
> >>> +++ b/drivers/vfio/vfio.c
> >>> @@ -76,6 +76,7 @@ struct vfio_group {
> >>> struct notifier_block nb;
> >>> struct list_head vfio_next;
> >>> struct list_head container_next;
> >>> + atomic_t opened;
> >>> };
> >>>
> >>> struct vfio_device {
> >>> @@ -206,6 +207,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
> >>> INIT_LIST_HEAD(&group->device_list);
> >>> mutex_init(&group->device_lock);
> >>> atomic_set(&group->container_users, 0);
> >>> + atomic_set(&group->opened, 0);
> >>> group->iommu_group = iommu_group;
> >>>
> >>> group->nb.notifier_call = vfio_iommu_group_notifier;
> >>> @@ -1236,12 +1238,22 @@ static long vfio_group_fops_compat_ioctl(struct file *filep,
> >>> static int vfio_group_fops_open(struct inode *inode, struct file *filep)
> >>> {
> >>> struct vfio_group *group;
> >>> + int opened;
> >>>
> >>> group = vfio_group_get_from_minor(iminor(inode));
> >>> if (!group)
> >>> return -ENODEV;
> >>>
> >>> + /* Do we need multiple instances of the group open? Seems not. */
> >>> + opened = atomic_cmpxchg(&group->opened, 0, 1);
> >>> + if (opened) {
> >>> + vfio_group_put(group);
> >>> + return -EBUSY;
> >>> + }
> >>> +
> >>> + /* Is something still in use from a previous open? */
> >>> if (group->container) {
> >>> + atomic_dec(&group->opened);
> >>> vfio_group_put(group);
> >>> return -EBUSY;
> >>> }
> >>> @@ -1259,6 +1271,8 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
> >>>
> >>> vfio_group_try_dissolve_container(group);
> >>>
> >>> + atomic_dec(&group->opened);
> >>> +
> >>> vfio_group_put(group);
> >>>
> >>> return 0;
> >>>
> >>
> >>
> >
> >
> >
>
>





\
 
 \ /
  Last update: 2013-06-22 05:21    [W:0.081 / U:0.812 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site