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

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

[Jan Kiszka]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

4.19 is not affected. Dovetail solves this differently, via 
local_irq_save_full which is not available in I-pipe and not worth to 
introduce for this purpose.

 arch/x86/include/asm/mmu_context.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 0d9dd08c2122..2b4afca4e15f 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -383,6 +383,7 @@ static inline temp_mm_state_t use_temporary_mm(struct mm_struct *mm)
 	temp_mm_state_t temp_state;
 
 	lockdep_assert_irqs_disabled();
+	hard_cond_local_irq_disable();
 	temp_state.mm = this_cpu_read(cpu_tlbstate.loaded_mm);
 	switch_mm_irqs_off(NULL, mm, current);
 
@@ -407,6 +408,7 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
 {
 	lockdep_assert_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
-- 
2.26.2

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

[Philippe Gerum]

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.

Thanks,

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> 4.19 is not affected. Dovetail solves this differently, via 
> local_irq_save_full which is not available in I-pipe and not worth to 
> introduce for this purpose.
>
>  arch/x86/include/asm/mmu_context.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index 0d9dd08c2122..2b4afca4e15f 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -383,6 +383,7 @@ static inline temp_mm_state_t use_temporary_mm(struct mm_struct *mm)
>  	temp_mm_state_t temp_state;
>  
>  	lockdep_assert_irqs_disabled();
> +	hard_cond_local_irq_disable();
>  	temp_state.mm = this_cpu_read(cpu_tlbstate.loaded_mm);
>  	switch_mm_irqs_off(NULL, mm, current);
>  
> @@ -407,6 +408,7 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
>  {
>  	lockdep_assert_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


-- 
Philippe.

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

[Jan Kiszka]

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.

Jan

> 
> Thanks,
> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> 4.19 is not affected. Dovetail solves this differently, via 
>> local_irq_save_full which is not available in I-pipe and not worth to 
>> introduce for this purpose.
>>
>>  arch/x86/include/asm/mmu_context.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index 0d9dd08c2122..2b4afca4e15f 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -383,6 +383,7 @@ static inline temp_mm_state_t use_temporary_mm(struct mm_struct *mm)
>>  	temp_mm_state_t temp_state;
>>  
>>  	lockdep_assert_irqs_disabled();
>> +	hard_cond_local_irq_disable();
>>  	temp_state.mm = this_cpu_read(cpu_tlbstate.loaded_mm);
>>  	switch_mm_irqs_off(NULL, mm, current);
>>  
>> @@ -407,6 +408,7 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
>>  {
>>  	lockdep_assert_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
> 
> 

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

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

[Jan Kiszka]

On 15.03.21 07:19, Jan Kiszka via Xenomai wrote:
> 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.
> 

IOW, this should be merged into the original patch changing it:

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index cbaa584a9f23..d6c9fb7bd790 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -908,7 +908,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	VM_BUG_ON(!ptep);
 
-	local_irq_save_full(flags);
+	local_irq_save(flags);
 
 	pte = mk_pte(pages[0], pgprot);
 	set_pte_at(poking_mm, poking_addr, ptep, pte);
@@ -959,7 +959,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	BUG_ON(memcmp(addr, opcode, len));
 
-	local_irq_restore_full(flags);
+	local_irq_restore(flags);
 	pte_unmap_unlock(ptep, ptl);
 	return addr;
 }

Without it, you get

[    0.352686] WARNING: CPU: 0 PID: 1 at ../arch/x86/kernel/alternative.c:824 __text_poke+0x265/0x4a0

Jan

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

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

[Philippe Gerum]

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.

> Jan
>
>> 
>> Thanks,
>> 
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> 4.19 is not affected. Dovetail solves this differently, via 
>>> local_irq_save_full which is not available in I-pipe and not worth to 
>>> introduce for this purpose.
>>>
>>>  arch/x86/include/asm/mmu_context.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>>> index 0d9dd08c2122..2b4afca4e15f 100644
>>> --- a/arch/x86/include/asm/mmu_context.h
>>> +++ b/arch/x86/include/asm/mmu_context.h
>>> @@ -383,6 +383,7 @@ static inline temp_mm_state_t use_temporary_mm(struct mm_struct *mm)
>>>  	temp_mm_state_t temp_state;
>>>  
>>>  	lockdep_assert_irqs_disabled();
>>> +	hard_cond_local_irq_disable();
>>>  	temp_state.mm = this_cpu_read(cpu_tlbstate.loaded_mm);
>>>  	switch_mm_irqs_off(NULL, mm, current);
>>>  
>>> @@ -407,6 +408,7 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
>>>  {
>>>  	lockdep_assert_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
>> 
>> 


-- 
Philippe.

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

[Philippe Gerum]

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

> On 15.03.21 07:19, Jan Kiszka via Xenomai wrote:
>> 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.
>> 
>
> IOW, this should be merged into the original patch changing it:
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index cbaa584a9f23..d6c9fb7bd790 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -908,7 +908,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
>  	 */
>  	VM_BUG_ON(!ptep);
>  
> -	local_irq_save_full(flags);
> +	local_irq_save(flags);
>  
>  	pte = mk_pte(pages[0], pgprot);
>  	set_pte_at(poking_mm, poking_addr, ptep, pte);
> @@ -959,7 +959,7 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
>  	 */
>  	BUG_ON(memcmp(addr, opcode, len));
>  
> -	local_irq_restore_full(flags);
> +	local_irq_restore(flags);
>  	pte_unmap_unlock(ptep, ptl);
>  	return addr;
>  }
>
> Without it, you get
>
> [    0.352686] WARNING: CPU: 0 PID: 1 at ../arch/x86/kernel/alternative.c:824 __text_poke+0x265/0x4a0
>
> Jan

Excluding the TLB flush from the irqs off section makes me nervous in
this case.

-- 
Philippe.

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

[Jan Kiszka]

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.

Jan

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

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

[Philippe Gerum]

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.

-- 
Philippe.

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

[Philippe Gerum]

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.
>

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.

-- 
Philippe.

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

[Jan Kiszka]

On 15.03.21 10:47, Philippe Gerum wrote:
> 
> 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.
>>
> 
> 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

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

[Philippe Gerum]

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.

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

[Philippe Gerum]

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

> On 15.03.21 10:47, Philippe Gerum wrote:
>> 
>> 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.
>>>
>> 
>> 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


I have almost the same here for Dovetail, except that I'm not
duplicating the assertion in unuse_temporary_mm since there is no reason
for the hard irq state flipping in between. No issue observed either.

-- 
Philippe.