Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()
On Sun, Nov 12, 2023 at 09:44:18AM -0800, Moritz Fischer wrote: > On Fri, Nov 03, 2023 at 01:44:55PM -0300, Jason Gunthorpe wrote: > > This call chain is using dev->iommu->fwspec to pass around the fwspec > > between the three parts (acpi_iommu_configure(), acpi_iommu_fwspec_init(), > > iommu_probe_device()). > > > > However there is no locking around the accesses to dev->iommu, so this is > > all racy. > > > > Allocate a clean, local, fwspec at the start of acpu_iommu_configure(), > Nit: s/acpu_iommu_configure/acpi_iommu_configure_id() ? Yep Thanks Jason > > pass it through all functions on the stack to fill it with data, and > > finally pass it into iommu_probe_device_fwspec() which will load it into > > dev->iommu under a lock. > > > > Signed-off-by: Jason Gunthorpe > > Reviewed-by: Moritz Fischer > > --- > > drivers/acpi/arm64/iort.c | 39 - > > drivers/acpi/scan.c | 89 ++- > > drivers/acpi/viot.c | 44 ++- > > drivers/iommu/iommu.c | 5 +-- > > include/acpi/acpi_bus.h | 8 ++-- > > include/linux/acpi_iort.h | 3 +- > > include/linux/acpi_viot.h | 5 ++- > > include/linux/iommu.h | 2 + > > 8 files changed, 97 insertions(+), 98 deletions(-) > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > index 6496ff5a6ba20d..accd01dcfe93f5 100644 > > --- a/drivers/acpi/arm64/iort.c > > +++ b/drivers/acpi/arm64/iort.c > > @@ -1218,10 +1218,9 @@ static bool iort_pci_rc_supports_ats(struct > > acpi_iort_node *node) > > return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED; > > } > > > > -static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node > > *node, > > - u32 streamid) > > +static int iort_iommu_xlate(struct iommu_fwspec *fwspec, struct device > > *dev, > > + struct acpi_iort_node *node, u32 streamid) > > { > > - const struct iommu_ops *ops; > > struct fwnode_handle *iort_fwnode; > > > > if (!node) > > @@ -1239,17 +1238,14 @@ static int iort_iommu_xlate(struct device *dev, > > struct acpi_iort_node *node, > > * in the kernel or not, defer the IOMMU configuration > > * or just abort it. > > */ > > - ops = iommu_ops_from_fwnode(iort_fwnode); > > - if (!ops) > > - return iort_iommu_driver_enabled(node->type) ? > > - -EPROBE_DEFER : -ENODEV; > > - > > - return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops); > > + return acpi_iommu_fwspec_init(fwspec, dev, streamid, iort_fwnode, > > + iort_iommu_driver_enabled(node->type)); > > } > > > > struct iort_pci_alias_info { > > struct device *dev; > > struct acpi_iort_node *node; > > + struct iommu_fwspec *fwspec; > > }; > > > > static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data) > > @@ -1260,7 +1256,7 @@ static int iort_pci_iommu_init(struct pci_dev *pdev, > > u16 alias, void *data) > > > > parent = iort_node_map_id(info->node, alias, &streamid, > > IORT_IOMMU_TYPE); > > - return iort_iommu_xlate(info->dev, parent, streamid); > > + return iort_iommu_xlate(info->fwspec, info->dev, parent, streamid); > > } > > > > static void iort_named_component_init(struct device *dev, > > @@ -1280,7 +1276,8 @@ static void iort_named_component_init(struct device > > *dev, > > dev_warn(dev, "Could not add device properties\n"); > > } > > > > -static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node > > *node) > > +static int iort_nc_iommu_map(struct iommu_fwspec *fwspec, struct device > > *dev, > > +struct acpi_iort_node *node) > > { > > struct acpi_iort_node *parent; > > int err = -ENODEV, i = 0; > > @@ -1293,13 +1290,13 @@ static int iort_nc_iommu_map(struct device *dev, > > struct acpi_iort_node *node) > >i++); > > > > if (parent) > > - err = iort_iommu_xlate(dev, parent, streamid); > > + err = iort_iommu_xlate(fwspec, dev, parent, streamid); > > } while (parent && !err); > > > > return err; > > } > > > > -static int iort_nc_iommu_map_id(struct device *dev, > > +static int iort_nc_iommu_map_id(struct iommu_fwspec *fwspec, struct device > > *dev, > > struct acpi_iort_node *node, > > const u32 *in_id) > > { > > @@ -1308,7 +1305,7 @@ static int iort_nc_iommu_map_id(struct device *dev, > > > > parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE); > > if (parent) > > - return iort_iommu_xlate(dev, parent, streamid); > > + return iort_iommu_xlate(fwspec, dev, parent, streamid); > > > > return -ENODEV; > > } > > @@ -1322,15 +1319,16 @@ static int iort_nc_iommu_map_id(struct device *dev, > > * > > * Returns: 0 on success, <0
Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()
Reviewed-by: Jerry Snitselaar
Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()
On Fri, Nov 03, 2023 at 01:44:55PM -0300, Jason Gunthorpe wrote: > This call chain is using dev->iommu->fwspec to pass around the fwspec > between the three parts (acpi_iommu_configure(), acpi_iommu_fwspec_init(), > iommu_probe_device()). > > However there is no locking around the accesses to dev->iommu, so this is > all racy. > > Allocate a clean, local, fwspec at the start of acpu_iommu_configure(), Nit: s/acpu_iommu_configure/acpi_iommu_configure_id() ? > pass it through all functions on the stack to fill it with data, and > finally pass it into iommu_probe_device_fwspec() which will load it into > dev->iommu under a lock. > > Signed-off-by: Jason Gunthorpe Reviewed-by: Moritz Fischer > --- > drivers/acpi/arm64/iort.c | 39 - > drivers/acpi/scan.c | 89 ++- > drivers/acpi/viot.c | 44 ++- > drivers/iommu/iommu.c | 5 +-- > include/acpi/acpi_bus.h | 8 ++-- > include/linux/acpi_iort.h | 3 +- > include/linux/acpi_viot.h | 5 ++- > include/linux/iommu.h | 2 + > 8 files changed, 97 insertions(+), 98 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 6496ff5a6ba20d..accd01dcfe93f5 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1218,10 +1218,9 @@ static bool iort_pci_rc_supports_ats(struct > acpi_iort_node *node) > return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED; > } > > -static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node, > - u32 streamid) > +static int iort_iommu_xlate(struct iommu_fwspec *fwspec, struct device *dev, > + struct acpi_iort_node *node, u32 streamid) > { > - const struct iommu_ops *ops; > struct fwnode_handle *iort_fwnode; > > if (!node) > @@ -1239,17 +1238,14 @@ static int iort_iommu_xlate(struct device *dev, > struct acpi_iort_node *node, >* in the kernel or not, defer the IOMMU configuration >* or just abort it. >*/ > - ops = iommu_ops_from_fwnode(iort_fwnode); > - if (!ops) > - return iort_iommu_driver_enabled(node->type) ? > --EPROBE_DEFER : -ENODEV; > - > - return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops); > + return acpi_iommu_fwspec_init(fwspec, dev, streamid, iort_fwnode, > + iort_iommu_driver_enabled(node->type)); > } > > struct iort_pci_alias_info { > struct device *dev; > struct acpi_iort_node *node; > + struct iommu_fwspec *fwspec; > }; > > static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data) > @@ -1260,7 +1256,7 @@ static int iort_pci_iommu_init(struct pci_dev *pdev, > u16 alias, void *data) > > parent = iort_node_map_id(info->node, alias, &streamid, > IORT_IOMMU_TYPE); > - return iort_iommu_xlate(info->dev, parent, streamid); > + return iort_iommu_xlate(info->fwspec, info->dev, parent, streamid); > } > > static void iort_named_component_init(struct device *dev, > @@ -1280,7 +1276,8 @@ static void iort_named_component_init(struct device > *dev, > dev_warn(dev, "Could not add device properties\n"); > } > > -static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node) > +static int iort_nc_iommu_map(struct iommu_fwspec *fwspec, struct device *dev, > + struct acpi_iort_node *node) > { > struct acpi_iort_node *parent; > int err = -ENODEV, i = 0; > @@ -1293,13 +1290,13 @@ static int iort_nc_iommu_map(struct device *dev, > struct acpi_iort_node *node) > i++); > > if (parent) > - err = iort_iommu_xlate(dev, parent, streamid); > + err = iort_iommu_xlate(fwspec, dev, parent, streamid); > } while (parent && !err); > > return err; > } > > -static int iort_nc_iommu_map_id(struct device *dev, > +static int iort_nc_iommu_map_id(struct iommu_fwspec *fwspec, struct device > *dev, > struct acpi_iort_node *node, > const u32 *in_id) > { > @@ -1308,7 +1305,7 @@ static int iort_nc_iommu_map_id(struct device *dev, > > parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE); > if (parent) > - return iort_iommu_xlate(dev, parent, streamid); > + return iort_iommu_xlate(fwspec, dev, parent, streamid); > > return -ENODEV; > } > @@ -1322,15 +1319,16 @@ static int iort_nc_iommu_map_id(struct device *dev, > * > * Returns: 0 on success, <0 on failure > */ > -int iort_iommu_configure_id(struct device *dev, const u32 *id_in) > +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev, > + const u32 *id_in) > { > struct acpi_iort_node *node; >
Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()
On Fri, Nov 3, 2023 at 5:45 PM Jason Gunthorpe wrote: > > This call chain is using dev->iommu->fwspec to pass around the fwspec > between the three parts (acpi_iommu_configure(), acpi_iommu_fwspec_init(), > iommu_probe_device()). > > However there is no locking around the accesses to dev->iommu, so this is > all racy. > > Allocate a clean, local, fwspec at the start of acpu_iommu_configure(), > pass it through all functions on the stack to fill it with data, and > finally pass it into iommu_probe_device_fwspec() which will load it into > dev->iommu under a lock. > > Signed-off-by: Jason Gunthorpe Acked-by: Rafael J. Wysocki > --- > drivers/acpi/arm64/iort.c | 39 - > drivers/acpi/scan.c | 89 ++- > drivers/acpi/viot.c | 44 ++- > drivers/iommu/iommu.c | 5 +-- > include/acpi/acpi_bus.h | 8 ++-- > include/linux/acpi_iort.h | 3 +- > include/linux/acpi_viot.h | 5 ++- > include/linux/iommu.h | 2 + > 8 files changed, 97 insertions(+), 98 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 6496ff5a6ba20d..accd01dcfe93f5 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -1218,10 +1218,9 @@ static bool iort_pci_rc_supports_ats(struct > acpi_iort_node *node) > return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED; > } > > -static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node, > - u32 streamid) > +static int iort_iommu_xlate(struct iommu_fwspec *fwspec, struct device *dev, > + struct acpi_iort_node *node, u32 streamid) > { > - const struct iommu_ops *ops; > struct fwnode_handle *iort_fwnode; > > if (!node) > @@ -1239,17 +1238,14 @@ static int iort_iommu_xlate(struct device *dev, > struct acpi_iort_node *node, > * in the kernel or not, defer the IOMMU configuration > * or just abort it. > */ > - ops = iommu_ops_from_fwnode(iort_fwnode); > - if (!ops) > - return iort_iommu_driver_enabled(node->type) ? > - -EPROBE_DEFER : -ENODEV; > - > - return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops); > + return acpi_iommu_fwspec_init(fwspec, dev, streamid, iort_fwnode, > + iort_iommu_driver_enabled(node->type)); > } > > struct iort_pci_alias_info { > struct device *dev; > struct acpi_iort_node *node; > + struct iommu_fwspec *fwspec; > }; > > static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data) > @@ -1260,7 +1256,7 @@ static int iort_pci_iommu_init(struct pci_dev *pdev, > u16 alias, void *data) > > parent = iort_node_map_id(info->node, alias, &streamid, > IORT_IOMMU_TYPE); > - return iort_iommu_xlate(info->dev, parent, streamid); > + return iort_iommu_xlate(info->fwspec, info->dev, parent, streamid); > } > > static void iort_named_component_init(struct device *dev, > @@ -1280,7 +1276,8 @@ static void iort_named_component_init(struct device > *dev, > dev_warn(dev, "Could not add device properties\n"); > } > > -static int iort_nc_iommu_map(struct device *dev, struct acpi_iort_node *node) > +static int iort_nc_iommu_map(struct iommu_fwspec *fwspec, struct device *dev, > +struct acpi_iort_node *node) > { > struct acpi_iort_node *parent; > int err = -ENODEV, i = 0; > @@ -1293,13 +1290,13 @@ static int iort_nc_iommu_map(struct device *dev, > struct acpi_iort_node *node) >i++); > > if (parent) > - err = iort_iommu_xlate(dev, parent, streamid); > + err = iort_iommu_xlate(fwspec, dev, parent, streamid); > } while (parent && !err); > > return err; > } > > -static int iort_nc_iommu_map_id(struct device *dev, > +static int iort_nc_iommu_map_id(struct iommu_fwspec *fwspec, struct device > *dev, > struct acpi_iort_node *node, > const u32 *in_id) > { > @@ -1308,7 +1305,7 @@ static int iort_nc_iommu_map_id(struct device *dev, > > parent = iort_node_map_id(node, *in_id, &streamid, IORT_IOMMU_TYPE); > if (parent) > - return iort_iommu_xlate(dev, parent, streamid); > + return iort_iommu_xlate(fwspec, dev, parent, streamid); > > return -ENODEV; > } > @@ -1322,15 +1319,16 @@ static int iort_nc_iommu_map_id(struct device *dev, > * > * Returns: 0 on success, <0 on failure > */ > -int iort_iommu_configure_id(struct device *dev, const u32 *id_in) > +int iort_iommu_configure_id(struct iommu_fwspec *fwspec, struct device *dev, > + const u32 *id_in) > { > struct acpi_iort_node *node; > int