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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 26/10/2021 17:28, Julien Grall wrote:
> 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.
Actually, digging through the code, I noticed that iommu_add_device() 
assume that it can only be called with platform/DT. Thankfully, AFAICT, 
the function would return -ENXIO because the fwspec would not be 
allocated for PCI device.
So was this patch tested with additional patch on top?

> > 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/ 
Cheers,

--
Julien Grall