It was found that under some .config options (notably L1_CACHE_SHIFT=7)
and compiler combinations this on-stack alignment leads to a 320 byte
stack usage, which then triggers a KASAN stack warning elsewhere.
Using 320 bytes of stack space for a 40 byte structure is ludicrous and
clearly not right.
Fixes: 515ab7c41306 ("x86/mm: Align TLB invalidation info")
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
Index: linux-2.6/arch/x86/mm/tlb.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/tlb.c
+++ linux-2.6/arch/x86/mm/tlb.c
@@ -728,7 +728,7 @@ void flush_tlb_mm_range(struct mm_struct
{
int cpu;
- struct flush_tlb_info info __aligned(SMP_CACHE_BYTES) = {
+ struct flush_tlb_info info = {
.mm = mm,
.stride_shift = stride_shift,
.freed_tables = freed_tables,
Commit-ID: 780e0106d468a2962b16b52fdf42898f2639e0a0
Gitweb: https://git.kernel.org/tip/780e0106d468a2962b16b52fdf42898f2639e0a0
Author: Peter Zijlstra <[email protected]>
AuthorDate: Tue, 16 Apr 2019 10:03:35 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 16 Apr 2019 10:10:13 +0200
x86/mm/tlb: Revert "x86/mm: Align TLB invalidation info"
Revert the following commit:
515ab7c41306: ("x86/mm: Align TLB invalidation info")
I found out (the hard way) that under some .config options (notably L1_CACHE_SHIFT=7)
and compiler combinations this on-stack alignment leads to a 320 byte
stack usage, which then triggers a KASAN stack warning elsewhere.
Using 320 bytes of stack space for a 40 byte structure is ludicrous and
clearly not right.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Linus Torvalds <[email protected]>
Acked-by: Nadav Amit <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Fixes: 515ab7c41306 ("x86/mm: Align TLB invalidation info")
Link: http://lkml.kernel.org/r/[email protected]
[ Minor changelog edits. ]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/mm/tlb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index bc4bc7b2f075..487b8474c01c 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -728,7 +728,7 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
{
int cpu;
- struct flush_tlb_info info __aligned(SMP_CACHE_BYTES) = {
+ struct flush_tlb_info info = {
.mm = mm,
.stride_shift = stride_shift,
.freed_tables = freed_tables,
On Tue, Apr 16, 2019 at 1:03 AM Peter Zijlstra <[email protected]> wrote:
>
> Using 320 bytes of stack space for a 40 byte structure is ludicrous and
> clearly not right.
Ack.
That said, I wish we didn't have these stack structures at all. Or at
least were more careful about them. For example, another case of this
struct on the stack looks really iffy too:
struct flush_tlb_info info;
info.start = start;
info.end = end;
on_each_cpu(do_kernel_range_flush, &info, 1);
note how it only initializes two of the fields, and leaves the others
entirely randomly initialized with garbage?
Yeah, yeah, "do_kernel_range_flush()" only uses those two fields, but
it still makes my skin crawl how we basically pass a largely
uninitialized structure and have other CPU's look at it.
And in another case we do have a nicely initialized structure
void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
{
struct flush_tlb_info info = {
.mm = NULL,
.start = 0UL,
.end = TLB_FLUSH_ALL,
};
but it looks like it shouldn't have been on the stack in the first
place, because as far as I can tell it's entirely constant, and it
should just be a "static const" structure initialized at compile time.
So as far as I can tell, we could do something like
-static void flush_tlb_func_local(void *info, enum tlb_flush_reason reason)
+static void flush_tlb_func_local(const void *info, enum
tlb_flush_reason reason)
- struct flush_tlb_info info = {
+ static const struct flush_tlb_info info = {
for that case.
End result: it looks like we have three of these stack things, and all
three had something odd in them.
So very much Ack on that patch, but maybe we could do a bit more cleanup here?
Linus
On Tue, Apr 16, 2019 at 10:45:05AM -0700, Linus Torvalds wrote:
> So very much Ack on that patch, but maybe we could do a bit more cleanup here?
Yeah, Nadav was going to try and clean that up. But I figured we should
get this revert in and backported while it's hot :-)
> On Apr 16, 2019, at 11:28 AM, Peter Zijlstra <[email protected]> wrote:
>
> On Tue, Apr 16, 2019 at 10:45:05AM -0700, Linus Torvalds wrote:
>> So very much Ack on that patch, but maybe we could do a bit more cleanup here?
>
> Yeah, Nadav was going to try and clean that up. But I figured we should
> get this revert in and backported while it's hot :-)
I will get to it hopefully next week. I need to do some benchmarking to see
the impact of getting it off the stack, although usually the IPI itself
dominates the TLB shootdown performance overhead.