Re: local_irq_disable_full() in x86's __bad_area_nosemaphore

[Philippe Gerum <rpm@xenomai.org>]

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

> On 12.11.26 10:03, Philippe Gerum wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> On 11.11.26 23:57, Florian Bezdeka wrote:
>>>> On Wed, 2026-11-11 at 13:58 +0100, Jan Kiszka wrote:
>>>>> Hi Philippe, hi Florian,
>>>>>
>>>>> I start to love pattern-matching machines:
>>>>>
>>>>> Since 6.19-dovetail, commit 6af591980d1750b29474f001902c7dc58c3f1ba2, we have
>>>>>
>>>>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>>>>> index b83a06739b511..024a67bd2de6a 100644
>>>>> --- a/arch/x86/mm/fault.c
>>>>> +++ b/arch/x86/mm/fault.c
>>>>> @@ -821,6 +821,8 @@ __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);
>>>>> +
>>>>> +	local_irq_disable_full();
>>>>>  }
>>>>>  
>>>>>  static noinline void
>>>>>
>>>>> The final code is
>>>>>
>>>>> ...
>>>>> 	oob_trap_notify(X86_TRAP_PF, regs);
>>>>> 	if (!running_inband())
>>>>> 		goto out;
>>>>>
>>>>> 	if (likely(show_unhandled_signals))
>>>>> 		show_signal_msg(regs, error_code, address, tsk);
>>>>>
>>>>> 	set_signal_archinfo(address, error_code);
>>>>>
>>>>> 	if (si_code == SEGV_PKUERR)
>>>>> 		force_sig_pkuerr((void __user *)address, pkey);
>>>>> 	else
>>>>> 		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
>>>>>
>>>>> 	local_irq_disable_full();
>>>>> out:
>>>>> 	oob_trap_unwind(X86_TRAP_PF, regs);
>>>>> }
>>>>>
>>>>> Do you remember why? As this was folded into the forward port, this 
>>>>> delta never made it into anything older. If it is valid, we would need 
>>>>> some reasoning. But this rather looks like a mismerge to me.
>>>>
>>>> This one needs some more cycles here. It might already start with the
>>>> call to cond_reenable_irqs_user() earlier in this function which is
>>>> unconditionally enabling hard IRQs, even if the oob stage might be
>>>> stalled. The possible in-band migration happens later.
>>>>
>>>> If we leave with hard IRQs enabled, we would corrupt the oob IRQ state.
>>>> I need to check the calling context in more detail. Maybe this cannot
>>>> happen, maybe it can...
>>>>
>>>
>>> The oracle says:
>>>
>>> "No. With a working companion core, the oob stage can't be stalled when
>>> this code runs, and the handler never returns with hard IRQs still enabled."
>>>
>>> The point is that the cond_reenable_irqs_user is only run for faults
>>> triggered over userspace, and there we should never have OOB stalled /
>>> hard IRQs off. Kernel faults are filtered early in the function
>>> ("if (!user_mode(regs))").
>>>
>>>> Touching the inband IRQ mask (one part of disable_full()) seems strange
>>>> to me - at this stage - especially as Linux does not do any kind of
>>>> "restore" here.
>>>
>>> This is why I suspect a mismerge during forward porting.
>>>
>> 
>> Although this might be the case due to the significant code churn in
>> this area which is even worse on arm, on x86 and arm64 for that matter,
>> the whole logic is driven by the generic irqentry/irqexit scheme
>> (irq-entry-common.h). In short, the exit path expects the in-band stage
>> to be stalled if running in-band, hard irqs off. There are lockdep
>> assertions to enforce this, and specific provisions to unstall the
>> in-band stage as appropriate right before unwinding to the asm entry
>> point, e.g.  exit_to_user_mode().
>> 
>
> This does not explain what makes things different here, both with and
> also without IRQ_PIPELINE.
>

Affecting !IRQ_PIPELINE is obviously a bug.

> The more I look at it, the more that local_irq_disable_full() looks
> stray, at best.
>

So, I asked git-blame which told me that 6af591980d1750 introduced
it. As far as I can tell, this is neither a merge fallout nor stray
stuff, but rather a misinterpretation of the local_irq_enable()
statement which existed prior the introduction of
cond_reenable_irqs_user() at that time. This re-enabling upstream wants
was deemed to be persistent, and I made it temporary for the duration of
the call, which is wrong.

Although this change is going to be properly undone in the irqexit path
I mentioned earlier, it is useless. It should not break the logic
either, since anything we must do with the in-band stage unstalled is
already done passed that point. So I'm ok for dropping that line.

Meanwhile, the assertion I mentioned in my comment of the fixup becomes:

WARN_ON_ONCE(irqs_pipelined() && irqs_disabled())

-- 
Philippe.