[PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Vlastimil Babka]

On 32bit PAE kernels on 64bit hardware with enough physical bits,
l1tf_pfn_limit() will overflow unsigned long. This in turn affects
max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
observed in a 32bit guest with 42 bits physical address size, where
max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces the
following warning to dmesg:

[    6.396845] Truncating oversized swap area, only using 0k out of 2047996k

Fix this by using unsigned long long instead.

Reported-by: Dominique Leuenberger <dimstar@suse.de>
Reported-by: Adrian Schroeter <adrian@suse.de>
Fixes: 17dbca119312 ("x86/speculation/l1tf: Add sysfs reporting for l1tf")
Fixes: 377eeaa8e11f ("x86/speculation/l1tf: Limit swap file size to MAX_PA/2")
Cc: stable@vger.kernel.org
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 arch/x86/include/asm/processor.h | 4 ++--
 arch/x86/mm/init.c               | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 682286aca881..a0a52274cb4a 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -181,9 +181,9 @@ extern const struct seq_operations cpuinfo_op;
 
 extern void cpu_detect(struct cpuinfo_x86 *c);
 
-static inline unsigned long l1tf_pfn_limit(void)
+static inline unsigned long long l1tf_pfn_limit(void)
 {
-	return BIT(boot_cpu_data.x86_phys_bits - 1 - PAGE_SHIFT) - 1;
+	return BIT_ULL(boot_cpu_data.x86_phys_bits - 1 - PAGE_SHIFT) - 1;
 }
 
 extern void early_cpu_init(void);
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index acfab322fbe0..02de3d6065c4 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -923,7 +923,7 @@ unsigned long max_swapfile_size(void)
 
 	if (boot_cpu_has_bug(X86_BUG_L1TF)) {
 		/* Limit the swap file size to MAX_PA/2 for L1TF workaround */
-		unsigned long l1tf_limit = l1tf_pfn_limit() + 1;
+		unsigned long long l1tf_limit = l1tf_pfn_limit() + 1;
 		/*
 		 * We encode swap offsets also with 3 bits below those for pfn
 		 * which makes the usable limit higher.
@@ -931,7 +931,7 @@ unsigned long max_swapfile_size(void)
 #if CONFIG_PGTABLE_LEVELS > 2
 		l1tf_limit <<= PAGE_SHIFT - SWP_OFFSET_FIRST_BIT;
 #endif
-		pages = min_t(unsigned long, l1tf_limit, pages);
+		pages = min_t(unsigned long long, l1tf_limit, pages);
 	}
 	return pages;
 }
-- 
2.18.0

Re: [PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Michal Hocko]

On Mon 20-08-18 11:58:35, Vlastimil Babka wrote:
> On 32bit PAE kernels on 64bit hardware with enough physical bits,
> l1tf_pfn_limit() will overflow unsigned long. This in turn affects
> max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
> observed in a 32bit guest with 42 bits physical address size, where
> max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces the
> following warning to dmesg:
> 
> [    6.396845] Truncating oversized swap area, only using 0k out of 2047996k
> 
> Fix this by using unsigned long long instead.
> 
> Reported-by: Dominique Leuenberger <dimstar@suse.de>
> Reported-by: Adrian Schroeter <adrian@suse.de>
> Fixes: 17dbca119312 ("x86/speculation/l1tf: Add sysfs reporting for l1tf")
> Fixes: 377eeaa8e11f ("x86/speculation/l1tf: Limit swap file size to MAX_PA/2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Looks good to me. I would probably use phys_addr_t which would be more
descriptive but this is just minor thing.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  arch/x86/include/asm/processor.h | 4 ++--
>  arch/x86/mm/init.c               | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 682286aca881..a0a52274cb4a 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -181,9 +181,9 @@ extern const struct seq_operations cpuinfo_op;
>  
>  extern void cpu_detect(struct cpuinfo_x86 *c);
>  
> -static inline unsigned long l1tf_pfn_limit(void)
> +static inline unsigned long long l1tf_pfn_limit(void)
>  {
> -	return BIT(boot_cpu_data.x86_phys_bits - 1 - PAGE_SHIFT) - 1;
> +	return BIT_ULL(boot_cpu_data.x86_phys_bits - 1 - PAGE_SHIFT) - 1;
>  }
>  
>  extern void early_cpu_init(void);
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index acfab322fbe0..02de3d6065c4 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -923,7 +923,7 @@ unsigned long max_swapfile_size(void)
>  
>  	if (boot_cpu_has_bug(X86_BUG_L1TF)) {
>  		/* Limit the swap file size to MAX_PA/2 for L1TF workaround */
> -		unsigned long l1tf_limit = l1tf_pfn_limit() + 1;
> +		unsigned long long l1tf_limit = l1tf_pfn_limit() + 1;
>  		/*
>  		 * We encode swap offsets also with 3 bits below those for pfn
>  		 * which makes the usable limit higher.
> @@ -931,7 +931,7 @@ unsigned long max_swapfile_size(void)
>  #if CONFIG_PGTABLE_LEVELS > 2
>  		l1tf_limit <<= PAGE_SHIFT - SWP_OFFSET_FIRST_BIT;
>  #endif
> -		pages = min_t(unsigned long, l1tf_limit, pages);
> +		pages = min_t(unsigned long long, l1tf_limit, pages);
>  	}
>  	return pages;
>  }
> -- 
> 2.18.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Vlastimil Babka]

On 08/20/2018 12:49 PM, Michal Hocko wrote:
> On Mon 20-08-18 11:58:35, Vlastimil Babka wrote:
>> On 32bit PAE kernels on 64bit hardware with enough physical bits,
>> l1tf_pfn_limit() will overflow unsigned long. This in turn affects
>> max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
>> observed in a 32bit guest with 42 bits physical address size, where
>> max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces the
>> following warning to dmesg:
>>
>> [    6.396845] Truncating oversized swap area, only using 0k out of 2047996k
>>
>> Fix this by using unsigned long long instead.
>>
>> Reported-by: Dominique Leuenberger <dimstar@suse.de>
>> Reported-by: Adrian Schroeter <adrian@suse.de>
>> Fixes: 17dbca119312 ("x86/speculation/l1tf: Add sysfs reporting for l1tf")
>> Fixes: 377eeaa8e11f ("x86/speculation/l1tf: Limit swap file size to MAX_PA/2")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Looks good to me. I would probably use phys_addr_t which would be more
> descriptive but this is just minor thing.

Hmm phys_addr_t is still 32bit on !PAE so there the overflow could still
happen. I guess max_swapfile_size() should skip the whole L1TF part for
!PAE since there is no pte inverting done anyway.

Also the value is "number of pages" which is not the same as "physical
address" so the phys_addr_t could be misleading anyway?

> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

Re: [PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Michal Hocko]

On Mon 20-08-18 13:41:03, Vlastimil Babka wrote:
> On 08/20/2018 12:49 PM, Michal Hocko wrote:
> > On Mon 20-08-18 11:58:35, Vlastimil Babka wrote:
> >> On 32bit PAE kernels on 64bit hardware with enough physical bits,
> >> l1tf_pfn_limit() will overflow unsigned long. This in turn affects
> >> max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
> >> observed in a 32bit guest with 42 bits physical address size, where
> >> max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces the
> >> following warning to dmesg:
> >>
> >> [    6.396845] Truncating oversized swap area, only using 0k out of 2047996k
> >>
> >> Fix this by using unsigned long long instead.
> >>
> >> Reported-by: Dominique Leuenberger <dimstar@suse.de>
> >> Reported-by: Adrian Schroeter <adrian@suse.de>
> >> Fixes: 17dbca119312 ("x86/speculation/l1tf: Add sysfs reporting for l1tf")
> >> Fixes: 377eeaa8e11f ("x86/speculation/l1tf: Limit swap file size to MAX_PA/2")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> > 
> > Looks good to me. I would probably use phys_addr_t which would be more
> > descriptive but this is just minor thing.
> 
> Hmm phys_addr_t is still 32bit on !PAE so there the overflow could still
> happen. I guess max_swapfile_size() should skip the whole L1TF part for
> !PAE since there is no pte inverting done anyway.

Yeah, I misremembered that we are already doing that.

> Also the value is "number of pages" which is not the same as "physical
> address" so the phys_addr_t could be misleading anyway?

right

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Andi Kleen]

On Mon, Aug 20, 2018 at 11:58:35AM +0200, Vlastimil Babka wrote:
> On 32bit PAE kernels on 64bit hardware with enough physical bits,
> l1tf_pfn_limit() will overflow unsigned long. This in turn affects
> max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
> observed in a 32bit guest with 42 bits physical address size, where
> max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces the
> following warning to dmesg:
> 
> [    6.396845] Truncating oversized swap area, only using 0k out of 2047996k
> 
> Fix this by using unsigned long long instead.

Looks good.

Acked-by: Andi Kleen <ak@linux.intel.com>

BTW our much worse problems right now are crash reports on several 
stable kernels, especially with large pages 

I'll dig into this more today, but if you have any hints from testing/fixing
your own backports please share them.

-Andi

Re: [PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Michal Hocko]

On Mon 20-08-18 07:20:27, Andi Kleen wrote:
> On Mon, Aug 20, 2018 at 11:58:35AM +0200, Vlastimil Babka wrote:
> > On 32bit PAE kernels on 64bit hardware with enough physical bits,
> > l1tf_pfn_limit() will overflow unsigned long. This in turn affects
> > max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
> > observed in a 32bit guest with 42 bits physical address size, where
> > max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces the
> > following warning to dmesg:
> > 
> > [    6.396845] Truncating oversized swap area, only using 0k out of 2047996k
> > 
> > Fix this by using unsigned long long instead.
> 
> Looks good.
> 
> Acked-by: Andi Kleen <ak@linux.intel.com>
> 
> BTW our much worse problems right now are crash reports on several 
> stable kernels, especially with large pages 

Do you have any reference?

> I'll dig into this more today, but if you have any hints from testing/fixing
> your own backports please share them.

Well, we have seen some issues on pre 3.12 kernels due to misbackporting
prot_none mitigations. PMD format is different in pre 4.4 kernels. Maybe
this is a similar issue.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] x86/speculation/l1tf: fix overflow on l1tf_pfn_limit() on 32bit

[Andi Kleen]

> > BTW our much worse problems right now are crash reports on several 
> > stable kernels, especially with large pages 
> 
> Do you have any reference?


https://lkml.org/lkml/2018/8/17/546
https://bugzilla.redhat.com/show_bug.cgi?id=1618792       (this is 4.17, not 4.18)

(might be different problems. I wasn't able to reproduce the 4.17 one
so far)


-Andi

[tip:x86/urgent] x86/speculation/l1tf: Fix overflow in l1tf_pfn_limit() on 32bit

[tip-bot for Vlastimil Babka]

Commit-ID:  9df9516940a61d29aedf4d91b483ca6597e7d480
Gitweb:     https://git.kernel.org/tip/9df9516940a61d29aedf4d91b483ca6597e7d480
Author:     Vlastimil Babka <vbabka@suse.cz>
AuthorDate: Mon, 20 Aug 2018 11:58:35 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 20 Aug 2018 18:04:42 +0200

x86/speculation/l1tf: Fix overflow in l1tf_pfn_limit() on 32bit

On 32bit PAE kernels on 64bit hardware with enough physical bits,
l1tf_pfn_limit() will overflow unsigned long. This in turn affects
max_swapfile_size() and can lead to swapon returning -EINVAL. This has been
observed in a 32bit guest with 42 bits physical address size, where
max_swapfile_size() overflows exactly to 1 << 32, thus zero, and produces
the following warning to dmesg:

[    6.396845] Truncating oversized swap area, only using 0k out of 2047996k

Fix this by using unsigned long long instead.

Fixes: 17dbca119312 ("x86/speculation/l1tf: Add sysfs reporting for l1tf")
Fixes: 377eeaa8e11f ("x86/speculation/l1tf: Limit swap file size to MAX_PA/2")
Reported-by: Dominique Leuenberger <dimstar@suse.de>
Reported-by: Adrian Schroeter <adrian@suse.de>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Andi Kleen <ak@linux.intel.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20180820095835.5298-1-vbabka@suse.cz

---
 arch/x86/include/asm/processor.h | 4 ++--
 arch/x86/mm/init.c               | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 682286aca881..a0a52274cb4a 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -181,9 +181,9 @@ extern const struct seq_operations cpuinfo_op;
 
 extern void cpu_detect(struct cpuinfo_x86 *c);
 
-static inline unsigned long l1tf_pfn_limit(void)
+static inline unsigned long long l1tf_pfn_limit(void)
 {
-	return BIT(boot_cpu_data.x86_phys_bits - 1 - PAGE_SHIFT) - 1;
+	return BIT_ULL(boot_cpu_data.x86_phys_bits - 1 - PAGE_SHIFT) - 1;
 }
 
 extern void early_cpu_init(void);
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index acfab322fbe0..02de3d6065c4 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -923,7 +923,7 @@ unsigned long max_swapfile_size(void)
 
 	if (boot_cpu_has_bug(X86_BUG_L1TF)) {
 		/* Limit the swap file size to MAX_PA/2 for L1TF workaround */
-		unsigned long l1tf_limit = l1tf_pfn_limit() + 1;
+		unsigned long long l1tf_limit = l1tf_pfn_limit() + 1;
 		/*
 		 * We encode swap offsets also with 3 bits below those for pfn
 		 * which makes the usable limit higher.
@@ -931,7 +931,7 @@ unsigned long max_swapfile_size(void)
 #if CONFIG_PGTABLE_LEVELS > 2
 		l1tf_limit <<= PAGE_SHIFT - SWP_OFFSET_FIRST_BIT;
 #endif
-		pages = min_t(unsigned long, l1tf_limit, pages);
+		pages = min_t(unsigned long long, l1tf_limit, pages);
 	}
 	return pages;
 }