Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
On Sun, Nov 19, 2023 at 06:19:43PM +0900, Hector Martin wrote:
> >> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> >> + struct device *dev,
> >> + struct fwnode_handle *iommu_fwnode)
> >> +{
> >> + const struct iommu_ops *ops;
> >> +
> >> + if (fwspec->iommu_fwnode) {
> >> + /*
> >> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
> >> + * case of multiple iommus for one device they must point to the
> >> + * same driver, checked via same ops.
> >> + */
> >> + ops = iommu_ops_from_fwnode(iommu_fwnode);
> >
> > This carries over a related bug from the original code: If a device has
> > two IOMMUs and the first one probes but the second one defers, ops will
> > be NULL here and the check will fail with EINVAL.
> >
> > Adding a check for that case here fixes it:
> >
> > if (!ops)
> > return driver_deferred_probe_check_state(dev);
Yes!
> > With that, for the whole series:
> >
> > Tested-by: Hector Martin
> >
> > I can't specifically test for the probe races the series intends to fix
> > though, since that bug we only hit extremely rarely. I'm just testing
> > that nothing breaks.
>
> Actually no, this fix is not sufficient. If the first IOMMU is ready
> then the xlate path allocates dev->iommu, which then
> __iommu_probe_device takes as a sign that all IOMMUs are ready and does
> the device init.
It doesn't.. The code there is:
if (!fwspec && dev->iommu)
fwspec = dev->iommu->fwspec;
if (fwspec)
ops = fwspec->ops;
else
ops = dev->bus->iommu_ops;
if (!ops) {
ret = -ENODEV;
goto out_unlock;
}
Which is sensitive only to !NULL fwspec, and if EPROBE_DEFER is
returned fwspec will be freed and dev->iommu->fwspec will be NULL
here.
In the NULL case it does a 'bus probe' with a NULL fwspec and all the
fwspec drivers return immediately from their probe functions.
Did I miss something?
> Then when the xlate comes along again after suceeding
> with the second IOMMU, __iommu_probe_device sees the device is already
> in a group and never initializes the second IOMMU, leaving the device
> with only one IOMMU.
This should be fixed by the first hunk to check every iommu and fail?
BTW, do you have a systems with same device attached to multiple
iommus?
I've noticed another bug here, many drivers don't actually support
differing iommu instances and nothing seems to check it..
Thanks,
Jason
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
On 2023/11/19 17:10, Hector Martin wrote:
> On 2023/11/15 23:05, Jason Gunthorpe wrote:
>> Allow fwspec to exist independently from the dev->iommu by providing
>> functions to allow allocating and freeing the raw struct iommu_fwspec.
>>
>> Reflow the existing paths to call the new alloc/dealloc functions.
>>
>> Reviewed-by: Jerry Snitselaar
>> Signed-off-by: Jason Gunthorpe
>> ---
>> drivers/iommu/iommu.c | 82 ---
>> include/linux/iommu.h | 11 +-
>> 2 files changed, 72 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 18a82a20934d53..86bbb9e75c7e03 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -361,10 +361,8 @@ static void dev_iommu_free(struct device *dev)
>> struct dev_iommu *param = dev->iommu;
>>
>> dev->iommu = NULL;
>> -if (param->fwspec) {
>> -fwnode_handle_put(param->fwspec->iommu_fwnode);
>> -kfree(param->fwspec);
>> -}
>> +if (param->fwspec)
>> +iommu_fwspec_dealloc(param->fwspec);
>> kfree(param);
>> }
>>
>> @@ -2920,10 +2918,61 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct
>> fwnode_handle *fwnode)
>> return ops;
>> }
>>
>> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
>> + struct device *dev,
>> + struct fwnode_handle *iommu_fwnode)
>> +{
>> +const struct iommu_ops *ops;
>> +
>> +if (fwspec->iommu_fwnode) {
>> +/*
>> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
>> + * case of multiple iommus for one device they must point to the
>> + * same driver, checked via same ops.
>> + */
>> +ops = iommu_ops_from_fwnode(iommu_fwnode);
>
> This carries over a related bug from the original code: If a device has
> two IOMMUs and the first one probes but the second one defers, ops will
> be NULL here and the check will fail with EINVAL.
>
> Adding a check for that case here fixes it:
>
> if (!ops)
> return driver_deferred_probe_check_state(dev);
>
> With that, for the whole series:
>
> Tested-by: Hector Martin
>
> I can't specifically test for the probe races the series intends to fix
> though, since that bug we only hit extremely rarely. I'm just testing
> that nothing breaks.
Actually no, this fix is not sufficient. If the first IOMMU is ready
then the xlate path allocates dev->iommu, which then
__iommu_probe_device takes as a sign that all IOMMUs are ready and does
the device init. Then when the xlate comes along again after suceeding
with the second IOMMU, __iommu_probe_device sees the device is already
in a group and never initializes the second IOMMU, leaving the device
with only one IOMMU.
This patch fixes it, but honestly, at this point I have no idea how to
"properly" fix this. There is *way* too much subtlety in this whole
codepath.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2477dec29740..2e4baf0572e7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2935,6 +2935,12 @@ int iommu_fwspec_of_xlate(struct iommu_fwspec
*fwspec, struct device *dev,
int ret;
ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode);
+ if (ret == -EPROBE_DEFER) {
+ mutex_lock(_probe_device_lock);
+ if (dev->iommu)
+ dev_iommu_free(dev);
+ mutex_unlock(_probe_device_lock);
+ }
if (ret)
return ret;
>
>> +if (fwspec->ops != ops)
>> +return -EINVAL;
>> +return 0;
>> +}
>> +
>> +if (!fwspec->ops) {
>> +ops = iommu_ops_from_fwnode(iommu_fwnode);
>> +if (!ops)
>> +return driver_deferred_probe_check_state(dev);
>> +fwspec->ops = ops;
>> +}
>> +
>> +of_node_get(to_of_node(iommu_fwnode));
>> +fwspec->iommu_fwnode = iommu_fwnode;
>> +return 0;
>> +}
>> +
>> +struct iommu_fwspec *iommu_fwspec_alloc(void)
>> +{
>> +struct iommu_fwspec *fwspec;
>> +
>> +fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
>> +if (!fwspec)
>> +return ERR_PTR(-ENOMEM);
>> +return fwspec;
>> +}
>> +
>> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec)
>> +{
>> +if (!fwspec)
>> +return;
>> +
>> +if (fwspec->iommu_fwnode)
>> +fwnode_handle_put(fwspec->iommu_fwnode);
>> +kfree(fwspec);
>> +}
>> +
>> int iommu_fwspec_init(struct device *dev, struct fwnode_handle
>> *iommu_fwnode,
>>const struct iommu_ops *ops)
>> {
>> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +int ret;
>>
>> if (fwspec)
>> return ops == fwspec->ops ? 0 : -EINVAL;
>> @@ -2931,29 +2980,22 @@ int iommu_fwspec_init(struct device *dev, struct
Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
On 2023/11/15 23:05, Jason Gunthorpe wrote:
> Allow fwspec to exist independently from the dev->iommu by providing
> functions to allow allocating and freeing the raw struct iommu_fwspec.
>
> Reflow the existing paths to call the new alloc/dealloc functions.
>
> Reviewed-by: Jerry Snitselaar
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/iommu.c | 82 ---
> include/linux/iommu.h | 11 +-
> 2 files changed, 72 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 18a82a20934d53..86bbb9e75c7e03 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -361,10 +361,8 @@ static void dev_iommu_free(struct device *dev)
> struct dev_iommu *param = dev->iommu;
>
> dev->iommu = NULL;
> - if (param->fwspec) {
> - fwnode_handle_put(param->fwspec->iommu_fwnode);
> - kfree(param->fwspec);
> - }
> + if (param->fwspec)
> + iommu_fwspec_dealloc(param->fwspec);
> kfree(param);
> }
>
> @@ -2920,10 +2918,61 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct
> fwnode_handle *fwnode)
> return ops;
> }
>
> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec,
> + struct device *dev,
> + struct fwnode_handle *iommu_fwnode)
> +{
> + const struct iommu_ops *ops;
> +
> + if (fwspec->iommu_fwnode) {
> + /*
> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare
> + * case of multiple iommus for one device they must point to the
> + * same driver, checked via same ops.
> + */
> + ops = iommu_ops_from_fwnode(iommu_fwnode);
This carries over a related bug from the original code: If a device has
two IOMMUs and the first one probes but the second one defers, ops will
be NULL here and the check will fail with EINVAL.
Adding a check for that case here fixes it:
if (!ops)
return driver_deferred_probe_check_state(dev);
With that, for the whole series:
Tested-by: Hector Martin
I can't specifically test for the probe races the series intends to fix
though, since that bug we only hit extremely rarely. I'm just testing
that nothing breaks.
> + if (fwspec->ops != ops)
> + return -EINVAL;
> + return 0;
> + }
> +
> + if (!fwspec->ops) {
> + ops = iommu_ops_from_fwnode(iommu_fwnode);
> + if (!ops)
> + return driver_deferred_probe_check_state(dev);
> + fwspec->ops = ops;
> + }
> +
> + of_node_get(to_of_node(iommu_fwnode));
> + fwspec->iommu_fwnode = iommu_fwnode;
> + return 0;
> +}
> +
> +struct iommu_fwspec *iommu_fwspec_alloc(void)
> +{
> + struct iommu_fwspec *fwspec;
> +
> + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> + if (!fwspec)
> + return ERR_PTR(-ENOMEM);
> + return fwspec;
> +}
> +
> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec)
> +{
> + if (!fwspec)
> + return;
> +
> + if (fwspec->iommu_fwnode)
> + fwnode_handle_put(fwspec->iommu_fwnode);
> + kfree(fwspec);
> +}
> +
> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> const struct iommu_ops *ops)
> {
> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> + int ret;
>
> if (fwspec)
> return ops == fwspec->ops ? 0 : -EINVAL;
> @@ -2931,29 +2980,22 @@ int iommu_fwspec_init(struct device *dev, struct
> fwnode_handle *iommu_fwnode,
> if (!dev_iommu_get(dev))
> return -ENOMEM;
>
> - fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> - if (!fwspec)
> - return -ENOMEM;
> + fwspec = iommu_fwspec_alloc();
> + if (IS_ERR(fwspec))
> + return PTR_ERR(fwspec);
>
> - of_node_get(to_of_node(iommu_fwnode));
> - fwspec->iommu_fwnode = iommu_fwnode;
> fwspec->ops = ops;
> + ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode);
> + if (ret) {
> + iommu_fwspec_dealloc(fwspec);
> + return ret;
> + }
> +
> dev_iommu_fwspec_set(dev, fwspec);
> return 0;
> }
> EXPORT_SYMBOL_GPL(iommu_fwspec_init);
>
> -void iommu_fwspec_free(struct device *dev)
> -{
> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> -
> - if (fwspec) {
> - fwnode_handle_put(fwspec->iommu_fwnode);
> - kfree(fwspec);
> - dev_iommu_fwspec_set(dev, NULL);
> - }
> -}
> -EXPORT_SYMBOL_GPL(iommu_fwspec_free);
>
> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e98a4ca8f536b7..c7c68cb59aa4dc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
>
