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 <[email protected]>
---
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;
Cool, thanks!
So we will need only this (to be called from KVM), and that will be it, right?
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 <[email protected]>
> ---
> 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;
>
--
Alexey
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,
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 <[email protected]>
> > ---
> > 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;
> >
>
>
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?
>
> 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 <[email protected]>
>>> ---
>>> 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;
>>>
>>
>>
>
>
>
--
Alexey
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 <[email protected]>
> >>> ---
> >>> 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;
> >>>
> >>
> >>
> >
> >
> >
>
>
On 06/22/2013 12:57 PM, Alex Williamson wrote:
> 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,
Oh. Thanks for the detailed explanation, I was missing this one.
Yeah. Looks like we need some other brand new lock now... Something like a
notifier from VFIO to KVM to inform KVM that it can or cannot use the group
now (on VFIO_IOMMU_ENABLE/VFIO_IOMMU_DISABLE or some new ioctls) so KVM
would not touch the group in not allowed.
typedef int notifier_fn(bool enable);
int vfio_group_iommu_id_from_file(struct file *filep, notifier_fn fn);
?
>
> 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 <[email protected]>
>>>>> ---
>>>>> 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;
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Alexey
On Sat, 2013-06-22 at 13:16 +1000, Alexey Kardashevskiy wrote:
> On 06/22/2013 12:57 PM, Alex Williamson wrote:
> > 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,
>
> Oh. Thanks for the detailed explanation, I was missing this one.
>
> Yeah. Looks like we need some other brand new lock now... Something like a
> notifier from VFIO to KVM to inform KVM that it can or cannot use the group
> now (on VFIO_IOMMU_ENABLE/VFIO_IOMMU_DISABLE or some new ioctls) so KVM
> would not touch the group in not allowed.
>
> typedef int notifier_fn(bool enable);
> int vfio_group_iommu_id_from_file(struct file *filep, notifier_fn fn);
>
> ?
I don't think we need either a new lock or a notifier. If we do as Ben
suggested and allow KVM to keep the group active then the requirements
are pretty much identical to having an open vfio device file descriptor.
This is why I suggested container_users. I think the code I proposed
will work, it just needs a few more conditions tested when the external
user is added to make sure the group is iommu protected. Thanks,
Alex