| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 7 Sep 2022 11:50:33 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: Robin Murphy <robin.murphy@....com>
Cc: joro@...tes.org, will@...nel.org, iommu@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, baolu.lu@...ux.intel.com,
kevin.tian@...el.com, suravee.suthikulpanit@....com,
vasant.hegde@....com, mjrosato@...ux.ibm.com,
schnelle@...ux.ibm.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 04/16] iommu: Always register bus notifiers
On Mon, Aug 15, 2022 at 9:20 AM Robin Murphy <robin.murphy@....com> wrote:
>
> The number of bus types that the IOMMU subsystem deals with is small and
> manageable, so pull that list into core code as a first step towards
> cleaning up all the boilerplate bus-awareness from drivers. Calling
> iommu_probe_device() before bus->iommu_ops is set will simply return
> -ENODEV and not break the notifier call chain, so there should be no
> harm in proactively registering all our bus notifiers at init time.
>
> Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
> Tested-by: Matthew Rosato <mjrosato@...ux.ibm.com> # s390
> Tested-by: Niklas Schnelle <schnelle@...ux.ibm.com> # s390
> Signed-off-by: Robin Murphy <robin.murphy@....com>
> ---
>
> v4: Squash iommu_bus_init() entirely, for maximum simplicity. Ignoring
> the return from bus_register_notifier() is common, so hopefully it's
> all pretty self-explanatory now.
>
> drivers/iommu/iommu.c | 72 ++++++++++++++++++++++---------------------
> 1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 780fb7071577..a8d14f2a1035 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -6,6 +6,7 @@
>
> #define pr_fmt(fmt) "iommu: " fmt
>
> +#include <linux/amba/bus.h>
> #include <linux/device.h>
> #include <linux/dma-iommu.h>
> #include <linux/kernel.h>
> @@ -16,11 +17,13 @@
> #include <linux/export.h>
> #include <linux/slab.h>
> #include <linux/errno.h>
> +#include <linux/host1x_context_bus.h>
> #include <linux/iommu.h>
> #include <linux/idr.h>
> #include <linux/err.h>
> #include <linux/pci.h>
> #include <linux/bitops.h>
> +#include <linux/platform_device.h>
> #include <linux/property.h>
> #include <linux/fsl/mc.h>
> #include <linux/module.h>
> @@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {
> #define IOMMU_CMD_LINE_DMA_API BIT(0)
> #define IOMMU_CMD_LINE_STRICT BIT(1)
>
> +static int iommu_bus_notifier(struct notifier_block *nb,
> + unsigned long action, void *data);
> static int iommu_alloc_default_domain(struct iommu_group *group,
> struct device *dev);
> static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> @@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name = \
> static LIST_HEAD(iommu_device_list);
> static DEFINE_SPINLOCK(iommu_device_lock);
>
> +static struct bus_type * const iommu_buses[] = {
> + &platform_bus_type,
> +#ifdef CONFIG_PCI
> + &pci_bus_type,
> +#endif
> +#ifdef CONFIG_ARM_AMBA
> + &amba_bustype,
> +#endif
> +#ifdef CONFIG_FSL_MC_BUS
> + &fsl_mc_bus_type,
> +#endif
> +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS
> + &host1x_context_device_bus_type,
> +#endif
> +};
> +
> /*
> * Use a function instead of an array here because the domain-type is a
> * bit-field, so an array would waste memory.
> @@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)
>
> static int __init iommu_subsys_init(void)
> {
> + struct notifier_block *nb;
> +
> if (!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API)) {
> if (IS_ENABLED(CONFIG_IOMMU_DEFAULT_PASSTHROUGH))
> iommu_set_default_passthrough(false);
> @@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void)
> (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> "(set via kernel command line)" : "");
>
> + nb = kcalloc(ARRAY_SIZE(iommu_buses), sizeof(*nb), GFP_KERNEL);
> + if (!nb)
> + return -ENOMEM;
> +
> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> + nb[i].notifier_call = iommu_bus_notifier;
> + bus_register_notifier(iommu_buses[i], &nb[i]);
> + }
> +
Carrying on the community's general disdain for notifiers, can we drop
the bus notifier and just call iommu_probe_device() directly from
device_add()? That way, you also won't need to keep an ifdef-ed array
of buses and it'll be easy to tell from driver core code that iommu
stuff is happening as devices are added. And I'd probably move that
call to be AFTER some of the fw_devlink stuff is done too.
Also, would it be possible to delay this until probe (sorry, don't
have enough of IOMMU details in my head) and call iommu_probe_device()
from really_probe() like we do to set up pinctrl, etc? That'd be
ideal.
-Saravana
> return 0;
> }
> subsys_initcall(iommu_subsys_init);
> @@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)
> return ret;
> }
>
> -static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
> -{
> - struct notifier_block *nb;
> - int err;
> -
> - nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
> - if (!nb)
> - return -ENOMEM;
> -
> - nb->notifier_call = iommu_bus_notifier;
> -
> - err = bus_register_notifier(bus, nb);
> - if (err)
> - goto out_free;
> -
> - err = bus_iommu_probe(bus);
> - if (err)
> - goto out_err;
> -
> -
> - return 0;
> -
> -out_err:
> - /* Clean up */
> - bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
> - bus_unregister_notifier(bus, nb);
> -
> -out_free:
> - kfree(nb);
> -
> - return err;
> -}
> -
> /**
> * bus_set_iommu - set iommu-callbacks for the bus
> * @bus: bus.
> @@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
> bus->iommu_ops = ops;
>
> /* Do IOMMU specific setup for this bus-type */
> - err = iommu_bus_init(bus, ops);
> - if (err)
> + err = bus_iommu_probe(bus);
> + if (err) {
> + /* Clean up */
> + bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
> bus->iommu_ops = NULL;
> + }
>
> return err;
> }
> --
> 2.36.1.dirty
>
>
Powered by blists - more mailing lists