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 <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Signed-off-by: Andy Lutomirski <[email protected]>
---
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)
{
- 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));
/* Are we prepared to handle this kernel fault? */
if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
@@ -743,7 +736,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
if (is_prefetch(regs, error_code, address))
return;
-oops:
page_fault_oops(regs, error_code, address);
}
@@ -790,7 +782,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
struct task_struct *tsk = current;
if (!user_mode(regs)) {
- no_context(regs, error_code, address, pkey, si_code);
+ kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
return;
}
@@ -922,7 +914,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
{
/* Kernel mode? Handle exceptions or die: */
if (!user_mode(regs)) {
- no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
+ kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
return;
}
@@ -1382,8 +1374,8 @@ void do_user_addr_fault(struct pt_regs *regs,
* has unlocked the mm for us if we get here.
*/
if (!user_mode(regs))
- no_context(regs, error_code, address, SIGBUS,
- BUS_ADRERR);
+ kernelmode_fixup_or_oops(regs, error_code, address,
+ SIGBUS, BUS_ADRERR);
return;
}
@@ -1403,15 +1395,15 @@ void do_user_addr_fault(struct pt_regs *regs,
return;
if (fatal_signal_pending(current) && !user_mode(regs)) {
- no_context(regs, error_code, address, 0, 0);
+ kernelmode_fixup_or_oops(regs, error_code, address, 0, 0);
return;
}
if (fault & VM_FAULT_OOM) {
/* Kernel mode? Handle exceptions or die: */
if (!user_mode(regs)) {
- no_context(regs, error_code, address,
- SIGSEGV, SEGV_MAPERR);
+ kernelmode_fixup_or_oops(regs, error_code, address,
+ SIGSEGV, SEGV_MAPERR);
return;
}
--
2.29.2
On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote:
> + kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
> if (!user_mode(regs)) {
> - no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
> + kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
These overly long lines are a little annoying..
On Mon, Feb 1, 2021 at 1:14 AM Christoph Hellwig <[email protected]> wrote:
>
> On Sun, Jan 31, 2021 at 09:24:40AM -0800, Andy Lutomirski wrote:
> > + kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
>
> > if (!user_mode(regs)) {
> > - no_context(regs, error_code, address, SIGBUS, BUS_ADRERR);
> > + kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
>
> These overly long lines are a little annoying..
It's not that long, and Linus seems to think you should make your
terminal wider :)
--Andy
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 <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
> 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" ?
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.
> {
> - 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
> On Feb 3, 2021, at 11:39 AM, Borislav Petkov <[email protected]> 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 <[email protected]>
>> Cc: Peter Zijlstra <[email protected]>
>> Signed-off-by: Andy Lutomirski <[email protected]>
>> ---
>> 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
On Wed, Feb 03, 2021 at 11:53:03AM -0800, Andy Lutomirski wrote:
> 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.
You practically pretty much have it already with the WARN_ON_ONCE. And
you can make the thing return 1 to denote it was in user_mode() and 0
otherwise. IINMSO, something like this:
---
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 08f5f74cf989..2b86d541b181 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -692,11 +692,12 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code,
oops_end(flags, regs, sig);
}
-static noinline void
+static noinline int
kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
unsigned long address, int signal, int si_code)
{
- WARN_ON_ONCE(user_mode(regs));
+ if (WARN_ON_ONCE(user_mode(regs)))
+ return 1;
/* Are we prepared to handle this kernel fault? */
if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
@@ -706,7 +707,7 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
* task context.
*/
if (in_interrupt())
- return;
+ return 0;
/*
* Per the above we're !in_interrupt(), aka. task context.
@@ -726,7 +727,7 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
/*
* Barring that, we can do the fixup and be happy.
*/
- return;
+ return 0;
}
/*
@@ -734,9 +735,11 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
* instruction.
*/
if (is_prefetch(regs, error_code, address))
- return;
+ return 0;
page_fault_oops(regs, error_code, address);
+
+ return 0;
}
/*
@@ -781,10 +784,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
{
struct task_struct *tsk = current;
- if (!user_mode(regs)) {
- kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code);
+ if (!kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code))
return;
- }
if (!(error_code & X86_PF_USER)) {
/* Implicit user access to kernel memory -- just oops */
@@ -913,10 +914,8 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
vm_fault_t fault)
{
/* Kernel mode? Handle exceptions or die: */
- if (!user_mode(regs)) {
- kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR);
+ if (!kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR))
return;
- }
/* User-space => ok to do another page fault: */
if (is_prefetch(regs, error_code, address))
@@ -1378,10 +1377,8 @@ void do_user_addr_fault(struct pt_regs *regs,
* Quick path to respond to signals. The core mm code
* has unlocked the mm for us if we get here.
*/
- if (!user_mode(regs))
- kernelmode_fixup_or_oops(regs, error_code, address,
- SIGBUS, BUS_ADRERR);
- return;
+ if (!kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR))
+ return;
}
/*
@@ -1399,18 +1396,15 @@ void do_user_addr_fault(struct pt_regs *regs,
if (likely(!(fault & VM_FAULT_ERROR)))
return;
- if (fatal_signal_pending(current) && !user_mode(regs)) {
- kernelmode_fixup_or_oops(regs, error_code, address, 0, 0);
- return;
+ if (fatal_signal_pending(current)) {
+ if (!kernelmode_fixup_or_oops(regs, error_code, address, 0, 0))
+ return;
}
if (fault & VM_FAULT_OOM) {
/* Kernel mode? Handle exceptions or die: */
- if (!user_mode(regs)) {
- kernelmode_fixup_or_oops(regs, error_code, address,
- SIGSEGV, SEGV_MAPERR);
+ if (!kernelmode_fixup_or_oops(regs, error_code, address, SIGSEGV, SEGV_MAPERR))
return;
- }
/*
* We ran out of memory, call the OOM killer, and return the
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Feb 3, 2021 at 12:07 PM Borislav Petkov <[email protected]> wrote:
>
> On Wed, Feb 03, 2021 at 11:53:03AM -0800, Andy Lutomirski wrote:
> > 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.
>
> You practically pretty much have it already with the WARN_ON_ONCE. And
> you can make the thing return 1 to denote it was in user_mode() and 0
> otherwise. IINMSO, something like this:
Hmm. I'm not convinced this is really much better. Maybe it is. Let
me think about it. I feel like it's somehow too close to the previous
tangle where too many functions did too many different things.
On Wed, Feb 03, 2021 at 12:14:24PM -0800, Andy Lutomirski wrote:
> Hmm. I'm not convinced this is really much better. Maybe it is. Let
> me think about it. I feel like it's somehow too close to the previous
> tangle where too many functions did too many different things.
I know what you mean.
Well, we can put it aside for now since you'll likely do more cleanup
here and it might become unnecessary after you're done. :-)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette