xenomai.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* problem while handling exceptions in oob stage
@ 2023-02-23 13:22 Johannes Kirchmair
  2023-02-23 17:17 ` Jan Kiszka
  2023-02-25 18:17 ` Philippe Gerum
  0 siblings, 2 replies; 6+ messages in thread
From: Johannes Kirchmair @ 2023-02-23 13:22 UTC (permalink / raw)
  To: Philippe Gerum, xenomai; +Cc: Jan Kiszka

Hello,

we encountered a problem, while handling invalid opcode exception in oob stage. 
Our fixup code works fine if we execute ud2 instruction the first time.
Upon second execution of ud2, we encounter the problem that our fixup code is not reached.

The problem is that the first time entering the kernel, mark_trap_entry_raw is called and in a subsequent call the _TLF_OOBTRAP flag is set.
But because we do not switch to inband stage, we do not call mark_trap_exit_raw and the _TLF_OOBTRAP flag is not cleared.

The second time we enter the kernel on execution of ud2,  we encounter (again) the following code:

static __always_inline void oob_trap_notify(unsigned int exception,
                                         struct pt_regs *regs)
{
         if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
                 __oob_trap_notify(exception, regs);
}

Because _TLF_OOBTRAP is set and because we are still oob, we just leave the kernel without calling __oob_trap_notify and subsequently our clean up code.
This results in re-execution of the same ud2 again and again.

I fixed it for me in the following way:

--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -414,8 +414,9 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
                handle_invalid_op(regs);
                instrumentation_end();
                irqentry_exit(regs, state);
-               mark_trap_exit_raw(X86_TRAP_UD, regs);
        }
+
+       mark_trap_exit_raw(X86_TRAP_UD, regs);
 }

I am not sure it this is a valid way to fix this issue.
If it is, I could work up a patch next week that addresses all the places where this problem could trigger.

What do you think?

@Philippe Gerum
Thanks for including the possibility to handle exceptions in oob stage. Actually noticed only at start of the year that you did it a while ago. I'm sorry for not testing it so for so long.

Best regards
Johannes

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: problem while handling exceptions in oob stage
  2023-02-23 13:22 problem while handling exceptions in oob stage Johannes Kirchmair
@ 2023-02-23 17:17 ` Jan Kiszka
  2023-02-25 18:17 ` Philippe Gerum
  1 sibling, 0 replies; 6+ messages in thread
From: Jan Kiszka @ 2023-02-23 17:17 UTC (permalink / raw)
  To: Johannes Kirchmair, Philippe Gerum, xenomai

On 23.02.23 14:22, Johannes Kirchmair wrote:
> Hello,
> 
> we encountered a problem, while handling invalid opcode exception in oob stage. 
> Our fixup code works fine if we execute ud2 instruction the first time.
> Upon second execution of ud2, we encounter the problem that our fixup code is not reached.
> 
> The problem is that the first time entering the kernel, mark_trap_entry_raw is called and in a subsequent call the _TLF_OOBTRAP flag is set.
> But because we do not switch to inband stage, we do not call mark_trap_exit_raw and the _TLF_OOBTRAP flag is not cleared.
> 
> The second time we enter the kernel on execution of ud2,  we encounter (again) the following code:
> 
> static __always_inline void oob_trap_notify(unsigned int exception,
>                                          struct pt_regs *regs)
> {
>          if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
>                  __oob_trap_notify(exception, regs);
> }
> 
> Because _TLF_OOBTRAP is set and because we are still oob, we just leave the kernel without calling __oob_trap_notify and subsequently our clean up code.
> This results in re-execution of the same ud2 again and again.
> 
> I fixed it for me in the following way:
> 
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -414,8 +414,9 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
>                 handle_invalid_op(regs);
>                 instrumentation_end();
>                 irqentry_exit(regs, state);
> -               mark_trap_exit_raw(X86_TRAP_UD, regs);
>         }
> +
> +       mark_trap_exit_raw(X86_TRAP_UD, regs);
>  }
> 
> I am not sure it this is a valid way to fix this issue.
> If it is, I could work up a patch next week that addresses all the places where this problem could trigger.
> 
> What do you think?
> 

Good catch. Looking at my old x86 patch, I was calling mark_trap_exit in
all cases as well.

Still, we should check what happens here if it is semantically correct.
When I look at __oob_trap_unwind which is now being call for both exits,
it contains the surely desired clearing of _TLF_OOBTRAP but also the
invocation of the handle_oob_trap_exit. That is harmless for Xenomai as
we don't use it, but maybe it is not desired. Philippe needs to comment
on this.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: problem while handling exceptions in oob stage
  2023-02-23 13:22 problem while handling exceptions in oob stage Johannes Kirchmair
  2023-02-23 17:17 ` Jan Kiszka
@ 2023-02-25 18:17 ` Philippe Gerum
  2023-02-26  9:47   ` Philippe Gerum
  2023-02-27  6:45   ` Jan Kiszka
  1 sibling, 2 replies; 6+ messages in thread
From: Philippe Gerum @ 2023-02-25 18:17 UTC (permalink / raw)
  To: Johannes Kirchmair; +Cc: xenomai, Jan Kiszka


Johannes Kirchmair <johannes.kirchmair@sigmatek.at> writes:

> Hello,
>
> we encountered a problem, while handling invalid opcode exception in oob stage. 
> Our fixup code works fine if we execute ud2 instruction the first time.
> Upon second execution of ud2, we encounter the problem that our fixup code is not reached.
>
> The problem is that the first time entering the kernel, mark_trap_entry_raw is called and in a subsequent call the _TLF_OOBTRAP flag is set.
> But because we do not switch to inband stage, we do not call mark_trap_exit_raw and the _TLF_OOBTRAP flag is not cleared.
>
> The second time we enter the kernel on execution of ud2,  we encounter (again) the following code:
>
> static __always_inline void oob_trap_notify(unsigned int exception,
>                                          struct pt_regs *regs)
> {
>          if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
>                  __oob_trap_notify(exception, regs);
> }
>
> Because _TLF_OOBTRAP is set and because we are still oob, we just leave the kernel without calling __oob_trap_notify and subsequently our clean up code.
> This results in re-execution of the same ud2 again and again.
>
> I fixed it for me in the following way:
>
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -414,8 +414,9 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
>                 handle_invalid_op(regs);
>                 instrumentation_end();
>                 irqentry_exit(regs, state);
> -               mark_trap_exit_raw(X86_TRAP_UD, regs);
>         }
> +
> +       mark_trap_exit_raw(X86_TRAP_UD, regs);
>  }
>
> I am not sure it this is a valid way to fix this issue.
> If it is, I could work up a patch next week that addresses all the places where this problem could trigger.
>
> What do you think?

That would work in the current implementation of mark_trap_exit*(), I
would recommend to keep the semantics of the mark_trap* pairs unchanged
though, meaning that no exit should take place if the corresponding
entry failed.

Dovetail-wise, the inner oob_trap_entry/oob_trap_unwind pair should mark
the start/end of the in-band handling of a trap, so that the core can do
some housekeeping before and after such handling. As you rightfully
noticed, because such entry was actually notified unconditionally to the
core, we should always tell it about the matching exit point as well,
especially if we plan not to do anything about the trap event.

Something like this should do for x86 (same issue for arm64 and arm,
I'll fix them up too):

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 7238f5c90b998..234e8ab84c221 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -174,6 +174,13 @@ bool mark_trap_entry(int trapnr, struct pt_regs *regs)
 		return true;
 	}
 
+	/*
+	 * If the oob core did not switch us inband, our caller is
+	 * expected to leave the trap handler immediately, so we may
+	 * notify the core about this right now.
+	 */
+	oob_trap_unwind(trapnr, regs);
+
 	return false;
 }
 
@@ -188,7 +195,13 @@ static __always_inline
 bool mark_trap_entry_raw(int trapnr, struct pt_regs *regs)
 {
 	oob_trap_notify(trapnr, regs);
-	return running_inband();
+
+	if (unlikely(running_oob())) {
+		oob_trap_unwind(trapnr, regs);
+		return false;
+	}
+
+	return true;
 }
 
 static __always_inline
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 4b697ef4bd678..e6dff1c91b408 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -922,10 +922,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		return;
 
 	oob_trap_notify(X86_TRAP_PF, regs);
