On Fri, Feb 10, 2023 at 03:06:37PM +0000, Marc Zyngier wrote:

On Fri, 10 Feb 2023 12:57:40 +0000,
Johan Hovold [off-list ref] wrote:

quoted

On Fri, Feb 10, 2023 at 11:38:58AM +0000, Marc Zyngier wrote:

quoted

On Fri, 10 Feb 2023 09:56:03 +0000,
Johan Hovold [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

@@ -1132,6 +1147,7 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
 	else
 		domain = irq_domain_create_tree(fwnode, ops, host_data);
 	if (domain) {
+		domain->root = parent->root;
 		domain->parent = parent;
 		domain->flags |= flags;

So we still have a bug here, as we have published a domain that we
keep updating. A parallel probing could find it in the interval and do
something completely wrong.

Indeed we do, even if device links should make this harder to hit these
days.

quoted

Splitting the work would help, as per the following patch.

Looks good to me. Do you want to submit that as a patch that I'll rebase
on or should I submit it as part of a v6?

Just take it directly.

Ok, thanks.

I've added a commit message and turned it into a patch to include in v6
now:

commit 3af395aa894c7df94ef2337e572e5e1710b4bbda (HEAD -> work)
Author: Marc Zyngier [off-list ref]
Date:   Thu Feb 9 16:00:55 2023 +0000

    irqdomain: Fix domain registration race
    
    Hierarchical domains created using irq_domain_create_hierarchy() are
    currently added to the domain list before having been fully initialised.
    
    This specifically means that a racing allocation request might fail to
    allocate irq data for the inner domains of a hierarchy in case the
    parent domain pointer has not yet been set up.
    
    Note that this is not really any issue for irqchip drivers that are
    registered early via IRQCHIP_DECLARE() or IRQCHIP_ACPI_DECLARE(), but
    could potentially cause trouble with drivers that are registered later
    (e.g. when using IRQCHIP_PLATFORM_DRIVER_BEGIN(), gpiochip drivers,
    etc.).
    
    Fixes: afb7da83b9f4 ("irqdomain: Introduce helper function irq_domain_add_hierarchy()")
    Cc: stable@vger.kernel.org      # 3.19
    ...
    [ johan: add a commit message ]
    Signed-off-by: Johan Hovold [off-list ref]

Could you just give your SoB for the diff here so I can credit you as
author?

quoted

I guess this turns the "Use irq_domain_create_hierarchy()" patches into
fixes that should be backported as well.

Maybe. Backports are not my immediate concern.

Turns out all of those drivers are registered early via
IRQCHIP_DECLARE() or IRQCHIP_ACPI_DECLARE() so there shouldn't really be
any risk of hitting this race for those.
 

quoted

But note that your proposed diff may not be sufficient to prevent
lookups from racing with domain registration generally. Many drivers
still update the bus token after the domain has been added (and
apparently some still set flags also after creating hierarchies I just
noticed, e.g. amd_iommu_create_irq_domain).

The bus token should only rarely be a problem, as it is often set on
an intermediate level which isn't directly looked-up by anything else.
And if it did happen, it would probably result in a the domain not
being found.

Flags, on the other hand, are more problematic. But I consider this a
driver bug which should be fixed independently.

I agree.
 

quoted

It seems we'd need to expose a separate allocation and registration
interface, or at least pass in the bus token to a new combined
interface.

Potentially, yes. But this could come later down the line. I'm more
concerned in getting this series into -next, as the merge window is
fast approaching.

I'll post a v6 first thing Monday if you can give me that SoB before
then.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel