[Xen-devel] [PATCH v2 0/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Xen-devel] [PATCH v2 0/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Andrew Cooper]

Reworked to allow DEFINE_PER_CPU_PAGE_ALIGNED() to specify the
__aligned(PAGE_SIZE) itself.

Andrew Cooper (2):
  xen/link: Introduce .bss.percpu.page_aligned
  x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

 xen/arch/arm/xen.lds.S          |  5 +++--
 xen/arch/x86/setup.c            |  3 ---
 xen/arch/x86/traps.c            |  6 ++++++
 xen/arch/x86/xen.lds.S          |  7 +++++--
 xen/include/asm-arm/percpu.h    |  6 ++----
 xen/include/asm-x86/percpu.h    |  6 ++----
 xen/include/asm-x86/processor.h |  4 ++--
 xen/include/xen/percpu.h        | 10 ++++++++--
 8 files changed, 28 insertions(+), 19 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

Future changes are going to need to page align some percpu data.

This means that the percpu area needs suitably aligning in the BSS so CPU0 has
correctly aligned data.  Shuffle the exact link order of items within the BSS
to give .bss.percpu.page_aligned appropriate alignment.

In addition, we need to be able to specify an alignment attribute to
__DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
that it is now possible to grep for .bss.percpu and find all the users.

Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which uses both section and
alignment attributes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>

v2:
 * Rework __DEFINE_PER_CPU() to allow for further attributes to be passed.
 * Specify __aligned(PAGE_SIZE) as part of DEFINE_PER_CPU_PAGE_ALIGNED().
---
 xen/arch/arm/xen.lds.S       |  5 +++--
 xen/arch/x86/xen.lds.S       |  5 +++--
 xen/include/asm-arm/percpu.h |  6 ++----
 xen/include/asm-x86/percpu.h |  6 ++----
 xen/include/xen/percpu.h     | 10 ++++++++--
 5 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 12c107f45d..07cbdf2543 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -201,14 +201,15 @@ SECTIONS
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
        *(.bss.page_aligned)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
        *(.bss.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(SMP_CACHE_BYTES);
        __bss_end = .;
   } :text
   _end = . ;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index a73139cd29..b8a2ea4259 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -293,14 +293,15 @@ SECTIONS
        __bss_start = .;
        *(.bss.stack_aligned)
        *(.bss.page_aligned*)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
        *(.bss.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(SMP_CACHE_BYTES);
        __bss_end = .;
   } :text
   _end = . ;
diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
index 9584b830d4..264120b192 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -10,10 +10,8 @@ extern char __per_cpu_start[], __per_cpu_data_end[];
 extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
 
 #define per_cpu(var, cpu)  \
     (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
index ff34dc7897..5b6cef04c4 100644
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 #endif
 
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
 
 /* var is in discarded region: offset to particular copy we want */
 #define per_cpu(var, cpu)  \
diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
index aeec5c19d6..71a31cc361 100644
--- a/xen/include/xen/percpu.h
+++ b/xen/include/xen/percpu.h
@@ -9,9 +9,15 @@
  * The _##name concatenation is being used here to prevent 'name' from getting
  * macro expanded, while still allowing a per-architecture symbol name prefix.
  */
-#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
+#define DEFINE_PER_CPU(type, name) \
+    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
+
+#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
+    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
+                     __aligned(PAGE_SIZE), type, _ ## name)
+
 #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
-	__DEFINE_PER_CPU(type, _##name, .read_mostly)
+    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)
 
 #define get_per_cpu_var(var)  (per_cpu__##var)
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Andrew Cooper]

The XPTI work restricted the visibility of most of memory, but missed a few
aspects when it came to the TSS.

Given that the TSS is just an object in percpu data, the 4k mapping for it
created in setup_cpu_root_pgt() maps adjacent percpu data, making it all
leakable via Meltdown, even when XPTI is in use.

Furthermore, no care is taken to check that the TSS doesn't cross a page
boundary.  As it turns out, struct tss_struct is aligned on its size which
does prevent it straddling a page boundary, but this will cease to be true
once CET and Shadow Stack support is added to Xen.

Move the TSS into the page aligned percpu area, so no adjacent data can be
leaked.  Move the definition from setup.c to traps.c, which is a more
appropriate place for it to live.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over changes to include __aligned() within
   DEFINE_PER_CPU_PAGE_ALIGNED()
 * Drop now-unused xen/percpu.h from setup.c