-	if (!running_inband()) {
-		local_irq_disable_full();
-		return;
-	}
+	if (!running_inband())
+		goto out;
 
 	if (likely(show_unhandled_signals))
 		show_signal_msg(regs, error_code, address, tsk);
@@ -936,7 +934,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		force_sig_pkuerr((void __user *)address, pkey);
 	else
 		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
-
+out:
 	local_irq_disable_full();
 	oob_trap_unwind(X86_TRAP_PF, regs);
 }
@@ -1401,7 +1399,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 */
 	oob_trap_notify(X86_TRAP_PF, regs);
 	if (!running_inband())
-		return;
+		goto out;
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);

>
> @Philippe Gerum
> Thanks for including the possibility to handle exceptions in oob stage. Actually noticed only at start of the year that you did it a while ago. I'm sorry for not testing it so for so long.
>

No prob. I did not announce this update either, so I could not blame
anyone for the lack of feedback here.

-- 
Philippe.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: problem while handling exceptions in oob stage
  2023-02-25 18:17 ` Philippe Gerum
@ 2023-02-26  9:47   ` Philippe Gerum
  2023-02-28 12:09     ` Johannes Kirchmair
  2023-02-27  6:45   ` Jan Kiszka
  1 sibling, 1 reply; 6+ messages in thread
From: Philippe Gerum @ 2023-02-26  9:47 UTC (permalink / raw)
  To: Johannes Kirchmair; +Cc: xenomai, Jan Kiszka


Philippe Gerum <rpm@xenomai.org> writes:

> Johannes Kirchmair <johannes.kirchmair@sigmatek.at> writes:
>
>> Hello,
>>
>> we encountered a problem, while handling invalid opcode exception in oob stage. 
>> Our fixup code works fine if we execute ud2 instruction the first time.
>> Upon second execution of ud2, we encounter the problem that our fixup code is not reached.
>>
>> The problem is that the first time entering the kernel, mark_trap_entry_raw is called and in a subsequent call the _TLF_OOBTRAP flag is set.
>> But because we do not switch to inband stage, we do not call mark_trap_exit_raw and the _TLF_OOBTRAP flag is not cleared.
>>
>> The second time we enter the kernel on execution of ud2,  we encounter (again) the following code:
>>
>> static __always_inline void oob_trap_notify(unsigned int exception,
>>                                          struct pt_regs *regs)
>> {
>>          if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
>>                  __oob_trap_notify(exception, regs);
>> }
>>
>> Because _TLF_OOBTRAP is set and because we are still oob, we just leave the kernel without calling __oob_trap_notify and subsequently our clean up code.
>> This results in re-execution of the same ud2 again and again.
>>
>> I fixed it for me in the following way:
>>
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -414,8 +414,9 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
>>                 handle_invalid_op(regs);
>>                 instrumentation_end();
>>                 irqentry_exit(regs, state);
>> -               mark_trap_exit_raw(X86_TRAP_UD, regs);
>>         }
>> +
>> +       mark_trap_exit_raw(X86_TRAP_UD, regs);
>>  }
>>
>> I am not sure it this is a valid way to fix this issue.
>> If it is, I could work up a patch next week that addresses all the places where this problem could trigger.
>>
>> What do you think?
>
> That would work in the current implementation of mark_trap_exit*(), I
> would recommend to keep the semantics of the mark_trap* pairs unchanged
> though, meaning that no exit should take place if the corresponding
> entry failed.
>
> Dovetail-wise, the inner oob_trap_entry/oob_trap_unwind pair should mark
> the start/end of the in-band handling of a trap, so that the core can do
> some housekeeping before and after such handling. As you rightfully
> noticed, because such entry was actually notified unconditionally to the
> core, we should always tell it about the matching exit point as well,
> especially if we plan not to do anything about the trap event.
>
> Something like this should do for x86 (same issue for arm64 and arm,
> I'll fix them up too):
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 7238f5c90b998..234e8ab84c221 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -174,6 +174,13 @@ bool mark_trap_entry(int trapnr, struct pt_regs *regs)
>  		return true;
>  	}
>  
> +	/*
> +	 * If the oob core did not switch us inband, our caller is
> +	 * expected to leave the trap handler immediately, so we may
> +	 * notify the core about this right now.
> +	 */
> +	oob_trap_unwind(trapnr, regs);
> +
>  	return false;
>  }
>  
> @@ -188,7 +195,13 @@ static __always_inline
>  bool mark_trap_entry_raw(int trapnr, struct pt_regs *regs)
>  {
>  	oob_trap_notify(trapnr, regs);
> -	return running_inband();
> +
> +	if (unlikely(running_oob())) {
> +		oob_trap_unwind(trapnr, regs);
> +		return false;
> +	}
> +
> +	return true;
>  }
>  
>  static __always_inline

