Re: [PATCH 5.4] x86: ipipe: Harden path between use_temporary_mm and unuse_temporary_mm

[Philippe Gerum <rpm@xenomai.org>]

Philippe Gerum <rpm@xenomai.org> writes:

> Jan Kiszka <jan.kiszka@siemens.com> writes:
>
>> On 15.03.21 10:00, Philippe Gerum wrote:
>>> 
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>> 
>>>> On 14.03.21 18:14, Philippe Gerum wrote:
>>>>>
>>>>> Jan Kiszka via Xenomai <xenomai@xenomai.org> writes:
>>>>>
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> 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.
>>
>
> This was likely caused by ipipe_unstall_root(void) force enabling hard
> irqs on. This is gone in Dovetail, and unstalling the inband stage with
> hard irqs off is even considered a bug now (inband_irq_enable()
> complains about this) so that we catch the callers who would still be
> making the original assumption.
>
> IOW, allocating page table directories when locking PTEs should not
> cause hard IRQs to be force enabled with Dovetail. I'm going to check
> this for peace of mind.

Confirmed (with enabling dynamic tracepoints), get_locked_pte() won't
change the hard irq state.

-- 
Philippe.