From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> To: Marc Zyngier <maz@kernel.org>, <linuxarm@huawei.com> Cc: Thomas Gleixner <tglx@linutronix.de>, Peter Zijlstra <peterz@infradead.org>, <linux-pm@vger.kernel.org>, <loongarch@lists.linux.dev>, <linux-acpi@vger.kernel.org>, <linux-arch@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <kvmarm@lists.linux.dev>, <x86@kernel.org>, Russell King <linux@armlinux.org.uk>, "Rafael J . Wysocki" <rafael@kernel.org>, Miguel Luis <miguel.luis@oracle.com>, "James Morse" <james.morse@arm.com>, Salil Mehta <salil.mehta@huawei.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Hanjun Guo <guohanjun@huawei.com>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, <linuxarm@huawei.com>, <justin.he@arm.com>, <jianyong.wu@arm.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, Sudeep Holla <sudeep.holla@arm.com> Subject: Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs Date: Mon, 29 Apr 2024 10:21:31 +0100 [thread overview] Message-ID: <20240429101938.000027b2@huawei.com> (raw) In-Reply-To: <87frv5u3p8.wl-maz@kernel.org> On Sun, 28 Apr 2024 12:28:03 +0100 Marc Zyngier <maz@kernel.org> wrote: > On Fri, 26 Apr 2024 19:28:58 +0100, > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > > > > I'll not send a formal v9 until early next week, so here is the current state > > if you have time to take another look before then. > > Don't bother resending this on my account -- you only sent it on > Friday and there hasn't been much response to it yet. There is still a > problem (see below), but looks otherwise OK. > > [...] > > > @@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header, > > (struct acpi_madt_generic_interrupt *)header; > > u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK; > > u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2; > > + int cpu = get_cpu_for_acpi_id(gicc->uid); > > I already commented that get_cpu_for_acpi_id() can... Indeed sorry - I blame Friday syndrome for me failing to address that. > > > void __iomem *redist_base; > > > > - if (!acpi_gicc_is_usable(gicc)) > > + /* Neither enabled or online capable means it doesn't exist, skip it */ > > + if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE))) > > return 0; > > > > + /* > > + * Capable but disabled CPUs can be brought online later. What about > > + * the redistributor? ACPI doesn't want to say! > > + * Virtual hotplug systems can use the MADT's "always-on" GICR entries. > > + * Otherwise, prevent such CPUs from being brought online. > > + */ > > + if (!(gicc->flags & ACPI_MADT_ENABLED)) { > > + pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu); > > + cpumask_set_cpu(cpu, &broken_rdists); > > ... return -EINVAL, and then be passed to cpumask_set_cpu(), with > interesting effects. It shouldn't happen, but I trust anything that > comes from firmware tables as much as I trust a campaigning > politician's promises. This should really result in the RD being > considered unusable, but without affecting any CPU (there is no valid > CPU the first place). > > Another question is what get_cpu_for acpi_id() returns for a disabled > CPU. A valid CPU number? Or -EINVAL? It's a match function that works by iterating over 0 to nr_cpu_ids and if (uid == get_acpi_id_for_cpu(cpu)) So the question become does get_acpi_id_for_cpu() return a valid CPU number for a disabled CPU. That uses acpi_cpu_get_madt_gicc(cpu)->uid so this all gets a bit circular. That looks it up via cpu_madt_gicc[cpu] which after the proposed updated patch is set if enabled or online capable. There are however a few other error checks in acpi_map_gic_cpu_interface() that could lead to it not being set (MPIDR validity checks). I suspect all of these end up being fatal elsewhere which is why this hasn't blown up before. If any of those cases are possible we could get a null pointer dereference. Easy to harden this case via the following (which will leave us with -EINVAL. There are other call sites that might trip over this. I'm inclined to harden them as a separate issue though so as not to get in the way of this patch set. diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index bc9a6656fc0c..a407f9cd549e 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -124,7 +124,8 @@ static inline int get_cpu_for_acpi_id(u32 uid) int cpu; for (cpu = 0; cpu < nr_cpu_ids; cpu++) - if (uid == get_acpi_id_for_cpu(cpu)) + if (acpi_cpu_get_madt_gicc(cpu) && + uid == get_acpi_id_for_cpu(cpu)) return cpu; return -EINVAL; I'll spin an additional patch to make that change after testing I haven't messed it up. At the call site in gic_acpi_parse_madt_gicc() I'm not sure we can do better than just skipping setting broken_rdists. I'll also pull the declaration of that cpu variable down into this condition so it's more obvious we only care about it in this error path. Jonathan > > Thanks, > > M. >
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> To: Marc Zyngier <maz@kernel.org>, <linuxarm@huawei.com> Cc: Thomas Gleixner <tglx@linutronix.de>, Peter Zijlstra <peterz@infradead.org>, <linux-pm@vger.kernel.org>, <loongarch@lists.linux.dev>, <linux-acpi@vger.kernel.org>, <linux-arch@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <kvmarm@lists.linux.dev>, <x86@kernel.org>, Russell King <linux@armlinux.org.uk>, "Rafael J . Wysocki" <rafael@kernel.org>, Miguel Luis <miguel.luis@oracle.com>, "James Morse" <james.morse@arm.com>, Salil Mehta <salil.mehta@huawei.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Hanjun Guo <guohanjun@huawei.com>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, <linuxarm@huawei.com>, <justin.he@arm.com>, <jianyong.wu@arm.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, Sudeep Holla <sudeep.holla@arm.com> Subject: Re: [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs Date: Mon, 29 Apr 2024 10:21:31 +0100 [thread overview] Message-ID: <20240429101938.000027b2@huawei.com> (raw) In-Reply-To: <87frv5u3p8.wl-maz@kernel.org> On Sun, 28 Apr 2024 12:28:03 +0100 Marc Zyngier <maz@kernel.org> wrote: > On Fri, 26 Apr 2024 19:28:58 +0100, > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > > > > > I'll not send a formal v9 until early next week, so here is the current state > > if you have time to take another look before then. > > Don't bother resending this on my account -- you only sent it on > Friday and there hasn't been much response to it yet. There is still a > problem (see below), but looks otherwise OK. > > [...] > > > @@ -2363,11 +2381,25 @@ gic_acpi_parse_madt_gicc(union acpi_subtable_headers *header, > > (struct acpi_madt_generic_interrupt *)header; > > u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK; > > u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2; > > + int cpu = get_cpu_for_acpi_id(gicc->uid); > > I already commented that get_cpu_for_acpi_id() can... Indeed sorry - I blame Friday syndrome for me failing to address that. > > > void __iomem *redist_base; > > > > - if (!acpi_gicc_is_usable(gicc)) > > + /* Neither enabled or online capable means it doesn't exist, skip it */ > > + if (!(gicc->flags & (ACPI_MADT_ENABLED | ACPI_MADT_GICC_ONLINE_CAPABLE))) > > return 0; > > > > + /* > > + * Capable but disabled CPUs can be brought online later. What about > > + * the redistributor? ACPI doesn't want to say! > > + * Virtual hotplug systems can use the MADT's "always-on" GICR entries. > > + * Otherwise, prevent such CPUs from being brought online. > > + */ > > + if (!(gicc->flags & ACPI_MADT_ENABLED)) { > > + pr_warn("CPU %u's redistributor is inaccessible: this CPU can't be brought online\n", cpu); > > + cpumask_set_cpu(cpu, &broken_rdists); > > ... return -EINVAL, and then be passed to cpumask_set_cpu(), with > interesting effects. It shouldn't happen, but I trust anything that > comes from firmware tables as much as I trust a campaigning > politician's promises. This should really result in the RD being > considered unusable, but without affecting any CPU (there is no valid > CPU the first place). > > Another question is what get_cpu_for acpi_id() returns for a disabled > CPU. A valid CPU number? Or -EINVAL? It's a match function that works by iterating over 0 to nr_cpu_ids and if (uid == get_acpi_id_for_cpu(cpu)) So the question become does get_acpi_id_for_cpu() return a valid CPU number for a disabled CPU. That uses acpi_cpu_get_madt_gicc(cpu)->uid so this all gets a bit circular. That looks it up via cpu_madt_gicc[cpu] which after the proposed updated patch is set if enabled or online capable. There are however a few other error checks in acpi_map_gic_cpu_interface() that could lead to it not being set (MPIDR validity checks). I suspect all of these end up being fatal elsewhere which is why this hasn't blown up before. If any of those cases are possible we could get a null pointer dereference. Easy to harden this case via the following (which will leave us with -EINVAL. There are other call sites that might trip over this. I'm inclined to harden them as a separate issue though so as not to get in the way of this patch set. diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index bc9a6656fc0c..a407f9cd549e 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -124,7 +124,8 @@ static inline int get_cpu_for_acpi_id(u32 uid) int cpu; for (cpu = 0; cpu < nr_cpu_ids; cpu++) - if (uid == get_acpi_id_for_cpu(cpu)) + if (acpi_cpu_get_madt_gicc(cpu) && + uid == get_acpi_id_for_cpu(cpu)) return cpu; return -EINVAL; I'll spin an additional patch to make that change after testing I haven't messed it up. At the call site in gic_acpi_parse_madt_gicc() I'm not sure we can do better than just skipping setting broken_rdists. I'll also pull the declaration of that cpu variable down into this condition so it's more obvious we only care about it in this error path. Jonathan > > Thanks, > > M. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-04-29 9:21 UTC|newest] Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-04-26 13:51 [PATCH v8 00/16] ACPI/arm64: add support for virtual cpu hotplug Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 01/16] ACPI: processor: Simplify initial onlining to use same path for cold and hotplug Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 16:05 ` Miguel Luis 2024-04-26 16:05 ` Miguel Luis 2024-04-26 17:21 ` Miguel Luis 2024-04-26 17:21 ` Miguel Luis 2024-04-26 17:49 ` Jonathan Cameron 2024-04-26 17:49 ` Jonathan Cameron 2024-04-26 17:57 ` Rafael J. Wysocki 2024-04-26 17:57 ` Rafael J. Wysocki 2024-04-26 18:09 ` Jonathan Cameron 2024-04-26 18:09 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 02/16] cpu: Do not warn on arch_register_cpu() returning -EPROBE_DEFER Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 03/16] ACPI: processor: Drop duplicated check on _STA (enabled + present) Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 04/16] ACPI: processor: Move checks and availability of acpi_processor earlier Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-30 4:17 ` Gavin Shan 2024-04-30 4:17 ` Gavin Shan 2024-04-30 9:28 ` Jonathan Cameron 2024-04-30 9:28 ` Jonathan Cameron 2024-04-30 10:12 ` Rafael J. Wysocki 2024-04-30 10:12 ` Rafael J. Wysocki 2024-04-30 10:13 ` Jonathan Cameron 2024-04-30 10:13 ` Jonathan Cameron 2024-04-30 10:17 ` Rafael J. Wysocki 2024-04-30 10:17 ` Rafael J. Wysocki 2024-04-30 10:45 ` Jonathan Cameron 2024-04-30 10:45 ` Jonathan Cameron 2024-04-30 10:47 ` Rafael J. Wysocki 2024-04-30 10:47 ` Rafael J. Wysocki 2024-04-30 13:42 ` Jonathan Cameron 2024-04-30 13:42 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 05/16] ACPI: processor: Add acpi_get_processor_handle() helper Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-30 4:26 ` Gavin Shan 2024-04-30 4:26 ` Gavin Shan 2024-04-30 11:07 ` Jonathan Cameron 2024-04-30 11:07 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 06/16] ACPI: processor: Register deferred CPUs from acpi_processor_get_info() Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 07/16] ACPI: scan: switch to flags for acpi_scan_check_and_detach() Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 08/16] ACPI: Add post_eject to struct acpi_scan_handler for cpu hotplug Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 09/16] arm64: acpi: Move get_cpu_for_acpi_id() to a header Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-30 16:37 ` Lorenzo Pieralisi 2024-04-30 16:37 ` Lorenzo Pieralisi 2024-04-26 13:51 ` [PATCH v8 10/16] irqchip/gic-v3: Don't return errors from gic_acpi_match_gicc() Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 15:14 ` Marc Zyngier 2024-04-26 15:14 ` Marc Zyngier 2024-04-26 13:51 ` [PATCH v8 11/16] irqchip/gic-v3: Add support for ACPI's disabled but 'online capable' CPUs Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 16:26 ` Marc Zyngier 2024-04-26 16:26 ` Marc Zyngier 2024-04-26 18:28 ` Jonathan Cameron 2024-04-26 18:28 ` Jonathan Cameron 2024-04-28 11:28 ` Marc Zyngier 2024-04-28 11:28 ` Marc Zyngier 2024-04-29 9:21 ` Jonathan Cameron [this message] 2024-04-29 9:21 ` Jonathan Cameron 2024-04-30 12:15 ` Jonathan Cameron 2024-04-30 12:15 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 12/16] arm64: psci: Ignore DENIED CPUs Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-30 4:29 ` Gavin Shan 2024-04-30 4:29 ` Gavin Shan 2024-04-26 13:51 ` [PATCH v8 13/16] arm64: arch_register_cpu() variant to check if an ACPI handle is now available Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-30 4:31 ` Gavin Shan 2024-04-30 4:31 ` Gavin Shan 2024-04-26 13:51 ` [PATCH v8 14/16] arm64: Kconfig: Enable hotplug CPU on arm64 if ACPI_PROCESSOR is enabled Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 15/16] arm64: document virtual CPU hotplug's expectations Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron 2024-04-26 13:51 ` [PATCH v8 16/16] cpumask: Add enabled cpumask for present CPUs that can be brought online Jonathan Cameron 2024-04-26 13:51 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240429101938.000027b2@huawei.com \ --to=jonathan.cameron@huawei.com \ --cc=bp@alien8.de \ --cc=catalin.marinas@arm.com \ --cc=dave.hansen@linux.intel.com \ --cc=guohanjun@huawei.com \ --cc=james.morse@arm.com \ --cc=jean-philippe@linaro.org \ --cc=jianyong.wu@arm.com \ --cc=justin.he@arm.com \ --cc=kvmarm@lists.linux.dev \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=linuxarm@huawei.com \ --cc=loongarch@lists.linux.dev \ --cc=lpieralisi@kernel.org \ --cc=maz@kernel.org \ --cc=miguel.luis@oracle.com \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=rafael@kernel.org \ --cc=salil.mehta@huawei.com \ --cc=sudeep.holla@arm.com \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.