On 20/12/16 03:28, Alex Williamson wrote:
> On Mon, 19 Dec 2016 18:41:05 +0800
> Jike Song <jike.song@xxxxxxxxx> wrote:
>
>> On 12/18/2016 09:28 AM, Alexey Kardashevskiy wrote:
>>> 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 <aik@xxxxxxxxx>
>>> ---
>>>
>>> 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 :)
>
>> 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?
>
>>> ---
>>> 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?
>
>>>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
