2022-11-10 16:23:34

by Ross Philipson

[permalink] [raw]
Subject: [PATCH v2 0/2] x86: Check return values for early memory/IO remap calls

While sending an earlier patch set it was discovered that there are a
number of places in early x86 code were the functions early_memremap()
and early_ioremap() are called but the returned pointer is not checked
for NULL. Since NULL can be returned for a couple of reasons, the return
value should be checked for NULL.

This set fixes the places where the checks were missing. It was not always
clear what the best failure mode should be when NULL is detected. In modules
where other places tended to pr_warn or panic e.g., the same was done for
the checks. In other places it was based on how significantly fatal the
failure would end up being. The review process may point out places where
this should be changed.

Changes in v2:
- Added notes in comments about why panic() was used in some cases and
the fact that maintainers approved the usage.
- Added pr_fmt macros in changed files to allow proper usage of pr_*
printing macros.

Ross Philipson (2):
x86: Check return values from early_memremap calls
x86: Check return values from early_ioremap calls

arch/x86/kernel/apic/x2apic_uv_x.c | 2 ++
arch/x86/kernel/devicetree.c | 13 ++++++++++
arch/x86/kernel/e820.c | 12 +++++++--
arch/x86/kernel/early_printk.c | 2 ++
arch/x86/kernel/jailhouse.c | 6 +++++
arch/x86/kernel/mpparse.c | 51 ++++++++++++++++++++++++++++----------
arch/x86/kernel/setup.c | 19 +++++++++++---
arch/x86/kernel/vsmp_64.c | 3 +++
arch/x86/xen/enlighten_hvm.c | 2 ++
arch/x86/xen/mmu_pv.c | 8 ++++++
arch/x86/xen/setup.c | 2 ++
11 files changed, 102 insertions(+), 18 deletions(-)

--
1.8.3.1



2023-01-03 21:14:23

by Ross Philipson

[permalink] [raw]
Subject: Re: [PATCH v2 0/2] x86: Check return values for early memory/IO remap calls

On 11/10/22 10:45, Ross Philipson wrote:
> While sending an earlier patch set it was discovered that there are a
> number of places in early x86 code were the functions early_memremap()
> and early_ioremap() are called but the returned pointer is not checked
> for NULL. Since NULL can be returned for a couple of reasons, the return
> value should be checked for NULL.
>
> This set fixes the places where the checks were missing. It was not always
> clear what the best failure mode should be when NULL is detected. In modules
> where other places tended to pr_warn or panic e.g., the same was done for
> the checks. In other places it was based on how significantly fatal the
> failure would end up being. The review process may point out places where
> this should be changed.

Borislav,

I just wanted to get your thoughts here since it was by your prompting
that I sent this second patch set to make checking of return values from
early_memremap() and early_ioremap() consistent. I have gotten
Reviewed-by's from some of the maintainers in specific areas that they
approve of the return handling. I also got two replies basically
questioning the underlying approach. I replied that I basically did what
you asked me to do. I have not heard back. How would you like me to proceed?

Thanks,

Ross Philipson

>
> Changes in v2:
> - Added notes in comments about why panic() was used in some cases and
> the fact that maintainers approved the usage.
> - Added pr_fmt macros in changed files to allow proper usage of pr_*
> printing macros.
>
> Ross Philipson (2):
> x86: Check return values from early_memremap calls
> x86: Check return values from early_ioremap calls
>
> arch/x86/kernel/apic/x2apic_uv_x.c | 2 ++
> arch/x86/kernel/devicetree.c | 13 ++++++++++
> arch/x86/kernel/e820.c | 12 +++++++--
> arch/x86/kernel/early_printk.c | 2 ++
> arch/x86/kernel/jailhouse.c | 6 +++++
> arch/x86/kernel/mpparse.c | 51 ++++++++++++++++++++++++++++----------
> arch/x86/kernel/setup.c | 19 +++++++++++---
> arch/x86/kernel/vsmp_64.c | 3 +++
> arch/x86/xen/enlighten_hvm.c | 2 ++
> arch/x86/xen/mmu_pv.c | 8 ++++++
> arch/x86/xen/setup.c | 2 ++
> 11 files changed, 102 insertions(+), 18 deletions(-)
>