On 15.03.21 10:47, Philippe Gerum wrote:
>
> Jan Kiszka <[email protected]> writes:
>
>> On 15.03.21 10:00, Philippe Gerum wrote:
>>>
>>> Jan Kiszka <[email protected]> writes:
>>>
>>>> On 14.03.21 18:14, Philippe Gerum wrote:
>>>>>
>>>>> Jan Kiszka via Xenomai <[email protected]> writes:
>>>>>
>>>>>> From: Jan Kiszka <[email protected]>
>>>>>>
>>>>>> This is only called during early init, e.g. for switching alternatives.
>>>>>> Still, switch_mm_irqs_off would complain without this, and we are better
>>>>>> safe than sorry.
>>>>>>
>>>>>
>>>>> The way this is done in Dovetail is fragile too, since the protection we
>>>>> have there still expects the pipeline entry code not to mess up on
>>>>> handling an interrupt, which defeats the purpose of such
>>>>> precaution. Besides, the temp_state should be snapshot under protection
>>>>> too. IOW, IRQs should be hard disabled fully while using the temporary
>>>>> mm.
>>>>>
>>>>> Upstreaming a similar patch for Dovetail.
>>>>
>>>> Just saw it: It's wrong as it left the hard_irq_save_full at the caller
>>>> site. Would raise a warning when debugging is enabled. Please fix.
>>>>
>>>
>>> I guess you mean local_irq_save_full(). After a second look, the hard
>>> irqs off section should cover the entire code poking new text into a
>>> live kernel. This is what switching to the _full() forms enforced
>>> already. So I see no point in changing the IRQ state in the use/unuse
>>> helpers eventually.
>>
>> Right, either way is needed. If the tlb flush should be fully protected,
>> the existing pattern is needed. But maybe more:
>>
>> Under I-pipe, playing with full disabling didn't work out well. Some
>> spinlock function called via get_locked_pte reenabled hard IRQs. Not
>> sure if dovetail is affected by that as well.
>>
>
> In the case at hand, the PTE of the poking address is locked and
> returned outside of the locked section, so this should not be a concern
> for live patching.
>
Yeah, I'm carrying this now:
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index cbaa584a9f23..affc594cc939 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -808,6 +808,7 @@ static inline temp_mm_state_t use_temporary_mm(struct
mm_struct *mm)
temp_mm_state_t temp_state;
lockdep_assert_irqs_disabled();
+ WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
/*
* Make sure not to be in TLB lazy mode, as otherwise we'll end up
@@ -821,8 +822,6 @@ static inline temp_mm_state_t use_temporary_mm(struct
mm_struct *mm)
* unuse_temporary_mm() assumes hardirqs were off on entry to
* use_temporary_mm(), assert this condition.
*/
- WARN_ON_ONCE(irq_pipeline_debug() && hard_irqs_disabled());
- hard_cond_local_irq_disable();
temp_state.mm = this_cpu_read(cpu_tlbstate.loaded_mm);
switch_mm_irqs_off(NULL, mm, current);
@@ -846,8 +845,8 @@ static inline temp_mm_state_t use_temporary_mm(struct
mm_struct *mm)
static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
{
lockdep_assert_irqs_disabled();
+ WARN_ON_ONCE(irq_pipeline_debug() && !hard_irqs_disabled());
switch_mm_irqs_off(NULL, prev_state.mm, current);
- hard_cond_local_irq_enable();
/*
* Restore the breakpoints if they were disabled before the temporary mm
And no warnings so far.
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