The patch for mm/fault.c above is not correct, the following one is
better, i.e. since we are still running oob, we must not alter the
in-band interrupt state, plus oob_trap_unwind() turns hard irqs off
unconditionally on entry and leaves them that way, therefore we can and
_must_ skip local_irq_disable_full():

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 4b697ef4bd678..c7999b493865b 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -922,10 +922,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		return;
 
 	oob_trap_notify(X86_TRAP_PF, regs);
-	if (!running_inband()) {
-		local_irq_disable_full();
-		return;
-	}
+	if (!running_inband())
+		goto out;
 
 	if (likely(show_unhandled_signals))
 		show_signal_msg(regs, error_code, address, tsk);
@@ -938,6 +936,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
 
 	local_irq_disable_full();
+out:
 	oob_trap_unwind(X86_TRAP_PF, regs);
 }
 
@@ -1401,7 +1400,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	 */
 	oob_trap_notify(X86_TRAP_PF, regs);
 	if (!running_inband())
-		return;
+		goto out;
 
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);

-- 
Philippe.

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: problem while handling exceptions in oob stage
  2023-02-25 18:17 ` Philippe Gerum
  2023-02-26  9:47   ` Philippe Gerum
@ 2023-02-27  6:45   ` Jan Kiszka
  1 sibling, 0 replies; 6+ messages in thread
From: Jan Kiszka @ 2023-02-27  6:45 UTC (permalink / raw)
  To: Philippe Gerum, Johannes Kirchmair; +Cc: xenomai

On 25.02.23 19:17, Philippe Gerum wrote:
> 
> Johannes Kirchmair <johannes.kirchmair@sigmatek.at> writes:
> 
>> Hello,
>>
>> we encountered a problem, while handling invalid opcode exception in oob stage. 
>> Our fixup code works fine if we execute ud2 instruction the first time.
>> Upon second execution of ud2, we encounter the problem that our fixup code is not reached.
>>
>> The problem is that the first time entering the kernel, mark_trap_entry_raw is called and in a subsequent call the _TLF_OOBTRAP flag is set.
>> But because we do not switch to inband stage, we do not call mark_trap_exit_raw and the _TLF_OOBTRAP flag is not cleared.
>>
>> The second time we enter the kernel on execution of ud2,  we encounter (again) the following code:
>>
>> static __always_inline void oob_trap_notify(unsigned int exception,
>>                                          struct pt_regs *regs)
>> {
>>          if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
>>                  __oob_trap_notify(exception, regs);
>> }
>>
>> Because _TLF_OOBTRAP is set and because we are still oob, we just leave the kernel without calling __oob_trap_notify and subsequently our clean up code.
>> This results in re-execution of the same ud2 again and again.
>>
>> I fixed it for me in the following way:
>>
>> --- a/arch/x86/kernel/traps.c
>> +++ b/arch/x86/kernel/traps.c
>> @@ -414,8 +414,9 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
>>                 handle_invalid_op(regs);
>>                 instrumentation_end();
>>                 irqentry_exit(regs, state);
>> -               mark_trap_exit_raw(X86_TRAP_UD, regs);
>>         }
>> +
>> +       mark_trap_exit_raw(X86_TRAP_UD, regs);
>>  }
>>
>> I am not sure it this is a valid way to fix this issue.
>> If it is, I could work up a patch next week that addresses all the places where this problem could trigger.
>>
>> What do you think?
> 
> That would work in the current implementation of mark_trap_exit*(), I
> would recommend to keep the semantics of the mark_trap* pairs unchanged
> though, meaning that no exit should take place if the corresponding
> entry failed.
> 
> Dovetail-wise, the inner oob_trap_entry/oob_trap_unwind pair should mark
> the start/end of the in-band handling of a trap, so that the core can do
> some housekeeping before and after such handling. As you rightfully
> noticed, because such entry was actually notified unconditionally to the
> core, we should always tell it about the matching exit point as well,
> especially if we plan not to do anything about the trap event.
> 
> Something like this should do for x86 (same issue for arm64 and arm,
> I'll fix them up too):
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 7238f5c90b998..234e8ab84c221 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -174,6 +174,13 @@ bool mark_trap_entry(int trapnr, struct pt_regs *regs)
>  		return true;
>  	}
>  
> +	/*
> +	 * If the oob core did not switch us inband, our caller is
> +	 * expected to leave the trap handler immediately, so we may
> +	 * notify the core about this right now.
> +	 */
> +	oob_trap_unwind(trapnr, regs);
> +
>  	return false;
>  }
>  
> @@ -188,7 +195,13 @@ static __always_inline
>  bool mark_trap_entry_raw(int trapnr, struct pt_regs *regs)
>  {
>  	oob_trap_notify(trapnr, regs);
> -	return running_inband();
> +
> +	if (unlikely(running_oob())) {

I don't think it is useful to declare oob the slow-path.

Jan

> +		oob_trap_unwind(trapnr, regs);
> +		return false;
> +	}
> +
> +	return true;
>  }
>  
>  static __always_inline

-- 
Siemens AG, Technology
Competence Center Embedded Linux


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: problem while handling exceptions in oob stage
  2023-02-26  9:47   ` Philippe Gerum
