Dovetail/x86 still broken /wrt exception fixups

Dovetail/x86 still broken /wrt exception fixups

[Jan Kiszka]

Hi Philippe,

we are leaking the inband stall state into user land when the kernel
takes a fixable exception in oob. The problem is either
cond_disable_irqs() or its (missing?) counterpart. Where is the stall
bit set in handle_page_fault->cond_disable_irqs supposed to be cleared
again?

I-pipe is fine in that regard, but it took us many years to get it into
that state - and it didn't have to deal with the impact of ca4c6a9858
("x86/traps: Make interrupt enable/disable symmetric in C code") anymore...

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: Dovetail/x86 still broken /wrt exception fixups

[Jan Kiszka]

On 01.06.21 23:24, Jan Kiszka via Xenomai wrote:
> Hi Philippe,
> 
> we are leaking the inband stall state into user land when the kernel
> takes a fixable exception in oob. The problem is either
> cond_disable_irqs() or its (missing?) counterpart. Where is the stall
> bit set in handle_page_fault->cond_disable_irqs supposed to be cleared
> again?
> 
> I-pipe is fine in that regard, but it took us many years to get it into
> that state - and it didn't have to deal with the impact of ca4c6a9858
> ("x86/traps: Make interrupt enable/disable symmetric in C code") anymore...
> 

A first attempt to fix the issue, but I'm only half-way through with 
understanding the related logic in dovetail:

diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 5e33248453e7..9f8c51e5f51c 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -526,7 +526,8 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
 	 * irqentry_enter() raised it in order to mirror the hardware
 	 * state.
 	 */
-	if (state.stage_info & IRQENTRY_INBAND_STALLED)
+	if (state.stage_info == IRQENTRY_INBAND_STALLED ||
+	    (running_inband() && state.stage_info == IRQENTRY_OOB_ENTRY))
 		unstall_inband();
 #endif
 	return;

State seems now consistent again.

BTW, all those tests for bits in stage_info should likely be converted 
to testing for the state - IRQENTRY_INBAND_STALLED and IRQENTRY_OOB_ENTRY 
are mutually exclusive. Would make the code more readable.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: Dovetail/x86 still broken /wrt exception fixups

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> Hi Philippe,
>
> we are leaking the inband stall state into user land when the kernel
> takes a fixable exception in oob. The problem is either
> cond_disable_irqs() or its (missing?) counterpart. Where is the stall
> bit set in handle_page_fault->cond_disable_irqs supposed to be cleared
> again?
>

It depends: irqentry_exit_to_user_mode if the fault was triggered by
user, otherwise the assumption is made that any entry from kernel
context, unstalled _and_ in_band would lead to IRQENTRY_INBAND_STALLED
being set into the stage info bits.

Which is unfortunately wrong in one case: when the execution stage was
downgraded by the companion core from oob to in-band as a result of
handling the fault (oob_trap_notify). This is basically what you are
looking at ATM.

i.e.

oob exec from kernel:
        raw_copy_to/from_user(bad_u_pointer)
                irqentry_enter, IRQENTRY_OOB_ENTRY is set
                       handle_page_fault
                           do_user_addr_fault
                                oob_trap_notify, switching in-band
in-band:                   cond_disable_irqs
                irqentry_exit
                        in-band but !IRQENTRY_INBAND_STALLED => bummer,
                                            won't unstall

Would the fault have happened from in-band kernel context, and
IRQENTRY_INBAND_STALLED would have been set, preventing the issue.

This also means that this particular issue is different from the RCU
stall bug we mentioned this morning during the community call: that
setup never exercises stage transition between oob and in-band task
contexts, so we could not observe it with CONFIG_DOVETAIL off.

> I-pipe is fine in that regard, but it took us many years to get it into
> that state - and it didn't have to deal with the impact of ca4c6a9858
> ("x86/traps: Make interrupt enable/disable symmetric in C code") anymore...
>

The I-pipe required many years of fixing because it does way too many
things from the asm code, which is error-prone and caused painful merges
in many occasions. There is no valid reason for the fault handling path
to require years of fixing pipeline-wise in any case.

The logic of #ca4c6a9858 makes a lot of sense. Dovetail never assumed
anything wrt what the asm might do regarding the IRQ state anyway,
framing what happens from C code is much easier.

-- 
Philippe.

Re: Dovetail/x86 still broken /wrt exception fixups

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 01.06.21 23:24, Jan Kiszka via Xenomai wrote:
>> Hi Philippe,
>> 
>> we are leaking the inband stall state into user land when the kernel
>> takes a fixable exception in oob. The problem is either
>> cond_disable_irqs() or its (missing?) counterpart. Where is the stall
>> bit set in handle_page_fault->cond_disable_irqs supposed to be cleared
>> again?
>> 
>> I-pipe is fine in that regard, but it took us many years to get it into
>> that state - and it didn't have to deal with the impact of ca4c6a9858
>> ("x86/traps: Make interrupt enable/disable symmetric in C code") anymore...
>> 
>
> A first attempt to fix the issue, but I'm only half-way through with 
> understanding the related logic in dovetail:
>
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index 5e33248453e7..9f8c51e5f51c 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -526,7 +526,8 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
>  	 * irqentry_enter() raised it in order to mirror the hardware
>  	 * state.
>  	 */
> -	if (state.stage_info & IRQENTRY_INBAND_STALLED)
> +	if (state.stage_info == IRQENTRY_INBAND_STALLED ||
> +	    (running_inband() && state.stage_info == IRQENTRY_OOB_ENTRY))
>  		unstall_inband();

You can drop the check for running_inband(), this is always true at this
point.

>  #endif
>  	return;
>
> State seems now consistent again.
>

Yes, that should cover the case described earlier.

> BTW, all those tests for bits in stage_info should likely be converted 
> to testing for the state - IRQENTRY_INBAND_STALLED and IRQENTRY_OOB_ENTRY 
> are mutually exclusive. Would make the code more readable.
>

Ack.

-- 
Philippe.