Re: [PATCH v3] x86,mm: print likely CPU at segfault time

From: Ingo Molnar
Date: Sat Aug 06 2022 - 04:50:00 EST


* Rik van Riel <riel@xxxxxxxxxxx> wrote:

> On Fri, 5 Aug 2022 16:27:40 +0200
> Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> > On Fri, Aug 05, 2022 at 10:16:44AM -0400, Rik van Riel wrote:
> > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > > index fad8faa29d04..c7a5bbf40367 100644
> > > --- a/arch/x86/mm/fault.c
> > > +++ b/arch/x86/mm/fault.c
> > > @@ -769,6 +769,8 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code,
> > > unsigned long address, struct task_struct *tsk)
> > > {
> > > const char *loglvl = task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG;
> > > + /* This is a racy snapshot, but it's better than nothing. */
> > > + int cpu = raw_smp_processor_id();
> >
> > Please read this in exc_page_fault() and hand it down to helpers.
>
> Below is the change that implements your suggestion.
>
> If there is consensus among the x86 maintainers that this is
> desirable, I am more than happy to merge that change into my
> patch and resubmit v4.
>
> I don't have a strong opinion either way.
>
> ---8<---
>
> From 444f8588f0edfd8586a86e85191ad8fa8b7c6a6c Mon Sep 17 00:00:00 2001
> From: Rik van Riel <riel@xxxxxxxxxxx>
> Date: Fri, 5 Aug 2022 10:32:11 -0400
> Subject: [PATCH 2/2] x86,mm: get CPU number for segfault printk before
> enabling preemption
>
> Get the CPU number for the segfault printk earlier in the page fault
> handler, before preemption is enabled.
>
> Suggested-by: Borislav Petkov <bp@xxxxxxxxx>
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> ---
> arch/x86/mm/fault.c | 58 +++++++++++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index c7a5bbf40367..bd06b22826b2 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -766,11 +766,9 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
> */
> static inline void
> show_signal_msg(struct pt_regs *regs, unsigned long error_code,
> - unsigned long address, struct task_struct *tsk)
> + unsigned long address, struct task_struct *tsk, int cpu)

> __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
> - unsigned long address, u32 pkey, int si_code)
> + unsigned long address, u32 pkey, int si_code, int cpu)

> - show_signal_msg(regs, error_code, address, tsk);
> + show_signal_msg(regs, error_code, address, tsk, cpu);

> - unsigned long address)
> + unsigned long address, int cpu)

> - unsigned long address, u32 pkey, int si_code)
> + unsigned long address, u32 pkey, int si_code, int cpu)

> - __bad_area_nosemaphore(regs, error_code, address, pkey, si_code);
> + __bad_area_nosemaphore(regs, error_code, address, pkey, si_code, cpu);

> -bad_area(struct pt_regs *regs, unsigned long error_code, unsigned long address)
> +bad_area(struct pt_regs *regs, unsigned long error_code, unsigned long address,
> + int cpu)
> {
> - __bad_area(regs, error_code, address, 0, SEGV_MAPERR);
> + __bad_area(regs, error_code, address, 0, SEGV_MAPERR, cpu);

> bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
> - unsigned long address, struct vm_area_struct *vma)
> + unsigned long address, struct vm_area_struct *vma,
> + int cpu)

> - __bad_area(regs, error_code, address, pkey, SEGV_PKUERR);
> + __bad_area(regs, error_code, address, pkey, SEGV_PKUERR, cpu);
> } else {
> - __bad_area(regs, error_code, address, 0, SEGV_ACCERR);
> + __bad_area(regs, error_code, address, 0, SEGV_ACCERR, cpu);

> static void
> do_kern_addr_fault(struct pt_regs *regs, unsigned long hw_error_code,
> - unsigned long address)
> + unsigned long address, int cpu)

> - bad_area_nosemaphore(regs, hw_error_code, address);
> + bad_area_nosemaphore(regs, hw_error_code, address, cpu);

> void do_user_addr_fault(struct pt_regs *regs,
> unsigned long error_code,
> - unsigned long address)
> + unsigned long address,
> + int cpu)

> - bad_area_nosemaphore(regs, error_code, address);
> + bad_area_nosemaphore(regs, error_code, address, cpu);

> - bad_area_nosemaphore(regs, error_code, address);
> + bad_area_nosemaphore(regs, error_code, address, cpu);

> - bad_area(regs, error_code, address);
> + bad_area(regs, error_code, address, cpu);

> - bad_area(regs, error_code, address);
> + bad_area(regs, error_code, address, cpu);

> - bad_area(regs, error_code, address);
> + bad_area(regs, error_code, address, cpu);

> - bad_area_access_error(regs, error_code, address, vma);
> + bad_area_access_error(regs, error_code, address, vma, cpu);

> - bad_area_nosemaphore(regs, error_code, address);
> + bad_area_nosemaphore(regs, error_code, address, cpu);

> - unsigned long address)
> + unsigned long address, int cpu)

> - do_kern_addr_fault(regs, error_code, address);
> + do_kern_addr_fault(regs, error_code, address, cpu);

> - do_user_addr_fault(regs, error_code, address);
> + do_user_addr_fault(regs, error_code, address, cpu);

> DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
> {
> unsigned long address = read_cr2();
> + int cpu = raw_smp_processor_id();
> irqentry_state_t state;
>
> prefetchw(&current->mm->mmap_lock);
> @@ -1547,7 +1549,7 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
> state = irqentry_enter(regs);
>
> instrumentation_begin();
> - handle_page_fault(regs, error_code, address);
> + handle_page_fault(regs, error_code, address, cpu);

Not convinced that this is a good change: this will bloat all the affected
code by a couple of dozen instructions - for no good reason in the context
of this patch.

Boris, why should we do this? Extracting a parameter at higher levels and
passing it down to lower levels is almost always a bad idea from a code
generation POV, unless the majority of lower levels needs this information
anyway (which isn't the case here).

Thanks,

Ingo