* [PATCH v2 1/9] vfio: Fix group release deadlock
[not found] <20170619170323.14047.26504.stgit@gimli.home>
@ 2017-06-19 17:14 ` Alex Williamson
2017-06-19 17:14 ` [PATCH v2 2/9] kvm-vfio: Decouple only when we match a group Alex Williamson
2017-06-19 17:14 ` [PATCH v2 3/9] vfio: New external user group/file match Alex Williamson
2 siblings, 0 replies; 6+ messages in thread
From: Alex Williamson @ 2017-06-19 17:14 UTC (permalink / raw)
To: kvm; +Cc: eric.auger, alex.williamson, linux-kernel, stable
If vfio_iommu_group_notifier() acquires a group reference and that
reference becomes the last reference to the group, then vfio_group_put
introduces a deadlock code path where we're trying to unregister from
the iommu notifier chain from within a callout of that chain. Use a
work_struct to release this reference asynchronously.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Cc: stable@vger.kernel.org
---
drivers/vfio/vfio.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 6a49485eb49d..54dd2fbf83d9 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -423,6 +423,34 @@ static void vfio_group_put(struct vfio_group *group)
kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock);
}
+struct vfio_group_put_work {
+ struct work_struct work;
+ struct vfio_group *group;
+};
+
+static void vfio_group_put_bg(struct work_struct *work)
+{
+ struct vfio_group_put_work *do_work;
+
+ do_work = container_of(work, struct vfio_group_put_work, work);
+
+ vfio_group_put(do_work->group);
+ kfree(do_work);
+}
+
+static void vfio_group_schedule_put(struct vfio_group *group)
+{
+ struct vfio_group_put_work *do_work;
+
+ do_work = kmalloc(sizeof(*do_work), GFP_KERNEL);
+ if (WARN_ON(!do_work))
+ return;
+
+ INIT_WORK(&do_work->work, vfio_group_put_bg);
+ do_work->group = group;
+ schedule_work(&do_work->work);
+}
+
/* Assume group_lock or group reference is held */
static void vfio_group_get(struct vfio_group *group)
{
@@ -762,7 +790,14 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
break;
}
- vfio_group_put(group);
+ /*
+ * If we're the last reference to the group, the group will be
+ * released, which includes unregistering the iommu group notifier.
+ * We hold a read-lock on that notifier list, unregistering needs
+ * a write-lock... deadlock. Release our reference asynchronously
+ * to avoid that situation.
+ */
+ vfio_group_schedule_put(group);
return NOTIFY_OK;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/9] kvm-vfio: Decouple only when we match a group
[not found] <20170619170323.14047.26504.stgit@gimli.home>
2017-06-19 17:14 ` [PATCH v2 1/9] vfio: Fix group release deadlock Alex Williamson
@ 2017-06-19 17:14 ` Alex Williamson
2017-06-20 2:34 ` Alexey Kardashevskiy
2017-06-19 17:14 ` [PATCH v2 3/9] vfio: New external user group/file match Alex Williamson
2 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2017-06-19 17:14 UTC (permalink / raw)
To: kvm
Cc: Alexey Kardashevskiy, linux-kernel, stable, eric.auger,
alex.williamson, Paolo Bonzini
Unset-KVM and decrement-assignment only when we find the group in our
list. Otherwise we can get out of sync if the user triggers this for
groups that aren't currently on our list.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: stable@vger.kernel.org
---
virt/kvm/vfio.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 37d9118fd84b..f1b0b7bca9a9 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -246,21 +246,19 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
continue;
list_del(&kvg->node);
+ kvm_arch_end_assignment(dev->kvm);
+#ifdef CONFIG_SPAPR_TCE_IOMMU
+ kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
+#endif
+ kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
break;
}
- kvm_arch_end_assignment(dev->kvm);
-
mutex_unlock(&kv->lock);
-#ifdef CONFIG_SPAPR_TCE_IOMMU
- kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
-#endif
- kvm_vfio_group_set_kvm(vfio_group, NULL);
-
kvm_vfio_group_put_external_user(vfio_group);
kvm_vfio_update_coherency(dev);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/9] kvm-vfio: Decouple only when we match a group
2017-06-19 17:14 ` [PATCH v2 2/9] kvm-vfio: Decouple only when we match a group Alex Williamson
@ 2017-06-20 2:34 ` Alexey Kardashevskiy
2017-06-20 2:42 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kardashevskiy @ 2017-06-20 2:34 UTC (permalink / raw)
To: Alex Williamson, kvm; +Cc: linux-kernel, stable, eric.auger, Paolo Bonzini
On 20/06/17 03:14, Alex Williamson wrote:
> Unset-KVM and decrement-assignment only when we find the group in our
> list. Otherwise we can get out of sync if the user triggers this for
> groups that aren't currently on our list.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: stable@vger.kernel.org
> ---
> virt/kvm/vfio.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 37d9118fd84b..f1b0b7bca9a9 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -246,21 +246,19 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
> continue;
>
> list_del(&kvg->node);
> + kvm_arch_end_assignment(dev->kvm);
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
> + kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
> +#endif
> + kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
> kvm_vfio_group_put_external_user(kvg->vfio_group);
> kfree(kvg);
> ret = 0;
> break;
> }
>
> - kvm_arch_end_assignment(dev->kvm);
> -
> mutex_unlock(&kv->lock);
>
> -#ifdef CONFIG_SPAPR_TCE_IOMMU
> - kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
> -#endif
> - kvm_vfio_group_set_kvm(vfio_group, NULL);
Tiny nit: vfio_group becomes kvg->vfio_group in kvm_vfio_group_set_kvm()
and does not in kvm_spapr_tce_release_vfio_group().
Anyway,
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> -
> kvm_vfio_group_put_external_user(vfio_group);
>
> kvm_vfio_update_coherency(dev);
>
--
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/9] kvm-vfio: Decouple only when we match a group
2017-06-20 2:34 ` Alexey Kardashevskiy
@ 2017-06-20 2:42 ` Alex Williamson
0 siblings, 0 replies; 6+ messages in thread
From: Alex Williamson @ 2017-06-20 2:42 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: kvm, linux-kernel, stable, eric.auger, Paolo Bonzini
On Tue, 20 Jun 2017 12:34:57 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 20/06/17 03:14, Alex Williamson wrote:
> > Unset-KVM and decrement-assignment only when we find the group in our
> > list. Otherwise we can get out of sync if the user triggers this for
> > groups that aren't currently on our list.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Cc: stable@vger.kernel.org
> > ---
> > virt/kvm/vfio.c | 12 +++++-------
> > 1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > index 37d9118fd84b..f1b0b7bca9a9 100644
> > --- a/virt/kvm/vfio.c
> > +++ b/virt/kvm/vfio.c
> > @@ -246,21 +246,19 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
> > continue;
> >
> > list_del(&kvg->node);
> > + kvm_arch_end_assignment(dev->kvm);
> > +#ifdef CONFIG_SPAPR_TCE_IOMMU
> > + kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
> > +#endif
> > + kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
> > kvm_vfio_group_put_external_user(kvg->vfio_group);
> > kfree(kvg);
> > ret = 0;
> > break;
> > }
> >
> > - kvm_arch_end_assignment(dev->kvm);
> > -
> > mutex_unlock(&kv->lock);
> >
> > -#ifdef CONFIG_SPAPR_TCE_IOMMU
> > - kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
> > -#endif
> > - kvm_vfio_group_set_kvm(vfio_group, NULL);
>
>
> Tiny nit: vfio_group becomes kvg->vfio_group in kvm_vfio_group_set_kvm()
> and does not in kvm_spapr_tce_release_vfio_group().
>
>
> Anyway,
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thanks, I made the following change for consistency:
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index f1b0b7bca9a9..6e002d0f3191 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -248,7 +248,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
list_del(&kvg->node);
kvm_arch_end_assignment(dev->kvm);
#ifdef CONFIG_SPAPR_TCE_IOMMU
- kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
+ kvm_spapr_tce_release_vfio_group(dev->kvm,
+ kvg->vfio_group);
#endif
kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
kvm_vfio_group_put_external_user(kvg->vfio_group);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/9] vfio: New external user group/file match
[not found] <20170619170323.14047.26504.stgit@gimli.home>
2017-06-19 17:14 ` [PATCH v2 1/9] vfio: Fix group release deadlock Alex Williamson
2017-06-19 17:14 ` [PATCH v2 2/9] kvm-vfio: Decouple only when we match a group Alex Williamson
@ 2017-06-19 17:14 ` Alex Williamson
2017-06-20 5:01 ` kbuild test robot
2 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2017-06-19 17:14 UTC (permalink / raw)
To: kvm; +Cc: eric.auger, alex.williamson, linux-kernel, stable, Paolo Bonzini
At the point where the kvm-vfio pseudo device wants to release its
vfio group reference, we can't always acquire a new reference to make
that happen. The group can be in a state where we wouldn't allow a
new reference to be added. This new helper function allows a caller
to match a file to a group to facilitate this. Given a file and
group, report if they match. Thus the caller needs to already have a
group reference to match to the file. This allows the deletion of a
group without acquiring a new reference.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org
---
drivers/vfio/vfio.c | 9 +++++++++
include/linux/vfio.h | 2 ++
virt/kvm/vfio.c | 27 +++++++++++++++++++--------
3 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 54dd2fbf83d9..7597a377eb4e 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1776,6 +1776,15 @@ void vfio_group_put_external_user(struct vfio_group *group)
}
EXPORT_SYMBOL_GPL(vfio_group_put_external_user);
+bool vfio_external_group_match_file(struct vfio_group *test_group,
+ struct file *filep)
+{
+ struct vfio_group *group = filep->private_data;
+
+ return (filep->f_op == &vfio_group_fops) && (group == test_group);
+}
+EXPORT_SYMBOL_GPL(vfio_external_group_match_file);
+
int vfio_external_user_iommu_id(struct vfio_group *group)
{
return iommu_group_id(group->iommu_group);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index edf9b2cad277..9b34d0af5d27 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -97,6 +97,8 @@ extern void vfio_unregister_iommu_driver(
*/
extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
extern void vfio_group_put_external_user(struct vfio_group *group);
+extern bool vfio_external_group_match_file(struct vfio_group *group,
+ struct file *filep);
extern int vfio_external_user_iommu_id(struct vfio_group *group);
extern long vfio_external_check_extension(struct vfio_group *group,
unsigned long arg);
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index f1b0b7bca9a9..9aba73127aac 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -51,6 +51,22 @@ static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
return vfio_group;
}
+static bool kvm_vfio_external_group_match_file(struct vfio_group *group,
+ struct file *filep)
+{
+ bool ret, (*fn)(struct vfio_group *, struct file *);
+
+ fn = symbol_get(vfio_external_group_match_file);
+ if (!fn)
+ return false;
+
+ ret = fn(group, filep);
+
+ symbol_put(vfio_external_group_match_file);
+
+ return ret;
+}
+
static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
{
void (*fn)(struct vfio_group *);
@@ -231,18 +247,13 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
if (!f.file)
return -EBADF;
- vfio_group = kvm_vfio_group_get_external_user(f.file);
- fdput(f);
-
- if (IS_ERR(vfio_group))
- return PTR_ERR(vfio_group);
-
ret = -ENOENT;
mutex_lock(&kv->lock);
list_for_each_entry(kvg, &kv->group_list, node) {
- if (kvg->vfio_group != vfio_group)
+ if (!kvm_vfio_external_group_match_file(kvg->vfio_group,
+ f.file))
continue;
list_del(&kvg->node);
@@ -259,7 +270,7 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
mutex_unlock(&kv->lock);
- kvm_vfio_group_put_external_user(vfio_group);
+ fdput(f);
kvm_vfio_update_coherency(dev);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 3/9] vfio: New external user group/file match
2017-06-19 17:14 ` [PATCH v2 3/9] vfio: New external user group/file match Alex Williamson
@ 2017-06-20 5:01 ` kbuild test robot
0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-06-20 5:01 UTC (permalink / raw)
To: Alex Williamson
Cc: kbuild-all, kvm, eric.auger, alex.williamson, linux-kernel,
stable, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]
Hi Alex,
[auto build test ERROR on vfio/next]
[also build test ERROR on v4.12-rc6 next-20170619]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alex-Williamson/vfio-Fix-release-ordering-races-and-use-driver_override/20170620-095741
base: https://github.com/awilliam/linux-vfio.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All errors (new ones prefixed by >>):
arch/powerpc/kvm/../../../virt/kvm/vfio.c: In function 'kvm_vfio_set_attr':
>> arch/powerpc/kvm/../../../virt/kvm/vfio.c:262:4: error: 'vfio_group' may be used uninitialized in this function [-Werror=maybe-uninitialized]
kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/../../../virt/kvm/vfio.c:190:21: note: 'vfio_group' was declared here
struct vfio_group *vfio_group;
^~~~~~~~~~
cc1: all warnings being treated as errors
vim +/vfio_group +262 arch/powerpc/kvm/../../../virt/kvm/vfio.c
600c6bde Alex Williamson 2017-06-19 256 f.file))
ec53500f Alex Williamson 2013-10-30 257 continue;
ec53500f Alex Williamson 2013-10-30 258
ec53500f Alex Williamson 2013-10-30 259 list_del(&kvg->node);
14979b3f Alex Williamson 2017-06-19 260 kvm_arch_end_assignment(dev->kvm);
14979b3f Alex Williamson 2017-06-19 261 #ifdef CONFIG_SPAPR_TCE_IOMMU
14979b3f Alex Williamson 2017-06-19 @262 kvm_spapr_tce_release_vfio_group(dev->kvm, vfio_group);
14979b3f Alex Williamson 2017-06-19 263 #endif
14979b3f Alex Williamson 2017-06-19 264 kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
ec53500f Alex Williamson 2013-10-30 265 kvm_vfio_group_put_external_user(kvg->vfio_group);
:::::: The code at line 262 was first introduced by commit
:::::: 14979b3f26fbbe87d4240e463db53e64dd127184 kvm-vfio: Decouple only when we match a group
:::::: TO: Alex Williamson <alex.williamson@redhat.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23359 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread