lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 29 Jun 2018 17:00:51 -0700 From: Fenghua Yu <fenghua.yu@...el.com> To: Thomas Gleixner <tglx@...utronix.de> Cc: Dave Hansen <dave.hansen@...el.com>, Fenghua Yu <fenghua.yu@...el.com>, Ingo Molnar <mingo@...hat.com>, H Peter Anvin <hpa@...or.com>, Ashok Raj <ashok.raj@...el.com>, Alan Cox <alan@...ux.intel.com>, Peter Zijlstra <peterz@...radead.org>, Rafael Wysocki <rafael.j.wysocki@...el.com>, Tony Luck <tony.luck@...el.com>, Ravi V Shankar <ravi.v.shankar@...el.com>, linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org> Subject: Re: [PATCH v2 2/4] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access On Fri, Jun 29, 2018 at 11:44:44PM +0200, Thomas Gleixner wrote: > On Fri, 29 Jun 2018, Dave Hansen wrote: > > > On 06/29/2018 01:38 PM, Fenghua Yu wrote: > > > How to handle data that is used in generic code which can be used on > > > non-Intel platform? For exmple, if I do this change for struct efi in > > > include/linux/efi.h because set_bit() sets bits in efi.flags: > > > - unsigned long flags; > > > + unsigned long flags __aligned(unsigned long); > > > } efi; > > > > > > People may argue that the alignment unnecessarily increases size of 'efi' > > > on non-Intel platform which doesn't have split lock issue. Do we care this > > > argument? > > > > Unaligned memory accesses are bad, pretty much universally. This is a > > general good practice that we should have been doing anyway. Let folks > > complain. Don't let it stop you. > > > > Also, look at the size of that structure. Look at how many pointers it > > has. Do you think *anyone* is going to complain about an extra 4 bytes > > in a 400-byte structure? > > But in the above case the compiler does already the right thing. Why? > Because struct members are aligned to their natural alignment unless the > struct is explicitely marked 'packed'. In that case the programmer has to > take care of the alignment. > > Just look at it with pahole: > > struct efi_memory_map memmap; /* 280 56 */ > > /* XXX last struct has 7 bytes of padding */ > > /* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */ > long unsigned int flags; /* 336 8 */ > > The issue with the capability arrays is that the data type is u32 which has > the natural alignment of 4 byte, while unsigned long has 8 byte on 64bit. > > So just slapping blindly aligned(unsigned long) to anything which is > accessed by locked instructions is pointless. > Thank you for you education! Below is part of the future patches that are supposed to fix more potential split lock issues. Could you please take a look and see if the changes are in the right direction before I move further? diff --git a/arch/x86/boot/cpuflags.h b/arch/x86/boot/cpuflags.h index 2e20814d3ce3..ca62c3784f9a 100644 --- a/arch/x86/boot/cpuflags.h +++ b/arch/x86/boot/cpuflags.h @@ -9,7 +9,7 @@ struct cpu_features { int level; /* Family, or 64 for x86-64 */ int family; /* Family, always */ int model; - u32 flags[NCAPINTS]; + u32 flags[NCAPINTS] __aligned(unsigned long); }; extern struct cpu_features cpu; diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index 8c7b3e5a2d01..24eac32b039d 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -133,7 +133,7 @@ struct mce_log_buffer { char signature[12]; /* "MACHINECHECK" */ unsigned len; /* = MCE_LOG_LEN */ unsigned next; - unsigned flags; + unsigned flags __aligned(unsigned long); unsigned recordlen; /* length of struct mce */ struct mce entry[MCE_LOG_LEN]; }; diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index eb4cb3efd20e..fe681c695638 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -488,8 +488,8 @@ static const char *table_lookup_model(struct cpuinfo_x86 *c) return NULL; /* Not found */ } -__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS]; -__u32 cpu_caps_set[NCAPINTS + NBUGINTS]; +__u32 cpu_caps_cleared[NCAPINTS + NBUGINTS] __aligned(unsigned long); +__u32 cpu_caps_set[NCAPINTS + NBUGINTS] __aligned(unsigned long); void load_percpu_segment(int cpu) { diff --git a/include/linux/efi.h b/include/linux/efi.h index 56add823f190..e1a3c17945b5 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -963,7 +963,7 @@ extern struct efi { efi_reset_system_t *reset_system; efi_set_virtual_address_map_t *set_virtual_address_map; struct efi_memory_map memmap; - unsigned long flags; + unsigned long flags __aligned(unsigned long); } efi; extern struct mm_struct efi_mm; diff --git a/include/linux/fs.h b/include/linux/fs.h index 5c91108846db..30b1f173d3ca 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -885,7 +885,7 @@ struct file { void *f_security; #endif /* needed for tty driver, and maybe others */ - void *private_data; + void *private_data __aligned(unsigned long); #ifdef CONFIG_EPOLL /* Used by fs/eventpoll.c to link all the hooks to this file */
Powered by blists - more mailing lists