[patch 0/3] x86/pti: Fix various fallout

[patch 0/3] x86/pti: Fix various fallout

[Thomas Gleixner]

The small series fixes the recent fallout of the x86/pti merge:

 - Remove the stale local_flush_tlb() invocations from the CPU hotplug code
    
 - Remove the stale preempt_disable/enable() pair from __native_flush_tlb()

 - Fix a bogus free in the write_ldt() error path

Thanks,

	tglx

---
 include/asm/tlbflush.h |   14 ++++++++------
 kernel/ldt.c           |    2 +-
 kernel/smpboot.c       |    9 ---------
 3 files changed, 9 insertions(+), 16 deletions(-)

[patch 1/3] x86/ldt: Free the right LDT memory in write_ldt() error path

[Thomas Gleixner]

[-- Attachment #1: x86-ldt--Free-the-right-thing-in-error-path.patch --]
[-- Type: text/plain, Size: 693 bytes --]

The error path in write_ldt() frees the already installed LDT memory
instead of the newly allocated which cannot be installed.

Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/ldt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -421,7 +421,7 @@ static int write_ldt(void __user *ptr, u
 	 */
 	error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
 	if (error) {
-		free_ldt_struct(old_ldt);
+		free_ldt_struct(new_ldt);
 		goto out_unlock;
 	}
 

Re: [patch 1/3] x86/ldt: Free the right LDT memory in write_ldt() error path

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> The error path in write_ldt() frees the already installed LDT memory
> instead of the newly allocated which cannot be installed.

s/newly allocated
 /newly allocated one

> 
> Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
> Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/ldt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/arch/x86/kernel/ldt.c
> +++ b/arch/x86/kernel/ldt.c
> @@ -421,7 +421,7 @@ static int write_ldt(void __user *ptr, u
>  	 */
>  	error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
>  	if (error) {
> -		free_ldt_struct(old_ldt);
> +		free_ldt_struct(new_ldt);
>  		goto out_unlock;
>  	}
>  

This bug kind of scares me ...

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[patch V2 1/3] x86/ldt: Plug memory leak in error path

[Thomas Gleixner]

The error path in write_ldt() tries to free old_ldt instead of the newly
allocated new_ldt resulting in a memory leak. It also misses to clean up a
half populated LDT pagetable, which is not a leak as it gets cleaned up
when the process exits.

Free both the potentially half populated LDT pagetable and the newly
allocated LDT struct. This can be done unconditionally because once a LDT
is mapped subsequent maps will succeed because the PTE page is already
populated and the two LDTs fit into that single page.

Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/ldt.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- a/arch/x86/kernel/ldt.c
+++ b/arch/x86/kernel/ldt.c
@@ -421,7 +421,13 @@ static int write_ldt(void __user *ptr, u
 	 */
 	error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
 	if (error) {
-		free_ldt_struct(old_ldt);
+		/*
+		 * This only can fail for the first LDT setup. If a LDT is
+		 * already installed then the PTE page is already
+		 * populated. Mop up a half populated page table.
+		 */
+		free_ldt_pgtables(mm);
+		free_ldt_struct(new_ldt);
 		goto out_unlock;
 	}
 

Re: [patch V2 1/3] x86/ldt: Plug memory leak in error path

[Andy Lutomirski]

> On Dec 31, 2017, at 2:24 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> The error path in write_ldt() tries to free old_ldt instead of the newly
> allocated new_ldt resulting in a memory leak. It also misses to clean up a
> half populated LDT pagetable, which is not a leak as it gets cleaned up
> when the process exits.
> 
> Free both the potentially half populated LDT pagetable and the newly
> allocated LDT struct. This can be done unconditionally because once a LDT
> is mapped subsequent maps will succeed because the PTE page is already
> populated and the two LDTs fit into that single page.
> 
> Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on")
> Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> arch/x86/kernel/ldt.c |    8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> --- a/arch/x86/kernel/ldt.c
> +++ b/arch/x86/kernel/ldt.c
> @@ -421,7 +421,13 @@ static int write_ldt(void __user *ptr, u
>     */
>    error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0);
>    if (error) {
> -        free_ldt_struct(old_ldt);
> +        /*
> +         * This only can fail for the first LDT setup. If a LDT is
> +         * already installed then the PTE page is already
> +         * populated. Mop up a half populated page table.
> +         */

I liked it better with the conditional.  If this ever fails due to fault injection, some silly accounting issue, a paravirt glitch, or whatever, then we'll oops.

> +        free_ldt_pgtables(mm);
> +        free_ldt_struct(new_ldt);
>        goto out_unlock;
>    }
> 

[patch 2/3] x86/smpboot: Remove stale tlb flush invocations

[Thomas Gleixner]

[-- Attachment #1: x86-smpboot--Remove-stale-tlb-flush-invocations.patch --]
[-- Type: text/plain, Size: 1504 bytes --]

smpboot_setup_warm_reset_vector() and smpboot_restore_warm_reset_vector()
invoke local_flush_tlb() for no obvious reason.

Digging in history revealed that the original code in the 2.1 aera added
those because the code manipulated a swapper_pg_dir pagetable entry. The
pagetable manipulation was removed long ago in the 2.3 timeframe, but the
tlb flush invocations stayed around forever.

Remove them along with the pointless pr_debugs which come from the same 2.1
change.

Reported-by: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/smpboot.c |    9 ---------
 1 file changed, 9 deletions(-)

--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -128,14 +128,10 @@ static inline void smpboot_setup_warm_re
 	spin_lock_irqsave(&rtc_lock, flags);
 	CMOS_WRITE(0xa, 0xf);
 	spin_unlock_irqrestore(&rtc_lock, flags);
-	local_flush_tlb();
-	pr_debug("1.\n");
 	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
 							start_eip >> 4;
-	pr_debug("2.\n");
 	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
 							start_eip & 0xf;
-	pr_debug("3.\n");
 }
 
 static inline void smpboot_restore_warm_reset_vector(void)
@@ -143,11 +139,6 @@ static inline void smpboot_restore_warm_
 	unsigned long flags;
 
 	/*
-	 * Install writable page 0 entry to set BIOS data area.
-	 */
-	local_flush_tlb();
-
-	/*
 	 * Paranoid:  Set warm reset code and vector here back
 	 * to default values.
 	 */

Re: [patch 2/3] x86/smpboot: Remove stale tlb flush invocations

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> smpboot_setup_warm_reset_vector() and smpboot_restore_warm_reset_vector()
> invoke local_flush_tlb() for no obvious reason.
> 
> Digging in history revealed that the original code in the 2.1 aera added
> those because the code manipulated a swapper_pg_dir pagetable entry. The
> pagetable manipulation was removed long ago in the 2.3 timeframe, but the
> tlb flush invocations stayed around forever.

s/tlb/TLB

> 
> Remove them along with the pointless pr_debugs which come from the same 2.1
> change.
> 
> Reported-by: Dominik Brodowski <linux@dominikbrodowski.net>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/smpboot.c |    9 ---------
>  1 file changed, 9 deletions(-)
> 
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -128,14 +128,10 @@ static inline void smpboot_setup_warm_re
>  	spin_lock_irqsave(&rtc_lock, flags);
>  	CMOS_WRITE(0xa, 0xf);
>  	spin_unlock_irqrestore(&rtc_lock, flags);
> -	local_flush_tlb();
> -	pr_debug("1.\n");
>  	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
>  							start_eip >> 4;
> -	pr_debug("2.\n");
>  	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
>  							start_eip & 0xf;
> -	pr_debug("3.\n");
>  }
>  
>  static inline void smpboot_restore_warm_reset_vector(void)
> @@ -143,11 +139,6 @@ static inline void smpboot_restore_warm_
>  	unsigned long flags;
>  
>  	/*
> -	 * Install writable page 0 entry to set BIOS data area.
> -	 */
> -	local_flush_tlb();
> -
> -	/*
>  	 * Paranoid:  Set warm reset code and vector here back
>  	 * to default values.
>  	 */

Really nice archeology! :-)

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[patch 3/3] x86/mm: Remove preempt_disable/enable() from __native_flush_tlb()

[Thomas Gleixner]

[-- Attachment #1: x86-mm--Remove-preempt_disable-enable---from-__native_tlb_flush--.patch --]
[-- Type: text/plain, Size: 1960 bytes --]

The preempt_disable/enable() pair in __native_flush_tlb() was added in
commit 5cf0791da5c1 ("x86/mm: Disable preemption during CR3 read+write") to
protect the UP variant of flush_tlb_mm_range().

That preempt_disable/enable() pair should have been added to the UP variant
of flush_tlb_mm_range() instead.

The UP variant was removed with commit ce4a4e565f52 ("x86/mm: Remove the UP
asm/tlbflush.h code, always use the (formerly) SMP code"), but the
preempt_disable/enable() pair stayed around.

The latest change to __native_flush_tlb() in commit 6fd166aae78c ("x86/mm:
Use/Fix PCID to optimize user/kernel switches") added an access to a per
cpu variable outside the preempt disabled regions which makes no sense at
all. __native_flush_tlb() must always be called with at least preemption
disabled.

Remove the preempt_disable/enable() pair and add a WARN_ON_ONCE() to catch
bad callers independent of the smp_processor_id() debugging.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/tlbflush.h |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -345,15 +345,17 @@ static inline void invalidate_user_asid(
  */
 static inline void __native_flush_tlb(void)
 {
-	invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
 	/*
-	 * If current->mm == NULL then we borrow a mm which may change
-	 * during a task switch and therefore we must not be preempted
-	 * while we write CR3 back:
+	 * Preemption or interrupts must be disabled to protect the access
+	 * to the per cpu variable and to prevent being preempted between
+	 * read_cr3() and write_cr3().
 	 */
-	preempt_disable();
+	WARN_ON_ONCE(preemptible());
+
+	invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
+
+	/* If current->mm == NULL then the read_cr3() "borrows" a mm */
 	native_write_cr3(__native_read_cr3());
-	preempt_enable();
 }
 
 /*

Re: [patch 3/3] x86/mm: Remove preempt_disable/enable() from __native_flush_tlb()

[Ingo Molnar]

The cleanup looks good to me, just a few speling nits:

* Thomas Gleixner <tglx@linutronix.de> wrote:

> The preempt_disable/enable() pair in __native_flush_tlb() was added in
> commit 5cf0791da5c1 ("x86/mm: Disable preemption during CR3 read+write") to
> protect the UP variant of flush_tlb_mm_range().
> 
> That preempt_disable/enable() pair should have been added to the UP variant
> of flush_tlb_mm_range() instead.
> 
> The UP variant was removed with commit ce4a4e565f52 ("x86/mm: Remove the UP
> asm/tlbflush.h code, always use the (formerly) SMP code"), but the
> preempt_disable/enable() pair stayed around.
> 
> The latest change to __native_flush_tlb() in commit 6fd166aae78c ("x86/mm:
> Use/Fix PCID to optimize user/kernel switches") added an access to a per
> cpu variable outside the preempt disabled regions which makes no sense at
> all. __native_flush_tlb() must always be called with at least preemption
> disabled.

s/cpu/CPU

> 
> Remove the preempt_disable/enable() pair and add a WARN_ON_ONCE() to catch
> bad callers independent of the smp_processor_id() debugging.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/tlbflush.h |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -345,15 +345,17 @@ static inline void invalidate_user_asid(
>   */
>  static inline void __native_flush_tlb(void)
>  {
> -	invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
>  	/*
> -	 * If current->mm == NULL then we borrow a mm which may change
> -	 * during a task switch and therefore we must not be preempted
> -	 * while we write CR3 back:
> +	 * Preemption or interrupts must be disabled to protect the access
> +	 * to the per cpu variable and to prevent being preempted between
> +	 * read_cr3() and write_cr3().
>  	 */
> -	preempt_disable();
> +	WARN_ON_ONCE(preemptible());
> +
> +	invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
> +
> +	/* If current->mm == NULL then the read_cr3() "borrows" a mm */
>  	native_write_cr3(__native_read_cr3());
> -	preempt_enable();

s/a mm/an mm

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [patch 0/3] x86/pti: Fix various fallout

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> The small series fixes the recent fallout of the x86/pti merge:
> 
>  - Remove the stale local_flush_tlb() invocations from the CPU hotplug code
>     
>  - Remove the stale preempt_disable/enable() pair from __native_flush_tlb()
> 
>  - Fix a bogus free in the write_ldt() error path

Linus, I suspect -rc6 is imminent, and it would be nice to at least have the LDT 
error path fix in. I'll send you these fixes tomorrow, but feel free to pick it up 
from email if you wanted to release -rc6 today.

Thanks,

	Ingo

Re: [patch 0/3] x86/pti: Fix various fallout

[Linus Torvalds]

On Sat, Dec 30, 2017 at 1:35 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Linus, I suspect -rc6 is imminent, and it would be nice to at least have the LDT
> error path fix in. I'll send you these fixes tomorrow, but feel free to pick it up
> from email if you wanted to release -rc6 today.

I'll do rc6 tomorrow probably around this time (early afternoon PST).
So I think I should be ok just waiting for your pull request.

Thanks,

                  Linus

Re: [patch 0/3] x86/pti: Fix various fallout

[Andy Lutomirski]

> On Dec 30, 2017, at 2:06 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Sat, Dec 30, 2017 at 1:35 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> 
>> Linus, I suspect -rc6 is imminent, and it would be nice to at least have the LDT
>> error path fix in. I'll send you these fixes tomorrow, but feel free to pick it up
>> from email if you wanted to release -rc6 today.
> 
> I'll do rc6 tomorrow probably around this time (early afternoon PST).
> So I think I should be ok just waiting for your pull request.

FWIW, I think this big is at worst just a memory leak.  Once an mm has any LDT mapped, mapping a second one can't fail because the entire LDT area is under 512 pages, meaning that nothing needs to be allocated, so there's no opportunity for failure.

So it's an embarrassing bug, but not catastrophic.

> 
> Thanks,
> 
>                  Linus