Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221727 > unrolled thread

[PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks Al Stone <al.stone@linaro.org> - 2015-09-09 23:20 +0200
    [PATCH v3 5/5] ACPI: remove definition of BAD_MADT_ENTRY macro Al Stone <al.stone@linaro.org> - 2015-09-09 23:20 +0200
    [PATCH v3 4/5] ACPI / X86: remove usage of BAD_MADT_ENTRY Al Stone <al.stone@linaro.org> - 2015-09-09 23:20 +0200
    [PATCH v3 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY Al Stone <al.stone@linaro.org> - 2015-09-09 23:20 +0200
    [PATCH v3 3/5] ACPI / IA64: remove usage of BAD_MADT_ENTRY Al Stone <al.stone@linaro.org> - 2015-09-09 23:20 +0200
    Re: [Linaro-acpi] [PATCH v3 0/5] ACPI: Provide better MADT subtable  sanity checks Graeme Gregory <graeme.gregory@linaro.org> - 2015-09-10 14:20 +0200
    Re: [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks Al Stone <ahs3@redhat.com> - 2015-09-15 23:20 +0200
      Re: [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-16 04:20 +0200
        Re: [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks Al Stone <ahs3@redhat.com> - 2015-09-16 18:30 +0200
          Re: [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks Al Stone <ahs3@redhat.com> - 2015-09-16 21:30 +0200
          Re: [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-17 02:10 +0200
        Re: [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks Al Stone <al.stone@linaro.org> - 2015-09-16 18:30 +0200

#1221727 — [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks

Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
check on the various subtables that are defined for the MADT.  The check
compares the size of the subtable data structure as defined by ACPICA to
the length entry in the subtable.  If they are not the same, the assumption
is that the subtable is incorrect.

Over time, the ACPI spec has allowed for MADT subtables where this can
never be true (the local SAPIC subtable, for example).  Or, more recently,
the spec has accumulated some minor flaws where there are three possible 
sizes for a subtable, all of which are valid, but only for specific versions
of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
subtables as bad when they are not.  In order to retain some sanity check
on the MADT subtables, we now have to special case these subtables.  Of
necessity, these special cases have ended up in arch-dependent code (arm64)
or an arch has simply decided to forgo the check (ia64).

This patch set replaces the BAD_MADT_ENTRY macro with a function called
bad_madt_entry().  This function uses a data set of details about the
subtables to provide more sanity checking than before:

	-- is the subtable legal for the version given in the FADT?

	-- is the subtable legal for the revision of the MADT in use?

	-- is the subtable of the proper length (including checking
	   on the one variable length subtable that is currently ignored),
	   given the FADT version and the MADT revision?

Further, this patch set adds in the call to bad_madt_entry() from the 
acpi_table_parse_madt() function, allowing it to be used consistently
by all architectures, for all subtables, and removing the need for each
of the subtable traversal callback functions to use BAD_MADT_ENTRY.

In theory, as the ACPI specification changes, we would only have to add
additional information to the data set describing the MADT subtables in
order to continue providing sanity checks, even when new subtables are
added.

These patches have been tested on an APM Mustang (arm64) and are known to
work there.  They have also been cross-compiled for x86 and ia64 with no
known failures.

Changes for v3:
   -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
   -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
   -- Double checked that inserting call to bad_madt_entry() into the
      function acpi_parse_entries() does not impact current behavior
      (Sudeep Holla)
   
Changes for v2:
   -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
   -- Correct faulty end of loop test found by Timur Tabi


Al Stone (5):
  ACPI: add in a bad_madt_entry() function to eventually replace the
    macro
  ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
  ACPI / IA64: remove usage of BAD_MADT_ENTRY
  ACPI / X86: remove usage of BAD_MADT_ENTRY
  ACPI: remove definition of BAD_MADT_ENTRY macro

 arch/arm64/include/asm/acpi.h |   8 --
 arch/arm64/kernel/smp.c       |   2 -
 arch/ia64/kernel/acpi.c       |  20 ----
 arch/x86/kernel/acpi/boot.c   |  27 -----
 drivers/acpi/tables.c         | 245 +++++++++++++++++++++++++++++++++++++++++-
 drivers/irqchip/irq-gic.c     |   6 --
 include/linux/acpi.h          |   4 -
 7 files changed, 244 insertions(+), 68 deletions(-)

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221729 — [PATCH v3 5/5] ACPI: remove definition of BAD_MADT_ENTRY macro

Now that we have introduced to bad_madt_entry(), and we have removed
all the usages of the BAD_MADT_ENTRY macro from all of the various
architectures that use it (arm64, ia64, x86), we can remove the macro
definition since it is no longer used.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 include/linux/acpi.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7235c48..a6d6326 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -127,10 +127,6 @@ static inline void acpi_initrd_override(void *data, size_t size)
 }
 #endif
 
-#define BAD_MADT_ENTRY(entry, end) (					    \
-		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
-		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221732 — [PATCH v3 4/5] ACPI / X86: remove usage of BAD_MADT_ENTRY

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <len.brown@intel.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/acpi/boot.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index ded848c..d37b84d 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -194,9 +194,6 @@ acpi_parse_x2apic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_x2apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	apic_id = processor->local_apic_id;
@@ -227,9 +224,6 @@ acpi_parse_lapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_apic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/*
@@ -252,9 +246,6 @@ acpi_parse_sapic(struct acpi_subtable_header *header, const unsigned long end)
 
 	processor = (struct acpi_madt_local_sapic *)header;
 
-	if (BAD_MADT_ENTRY(processor, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */
@@ -271,9 +262,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic_addr_ovr = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic_addr_ovr, end))
-		return -EINVAL;
-
 	acpi_lapic_addr = lapic_addr_ovr->address;
 
 	return 0;
@@ -287,9 +275,6 @@ acpi_parse_x2apic_nmi(struct acpi_subtable_header *header,
 
 	x2apic_nmi = (struct acpi_madt_local_x2apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(x2apic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (x2apic_nmi->lint != 1)
@@ -305,9 +290,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lapic_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lapic_nmi, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (lapic_nmi->lint != 1)
@@ -411,9 +393,6 @@ acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	ioapic = (struct acpi_madt_io_apic *)header;
 
-	if (BAD_MADT_ENTRY(ioapic, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* Statically assign IRQ numbers for IOAPICs hosting legacy IRQs */
@@ -463,9 +442,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	intsrc = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(intsrc, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	if (intsrc->source_irq == acpi_gbl_FADT.sci_interrupt) {
@@ -504,9 +480,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	acpi_table_print_madt_entry(header);
 
 	/* TBD: Support nimsrc entries? */
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221733 — [PATCH v3 2/5] ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro, or in the case
of arm64, the BAD_MADT_GICC_ENTRY, too.

Signed-off-by: Al Stone <al.stone@linaro.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm64/include/asm/acpi.h | 8 --------
 arch/arm64/kernel/smp.c       | 2 --
 drivers/irqchip/irq-gic.c     | 6 ------
 3 files changed, 16 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 208cec0..ed7e212 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,14 +19,6 @@
 #include <asm/cputype.h>
 #include <asm/smp_plat.h>
 
-/* Macros for consistency checks of the GICC subtable of MADT */
-#define ACPI_MADT_GICC_LENGTH	\
-	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
-
-#define BAD_MADT_GICC_ENTRY(entry, end)						\
-	(!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||	\
-	 (entry)->header.length != ACPI_MADT_GICC_LENGTH)
-
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 /* ACPI table mapping after acpi_gbl_permanent_mmap is set */
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index dbdaacd..66cc8c4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -451,8 +451,6 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
 	struct acpi_madt_generic_interrupt *processor;
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
-	if (BAD_MADT_GICC_ENTRY(processor, end))
-		return -EINVAL;
 
 	acpi_table_print_madt_entry(header);
 
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index e6b7ed5..b3530e3 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1189,9 +1189,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
-	if (BAD_MADT_GICC_ENTRY(processor, end))
-		return -EINVAL;
-
 	/*
 	 * There is no support for non-banked GICv1/2 register in ACPI spec.
 	 * All CPU interface addresses have to be the same.
@@ -1213,9 +1210,6 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
 
 	dist = (struct acpi_madt_generic_distributor *)header;
 
-	if (BAD_MADT_ENTRY(dist, end))
-		return -EINVAL;
-
 	dist_phy_base = dist->base_address;
 	return 0;
 }
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221737 — [PATCH v3 3/5] ACPI / IA64: remove usage of BAD_MADT_ENTRY

Now that we have introduced the bad_madt_entry() function, and that
function is being invoked in acpi_table_parse_madt() for us, there
is no longer any need to use the BAD_MADT_ENTRY macro.

Signed-off-by: Al Stone <al.stone@linaro.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/ia64/kernel/acpi.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index b1698bc..efa3f0a 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -184,9 +184,6 @@ acpi_parse_lapic_addr_ovr(struct acpi_subtable_header * header,
 
 	lapic = (struct acpi_madt_local_apic_override *)header;
 
-	if (BAD_MADT_ENTRY(lapic, end))
-		return -EINVAL;
-
 	if (lapic->address) {
 		iounmap(ipi_base_addr);
 		ipi_base_addr = ioremap(lapic->address, 0);
@@ -201,8 +198,6 @@ acpi_parse_lsapic(struct acpi_subtable_header * header, const unsigned long end)
 
 	lsapic = (struct acpi_madt_local_sapic *)header;
 
-	/*Skip BAD_MADT_ENTRY check, as lsapic size could vary */
-
 	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
 #ifdef CONFIG_SMP
 		smp_boot_data.cpu_phys_id[available_cpus] =
@@ -222,9 +217,6 @@ acpi_parse_lapic_nmi(struct acpi_subtable_header * header, const unsigned long e
 
 	lacpi_nmi = (struct acpi_madt_local_apic_nmi *)header;
 
-	if (BAD_MADT_ENTRY(lacpi_nmi, end))
-		return -EINVAL;
-
 	/* TBD: Support lapic_nmi entries */
 	return 0;
 }
@@ -236,9 +228,6 @@ acpi_parse_iosapic(struct acpi_subtable_header * header, const unsigned long end
 
 	iosapic = (struct acpi_madt_io_sapic *)header;
 
-	if (BAD_MADT_ENTRY(iosapic, end))
-		return -EINVAL;
-
 	return iosapic_init(iosapic->address, iosapic->global_irq_base);
 }
 
@@ -253,9 +242,6 @@ acpi_parse_plat_int_src(struct acpi_subtable_header * header,
 
 	plintsrc = (struct acpi_madt_interrupt_source *)header;
 
-	if (BAD_MADT_ENTRY(plintsrc, end))
-		return -EINVAL;
-
 	/*
 	 * Get vector assignment for this interrupt, set attributes,
 	 * and program the IOSAPIC routing table.
@@ -336,9 +322,6 @@ acpi_parse_int_src_ovr(struct acpi_subtable_header * header,
 
 	p = (struct acpi_madt_interrupt_override *)header;
 
-	if (BAD_MADT_ENTRY(p, end))
-		return -EINVAL;
-
 	iosapic_override_isa_irq(p->source_irq, p->global_irq,
 				 ((p->inti_flags & ACPI_MADT_POLARITY_MASK) ==
 				  ACPI_MADT_POLARITY_ACTIVE_LOW) ?
@@ -356,9 +339,6 @@ acpi_parse_nmi_src(struct acpi_subtable_header * header, const unsigned long end
 
 	nmi_src = (struct acpi_madt_nmi_source *)header;
 
-	if (BAD_MADT_ENTRY(nmi_src, end))
-		return -EINVAL;
-
 	/* TBD: Support nimsrc entries */
 	return 0;
 }
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222157 — Re: [Linaro-acpi] [PATCH v3 0/5] ACPI: Provide better MADT subtable sanity checks

On Wed, Sep 09, 2015 at 03:09:46PM -0600, Al Stone wrote:
> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
> check on the various subtables that are defined for the MADT.  The check
> compares the size of the subtable data structure as defined by ACPICA to
> the length entry in the subtable.  If they are not the same, the assumption
> is that the subtable is incorrect.
> 
> Over time, the ACPI spec has allowed for MADT subtables where this can
> never be true (the local SAPIC subtable, for example).  Or, more recently,
> the spec has accumulated some minor flaws where there are three possible 
> sizes for a subtable, all of which are valid, but only for specific versions
> of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
> subtables as bad when they are not.  In order to retain some sanity check
> on the MADT subtables, we now have to special case these subtables.  Of
> necessity, these special cases have ended up in arch-dependent code (arm64)
> or an arch has simply decided to forgo the check (ia64).
> 
> This patch set replaces the BAD_MADT_ENTRY macro with a function called
> bad_madt_entry().  This function uses a data set of details about the
> subtables to provide more sanity checking than before:
> 
> 	-- is the subtable legal for the version given in the FADT?
> 
> 	-- is the subtable legal for the revision of the MADT in use?
> 
> 	-- is the subtable of the proper length (including checking
> 	   on the one variable length subtable that is currently ignored),
> 	   given the FADT version and the MADT revision?
> 
> Further, this patch set adds in the call to bad_madt_entry() from the 
> acpi_table_parse_madt() function, allowing it to be used consistently
> by all architectures, for all subtables, and removing the need for each
> of the subtable traversal callback functions to use BAD_MADT_ENTRY.
> 
> In theory, as the ACPI specification changes, we would only have to add
> additional information to the data set describing the MADT subtables in
> order to continue providing sanity checks, even when new subtables are
> added.
> 
> These patches have been tested on an APM Mustang (arm64) and are known to
> work there.  They have also been cross-compiled for x86 and ia64 with no
> known failures.
> 
> Changes for v3:
>    -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
>    -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
>    -- Double checked that inserting call to bad_madt_entry() into the
>       function acpi_parse_entries() does not impact current behavior
>       (Sudeep Holla)
>    
> Changes for v2:
>    -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
>    -- Correct faulty end of loop test found by Timur Tabi
> 
> 
> Al Stone (5):
>   ACPI: add in a bad_madt_entry() function to eventually replace the
>     macro
>   ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
>   ACPI / IA64: remove usage of BAD_MADT_ENTRY
>   ACPI / X86: remove usage of BAD_MADT_ENTRY
>   ACPI: remove definition of BAD_MADT_ENTRY macro
> 
>  arch/arm64/include/asm/acpi.h |   8 --
>  arch/arm64/kernel/smp.c       |   2 -
>  arch/ia64/kernel/acpi.c       |  20 ----
>  arch/x86/kernel/acpi/boot.c   |  27 -----
>  drivers/acpi/tables.c         | 245 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/irqchip/irq-gic.c     |   6 --
>  include/linux/acpi.h          |   4 -
>  7 files changed, 244 insertions(+), 68 deletions(-)
> 

Whole series looks good to me apart from minor whitespace nit.

Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>

Thanks

G

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225550

On 09/09/2015 03:09 PM, Al Stone wrote:
> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
> check on the various subtables that are defined for the MADT.  The check
> compares the size of the subtable data structure as defined by ACPICA to
> the length entry in the subtable.  If they are not the same, the assumption
> is that the subtable is incorrect.
> 
> Over time, the ACPI spec has allowed for MADT subtables where this can
> never be true (the local SAPIC subtable, for example).  Or, more recently,
> the spec has accumulated some minor flaws where there are three possible 
> sizes for a subtable, all of which are valid, but only for specific versions
> of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
> subtables as bad when they are not.  In order to retain some sanity check
> on the MADT subtables, we now have to special case these subtables.  Of
> necessity, these special cases have ended up in arch-dependent code (arm64)
> or an arch has simply decided to forgo the check (ia64).
> 
> This patch set replaces the BAD_MADT_ENTRY macro with a function called
> bad_madt_entry().  This function uses a data set of details about the
> subtables to provide more sanity checking than before:
> 
> 	-- is the subtable legal for the version given in the FADT?
> 
> 	-- is the subtable legal for the revision of the MADT in use?
> 
> 	-- is the subtable of the proper length (including checking
> 	   on the one variable length subtable that is currently ignored),
> 	   given the FADT version and the MADT revision?
> 
> Further, this patch set adds in the call to bad_madt_entry() from the 
> acpi_table_parse_madt() function, allowing it to be used consistently
> by all architectures, for all subtables, and removing the need for each
> of the subtable traversal callback functions to use BAD_MADT_ENTRY.
> 
> In theory, as the ACPI specification changes, we would only have to add
> additional information to the data set describing the MADT subtables in
> order to continue providing sanity checks, even when new subtables are
> added.
> 
> These patches have been tested on an APM Mustang (arm64) and are known to
> work there.  They have also been cross-compiled for x86 and ia64 with no
> known failures.
> 
> Changes for v3:
>    -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
>    -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
>    -- Double checked that inserting call to bad_madt_entry() into the
>       function acpi_parse_entries() does not impact current behavior
>       (Sudeep Holla)
>    
> Changes for v2:
>    -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
>    -- Correct faulty end of loop test found by Timur Tabi
> 
> 
> Al Stone (5):
>   ACPI: add in a bad_madt_entry() function to eventually replace the
>     macro
>   ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
>   ACPI / IA64: remove usage of BAD_MADT_ENTRY
>   ACPI / X86: remove usage of BAD_MADT_ENTRY
>   ACPI: remove definition of BAD_MADT_ENTRY macro
> 
>  arch/arm64/include/asm/acpi.h |   8 --
>  arch/arm64/kernel/smp.c       |   2 -
>  arch/ia64/kernel/acpi.c       |  20 ----
>  arch/x86/kernel/acpi/boot.c   |  27 -----
>  drivers/acpi/tables.c         | 245 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/irqchip/irq-gic.c     |   6 --
>  include/linux/acpi.h          |   4 -
>  7 files changed, 244 insertions(+), 68 deletions(-)
> 

Ping?  Any additional comments on this version?  I have only received
feedback from arm64 reviewers so far, over three revisions, even though
everyone that needs to be (ACPI, ia64, x86) has also been CCd.

Anyone else before I fix a couple of things for v4 that the arm64 folks
found?  ACKs?  NAKs?  Please don't bother me, I'm in the merge window :)?

Thanks.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225674

On Tuesday, September 15, 2015 03:13:12 PM Al Stone wrote:
> On 09/09/2015 03:09 PM, Al Stone wrote:
> > Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
> > check on the various subtables that are defined for the MADT.  The check
> > compares the size of the subtable data structure as defined by ACPICA to
> > the length entry in the subtable.  If they are not the same, the assumption
> > is that the subtable is incorrect.
> > 
> > Over time, the ACPI spec has allowed for MADT subtables where this can
> > never be true (the local SAPIC subtable, for example).  Or, more recently,
> > the spec has accumulated some minor flaws where there are three possible 
> > sizes for a subtable, all of which are valid, but only for specific versions
> > of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
> > subtables as bad when they are not.  In order to retain some sanity check
> > on the MADT subtables, we now have to special case these subtables.  Of
> > necessity, these special cases have ended up in arch-dependent code (arm64)
> > or an arch has simply decided to forgo the check (ia64).
> > 
> > This patch set replaces the BAD_MADT_ENTRY macro with a function called
> > bad_madt_entry().  This function uses a data set of details about the
> > subtables to provide more sanity checking than before:
> > 
> > 	-- is the subtable legal for the version given in the FADT?
> > 
> > 	-- is the subtable legal for the revision of the MADT in use?
> > 
> > 	-- is the subtable of the proper length (including checking
> > 	   on the one variable length subtable that is currently ignored),
> > 	   given the FADT version and the MADT revision?
> > 
> > Further, this patch set adds in the call to bad_madt_entry() from the 
> > acpi_table_parse_madt() function, allowing it to be used consistently
> > by all architectures, for all subtables, and removing the need for each
> > of the subtable traversal callback functions to use BAD_MADT_ENTRY.
> > 
> > In theory, as the ACPI specification changes, we would only have to add
> > additional information to the data set describing the MADT subtables in
> > order to continue providing sanity checks, even when new subtables are
> > added.
> > 
> > These patches have been tested on an APM Mustang (arm64) and are known to
> > work there.  They have also been cross-compiled for x86 and ia64 with no
> > known failures.
> > 
> > Changes for v3:
> >    -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
> >    -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
> >    -- Double checked that inserting call to bad_madt_entry() into the
> >       function acpi_parse_entries() does not impact current behavior
> >       (Sudeep Holla)
> >    
> > Changes for v2:
> >    -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
> >    -- Correct faulty end of loop test found by Timur Tabi
> > 
> > 
> > Al Stone (5):
> >   ACPI: add in a bad_madt_entry() function to eventually replace the
> >     macro
> >   ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
> >   ACPI / IA64: remove usage of BAD_MADT_ENTRY
> >   ACPI / X86: remove usage of BAD_MADT_ENTRY
> >   ACPI: remove definition of BAD_MADT_ENTRY macro
> > 
> >  arch/arm64/include/asm/acpi.h |   8 --
> >  arch/arm64/kernel/smp.c       |   2 -
> >  arch/ia64/kernel/acpi.c       |  20 ----
> >  arch/x86/kernel/acpi/boot.c   |  27 -----
> >  drivers/acpi/tables.c         | 245 +++++++++++++++++++++++++++++++++++++++++-
> >  drivers/irqchip/irq-gic.c     |   6 --
> >  include/linux/acpi.h          |   4 -
> >  7 files changed, 244 insertions(+), 68 deletions(-)
> > 
> 
> Ping?  Any additional comments on this version?  I have only received
> feedback from arm64 reviewers so far, over three revisions, even though
> everyone that needs to be (ACPI, ia64, x86) has also been CCd.
> 
> Anyone else before I fix a couple of things for v4 that the arm64 folks
> found?  ACKs?  NAKs?  Please don't bother me, I'm in the merge window :)?

The merge window is actually over, so why would you expect anything like that?

I'm going to apply this series if people have no problems with it.  I do think
it is slightly overkill, but then as long as it works ...

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226241

On 09/16/2015 10:24 AM, Al Stone wrote:
> On 09/15/2015 08:44 PM, Rafael J. Wysocki wrote:
>> On Tuesday, September 15, 2015 03:13:12 PM Al Stone wrote:
>>> On 09/09/2015 03:09 PM, Al Stone wrote:
>>>> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
>>>> check on the various subtables that are defined for the MADT.  The check
>>>> compares the size of the subtable data structure as defined by ACPICA to
>>>> the length entry in the subtable.  If they are not the same, the assumption
>>>> is that the subtable is incorrect.
>>>>

[snip...]

>> I'm going to apply this series if people have no problems with it.  I do think
>> it is slightly overkill, but then as long as it works ...
>>
>> Thanks,
>> Rafael
>>

Would you prefer I send out the updated version with Sudeep's fix to apply
instead of this version?  Or have you picked that up already?  Easily updated
and sent out, if it'll make your life simpler....

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226415

On 09/16/2015 10:27 AM, Al Stone wrote:
> On 09/16/2015 10:24 AM, Al Stone wrote:
>> On 09/15/2015 08:44 PM, Rafael J. Wysocki wrote:
>>> On Tuesday, September 15, 2015 03:13:12 PM Al Stone wrote:
>>>> On 09/09/2015 03:09 PM, Al Stone wrote:
>>>>> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
>>>>> check on the various subtables that are defined for the MADT.  The check
>>>>> compares the size of the subtable data structure as defined by ACPICA to
>>>>> the length entry in the subtable.  If they are not the same, the assumption
>>>>> is that the subtable is incorrect.
>>>>>
> 
> [snip...]
> 
>>> I'm going to apply this series if people have no problems with it.  I do think
>>> it is slightly overkill, but then as long as it works ...

Indeed it does.  We've found and fixed two MADTs with legitimate errors in them
with these patches (both arm64 tables still in development).

>>> Thanks,
>>> Rafael
>>>
> 
> Would you prefer I send out the updated version with Sudeep's fix to apply
> instead of this version?  Or have you picked that up already?  Easily updated
> and sent out, if it'll make your life simpler....
> 

Bah.  Never mind.  An inadvertent use of the 01day builder by someone else
found another nit; I'll spin out a v4 later today just so everything is very
clear.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226590

On Wednesday, September 16, 2015 10:27:57 AM Al Stone wrote:
> On 09/16/2015 10:24 AM, Al Stone wrote:
> > On 09/15/2015 08:44 PM, Rafael J. Wysocki wrote:
> >> On Tuesday, September 15, 2015 03:13:12 PM Al Stone wrote:
> >>> On 09/09/2015 03:09 PM, Al Stone wrote:
> >>>> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
> >>>> check on the various subtables that are defined for the MADT.  The check
> >>>> compares the size of the subtable data structure as defined by ACPICA to
> >>>> the length entry in the subtable.  If they are not the same, the assumption
> >>>> is that the subtable is incorrect.
> >>>>
> 
> [snip...]
> 
> >> I'm going to apply this series if people have no problems with it.  I do think
> >> it is slightly overkill, but then as long as it works ...
> >>
> >> Thanks,
> >> Rafael
> >>
> 
> Would you prefer I send out the updated version with Sudeep's fix to apply
> instead of this version?

Yes, I would.

> Or have you picked that up already?

No, I haven't.

> Easily updated and sent out, if it'll make your life simpler....

Yes, please.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226242

On 09/15/2015 08:44 PM, Rafael J. Wysocki wrote:
> On Tuesday, September 15, 2015 03:13:12 PM Al Stone wrote:
>> On 09/09/2015 03:09 PM, Al Stone wrote:
>>> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
>>> check on the various subtables that are defined for the MADT.  The check
>>> compares the size of the subtable data structure as defined by ACPICA to
>>> the length entry in the subtable.  If they are not the same, the assumption
>>> is that the subtable is incorrect.
>>>
>>> Over time, the ACPI spec has allowed for MADT subtables where this can
>>> never be true (the local SAPIC subtable, for example).  Or, more recently,
>>> the spec has accumulated some minor flaws where there are three possible 
>>> sizes for a subtable, all of which are valid, but only for specific versions
>>> of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
>>> subtables as bad when they are not.  In order to retain some sanity check
>>> on the MADT subtables, we now have to special case these subtables.  Of
>>> necessity, these special cases have ended up in arch-dependent code (arm64)
>>> or an arch has simply decided to forgo the check (ia64).
>>>
>>> This patch set replaces the BAD_MADT_ENTRY macro with a function called
>>> bad_madt_entry().  This function uses a data set of details about the
>>> subtables to provide more sanity checking than before:
>>>
>>> 	-- is the subtable legal for the version given in the FADT?
>>>
>>> 	-- is the subtable legal for the revision of the MADT in use?
>>>
>>> 	-- is the subtable of the proper length (including checking
>>> 	   on the one variable length subtable that is currently ignored),
>>> 	   given the FADT version and the MADT revision?
>>>
>>> Further, this patch set adds in the call to bad_madt_entry() from the 
>>> acpi_table_parse_madt() function, allowing it to be used consistently
>>> by all architectures, for all subtables, and removing the need for each
>>> of the subtable traversal callback functions to use BAD_MADT_ENTRY.
>>>
>>> In theory, as the ACPI specification changes, we would only have to add
>>> additional information to the data set describing the MADT subtables in
>>> order to continue providing sanity checks, even when new subtables are
>>> added.
>>>
>>> These patches have been tested on an APM Mustang (arm64) and are known to
>>> work there.  They have also been cross-compiled for x86 and ia64 with no
>>> known failures.
>>>
>>> Changes for v3:
>>>    -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
>>>    -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
>>>    -- Double checked that inserting call to bad_madt_entry() into the
>>>       function acpi_parse_entries() does not impact current behavior
>>>       (Sudeep Holla)
>>>    
>>> Changes for v2:
>>>    -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
>>>    -- Correct faulty end of loop test found by Timur Tabi
>>>
>>>
>>> Al Stone (5):
>>>   ACPI: add in a bad_madt_entry() function to eventually replace the
>>>     macro
>>>   ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
>>>   ACPI / IA64: remove usage of BAD_MADT_ENTRY
>>>   ACPI / X86: remove usage of BAD_MADT_ENTRY
>>>   ACPI: remove definition of BAD_MADT_ENTRY macro
>>>
>>>  arch/arm64/include/asm/acpi.h |   8 --
>>>  arch/arm64/kernel/smp.c       |   2 -
>>>  arch/ia64/kernel/acpi.c       |  20 ----
>>>  arch/x86/kernel/acpi/boot.c   |  27 -----
>>>  drivers/acpi/tables.c         | 245 +++++++++++++++++++++++++++++++++++++++++-
>>>  drivers/irqchip/irq-gic.c     |   6 --
>>>  include/linux/acpi.h          |   4 -
>>>  7 files changed, 244 insertions(+), 68 deletions(-)
>>>
>>
>> Ping?  Any additional comments on this version?  I have only received
>> feedback from arm64 reviewers so far, over three revisions, even though
>> everyone that needs to be (ACPI, ia64, x86) has also been CCd.
>>
>> Anyone else before I fix a couple of things for v4 that the arm64 folks
>> found?  ACKs?  NAKs?  Please don't bother me, I'm in the merge window :)?
> 
> The merge window is actually over, so why would you expect anything like that?

I know.  Merely a feeble attempt at humor....

> I'm going to apply this series if people have no problems with it.  I do think
> it is slightly overkill, but then as long as it works ...
> 
> Thanks,
> Rafael
> 

Thanks, Rafael.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web