RE: [PATCH] iommu: Remove functions that support private domain

["Prakhya, Sai Praneeth" <sai.praneeth.prakhya@intel.com>]

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Friday, May 15, 2020 8:46 AM
> To: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>;
> iommu@lists.linux-foundation.org
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Fri, May 15, 2020 at 08:55:42PM +0800, Lu Baolu wrote:
> > It seems that we can do like this:
> >
> > [1] mutex_lock(&group->lock)
> > [2] for_each_group_device(device_lock())
> > [3] if (for_each_group_device(!device_is_bound()))
> > 	change_default_domain()
> > [4] for_each_group_device_reverse(device_unlock())
> > [5] mutex_unlock(&group->lock)

>> A possible problem exists at step 2 when another thread is trying to lock devices in the reverse order at the same time. -> By Allen

Makes sense.. this could happen and we could prevent this if the iommu_group has only one device. So, going with Joerg's suggestion, if we support dynamic switching of default-domain of a group with only one device, I think we shouldn't hit this issue.

> The problem here is that I am pretty sure we also have:
> 
> 	device_lock() /* from device/driver core code */
> 	-> bus_notifier()
> 	  -> iommu_bus_notifier()
> 	    -> ...
> 	      -> mutex_lock(&group->lock)
> 
> Which would cause lock-inversion with the above code.

Thanks for this call chain, Joerg. It makes sense to me how lock inversion could happen

iommu_bus_notifier()
-> iommu_release_device()
 -> ops->release_device() (Eg: intel_iommu_release_device())
  -> iommu_group_remove_device()
   -> mutex_lock()

But, I added print statements to iommu_bus_notifier() and iommu_group_remove_device() and noticed that iommu_group_remove_device() wasn't being called upon modprobe -r <driver_name> and iommu_probe_device() isn't called upon modprobe <driver_name> because

	if (action == BUS_NOTIFY_ADD_DEVICE) {
		int ret;

		ret = iommu_probe_device(dev);
		return (ret) ? NOTIFY_DONE : NOTIFY_OK;
	} else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
		iommu_release_device(dev);
		return NOTIFY_OK;
	}

"action" was != BUS_NOTIFY_ADD_DEVICE || BUS_NOTIFY_REMOVED_DEVICE upon modprobe. So (after booting [1]), I am wondering when would action be == to one of the above so that these iommu functions get called.

And,
	switch (action) {
	case BUS_NOTIFY_BIND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_BIND_DRIVER;
		break;
	case BUS_NOTIFY_BOUND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_BOUND_DRIVER;
		break;
	case BUS_NOTIFY_UNBIND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_UNBIND_DRIVER;
		break;
	case BUS_NOTIFY_UNBOUND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER;
		break;
	}

	if (group_action)
		blocking_notifier_call_chain(&group->notifier,
					     group_action, dev);

I also noticed that vfio is the only one that registers for group notifiers and from a first look it wasn't very obvious if vfio is trying to get group->mutex.

[1] I am interested in after booting because dynamic switching of iommu_group default-domain is supported only through sysfs.

Regards,
Sai
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu