| Message ID | 20221025044409.448755-3-bgray@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use per-CPU temporary mappings for patching on Radix MMU | expand |
Le 25/10/2022 à 06:44, Benjamin Gray a écrit : > Detect and abort __do_patch_instruction() when there is no text_poke_area, > which implies there is no patching address. This allows patch_instruction() > to fail gracefully and let the caller decide what to do, as opposed to > the current behaviour of kernel panicking when the null pointer is > dereferenced. Is there any reason at all to have no text_poke_area ? If that's the boot CPU, then it means we are really early in the boot process and will use raw_patch_instruction() directly. Or it means we don't have enough memory for the boot CPU, in which case you are going to have so many problems that it is not worth any effort. If it is not the boot CPU, isn't there a way to not add a CPU for which the allocation has failed ? If text_area_cpu_up() returns an error, isn't the CPU deactivated ? > > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> > --- > v9: * New in v9 > --- > arch/powerpc/lib/code-patching.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index ad0cf3108dd0..54e145247643 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -76,6 +76,7 @@ static int text_area_cpu_up(unsigned int cpu) > static int text_area_cpu_down(unsigned int cpu) > { > free_vm_area(this_cpu_read(text_poke_area)); > + this_cpu_write(text_poke_area, NULL); > return 0; > } > > @@ -151,11 +152,16 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr) > { > int err; > u32 *patch_addr; > + struct vm_struct *area; > unsigned long text_poke_addr; > pte_t *pte; > unsigned long pfn = get_patch_pfn(addr); > > - text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK; > + area = __this_cpu_read(text_poke_area); > + if (unlikely(!area)) > + return -ENOMEM; > + > + text_poke_addr = (unsigned long)area->addr & PAGE_MASK; > patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr)); > > pte = virt_to_kpte(text_poke_addr);
On Wed, 2022-11-02 at 09:36 +0000, Christophe Leroy wrote: > Le 25/10/2022 à 06:44, Benjamin Gray a écrit : > > Detect and abort __do_patch_instruction() when there is no > > text_poke_area, > > which implies there is no patching address. This allows > > patch_instruction() > > to fail gracefully and let the caller decide what to do, as opposed > > to > > the current behaviour of kernel panicking when the null pointer is > > dereferenced. > > Is there any reason at all to have no text_poke_area ? > > If that's the boot CPU, then it means we are really early in the boot > process and will use raw_patch_instruction() directly. Or it means we > don't have enough memory for the boot CPU, in which case you are > going > to have so many problems that it is not worth any effort. > > If it is not the boot CPU, isn't there a way to not add a CPU for > which > the allocation has failed ? If text_area_cpu_up() returns an error, > isn't the CPU deactivated ? Right, I hadn't seen that the CPU isn't onlined if the startup fails. That would make the check redundant then.
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index ad0cf3108dd0..54e145247643 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -76,6 +76,7 @@ static int text_area_cpu_up(unsigned int cpu) static int text_area_cpu_down(unsigned int cpu) { free_vm_area(this_cpu_read(text_poke_area)); + this_cpu_write(text_poke_area, NULL); return 0; } @@ -151,11 +152,16 @@ static int __do_patch_instruction(u32 *addr, ppc_inst_t instr) { int err; u32 *patch_addr; + struct vm_struct *area; unsigned long text_poke_addr; pte_t *pte; unsigned long pfn = get_patch_pfn(addr); - text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr & PAGE_MASK; + area = __this_cpu_read(text_poke_area); + if (unlikely(!area)) + return -ENOMEM; + + text_poke_addr = (unsigned long)area->addr & PAGE_MASK; patch_addr = (u32 *)(text_poke_addr + offset_in_page(addr)); pte = virt_to_kpte(text_poke_addr);
Detect and abort __do_patch_instruction() when there is no text_poke_area, which implies there is no patching address. This allows patch_instruction() to fail gracefully and let the caller decide what to do, as opposed to the current behaviour of kernel panicking when the null pointer is dereferenced. Signed-off-by: Benjamin Gray <bgray@linux.ibm.com> --- v9: * New in v9 --- arch/powerpc/lib/code-patching.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)