[dovetail][PATCH] x86: dovetail: Drop bogus local_irq_disable_full from __bad_area_nosemaphore

[dovetail][PATCH] x86: dovetail: Drop bogus local_irq_disable_full from __bad_area_nosemaphore

[Jan Kiszka]

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

There is no technical reason to disable IRQs here again, neither when
the IRQ pipeline is enable nor when it is off, which this change also
causes. We are never passing this point from OOB because of the
!running_inband check and jump earlier in function. The inband case is
identical to the !IRQ_PIPELINE case, and that one does not need any
additional IRQ disabling as well (upstream code path).

The addition was most likely a mismerged during the forward-port from
6.18 to 6.19. Remove it.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/mm/fault.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index e41ed2092730c..e9a08a5d26e60 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -886,8 +886,6 @@ __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();
 out:
 	oob_trap_unwind(X86_TRAP_PF, regs);
 }
-- 
2.47.3

Re: [dovetail][PATCH] x86: dovetail: Drop bogus local_irq_disable_full from __bad_area_nosemaphore

[Philippe Gerum]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> There is no technical reason to disable IRQs here again, neither when
> the IRQ pipeline is enable nor when it is off, which this change also
> causes. We are never passing this point from OOB because of the
> !running_inband check and jump earlier in function. The inband case is
> identical to the !IRQ_PIPELINE case, and that one does not need any
> additional IRQ disabling as well (upstream code path).
>
> The addition was most likely a mismerged during the forward-port from
> 6.18 to 6.19. Remove it.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/mm/fault.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index e41ed2092730c..e9a08a5d26e60 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -886,8 +886,6 @@ __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();
>  out:
>  	oob_trap_unwind(X86_TRAP_PF, regs);
>  }

When removing such code, replacing it by an assertion like
WARN_ON_ONCE(irqs_pipelined() && !irqs_disabled()) would catch wrong
moves.

-- 
Philippe.

Re: [dovetail][PATCH] x86: dovetail: Drop bogus local_irq_disable_full from __bad_area_nosemaphore

[Jan Kiszka]

On 12.11.26 14:09, Philippe Gerum wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> There is no technical reason to disable IRQs here again, neither when
>> the IRQ pipeline is enable nor when it is off, which this change also
>> causes. We are never passing this point from OOB because of the
>> !running_inband check and jump earlier in function. The inband case is
>> identical to the !IRQ_PIPELINE case, and that one does not need any
>> additional IRQ disabling as well (upstream code path).
>>
>> The addition was most likely a mismerged during the forward-port from
>> 6.18 to 6.19. Remove it.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/mm/fault.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index e41ed2092730c..e9a08a5d26e60 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -886,8 +886,6 @@ __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();
>>  out:
>>  	oob_trap_unwind(X86_TRAP_PF, regs);
>>  }
> 
> When removing such code, replacing it by an assertion like
> WARN_ON_ONCE(irqs_pipelined() && !irqs_disabled()) would catch wrong
> moves.
> 

IRQs are enabled at the point where local_irq_disable_full was located
(-> cond_reenable_irqs_user / local_irq_enable), so this warning would
always trigger.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [dovetail][PATCH] x86: dovetail: Drop bogus local_irq_disable_full from __bad_area_nosemaphore

[Philippe Gerum]

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

> On 12.11.26 14:09, Philippe Gerum wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> There is no technical reason to disable IRQs here again, neither when
>>> the IRQ pipeline is enable nor when it is off, which this change also
>>> causes. We are never passing this point from OOB because of the
>>> !running_inband check and jump earlier in function. The inband case is
>>> identical to the !IRQ_PIPELINE case, and that one does not need any
>>> additional IRQ disabling as well (upstream code path).
>>>
>>> The addition was most likely a mismerged during the forward-port from
>>> 6.18 to 6.19. Remove it.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  arch/x86/mm/fault.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>>> index e41ed2092730c..e9a08a5d26e60 100644
>>> --- a/arch/x86/mm/fault.c
>>> +++ b/arch/x86/mm/fault.c
>>> @@ -886,8 +886,6 @@ __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();
>>>  out:
>>>  	oob_trap_unwind(X86_TRAP_PF, regs);
>>>  }
>> 
>> When removing such code, replacing it by an assertion like
>> WARN_ON_ONCE(irqs_pipelined() && !irqs_disabled()) would catch wrong
>> moves.
>> 
>
> IRQs are enabled at the point where local_irq_disable_full was located
> (-> cond_reenable_irqs_user / local_irq_enable), so this warning would
> always trigger.
>

See my previous answer.

-- 
Philippe.

Re: [dovetail][PATCH] x86: dovetail: Drop bogus local_irq_disable_full from __bad_area_nosemaphore

[Jan Kiszka]

On 12.11.26 15:01, Philippe Gerum wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 12.11.26 14:09, Philippe Gerum wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> There is no technical reason to disable IRQs here again, neither when
>>>> the IRQ pipeline is enable nor when it is off, which this change also
>>>> causes. We are never passing this point from OOB because of the
>>>> !running_inband check and jump earlier in function. The inband case is
>>>> identical to the !IRQ_PIPELINE case, and that one does not need any
>>>> additional IRQ disabling as well (upstream code path).
>>>>
>>>> The addition was most likely a mismerged during the forward-port from
>>>> 6.18 to 6.19. Remove it.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  arch/x86/mm/fault.c | 2 --
>>>>  1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>>>> index e41ed2092730c..e9a08a5d26e60 100644
>>>> --- a/arch/x86/mm/fault.c
>>>> +++ b/arch/x86/mm/fault.c
>>>> @@ -886,8 +886,6 @@ __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();
>>>>  out:
>>>>  	oob_trap_unwind(X86_TRAP_PF, regs);
>>>>  }
>>>
>>> When removing such code, replacing it by an assertion like
>>> WARN_ON_ONCE(irqs_pipelined() && !irqs_disabled()) would catch wrong
>>> moves.
>>>
>>
>> IRQs are enabled at the point where local_irq_disable_full was located
>> (-> cond_reenable_irqs_user / local_irq_enable), so this warning would
>> always trigger.
>>
> 
> See my previous answer.
> 

OK, LLM may again help us out of this confusion, it likely found the
source of it:

Before 614da1d3d4cdbd6e41aea06bc97ec15aacff6daf (late in 6.19), there
was indeed a local_irq_disable() at the end of __bad_area_nosemaphore.
But that commit moved it to handle_page_fault() where we also convert it
to cond_disable_irqs(). And that commit was also backported to stable
versions.

So, the current local_irq_disable_full() in 6.19+ became misplaced AND
checking for irqs_disabled() at that point would be premature before
they are actually disabled again in handle_page_fault().

I can try to rephrase the commit message accordingly.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center