[PATCH] x86/alternatives: Let __text_poke() acquire the pte lock with enabled interrupts

[PATCH] x86/alternatives: Let __text_poke() acquire the pte lock with enabled interrupts

[Sebastian Andrzej Siewior]

The pte lock is never acquired from an IRQ-off region so it does not
require the interrupts to be disabled.
RT complains here because the spinlock_t must not be acquired with
disabled interrupts.

use_temporary_mm() expects interrupts to be off because it invokes
switch_mm_irqs_off() and uses per-CPU (current active mm) data.

Move local_irq_save() after the the pte lock has been acquired. Move
local_irq_restore() after the pte lock has been released.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/alternative.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 8fd39ff74a499..7c59a87ebbde8 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -872,8 +872,6 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
 
-	local_irq_save(flags);
-
 	/*
 	 * Map the page without the global bit, as TLB flushing is done with
 	 * flush_tlb_mm_range(), which is intended for non-global PTEs.
@@ -890,6 +888,8 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	VM_BUG_ON(!ptep);
 
+	local_irq_save(flags);
+
 	pte = mk_pte(pages[0], pgprot);
 	set_pte_at(poking_mm, poking_addr, ptep, pte);
 
@@ -939,8 +939,8 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	BUG_ON(memcmp(addr, opcode, len));
 
-	pte_unmap_unlock(ptep, ptl);
 	local_irq_restore(flags);
+	pte_unmap_unlock(ptep, ptl);
 	return addr;
 }
 
-- 
2.27.0

Re: [PATCH] x86/alternatives: Let __text_poke() acquire the pte lock with enabled interrupts

[Thomas Gleixner]

Sebastian,

Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:

sorry this fell through the cracks ...

> The pte lock is never acquired from an IRQ-off region so it does not
> require the interrupts to be disabled.

I doubt that this is true. It surely is acquired within other locks
which might be taken with spin_lock_irq(). Which is completely fine on
RT.

But that's not the point. The point is that pte_lock() does not require
to be taken with interrupts disabled.

Please be precise about these kind of things. Handwavy descriptions
cause more problems than they solve.

> RT complains here because the spinlock_t must not be acquired with
> disabled interrupts.
>
> use_temporary_mm() expects interrupts to be off because it invokes
> switch_mm_irqs_off() and uses per-CPU (current active mm) data.
>
> Move local_irq_save() after the the pte lock has been acquired. Move
> local_irq_restore() after the pte lock has been released.

While part 1 is correct, part 2 is the exact opposite of what the patch
does.

  Move the PTE lock handling outside the interrupt disabled region.

describes precisely what this is about without any gory details which
can be seen in the patch itself. Hmm?

Thanks,

        tglx

Re: [PATCH] x86/alternatives: Let __text_poke() acquire the pte lock with enabled interrupts

[Sebastian Andrzej Siewior]

On 2020-08-12 16:39:41 [+0200], Thomas Gleixner wrote:
> Sebastian,
Hi tglx,

> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes:
> 
> > The pte lock is never acquired from an IRQ-off region so it does not
> > require the interrupts to be disabled.
> 
> I doubt that this is true. It surely is acquired within other locks
> which might be taken with spin_lock_irq(). Which is completely fine on
> RT.
> 
> But that's not the point. The point is that pte_lock() does not require
> to be taken with interrupts disabled.

The IRQ-off vs in-IRQ working was chosen poorly.

> Please be precise about these kind of things. Handwavy descriptions
> cause more problems than they solve.
> 
> > RT complains here because the spinlock_t must not be acquired with
> > disabled interrupts.
> >
> > use_temporary_mm() expects interrupts to be off because it invokes
> > switch_mm_irqs_off() and uses per-CPU (current active mm) data.
> >
> > Move local_irq_save() after the the pte lock has been acquired. Move
> > local_irq_restore() after the pte lock has been released.
> 
> While part 1 is correct, part 2 is the exact opposite of what the patch
> does.
> 
>   Move the PTE lock handling outside the interrupt disabled region.
> 
> describes precisely what this is about without any gory details which
> can be seen in the patch itself. Hmm?

Oki reworded.

> Thanks,
> 
>         tglx

Sebastian

[PATCH v2] x86/alternatives: Let __text_poke() acquire the pte lock with enabled interrupts

[Sebastian Andrzej Siewior]

The pte lock is never acquired in-IRQ context so it does not require the
interrupts to be disabled.

RT complains here because the spinlock_t must not be acquired with
disabled interrupts.

use_temporary_mm() expects interrupts to be off because it invokes
switch_mm_irqs_off() and uses per-CPU (current active mm) data.

Move the PTE lock handling outside the interrupt disabled region.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Reword the patch description.

 arch/x86/kernel/alternative.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -875,8 +875,6 @@ static void *__text_poke(void *addr, con
 	 */
 	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
 
