lkml.org 
[lkml]   [2023]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/4] iommu: Add dev_iommu->ops_rwsem
From
On 2023-02-15 05:34, Baolu Lu wrote:
> On 2/13/23 10:16 PM, Jason Gunthorpe wrote:
>> On Mon, Feb 13, 2023 at 03:49:38PM +0800, Lu Baolu wrote:
>>
>>> +static int iommu_group_freeze_dev_ops(struct iommu_group *group)
>>> +{
>>> +    struct group_device *device;
>>> +    struct device *dev;
>>> +
>>> +    mutex_lock(&group->mutex);
>>> +    list_for_each_entry(device, &group->devices, list) {
>>> +        dev = device->dev;
>>> +        down_read(&dev->iommu->ops_rwsem);
>>
>> This isn't allowed, you can't obtain locks in a loop like this, it
>> will deadlock.
>>
>> You don't need more locks, we already have the group mutex, the
>> release path should be fixed to use it properly as I was trying to do
>> here:
>>
>> https://lore.kernel.org/kvm/4-v1-ef00ffecea52+2cb-iommu_group_lifetime_jgg@nvidia.com/
>> https://lore.kernel.org/kvm/YyyTxx0HnA3maxEk@nvidia.com/
>>
>> Then what you'd do in a path like this is:
>>
>>    mutex_lock(&group->mutex);
>>    dev = iommu_group_first_device(group)
>>    if (!dev)
>>       /* Racing with group cleanup */
>>       return -EINVAL;
>>    /* Now dev->ops is valid and must remain valid so long as
>>       group->mutex is held */
>>
>> The only reason this doesn't work already is because of the above
>> stuff about release not holding the group mutex when manipulating the
>> devices in the group. This is simply mis-locked.
>>
>> Robin explained it was done like this because
>> arm_iommu_detach_device() re-enters the iommu core during release_dev,
>> so I suggest fixing that by simply not doing that (see above)
>>
>> Below is the lastest attempt I had tried, I didn't have time to get back
>> to it and we fixed the bug another way. It needs a bit of adjusting to
>> also remove the device from the group after release is called within
>> the same mutex critical region.
>
> Yes. If we can make remove device from list and device release in the
> same mutex critical region, we don't need any other lock mechanism
> anymore.
>
> The ipmmu driver supports default domain.

It supports default domains *on arm64*. Nothing on 32-bit ARM uses
default domains, they won't even exist since iommu-dma is not enabled,
and either way the ARM DMA ops don't understand groups. I don't see an
obvious satisfactory solution while the arm_iommu_* APIs still exist,
but if we need bodges in the interim could we please try to concentrate
them in ARM-specific code?

Thanks,
Robin.

> When code comes to release
> device, the device driver has already been unbound. The default domain
> should have been attached to the device. Then iommu_detach_device() does
> nothing because what it really does is just attaching default domain.
>
> How about removing iommu_detach_device() from ipmmu's release path like
> below?
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index bdf1a4e5eae0..0bc29009703e 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -820,7 +820,16 @@ static void ipmmu_probe_finalize(struct device *dev)
>
>  static void ipmmu_release_device(struct device *dev)
>  {
> -       arm_iommu_detach_device(dev);
> +       struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +
> +       if (!mapping) {
> +               dev_warn(dev, "Not attached\n");
> +               return;
> +       }
> +
> +       arm_iommu_release_mapping(mapping);
> +       to_dma_iommu_mapping(dev) = NULL;
> +       set_dma_ops(dev, NULL);
>  }
>
> After fixing this in ipmmu driver, we can safely put removing device and
> release_device in a group->mutex critical region in two steps:
>
> Step 1: Refactor iommu_group_remove_device() w/o functionality change:
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 05522eace439..17b2e358a6fd 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1065,6 +1065,46 @@ int iommu_group_add_device(struct iommu_group
> *group, struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_add_device);
>
> +/*
> + * Remove a device from a group's device list and return the group device
> + * if successful.
> + */
> +static struct group_device *
> +__iommu_group_remove_device(struct iommu_group *group, struct device *dev)
> +{
> +    struct group_device *device;
> +
> +    lockdep_assert_held(&group->mutex);
> +    list_for_each_entry(device, &group->devices, list) {
> +        if (device->dev == dev) {
> +            list_del(&device->list);
> +            return device;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +/*
> + * Release a device from its group and decrements the iommu group
> reference
> + * count.
> + */
> +static void __iommu_group_release_device(struct iommu_group *group,
> +                     struct group_device *grp_dev)
> +{
> +    struct device *dev = grp_dev->dev;
> +
> +    sysfs_remove_link(group->devices_kobj, grp_dev->name);
> +    sysfs_remove_link(&dev->kobj, "iommu_group");
> +
> +    trace_remove_device_from_group(group->id, dev);
> +
> +    kfree(grp_dev->name);
> +    kfree(grp_dev);
> +    dev->iommu_group = NULL;
> +    kobject_put(group->devices_kobj);
> +}
> +
>  /**
>   * iommu_group_remove_device - remove a device from it's current group
>   * @dev: device to be removed
> @@ -1075,7 +1115,7 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device);
>  void iommu_group_remove_device(struct device *dev)
>  {
>      struct iommu_group *group = dev->iommu_group;
> -    struct group_device *tmp_device, *device = NULL;
> +    struct group_device *device;
>
>      if (!group)
>          return;
> @@ -1083,27 +1123,11 @@ void iommu_group_remove_device(struct device *dev)
>      dev_info(dev, "Removing from iommu group %d\n", group->id);
>
>      mutex_lock(&group->mutex);
> -    list_for_each_entry(tmp_device, &group->devices, list) {
> -        if (tmp_device->dev == dev) {
> -            device = tmp_device;
> -            list_del(&device->list);
> -            break;
> -        }
> -    }
> +    device = __iommu_group_remove_device(group, dev);
>      mutex_unlock(&group->mutex);
>
> -    if (!device)
> -        return;
> -
> -    sysfs_remove_link(group->devices_kobj, device->name);
> -    sysfs_remove_link(&dev->kobj, "iommu_group");
> -
> -    trace_remove_device_from_group(group->id, dev);
> -
> -    kfree(device->name);
> -    kfree(device);
> -    dev->iommu_group = NULL;
> -    kobject_put(group->devices_kobj);
> +    if (device)
> +        __iommu_group_release_device(group, device);
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_remove_device);
>
>
> Step 2: Put removing group and release_device in a same critical region:
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 17b2e358a6fd..eeb2907472bc 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -101,6 +101,10 @@ static int
> iommu_create_device_direct_mappings(struct iommu_group *group,
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>  static ssize_t iommu_group_store_type(struct iommu_group *group,
>                        const char *buf, size_t count);
> +static struct group_device *
> +__iommu_group_remove_device(struct iommu_group *group, struct device
> *dev);
> +static void __iommu_group_release_device(struct iommu_group *group,
> +                     struct group_device *grp_dev);
>
>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)        \
>  struct iommu_group_attribute iommu_group_attr_##_name =        \
> @@ -466,18 +470,25 @@ int iommu_probe_device(struct device *dev)
>
>  void iommu_release_device(struct device *dev)
>  {
> +    struct iommu_group *group = dev->iommu_group;
> +    struct group_device *device;
>      const struct iommu_ops *ops;
>
> -    if (!dev->iommu)
> +    if (!dev->iommu || !group)
>          return;
>
>      iommu_device_unlink(dev->iommu->iommu_dev, dev);
>
> +    mutex_lock(&group->mutex);
>      ops = dev_iommu_ops(dev);
>      if (ops->release_device)
>          ops->release_device(dev);
> +    device = __iommu_group_remove_device(group, dev);
> +    mutex_unlock(&group->mutex);
> +
> +    if (device)
> +        __iommu_group_release_device(group, device);
>
> -    iommu_group_remove_device(dev);
>      module_put(ops->owner);
>      dev_iommu_free(dev);
>  }
>
> Best regards,
> baolu

\
 
 \ /
  Last update: 2023-03-27 00:24    [W:2.184 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site