Re: [patch-4.16] arm/smmuv1,v2: Protect smmu master list with a lock

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 26/10/2021 13:29, Michal Orzel wrote:
> If a device is added to SMMUv1/v2 from DT and PCI
> at the same time, there is a concurrent access
> to a smmu master list. This could lead to a
> scenario where one is looking into a list that
> is being modified at the same time. Add a lock
> to prevent this issue.
Did you intend to say "Hold" rather than "Add"?

> Reuse the existing spinlock arm_smmu_devices_lock
> as it is already protecting find_smmu_master.
>
> ipmmu-smmu and smmuv3 are not impacted by this
> issue as there is no access or modification of
> a global resource during add_device.
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
> This patch aims for 4.16 release.
> Benefits:
> Remove a bug that could lead to a corruption of the
> smmu master list, which would be very hard to debug.
From my understanding, this corruption would only happen with 
CONFIG_HAS_PCI. At the moment, this is a experimental feature as it is 
not fully complete.
> Risks:
> Overall the risk is low as we are touching init code
> rather than a runtime one. In case of any issue, the
> problem would be catched during system boot or guest
> start.
With the PCI feature enabled, this can be called at runtime as this 
plumbed through a PHYSDEVOP. That said, it doesn't matter here as for 
supported code (platform/DT device), this will only happen during boot.
The patch looks straighforward, so I would not mind to have it in Xen 
4.16. Ian, what do you think?
> ---
>   xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index c9dfc4caa0..be62a66a28 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -820,21 +820,25 @@ static int arm_smmu_dt_add_device_legacy(struct 
> arm_smmu_device *smmu,
>                                          struct device *dev,
>                                          struct iommu_fwspec *fwspec)
>   {
> -       int i;
> +       int i, ret;
>         struct arm_smmu_master *master;
>         struct device_node *dev_node = dev_get_dev_node(dev);
>   
> +	spin_lock(&arm_smmu_devices_lock);
>         master = find_smmu_master(smmu, dev_node);
>         if (master) {
>                 dev_err(dev,
>                         "rejecting multiple registrations for master device 
> %s\n",
>                         dev_node->name);
> -               return -EBUSY;
> +               ret = -EBUSY;
> +               goto out_unlock;
>         }
>   
>   	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> -       if (!master)
> -               return -ENOMEM;
> +       if (!master) {
> +               ret = -ENOMEM;
> +               goto out_unlock;
> +       }
>         master->of_node = dev_node;
>   
>   	/* Xen: Let Xen know that the device is protected by an SMMU */
> @@ -846,11 +850,17 @@ static int arm_smmu_dt_add_device_legacy(struct 
> arm_smmu_device *smmu,
>                         dev_err(dev,
>                                 "stream ID for master device %s greater than maximum 
> allowed (%d)\n",
>                                 dev_node->name, smmu->num_mapping_groups);
> -                       return -ERANGE;
> +                       ret = -ERANGE;
> +                       goto out_unlock;
>                 }
>                 master->cfg.smendx[i] = INVALID_SMENDX;
>         }
> -       return insert_smmu_master(smmu, master);
> +
> +       ret = insert_smmu_master(smmu, master);
> +
> +out_unlock:
> +       spin_unlock(&arm_smmu_devices_lock);
Please add a newline here.

> +       return ret;
>   }
>   
>   static int register_smmu_master(struct arm_smmu_device *smmu,
> @@ -2056,7 +2066,10 @@ static int arm_smmu_add_device(struct device *dev)
>         } else {
>                 struct arm_smmu_master *master;
>   
> +		spin_lock(&arm_smmu_devices_lock);
>                 master = find_smmu_master(smmu, dev->of_node);
> +               spin_unlock(&arm_smmu_devices_lock);
At the moment, unlocking here is fine because we don't remove the 
device. However, there are a series to supporting removing a device (see 
[1]). So I think it would be preferable to unlock after the last use of 
'cfg'.
Looking at the code, I also noticed that the error path is not correct 
for at least the PCI device and we would leak memory. We also assume 
that Stream ID == Requester ID. Are both of them in your radar for PCI 
passthrough?
Cheers,

[1] 
https://lore.kernel.org/xen-devel/1630562763-390068-9-git-send-email-fnu.vikram@xxxxxxxxxx/
--
Julien Grall