-	local_irq_save(flags);
-
 	/*
 	 * Map the page without the global bit, as TLB flushing is done with
 	 * flush_tlb_mm_range(), which is intended for non-global PTEs.
@@ -893,6 +891,8 @@ static void *__text_poke(void *addr, con
 	 */
 	VM_BUG_ON(!ptep);
 
+	local_irq_save(flags);
+
 	pte = mk_pte(pages[0], pgprot);
 	set_pte_at(poking_mm, poking_addr, ptep, pte);
 
@@ -942,8 +942,8 @@ static void *__text_poke(void *addr, con
 	 */
 	BUG_ON(memcmp(addr, opcode, len));
 
-	pte_unmap_unlock(ptep, ptl);
 	local_irq_restore(flags);
+	pte_unmap_unlock(ptep, ptl);
 	return addr;
 }
 

Re: [PATCH v2] x86/alternatives: Let __text_poke() acquire the pte lock with enabled interrupts

[peterz]

On Thu, Aug 13, 2020 at 12:50:26PM +0200, Sebastian Andrzej Siewior wrote:
> The pte lock is never acquired in-IRQ context so it does not require the
> interrupts to be disabled.
> 
> RT complains here because the spinlock_t must not be acquired with
> disabled interrupts.
> 
> use_temporary_mm() expects interrupts to be off because it invokes
> switch_mm_irqs_off() and uses per-CPU (current active mm) data.
> 
> Move the PTE lock handling outside the interrupt disabled region.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Agreed, this should be fine.

Acked-by; Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
> v1…v2: Reword the patch description.
> 
>  arch/x86/kernel/alternative.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -875,8 +875,6 @@ static void *__text_poke(void *addr, con
>  	 */
>  	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
>  
> -	local_irq_save(flags);
> -
>  	/*
>  	 * Map the page without the global bit, as TLB flushing is done with
>  	 * flush_tlb_mm_range(), which is intended for non-global PTEs.
> @@ -893,6 +891,8 @@ static void *__text_poke(void *addr, con
>  	 */
>  	VM_BUG_ON(!ptep);
>  
> +	local_irq_save(flags);
> +
>  	pte = mk_pte(pages[0], pgprot);
>  	set_pte_at(poking_mm, poking_addr, ptep, pte);
>  
> @@ -942,8 +942,8 @@ static void *__text_poke(void *addr, con
>  	 */
>  	BUG_ON(memcmp(addr, opcode, len));
>  
> -	pte_unmap_unlock(ptep, ptl);
>  	local_irq_restore(flags);
> +	pte_unmap_unlock(ptep, ptl);
>  	return addr;
>  }
>  

[tip: x86/urgent] x86/alternatives: Acquire pte lock with interrupts enabled

[tip-bot2 for Sebastian Andrzej Siewior]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     a6d996cbd38b42341ad3fce74506b9fdc280e395
Gitweb:        https://git.kernel.org/tip/a6d996cbd38b42341ad3fce74506b9fdc280e395
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Thu, 13 Aug 2020 12:50:26 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 13 Aug 2020 14:11:54 +02:00

x86/alternatives: Acquire pte lock with interrupts enabled

pte lock is never acquired in-IRQ context so it does not require interrupts
to be disabled. The lock is a regular spinlock which cannot be acquired
with interrupts disabled on RT.

RT complains about pte_lock() in __text_poke() because it's invoked after
disabling interrupts.

__text_poke() has to disable interrupts as use_temporary_mm() expects
interrupts to be off because it invokes switch_mm_irqs_off() and uses
per-CPU (current active mm) data.

Move the PTE lock handling outside the interrupt disabled region.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by; Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20200813105026.bvugytmsso6muljw@linutronix.de

---
 arch/x86/kernel/alternative.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index c826cdd..34a1b85 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -874,8 +874,6 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
 
-	local_irq_save(flags);
-
 	/*
 	 * Map the page without the global bit, as TLB flushing is done with
 	 * flush_tlb_mm_range(), which is intended for non-global PTEs.
@@ -892,6 +890,8 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	VM_BUG_ON(!ptep);
 
+	local_irq_save(flags);
+
 	pte = mk_pte(pages[0], pgprot);
 	set_pte_at(poking_mm, poking_addr, ptep, pte);
 
@@ -941,8 +941,8 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 	 */
 	BUG_ON(memcmp(addr, opcode, len));
 
-	pte_unmap_unlock(ptep, ptl);
 	local_irq_restore(flags);
+	pte_unmap_unlock(ptep, ptl);
 	return addr;
 }