| Message ID | 20160125102148.GE24726@rric.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016/1/25 18:21, Robert Richter wrote: > On 23.01.16 17:39:20, Hanjun Guo wrote: >> diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c >> new file mode 100644 >> index 0000000..f7f7533 >> --- /dev/null >> +++ b/arch/arm64/kernel/acpi_numa.c >> +/* Callback for parsing of the Proximity Domain <-> Memory Area mappings */ >> +int __init acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) >> +{ >> + u64 start, end; >> + int node, pxm; >> + >> + if (srat_disabled()) >> + return -EINVAL; >> + >> + if (ma->header.length != sizeof(struct acpi_srat_mem_affinity)) { > Must be: > > ma->header.length < sizeof(struct acpi_srat_mem_affinity)) { > > Allow extensions to struct acpi_srat_mem_affinity in newer versions. Hmm, I think we need to remove the check here now. There are three cases: - firmware ACPI version is consistent with the ACPICA one, then ma->header.length == sizeof(struct acpi_srat_mem_affinity ) - firmware ACPI version is not consistent with the ACPICA one, for example, struct acpi_srat_mem_affinity is extended in new ACI version, but the formware is using the older one, then it's ok to use ma->header.length < sizeof(struct acpi_srat_mem_affinity ) - but if we use the older kernel + updated new firmware, then ma->header.length > sizeof(struct acpi_srat_mem_affinity ) will be the case, right? > >> + bad_srat(); >> + return -EINVAL; > We need a pr_err() here to avoid that numa setup fails silently due to > bad fw. This applies to all error paths. > > See my delta patch below. You can merge it with your patch. Thanks! I wil merge it into next version. Hanjun
On 27.01.16 15:12:15, Hanjun Guo wrote: > On 2016/1/25 18:21, Robert Richter wrote: > > On 23.01.16 17:39:20, Hanjun Guo wrote: > >> diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c > >> new file mode 100644 > >> index 0000000..f7f7533 > >> --- /dev/null > >> +++ b/arch/arm64/kernel/acpi_numa.c > >> +/* Callback for parsing of the Proximity Domain <-> Memory Area mappings */ > >> +int __init acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) > >> +{ > >> + u64 start, end; > >> + int node, pxm; > >> + > >> + if (srat_disabled()) > >> + return -EINVAL; > >> + > >> + if (ma->header.length != sizeof(struct acpi_srat_mem_affinity)) { > > Must be: > > > > ma->header.length < sizeof(struct acpi_srat_mem_affinity)) { > > > > Allow extensions to struct acpi_srat_mem_affinity in newer versions. > > Hmm, I think we need to remove the check here now. No, we might have an out-of-bound access then. > > There are three cases: > > - firmware ACPI version is consistent with the ACPICA one, then > ma->header.length == sizeof(struct acpi_srat_mem_affinity ) > > - firmware ACPI version is not consistent with the ACPICA one, > for example, struct acpi_srat_mem_affinity is extended in > new ACI version, but the formware is using the older one, > then it's ok to use > ma->header.length < sizeof(struct acpi_srat_mem_affinity ) The check above is ok as we need at least struct acpi_srat_mem_affinity as it is now. If we later change the kernel to support multiple versions of struct acpi_srat_mem_affinity, i.e. use data from an extended section, we will need to add code to handle that. This will include support of data with length < acpi_srat_mem_affinity, in this case we may not use extended data. > > - but if we use the older kernel + updated new firmware, > then > ma->header.length > sizeof(struct acpi_srat_mem_affinity ) > will be the case, right? Right, and this is a valid case not resulting in an error with my suggestion above. > > > > >> + bad_srat(); > >> + return -EINVAL; > > We need a pr_err() here to avoid that numa setup fails silently due to > > bad fw. This applies to all error paths. > > > > See my delta patch below. You can merge it with your patch. > > Thanks! I wil merge it into next version. Thanks, -Robert
On 2016/1/27 22:01, Robert Richter wrote: > On 27.01.16 15:12:15, Hanjun Guo wrote: >> On 2016/1/25 18:21, Robert Richter wrote: >>> On 23.01.16 17:39:20, Hanjun Guo wrote: >>>> diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c >>>> new file mode 100644 >>>> index 0000000..f7f7533 >>>> --- /dev/null >>>> +++ b/arch/arm64/kernel/acpi_numa.c >>>> +/* Callback for parsing of the Proximity Domain <-> Memory Area mappings */ >>>> +int __init acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) >>>> +{ >>>> + u64 start, end; >>>> + int node, pxm; >>>> + >>>> + if (srat_disabled()) >>>> + return -EINVAL; >>>> + >>>> + if (ma->header.length != sizeof(struct acpi_srat_mem_affinity)) { >>> Must be: >>> >>> ma->header.length < sizeof(struct acpi_srat_mem_affinity)) { >>> >>> Allow extensions to struct acpi_srat_mem_affinity in newer versions. >> Hmm, I think we need to remove the check here now. > No, we might have an out-of-bound access then. > >> There are three cases: >> >> - firmware ACPI version is consistent with the ACPICA one, then >> ma->header.length == sizeof(struct acpi_srat_mem_affinity ) >> >> - firmware ACPI version is not consistent with the ACPICA one, >> for example, struct acpi_srat_mem_affinity is extended in >> new ACI version, but the formware is using the older one, >> then it's ok to use >> ma->header.length < sizeof(struct acpi_srat_mem_affinity ) > The check above is ok as we need at least struct > acpi_srat_mem_affinity as it is now. > > If we later change the kernel to support multiple versions of struct > acpi_srat_mem_affinity, i.e. use data from an extended section, we > will need to add code to handle that. This will include support of > data with length < acpi_srat_mem_affinity, in this case we may not use > extended data. I checked the ACPI spec about memory affinity structure, it still have 10 bytes reserved for future use, so I think it's safe as you suggested for next few years. > >> - but if we use the older kernel + updated new firmware, >> then >> ma->header.length > sizeof(struct acpi_srat_mem_affinity ) >> will be the case, right? > Right, and this is a valid case not resulting in an error with my > suggestion above. Yes, I just mixed up those two cases. I will sync with Ganapat to prepare a new version and test it on x86 and IA64 to make sure this patch set don't break anything. Thanks Hanjun
diff --git a/arch/arm64/kernel/acpi_numa.c b/arch/arm64/kernel/acpi_numa.c index f7f7533761cd..936ad6c43740 100644 --- a/arch/arm64/kernel/acpi_numa.c +++ b/arch/arm64/kernel/acpi_numa.c @@ -143,6 +143,8 @@ void __init acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) return; if (pa->header.length < sizeof(struct acpi_srat_gicc_affinity)) { + pr_err("SRAT: Invalid SRAT header length: %d\n", + pa->header.length); bad_srat(); return; } @@ -166,7 +168,7 @@ void __init acpi_numa_gicc_affinity_init(struct acpi_srat_gicc_affinity *pa) } if (get_mpidr_in_madt(pa->acpi_processor_uid, &mpidr)) { - pr_warn("SRAT: PXM %d with ACPI ID %d has no valid MPIDR in MADT\n", + pr_err("SRAT: PXM %d with ACPI ID %d has no valid MPIDR in MADT\n", pxm, pa->acpi_processor_uid); bad_srat(); return; @@ -190,7 +192,9 @@ int __init acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) if (srat_disabled()) return -EINVAL; - if (ma->header.length != sizeof(struct acpi_srat_mem_affinity)) { + if (ma->header.length < sizeof(struct acpi_srat_mem_affinity)) { + pr_err("SRAT: Unexpected header length: %d\n", + ma->header.length); bad_srat(); return -EINVAL; } @@ -216,10 +220,17 @@ int __init acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) (unsigned long long) start, (unsigned long long) end - 1); if (numa_add_memblk(node, start, (end - start)) < 0) { + pr_err("SRAT: Failed to add memblk to node %u [mem %#010Lx-%#010Lx]\n", + node, (unsigned long long) start, + (unsigned long long) end - 1); bad_srat(); return -EINVAL; } + pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n", + node, pxm, + (unsigned long long) start, (unsigned long long) end - 1); + return 0; }