Messages in this thread | | | From | Andy Lutomirski <> | Subject | Re: [PATCH 09/11] x86/fault: Rename no_context() to kernelmode_fixup_or_oops() | Date | Wed, 3 Feb 2021 11:53:03 -0800 |
| |
> On Feb 3, 2021, at 11:39 AM, Borislav Petkov <bp@alien8.de> wrote: > > On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote: >> The name no_context() has never been very clear. It's only called for >> faults from kernel mode, so rename it and change the no-longer-useful >> user_mode(regs) check to a WARN_ON_ONCE. >> >> Cc: Dave Hansen <dave.hansen@linux.intel.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> --- >> arch/x86/mm/fault.c | 28 ++++++++++------------------ >> 1 file changed, 10 insertions(+), 18 deletions(-) >> >> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >> index 177b612c7f33..04cc98ec2423 100644 >> --- a/arch/x86/mm/fault.c >> +++ b/arch/x86/mm/fault.c >> @@ -693,17 +693,10 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code, >> } >> >> static noinline void >> -no_context(struct pt_regs *regs, unsigned long error_code, >> - unsigned long address, int signal, int si_code) >> +kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code, >> + unsigned long address, int signal, int si_code) > > Ew, I don't like functions with "or" in the name - they're probably not > doing one thing only as they should. > > Why not simply "handle_kernel_fault" ?
Hmm, I could maybe get behind handle_kernelmode_fault. I’ll contemplate it. I like the name of the function indicating that either it returns after fixing it or it doesn’t return.
I refuse to say “kernel” without qualification. In this $@!$ file, we have kernel _mode_, kernel _address_, and kernel _privilege_, and they are all different.
> > Also, all the callsites now do: > > if (!user_mode(regs)) { > kernelmode_fixup_or_oops > ... > > I guess you can push the "user_mode" check inside that function for less > hairy code at the callsites.
I feel like that would be more obfuscated — then the function would return without fixing anything for usermode faults, return after fixing it for kernel mode faults, or oops.
> >> { >> - if (user_mode(regs)) { >> - /* >> - * This is an implicit supervisor-mode access from user >> - * mode. Bypass all the kernel-mode recovery code and just >> - * OOPS. >> - */ >> - goto oops; >> - } >> + WARN_ON_ONCE(user_mode(regs)); > > I guess... > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
| |