On 20/12/16 03:28, Alex Williamson wrote:

On Mon, 19 Dec 2016 18:41:05 +0800
Jike Song [off-list ref] wrote:

quoted

On 12/18/2016 09:28 AM, Alexey Kardashevskiy wrote:

quoted

This moves a check for unregistered notifiers from fops release
callback to the place where the group will actually be released.

Signed-off-by: Alexey Kardashevskiy <redacted>
---

This is going to be used in the following patch in cleanup
path. Since the next patch is RFC, this one might not be needed.  

Alexey, this is intended to be a bug fix, it should be sent separately,
not buried in an unrelated patch series.

Well, I was pretty unsure this is a correct fix and I was sure that it
would make sense without the context which is quite weird :)

quoted

I didn't find any use in the following patch 11/11, did you mean
something else?

BTW the warning in vfio_group_release seems too late, the user
should actually unregister everything by close()?

The thing is, it's not the user that registered the notifiers, it's the
vendor driver.  The vendor driver should know via the device release to
unregister the notifier, which we're counting on to happen before the
group release.  Can we rely on that ordering even in the case where a
user is SIGKILL'd?

quoted

quoted

---
 drivers/vfio/vfio.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 6b9a98508939..083b581e87c0 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c

@@ -403,6 +403,8 @@ static void vfio_group_release(struct kref *kref)
 	struct iommu_group *iommu_group = group->iommu_group;
 
 	WARN_ON(!list_empty(&group->device_list));
+	/* Any user didn't unregister? */
+	WARN_ON(group->notifier.head);

Even if this is a bug, this is the wrong fix.  This is when the group
is being destroyed.  Yes, it would be a bug to still have any notifiers
here, but the intention is to make sure there are no notifiers when the
group is idle and unused.

Out of curiosity - vendor drivers are supposed to hold a group file open,
not just reference vfio_grop/iommu_group objects?

quoted

quoted

 
 	list_for_each_entry_safe(unbound, tmp,
 				 &group->unbound_list, unbound_next) {

@@ -1584,9 +1586,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 
 	filep->private_data = NULL;
 
-	/* Any user didn't unregister? */
-	WARN_ON(group->notifier.head);
-
 	vfio_group_try_dissolve_container(group);
 
 	atomic_dec(&group->opened);
  

-- 
Alexey