@ 2023-02-28 12:09     ` Johannes Kirchmair
  0 siblings, 0 replies; 6+ messages in thread
From: Johannes Kirchmair @ 2023-02-28 12:09 UTC (permalink / raw)
  To: Philippe Gerum; +Cc: xenomai, Jan Kiszka

Hi,

just tested the patch. 
It resolves my problem.

Best regards
Johannes

> -----Original Message-----
> From: Philippe Gerum <rpm@xenomai.org>
> Sent: Sonntag, 26. Februar 2023 10:48
> To: Johannes Kirchmair <johannes.kirchmair@sigmatek.at>
> Cc: xenomai@lists.linux.dev; Jan Kiszka <jan.kiszka@siemens.com>
> Subject: Re: problem while handling exceptions in oob stage
> 
> CAUTION: External E-Mail !
> 
> Philippe Gerum <rpm@xenomai.org> writes:
> 
> > Johannes Kirchmair <johannes.kirchmair@sigmatek.at> writes:
> >
> >> Hello,
> >>
> >> we encountered a problem, while handling invalid opcode exception in oob
> stage.
> >> Our fixup code works fine if we execute ud2 instruction the first time.
> >> Upon second execution of ud2, we encounter the problem that our fixup
> code is not reached.
> >>
> >> The problem is that the first time entering the kernel, mark_trap_entry_raw is
> called and in a subsequent call the _TLF_OOBTRAP flag is set.
> >> But because we do not switch to inband stage, we do not call
> mark_trap_exit_raw and the _TLF_OOBTRAP flag is not cleared.
> >>
> >> The second time we enter the kernel on execution of ud2,  we encounter
> (again) the following code:
> >>
> >> static __always_inline void oob_trap_notify(unsigned int exception,
> >>                                          struct pt_regs *regs)
> >> {
> >>          if (running_oob() && !test_thread_local_flags(_TLF_OOBTRAP))
> >>                  __oob_trap_notify(exception, regs);
> >> }
> >>
> >> Because _TLF_OOBTRAP is set and because we are still oob, we just leave the
> kernel without calling __oob_trap_notify and subsequently our clean up code.
> >> This results in re-execution of the same ud2 again and again.
> >>
> >> I fixed it for me in the following way:
> >>
> >> --- a/arch/x86/kernel/traps.c
> >> +++ b/arch/x86/kernel/traps.c
> >> @@ -414,8 +414,9 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
> >>                 handle_invalid_op(regs);
> >>                 instrumentation_end();
> >>                 irqentry_exit(regs, state);
> >> -               mark_trap_exit_raw(X86_TRAP_UD, regs);
> >>         }
> >> +
> >> +       mark_trap_exit_raw(X86_TRAP_UD, regs);
> >>  }
> >>
> >> I am not sure it this is a valid way to fix this issue.
> >> If it is, I could work up a patch next week that addresses all the places where
> this problem could trigger.
> >>
> >> What do you think?
> >
> > That would work in the current implementation of mark_trap_exit*(), I
> > would recommend to keep the semantics of the mark_trap* pairs unchanged
> > though, meaning that no exit should take place if the corresponding
> > entry failed.
> >
> > Dovetail-wise, the inner oob_trap_entry/oob_trap_unwind pair should mark
> > the start/end of the in-band handling of a trap, so that the core can do
> > some housekeeping before and after such handling. As you rightfully
> > noticed, because such entry was actually notified unconditionally to the
> > core, we should always tell it about the matching exit point as well,
> > especially if we plan not to do anything about the trap event.
> >
> > Something like this should do for x86 (same issue for arm64 and arm,
> > I'll fix them up too):
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 7238f5c90b998..234e8ab84c221 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -174,6 +174,13 @@ bool mark_trap_entry(int trapnr, struct pt_regs *regs)
> >               return true;
> >       }
> >
> > +     /*
> > +      * If the oob core did not switch us inband, our caller is
> > +      * expected to leave the trap handler immediately, so we may
> > +      * notify the core about this right now.
> > +      */
> > +     oob_trap_unwind(trapnr, regs);
> > +
> >       return false;
> >  }
> >
> > @@ -188,7 +195,13 @@ static __always_inline
> >  bool mark_trap_entry_raw(int trapnr, struct pt_regs *regs)
> >  {
> >       oob_trap_notify(trapnr, regs);
> > -     return running_inband();
> > +
> > +     if (unlikely(running_oob())) {
> > +             oob_trap_unwind(trapnr, regs);
> > +             return false;
> > +     }
> > +
> > +     return true;
> >  }
> >
> >  static __always_inline
> 
> The patch for mm/fault.c above is not correct, the following one is
> better, i.e. since we are still running oob, we must not alter the
> in-band interrupt state, plus oob_trap_unwind() turns hard irqs off
> unconditionally on entry and leaves them that way, therefore we can and
> _must_ skip local_irq_disable_full():
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 4b697ef4bd678..c7999b493865b 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -922,10 +922,8 @@ __bad_area_nosemaphore(struct pt_regs *regs,
> unsigned long error_code,
>                 return;
> 
>         oob_trap_notify(X86_TRAP_PF, regs);
> -       if (!running_inband()) {
> -               local_irq_disable_full();
> -               return;
> -       }
> +       if (!running_inband())
> +               goto out;
> 
>         if (likely(show_unhandled_signals))
>                 show_signal_msg(regs, error_code, address, tsk);
> @@ -938,6 +936,7 @@ __bad_area_nosemaphore(struct pt_regs *regs,
> unsigned long error_code,
>                 force_sig_fault(SIGSEGV, si_code, (void __user *)address);
> 
>         local_irq_disable_full();
> +out:
>         oob_trap_unwind(X86_TRAP_PF, regs);
>  }
> 
> @@ -1401,7 +1400,7 @@ void do_user_addr_fault(struct pt_regs *regs,
>          */
>         oob_trap_notify(X86_TRAP_PF, regs);
>         if (!running_inband())
> -               return;
> +               goto out;
> 
>         perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
> 
> --
> Philippe.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-28 12:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-23 13:22 problem while handling exceptions in oob stage Johannes Kirchmair
2023-02-23 17:17 ` Jan Kiszka
2023-02-25 18:17 ` Philippe Gerum
2023-02-26  9:47   ` Philippe Gerum
2023-02-28 12:09     ` Johannes Kirchmair
2023-02-27  6:45   ` Jan Kiszka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).