2022-03-17 03:20:09

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv6 06/30] x86/traps: Refactor exc_general_protection()

TDX brings a new exception -- Virtualization Exception (#VE). Handling
of #VE structurally very similar to handling #GP.

Extract two helpers from exc_general_protection() that can be reused for
handling #VE.

No functional changes.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
---
arch/x86/kernel/traps.c | 60 +++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 7ef00dee35be..59ed14d8c53f 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -611,13 +611,43 @@ static bool try_fixup_enqcmd_gp(void)
#endif
}

+static bool gp_try_fixup_and_notify(struct pt_regs *regs, int trapnr,
+ unsigned long error_code, const char *str)
+{
+ int ret;
+
+ if (fixup_exception(regs, trapnr, error_code, 0))
+ return true;
+
+ current->thread.error_code = error_code;
+ current->thread.trap_nr = trapnr;
+
+ /*
+ * To be potentially processing a kprobe fault and to trust the result
+ * from kprobe_running(), we have to be non-preemptible.
+ */
+ if (!preemptible() && kprobe_running() &&
+ kprobe_fault_handler(regs, trapnr))
+ return true;
+
+ ret = notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV);
+ return ret == NOTIFY_STOP;
+}
+
+static void gp_user_force_sig_segv(struct pt_regs *regs, int trapnr,
+ unsigned long error_code, const char *str)
+{
+ current->thread.error_code = error_code;
+ current->thread.trap_nr = trapnr;
+ show_signal(current, SIGSEGV, "", str, regs, error_code);
+ force_sig(SIGSEGV);
+}
+
DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
{
char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
enum kernel_gp_hint hint = GP_NO_HINT;
- struct task_struct *tsk;
unsigned long gp_addr;
- int ret;

if (user_mode(regs) && try_fixup_enqcmd_gp())
return;
@@ -636,40 +666,18 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
return;
}

- tsk = current;
-
if (user_mode(regs)) {
if (fixup_iopl_exception(regs))
goto exit;

- tsk->thread.error_code = error_code;
- tsk->thread.trap_nr = X86_TRAP_GP;
-
if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
goto exit;

- show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
- force_sig(SIGSEGV);
+ gp_user_force_sig_segv(regs, X86_TRAP_GP, error_code, desc);
goto exit;
}

- if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
- goto exit;
-
- tsk->thread.error_code = error_code;
- tsk->thread.trap_nr = X86_TRAP_GP;
-
- /*
- * To be potentially processing a kprobe fault and to trust the result
- * from kprobe_running(), we have to be non-preemptible.
- */
- if (!preemptible() &&
- kprobe_running() &&
- kprobe_fault_handler(regs, X86_TRAP_GP))
- goto exit;
-
- ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);
- if (ret == NOTIFY_STOP)
+ if (gp_try_fixup_and_notify(regs, X86_TRAP_GP, error_code, desc))
goto exit;

if (error_code)
--
2.34.1


2022-03-17 05:59:38

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCHv6 06/30] x86/traps: Refactor exc_general_protection()

On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> TDX brings a new exception -- Virtualization Exception (#VE). Handling
> of #VE structurally very similar to handling #GP.
>
> Extract two helpers from exc_general_protection() that can be reused for
> handling #VE.
>
> No functional changes.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Reviewed-by: Dave Hansen <[email protected]>

Not that I care much, but this is the second instance of something I
suggested. We have tags for that...

> +static bool gp_try_fixup_and_notify(struct pt_regs *regs, int trapnr,
> + unsigned long error_code, const char *str)
> +{
> + int ret;

Adding this to make my suggestion compile is the easy way out, but...

> +
> + if (fixup_exception(regs, trapnr, error_code, 0))
> + return true;
> +
> + current->thread.error_code = error_code;
> + current->thread.trap_nr = trapnr;
> +
> + /*
> + * To be potentially processing a kprobe fault and to trust the result
> + * from kprobe_running(), we have to be non-preemptible.
> + */
> + if (!preemptible() && kprobe_running() &&
> + kprobe_fault_handler(regs, trapnr))
> + return true;
> +
> + ret = notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV);
> + return ret == NOTIFY_STOP;

Why not doing the obvious:

return notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV) == NOTIFY_STOP;

Hmm?

Thanks,

tglx

2022-03-17 20:06:20

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv6 06/30] x86/traps: Refactor exc_general_protection()

On Thu, Mar 17, 2022 at 01:21:53AM +0100, Thomas Gleixner wrote:
> On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
> > TDX brings a new exception -- Virtualization Exception (#VE). Handling
> > of #VE structurally very similar to handling #GP.
> >
> > Extract two helpers from exc_general_protection() that can be reused for
> > handling #VE.
> >
> > No functional changes.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > Reviewed-by: Dave Hansen <[email protected]>
>
> Not that I care much, but this is the second instance of something I
> suggested. We have tags for that...

Sorry, will add the tag.

By the other instance you meant common base for SEAMCALL and TDCALL C
wrappers, right? Will fix too.

> > +static bool gp_try_fixup_and_notify(struct pt_regs *regs, int trapnr,
> > + unsigned long error_code, const char *str)
> > +{
> > + int ret;
>
> Adding this to make my suggestion compile is the easy way out, but...
>
> > +
> > + if (fixup_exception(regs, trapnr, error_code, 0))
> > + return true;
> > +
> > + current->thread.error_code = error_code;
> > + current->thread.trap_nr = trapnr;
> > +
> > + /*
> > + * To be potentially processing a kprobe fault and to trust the result
> > + * from kprobe_running(), we have to be non-preemptible.
> > + */
> > + if (!preemptible() && kprobe_running() &&
> > + kprobe_fault_handler(regs, trapnr))
> > + return true;
> > +
> > + ret = notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV);
> > + return ret == NOTIFY_STOP;
>
> Why not doing the obvious:
>
> return notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV) == NOTIFY_STOP;
>
> Hmm?

I don't like lines this long (although my variant doesn't look good too).

Will do your way.

--
Kirill A. Shutemov