---
 xen/arch/x86/setup.c            | 3 ---
 xen/arch/x86/traps.c            | 6 ++++++
 xen/arch/x86/xen.lds.S          | 2 ++
 xen/include/asm-x86/processor.h | 4 ++--
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d2011910fa..f9d38155d3 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -16,7 +16,6 @@
 #include <xen/domain_page.h>
 #include <xen/version.h>
 #include <xen/gdbstub.h>
-#include <xen/percpu.h>
 #include <xen/hypercall.h>
 #include <xen/keyhandler.h>
 #include <xen/numa.h>
@@ -100,8 +99,6 @@ unsigned long __read_mostly xen_phys_start;
 
 unsigned long __read_mostly xen_virt_end;
 
-DEFINE_PER_CPU(struct tss_struct, init_tss);
-
 char __section(".bss.stack_aligned") __aligned(STACK_SIZE)
     cpu0_stack[STACK_SIZE];
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 38d12013db..de3ac135f5 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -108,6 +108,12 @@ idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
 /* Pointer to the IDT of every CPU. */
 idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
 
+/*
+ * The TSS is smaller than a page, but we give it a full page to avoid
+ * adjacent per-cpu data leaking via Meltdown when XPTI is in use.
+ */
+DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_struct, init_tss);
+
 bool (*ioemul_handle_quirk)(
     u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index b8a2ea4259..c82e1e504a 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -368,6 +368,8 @@ ASSERT(IS_ALIGNED(__2M_rwdata_end,   SECTION_ALIGN), "__2M_rwdata_end misaligned
 
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
 
+ASSERT(IS_ALIGNED(per_cpu__init_tss, PAGE_SIZE), "per_cpu(init_tss) misaligned")
+
 ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned")
 ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZE), "__init_end misaligned")
 
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 2862321eee..b5bee94931 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
 #define IOBMP_BYTES             8192
 #define IOBMP_INVALID_OFFSET    0x8000
 
-struct __packed __cacheline_aligned tss_struct {
+struct __packed tss_struct {
     uint32_t :32;
     uint64_t rsp0, rsp1, rsp2;
     uint64_t :64;
@@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
     /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
     uint8_t __cacheline_filler[24];
 };
+DECLARE_PER_CPU(struct tss_struct, init_tss);
 
 #define IST_NONE 0UL
 #define IST_DF   1UL
@@ -463,7 +464,6 @@ static inline void disable_each_ist(idt_entry_t *idt)
 extern idt_entry_t idt_table[];
 extern idt_entry_t *idt_tables[];
 
-DECLARE_PER_CPU(struct tss_struct, init_tss);
 DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
 
 extern void write_ptbase(struct vcpu *v);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Julien Grall]

Hi Andrew,

On 7/26/19 9:32 PM, Andrew Cooper wrote:
> Future changes are going to need to page align some percpu data.
> 
> This means that the percpu area needs suitably aligning in the BSS so CPU0 has
> correctly aligned data.  Shuffle the exact link order of items within the BSS
> to give .bss.percpu.page_aligned appropriate alignment.
> 
> In addition, we need to be able to specify an alignment attribute to
> __DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
> adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
> that it is now possible to grep for .bss.percpu and find all the users.
> 
> Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which uses both section and
> alignment attributes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> 
> v2:
>   * Rework __DEFINE_PER_CPU() to allow for further attributes to be passed.
>   * Specify __aligned(PAGE_SIZE) as part of DEFINE_PER_CPU_PAGE_ALIGNED().
> ---
>   xen/arch/arm/xen.lds.S       |  5 +++--
>   xen/arch/x86/xen.lds.S       |  5 +++--
>   xen/include/asm-arm/percpu.h |  6 ++----
>   xen/include/asm-x86/percpu.h |  6 ++----
>   xen/include/xen/percpu.h     | 10 ++++++++--
>   5 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 12c107f45d..07cbdf2543 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -201,14 +201,15 @@ SECTIONS
>          *(.bss.stack_aligned)
>          . = ALIGN(PAGE_SIZE);
>          *(.bss.page_aligned)
> -       *(.bss)
> -       . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_start = .;
> +       *(.bss.percpu.page_aligned)
>          *(.bss.percpu)
>          . = ALIGN(SMP_CACHE_BYTES);
>          *(.bss.percpu.read_mostly)
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
> +       *(.bss)
> +       . = ALIGN(SMP_CACHE_BYTES);
>          __bss_end = .;
>     } :text
>     _end = . ;
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index a73139cd29..b8a2ea4259 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -293,14 +293,15 @@ SECTIONS
>          __bss_start = .;
>          *(.bss.stack_aligned)
>          *(.bss.page_aligned*)
> -       *(.bss)
> -       . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_start = .;
> +       *(.bss.percpu.page_aligned)
>          *(.bss.percpu)
>          . = ALIGN(SMP_CACHE_BYTES);
>          *(.bss.percpu.read_mostly)
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
> +       *(.bss)
> +       . = ALIGN(SMP_CACHE_BYTES);
>          __bss_end = .;
>     } :text
>     _end = . ;
> diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
> index 9584b830d4..264120b192 100644
> --- a/xen/include/asm-arm/percpu.h
> +++ b/xen/include/asm-arm/percpu.h
> @@ -10,10 +10,8 @@ extern char __per_cpu_start[], __per_cpu_data_end[];
>   extern unsigned long __per_cpu_offset[NR_CPUS];
>   void percpu_init_areas(void);
>   
> -/* Separate out the type, so (int[3], foo) works. */
> -#define __DEFINE_PER_CPU(type, name, suffix)                    \
> -    __section(".bss.percpu" #suffix)                            \
> -    __typeof__(type) per_cpu_##name
> +#define __DEFINE_PER_CPU(attr, type, name) \
> +    attr __typeof__(type) per_cpu_ ## name
>   
>   #define per_cpu(var, cpu)  \
>       (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
> diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
> index ff34dc7897..5b6cef04c4 100644
> --- a/xen/include/asm-x86/percpu.h
> +++ b/xen/include/asm-x86/percpu.h
> @@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR_CPUS];
>   void percpu_init_areas(void);
>   #endif
>   
> -/* Separate out the type, so (int[3], foo) works. */
> -#define __DEFINE_PER_CPU(type, name, suffix)                    \
> -    __section(".bss.percpu" #suffix)                            \
> -    __typeof__(type) per_cpu_##name
> +#define __DEFINE_PER_CPU(attr, type, name) \
> +    attr __typeof__(type) per_cpu_ ## name
>   
>   /* var is in discarded region: offset to particular copy we want */
>   #define per_cpu(var, cpu)  \
> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
> index aeec5c19d6..71a31cc361 100644
> --- a/xen/include/xen/percpu.h
> +++ b/xen/include/xen/percpu.h
> @@ -9,9 +9,15 @@
>    * The _##name concatenation is being used here to prevent 'name' from getting
>    * macro expanded, while still allowing a per-architecture symbol name prefix.
>    */
> -#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
> +#define DEFINE_PER_CPU(type, name) \
> +    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
> +
> +#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
> +    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
> +                     __aligned(PAGE_SIZE), type, _ ## name)
> +
>   #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
> -	__DEFINE_PER_CPU(type, _##name, .read_mostly)
> +    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)
>   
>   #define get_per_cpu_var(var)  (per_cpu__##var)
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Roger Pau Monné]

On Fri, Jul 26, 2019 at 09:32:21PM +0100, Andrew Cooper wrote:
> Future changes are going to need to page align some percpu data.
> 
> This means that the percpu area needs suitably aligning in the BSS so CPU0 has
> correctly aligned data.  Shuffle the exact link order of items within the BSS
> to give .bss.percpu.page_aligned appropriate alignment.
> 
> In addition, we need to be able to specify an alignment attribute to
> __DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
> adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
> that it is now possible to grep for .bss.percpu and find all the users.
> 
> Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which uses both section and
> alignment attributes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
> index ff34dc7897..5b6cef04c4 100644
> --- a/xen/include/asm-x86/percpu.h
> +++ b/xen/include/asm-x86/percpu.h
> @@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR_CPUS];
>  void percpu_init_areas(void);
>  #endif
>  
> -/* Separate out the type, so (int[3], foo) works. */
> -#define __DEFINE_PER_CPU(type, name, suffix)                    \
> -    __section(".bss.percpu" #suffix)                            \
> -    __typeof__(type) per_cpu_##name
> +#define __DEFINE_PER_CPU(attr, type, name) \
> +    attr __typeof__(type) per_cpu_ ## name
>  
>  /* var is in discarded region: offset to particular copy we want */
>  #define per_cpu(var, cpu)  \
> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
> index aeec5c19d6..71a31cc361 100644
> --- a/xen/include/xen/percpu.h
> +++ b/xen/include/xen/percpu.h
> @@ -9,9 +9,15 @@
>   * The _##name concatenation is being used here to prevent 'name' from getting
>   * macro expanded, while still allowing a per-architecture symbol name prefix.
>   */
> -#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
> +#define DEFINE_PER_CPU(type, name) \
> +    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
> +
> +#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
> +    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
> +                     __aligned(PAGE_SIZE), type, _ ## name)
> +
>  #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
> -	__DEFINE_PER_CPU(type, _##name, .read_mostly)
> +    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)

AFAICT also adding a '_' here will result in variable names with
per_cpu__foo, which is inline with the previous behaviour, but I'm not
sure of the point of the double underscore.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Roger Pau Monné]

On Fri, Jul 26, 2019 at 09:32:22PM +0100, Andrew Cooper wrote:
> The XPTI work restricted the visibility of most of memory, but missed a few
> aspects when it came to the TSS.
> 
> Given that the TSS is just an object in percpu data, the 4k mapping for it
> created in setup_cpu_root_pgt() maps adjacent percpu data, making it all
> leakable via Meltdown, even when XPTI is in use.
> 
> Furthermore, no care is taken to check that the TSS doesn't cross a page
> boundary.  As it turns out, struct tss_struct is aligned on its size which
> does prevent it straddling a page boundary, but this will cease to be true
> once CET and Shadow Stack support is added to Xen.
> 
> Move the TSS into the page aligned percpu area, so no adjacent data can be
> leaked.  Move the definition from setup.c to traps.c, which is a more
> appropriate place for it to live.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

On 29/07/2019 09:52, Roger Pau Monné wrote:
> On Fri, Jul 26, 2019 at 09:32:21PM +0100, Andrew Cooper wrote:
>> Future changes are going to need to page align some percpu data.
>>
>> This means that the percpu area needs suitably aligning in the BSS so CPU0 has
>> correctly aligned data.  Shuffle the exact link order of items within the BSS
>> to give .bss.percpu.page_aligned appropriate alignment.
>>
>> In addition, we need to be able to specify an alignment attribute to
>> __DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
>> adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
>> that it is now possible to grep for .bss.percpu and find all the users.
>>
>> Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which uses both section and
>> alignment attributes.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
>> index ff34dc7897..5b6cef04c4 100644
>> --- a/xen/include/asm-x86/percpu.h
>> +++ b/xen/include/asm-x86/percpu.h
>> @@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR_CPUS];
>>  void percpu_init_areas(void);
>>  #endif
>>  
>> -/* Separate out the type, so (int[3], foo) works. */
>> -#define __DEFINE_PER_CPU(type, name, suffix)                    \
>> -    __section(".bss.percpu" #suffix)                            \
>> -    __typeof__(type) per_cpu_##name
>> +#define __DEFINE_PER_CPU(attr, type, name) \
>> +    attr __typeof__(type) per_cpu_ ## name
>>  
>>  /* var is in discarded region: offset to particular copy we want */
>>  #define per_cpu(var, cpu)  \
>> diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
>> index aeec5c19d6..71a31cc361 100644
>> --- a/xen/include/xen/percpu.h
>> +++ b/xen/include/xen/percpu.h
>> @@ -9,9 +9,15 @@
>>   * The _##name concatenation is being used here to prevent 'name' from getting
>>   * macro expanded, while still allowing a per-architecture symbol name prefix.
>>   */
>> -#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
>> +#define DEFINE_PER_CPU(type, name) \
>> +    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
>> +
>> +#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
>> +    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
>> +                     __aligned(PAGE_SIZE), type, _ ## name)
>> +
>>  #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
>> -	__DEFINE_PER_CPU(type, _##name, .read_mostly)
>> +    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)
> AFAICT also adding a '_' here will result in variable names with
> per_cpu__foo, which is inline with the previous behaviour, but I'm not
> sure of the point of the double underscore.

Its double, to avoid token expansion.  See the comment in context at the
top of this hunk.

Without it, the schedulers don't compile because cpumask_scratch is both
the name of a percpu variable, and an alias to
&this_cpu(cpumask_scratch).  Omitting the token concatenation here
causes &this_cpu() to be expanded into __DEFINE_PER_CPU().

~Andrew

P.S. Guess who tried to remove this piece of "fun" to begin with...

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich]

On 26.07.2019 22:32, Andrew Cooper wrote:
> Future changes are going to need to page align some percpu data.
> 
> This means that the percpu area needs suitably aligning in the BSS so CPU0 has
> correctly aligned data.  Shuffle the exact link order of items within the BSS
> to give .bss.percpu.page_aligned appropriate alignment.
> 
> In addition, we need to be able to specify an alignment attribute to
> __DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
> adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
> that it is now possible to grep for .bss.percpu and find all the users.

And it has the meaningful downside of now every use site needing to get
things right. This is not really a problem solely because
__DEFINE_PER_CPU() is a helper for all the real DEFINE_PER_CPU*(). The
grep-ing argument is not a really meaningful one imo anyway - you could
as well grep for DEFINE_PER_CPU.

Anyway - this is not an objection to the chosen approach, just a remark.
I'd like to note though that you explicitly undo something I had done
(iirc), and I may find odd when running into again down the road,
potentially resulting in an "undo-the-undo" patch. I think we really
need to find a way to avoid re-doing things that were done intentionally
in certain ways, when the difference between variants is merely personal
taste.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -293,14 +293,15 @@ SECTIONS
>          __bss_start = .;
>          *(.bss.stack_aligned)
>          *(.bss.page_aligned*)
> -       *(.bss)
> -       . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_start = .;
> +       *(.bss.percpu.page_aligned)

Now this is a case where I think an explicit ALIGN(PAGE_SIZE) would be
desirable: If the last item in .bss.page_aligned was not a multiple of
PAGE_SIZE in size, then __per_cpu_start would live needlessly early,
possibly increasing our memory overhead by a page per CPU for no gain
at all.

>          *(.bss.percpu)
>          . = ALIGN(SMP_CACHE_BYTES);
>          *(.bss.percpu.read_mostly)
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
> +       *(.bss)
> +       . = ALIGN(SMP_CACHE_BYTES);
>          __bss_end = .;

Why is this last ALIGN() needed?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Jan Beulich]

On 26.07.2019 22:32, Andrew Cooper wrote:
> The XPTI work restricted the visibility of most of memory, but missed a few
> aspects when it came to the TSS.

None of these were "missed" afair - we'd been aware, and accepted things
to be the way they are now for the first step. Remember that at the time
XPTI was called "XPTI light", in anticipation for this to just be a
temporary solution.

> Given that the TSS is just an object in percpu data, the 4k mapping for it
> created in setup_cpu_root_pgt() maps adjacent percpu data, making it all
> leakable via Meltdown, even when XPTI is in use.
> 
> Furthermore, no care is taken to check that the TSS doesn't cross a page
> boundary.  As it turns out, struct tss_struct is aligned on its size which
> does prevent it straddling a page boundary, but this will cease to be true
> once CET and Shadow Stack support is added to Xen.

Please can you point me at the CET aspect in documentation here? Aiui
it's only task switches which are affected, and hence only 32-bit TSSes
which would grow (and even then not enough to exceed 128 bytes). For
the purposes 64-bit has there are MSRs to load SSP from.

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
>   #define IOBMP_BYTES             8192
>   #define IOBMP_INVALID_OFFSET    0x8000
>   
> -struct __packed __cacheline_aligned tss_struct {
> +struct __packed tss_struct {
>       uint32_t :32;
>       uint64_t rsp0, rsp1, rsp2;
>       uint64_t :64;
> @@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
>       /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>       uint8_t __cacheline_filler[24];
>   };
> +DECLARE_PER_CPU(struct tss_struct, init_tss);

Taking patch 1 this expands to

     __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
                      __aligned(PAGE_SIZE), struct tss_struct, _init_tss);

and then

     __section(".bss.percpu.page_aligned") __aligned(PAGE_SIZE)
     __typeof__(struct tss_struct) per_cpu__init_tss;

which is not what you want: You have an object of size
sizeof(struct tss_struct) which is PAGE_SIZE aligned. Afaict you
therefore still leak everything that follows in the same page. There
was a reason for __cacheline_aligned's original placement, albeit I
agree that it was/is against the intention of having the struct
define an interface to the hardware (which doesn't have such an
alignment requirement). Perhaps the solution is a two-layer
approach:

struct __aligned(PAGE_SIZE) xen_tss {
     struct __packed tss_struct {
         ...
     };
};

where the inner structure describes the hardware interface and the
containing one our own requirement(s). But personally I also
wouldn't mind putting the __aligned(PAGE_SIZE) right on struct
tss_struct, where __cacheline_aligned has been sitting.

Of course either approach goes against the idea of avoiding usage
mistakes (as pointed out by others in the v1 discussion, iirc):
There better wouldn't be a need to get the two "page aligned"
attributes in sync, i.e. the instantiation of the structure
would better enforce the requested alignment. I've not thought
through whether there's trickery to actually make this work, but
I'd hope we could at the very least detect things not being in
sync at compile time.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

On 29/07/2019 14:17, Jan Beulich wrote:
> On 26.07.2019 22:32, Andrew Cooper wrote:
>> Future changes are going to need to page align some percpu data.
>>
>> This means that the percpu area needs suitably aligning in the BSS so CPU0 has
>> correctly aligned data.  Shuffle the exact link order of items within the BSS
>> to give .bss.percpu.page_aligned appropriate alignment.
>>
>> In addition, we need to be able to specify an alignment attribute to
>> __DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
>> adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
>> that it is now possible to grep for .bss.percpu and find all the users.
> And it has the meaningful downside of now every use site needing to get
> things right.

You say this as if it the current way of doing things is anything more
than an illusion of protection.

>  This is not really a problem solely because
> __DEFINE_PER_CPU() is a helper for all the real DEFINE_PER_CPU*(). The
> grep-ing argument is not a really meaningful one imo anyway - you could
> as well grep for DEFINE_PER_CPU.

And as usual, our points of view differ substantially here.  Looking for
DEFINE_PER_CPU() doesn't help you identify the sections in use.  That
requires a further level of indirection.

> Anyway - this is not an objection to the chosen approach, just a remark.
> I'd like to note though that you explicitly undo something I had done
> (iirc), and I may find odd when running into again down the road,
> potentially resulting in an "undo-the-undo" patch. I think we really
> need to find a way to avoid re-doing things that were done intentionally
> in certain ways, when the difference between variants is merely personal
> taste.

Keir introduced percpu in ea608cc36d when DEFINE_PER_CPU() was private
to x86 and had the __section() implicit in it.

You changed DEFINE_PER_CPU() to include a suffix for the purpose of
introducing DEFINE_PER_CPU_READ_MOSTLY() in cfbf17ffbb0, but nowhere is
there any hint of a suggestion that the end result was anything more
than just "how it happened to turn out".


As to "this being intentional to remove mistakes".  There are plenty of
ways to screw this up, including ways which don't involve using
__DEFINE_PER_CPU(,, "foo"), or manually inserting something into
.bss.per_cpu outside of any of the percpu infrastructure, and no amount
of technical measures can catch this.

The only recourse is sensible code review, and any opencoded use of
__DEFINE_PER_CPU() or __section(".bss.per_cpu" ...) stick out in an
obvious manner.

>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -293,14 +293,15 @@ SECTIONS
>>          __bss_start = .;
>>          *(.bss.stack_aligned)
>>          *(.bss.page_aligned*)
>> -       *(.bss)
>> -       . = ALIGN(SMP_CACHE_BYTES);
>>          __per_cpu_start = .;
>> +       *(.bss.percpu.page_aligned)
> Now this is a case where I think an explicit ALIGN(PAGE_SIZE) would be
> desirable: If the last item in .bss.page_aligned was not a multiple of
> PAGE_SIZE in size, then __per_cpu_start would live needlessly early,
> possibly increasing our memory overhead by a page per CPU for no gain
> at all.

Hmm, yes.  We should do our best to defend against bugs like this.

>
>>          *(.bss.percpu)
>>          . = ALIGN(SMP_CACHE_BYTES);
>>          *(.bss.percpu.read_mostly)
>>          . = ALIGN(SMP_CACHE_BYTES);
>>          __per_cpu_data_end = .;
>> +       *(.bss)
>> +       . = ALIGN(SMP_CACHE_BYTES);
>>          __bss_end = .;
> Why is this last ALIGN() needed?

Try taking it out and the linker will make its feelings known.

Technically, it only needs 8 byte alignment (its so we can use rep stosq
to clear), which is more relaxed than SMP_CACHE_BYTES.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Andrew Cooper]

On 29/07/2019 14:51, Jan Beulich wrote:
> On 26.07.2019 22:32, Andrew Cooper wrote:
>> The XPTI work restricted the visibility of most of memory, but missed a few
>> aspects when it came to the TSS.
> None of these were "missed" afair - we'd been aware, and accepted things
> to be the way they are now for the first step. Remember that at the time
> XPTI was called "XPTI light", in anticipation for this to just be a
> temporary solution.

Did the term "XPTI light" survive past the first RFC posting?

Sure - we did things in an incremental way because it was a technically
complex change and Meltdown was out in the wild at the time.

However, I would have fixed this at the same time as .entry.text if I
had noticed, because the purpose of that series was identical to this
series - avoid leaking things we don't absolutely need to leak.

>> Given that the TSS is just an object in percpu data, the 4k mapping for it
>> created in setup_cpu_root_pgt() maps adjacent percpu data, making it all
>> leakable via Meltdown, even when XPTI is in use.
>>
>> Furthermore, no care is taken to check that the TSS doesn't cross a page
>> boundary.  As it turns out, struct tss_struct is aligned on its size which
>> does prevent it straddling a page boundary, but this will cease to be true
>> once CET and Shadow Stack support is added to Xen.
> Please can you point me at the CET aspect in documentation here? Aiui
> it's only task switches which are affected, and hence only 32-bit TSSes
> which would grow (and even then not enough to exceed 128 bytes). For
> the purposes 64-bit has there are MSRs to load SSP from.

Ah - it was v1 of the CET spec.  I see v3 no longer has the shadow stack
pointer in the TSS.

I'll drop this part of the message.

>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
>>   #define IOBMP_BYTES             8192
>>   #define IOBMP_INVALID_OFFSET    0x8000
>>   
>> -struct __packed __cacheline_aligned tss_struct {
>> +struct __packed tss_struct {
>>       uint32_t :32;
>>       uint64_t rsp0, rsp1, rsp2;
>>       uint64_t :64;
>> @@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
>>       /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>>       uint8_t __cacheline_filler[24];
>>   };
>> +DECLARE_PER_CPU(struct tss_struct, init_tss);
> Taking patch 1 this expands to
>
>      __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
>                       __aligned(PAGE_SIZE), struct tss_struct, _init_tss);
>
> and then
>
>      __section(".bss.percpu.page_aligned") __aligned(PAGE_SIZE)
>      __typeof__(struct tss_struct) per_cpu__init_tss;
>
> which is not what you want: You have an object of size
> sizeof(struct tss_struct) which is PAGE_SIZE aligned. Afaict you
> therefore still leak everything that follows in the same page.

What data might this be?

Every object put into this section is suitably aligned, so nothing will
sit in the slack between the TSS and the end of the page.

> There was a reason for __cacheline_aligned's original placement, albeit I
> agree that it was/is against the intention of having the struct
> define an interface to the hardware (which doesn't have such an
> alignment requirement).

There is a hard requirement to have the first 104 bytes be physically
contiguous, because on a task switch, some CPUs translate the TSS base
and offset directly from there.

I expect that is where the __cacheline_aligned(), being 128, comes in.

However, the manual also makes it clear that this is only on a task
switch, which is inapplicable for us.

Finally, were we to put a structure like this on the stack (e.g. like
hvm_task_switch() does with tss32), we specifically wouldn't want any
unnecessary alignment.

> Perhaps the solution is a two-layer approach:
>
> struct __aligned(PAGE_SIZE) xen_tss {
>      struct __packed tss_struct {
>          ...
>      };
> };
>
> where the inner structure describes the hardware interface and the
> containing one our own requirement(s). But personally I also
> wouldn't mind putting the __aligned(PAGE_SIZE) right on struct
> tss_struct, where __cacheline_aligned has been sitting.

The only way that would make things more robust is if xen_tss was a
union with char[4096] to extend its size.

However, I think this is overkill, given the internals of
DEFINE_PER_CPU_PAGE_ALIGNED()

> Of course either approach goes against the idea of avoiding usage
> mistakes (as pointed out by others in the v1 discussion, iirc):
> There better wouldn't be a need to get the two "page aligned"
> attributes in sync, i.e. the instantiation of the structure
> would better enforce the requested alignment. I've not thought
> through whether there's trickery to actually make this work, but
> I'd hope we could at the very least detect things not being in
> sync at compile time.

There is a reason why I put in a linker assertion for the TSS being
non-aligned.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Andrew Cooper]

On 29/07/2019 16:55, Andrew Cooper wrote:
>>> --- a/xen/include/asm-x86/processor.h
>>> +++ b/xen/include/asm-x86/processor.h
>>> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
>>>   #define IOBMP_BYTES             8192
>>>   #define IOBMP_INVALID_OFFSET    0x8000
>>>   
>>> -struct __packed __cacheline_aligned tss_struct {
>>> +struct __packed tss_struct {
>>>       uint32_t :32;
>>>       uint64_t rsp0, rsp1, rsp2;
>>>       uint64_t :64;
>>> @@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
>>>       /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>>>       uint8_t __cacheline_filler[24];
>>>   };
>>> +DECLARE_PER_CPU(struct tss_struct, init_tss);
>> Taking patch 1 this expands to
>>
>>      __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
>>                       __aligned(PAGE_SIZE), struct tss_struct, _init_tss);
>>
>> and then
>>
>>      __section(".bss.percpu.page_aligned") __aligned(PAGE_SIZE)
>>      __typeof__(struct tss_struct) per_cpu__init_tss;
>>
>> which is not what you want: You have an object of size
>> sizeof(struct tss_struct) which is PAGE_SIZE aligned. Afaict you
>> therefore still leak everything that follows in the same page.
> What data might this be?
>
> Every object put into this section is suitably aligned, so nothing will
> sit in the slack between the TSS and the end of the page.

And of course, I should have actually checked...

ffff82d08092e000 B per_cpu__init_tss
ffff82d08092e000 B __per_cpu_start
ffff82d08092e080 B per_cpu__cpupool
ffff82d08092e088 b per_cpu__continue_info

This needs fixing with suitable alignment in the linker script.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Jan Beulich]

On 29.07.2019 17:55, Andrew Cooper wrote:
> On 29/07/2019 14:51, Jan Beulich wrote:
>> On 26.07.2019 22:32, Andrew Cooper wrote:
>>> --- a/xen/include/asm-x86/processor.h
>>> +++ b/xen/include/asm-x86/processor.h
>>> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
>>>    #define IOBMP_BYTES             8192
>>>    #define IOBMP_INVALID_OFFSET    0x8000
>>>    
>>> -struct __packed __cacheline_aligned tss_struct {
>>> +struct __packed tss_struct {
>>>        uint32_t :32;
>>>        uint64_t rsp0, rsp1, rsp2;
>>>        uint64_t :64;
>>> @@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
>>>        /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>>>        uint8_t __cacheline_filler[24];
>>>    };
>>> +DECLARE_PER_CPU(struct tss_struct, init_tss);
>> Taking patch 1 this expands to
>>
>>       __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
>>                        __aligned(PAGE_SIZE), struct tss_struct, _init_tss);
>>
>> and then
>>
>>       __section(".bss.percpu.page_aligned") __aligned(PAGE_SIZE)
>>       __typeof__(struct tss_struct) per_cpu__init_tss;
>>
>> which is not what you want: You have an object of size
>> sizeof(struct tss_struct) which is PAGE_SIZE aligned. Afaict you
>> therefore still leak everything that follows in the same page.
> 
> What data might this be?

Whatever sits early enough in .bss.percpu. There's no page size
alignment enforced between the two sections, ...

> Every object put into this section is suitably aligned, so nothing will
> sit in the slack between the TSS and the end of the page.

... so for the object being last in .bss.percpu.page_aligned it
is its size that matters, not its alignment.

>> Perhaps the solution is a two-layer approach:
>>
>> struct __aligned(PAGE_SIZE) xen_tss {
>>       struct __packed tss_struct {
>>           ...
>>       };
>> };
>>
>> where the inner structure describes the hardware interface and the
>> containing one our own requirement(s). But personally I also
>> wouldn't mind putting the __aligned(PAGE_SIZE) right on struct
>> tss_struct, where __cacheline_aligned has been sitting.
> 
> The only way that would make things more robust is if xen_tss was a
> union with char[4096] to extend its size.
> 
> However, I think this is overkill, given the internals of
> DEFINE_PER_CPU_PAGE_ALIGNED()
> 
>> Of course either approach goes against the idea of avoiding usage
>> mistakes (as pointed out by others in the v1 discussion, iirc):
>> There better wouldn't be a need to get the two "page aligned"
>> attributes in sync, i.e. the instantiation of the structure
>> would better enforce the requested alignment. I've not thought
>> through whether there's trickery to actually make this work, but
>> I'd hope we could at the very least detect things not being in
>> sync at compile time.
> 
> There is a reason why I put in a linker assertion for the TSS being
> non-aligned.

Hmm, I indeed didn't have that one on my radar when writing the reply.
However, a compile time diagnostic was what I would prefer: Having to
add linker assertions for each and every object we put in this new
section wouldn't really be nice, even if right now we certainly hope
for this to be the only such object.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel