* [patch V3 01/32] mm/slab: Fix broken stack trace storage
[not found] <20190414155936.679808307@linutronix.de>
@ 2019-04-14 15:59 ` Thomas Gleixner
2019-04-14 16:16 ` Andy Lutomirski
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-14 15:59 UTC (permalink / raw)
To: LKML
Cc: x86, Andy Lutomirski, Josh Poimboeuf, Sean Christopherson,
Andrew Morton, Pekka Enberg, linux-mm
kstack_end() is broken on interrupt stacks as they are not guaranteed to be
sized THREAD_SIZE and THREAD_SIZE aligned.
Use the stack tracer instead. Remove the pointless pointer increment at the
end of the function while at it.
Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: linux-mm@kvack.org
---
mm/slab.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1470,33 +1470,29 @@ static bool is_debug_pagealloc_cache(str
static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
unsigned long caller)
{
- int size = cachep->object_size;
+ int size = cachep->object_size / sizeof(unsigned long);
addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
- if (size < 5 * sizeof(unsigned long))
+ if (size < 5)
return;
*addr++ = 0x12345678;
*addr++ = caller;
*addr++ = smp_processor_id();
- size -= 3 * sizeof(unsigned long);
+#ifdef CONFIG_STACKTRACE
{
- unsigned long *sptr = &caller;
- unsigned long svalue;
-
- while (!kstack_end(sptr)) {
- svalue = *sptr++;
- if (kernel_text_address(svalue)) {
- *addr++ = svalue;
- size -= sizeof(unsigned long);
- if (size <= sizeof(unsigned long))
- break;
- }
- }
+ struct stack_trace trace = {
+ .max_entries = size - 4;
+ .entries = addr;
+ .skip = 3;
+ };
+ save_stack_trace(&trace);
+ addr += trace.nr_entries;
}
- *addr++ = 0x87654321;
+#endif
+ *addr = 0x87654321;
}
static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V3 01/32] mm/slab: Fix broken stack trace storage
2019-04-14 15:59 ` [patch V3 01/32] mm/slab: Fix broken stack trace storage Thomas Gleixner
@ 2019-04-14 16:16 ` Andy Lutomirski
2019-04-14 16:34 ` Thomas Gleixner
0 siblings, 1 reply; 15+ messages in thread
From: Andy Lutomirski @ 2019-04-14 16:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, X86 ML, Andy Lutomirski, Josh Poimboeuf,
Sean Christopherson, Andrew Morton, Pekka Enberg, Linux-MM
On Sun, Apr 14, 2019 at 9:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> kstack_end() is broken on interrupt stacks as they are not guaranteed to be
> sized THREAD_SIZE and THREAD_SIZE aligned.
>
> Use the stack tracer instead. Remove the pointless pointer increment at the
> end of the function while at it.
>
> Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: linux-mm@kvack.org
> ---
> mm/slab.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1470,33 +1470,29 @@ static bool is_debug_pagealloc_cache(str
> static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
> unsigned long caller)
> {
> - int size = cachep->object_size;
> + int size = cachep->object_size / sizeof(unsigned long);
>
> addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
>
> - if (size < 5 * sizeof(unsigned long))
> + if (size < 5)
> return;
>
> *addr++ = 0x12345678;
> *addr++ = caller;
> *addr++ = smp_processor_id();
> - size -= 3 * sizeof(unsigned long);
> +#ifdef CONFIG_STACKTRACE
> {
> - unsigned long *sptr = &caller;
> - unsigned long svalue;
> -
> - while (!kstack_end(sptr)) {
> - svalue = *sptr++;
> - if (kernel_text_address(svalue)) {
> - *addr++ = svalue;
> - size -= sizeof(unsigned long);
> - if (size <= sizeof(unsigned long))
> - break;
> - }
> - }
> + struct stack_trace trace = {
> + .max_entries = size - 4;
> + .entries = addr;
> + .skip = 3;
> + };
This looks correct, but I think that it would have been clearer if you
left the size -= 3 above. You're still incrementing addr, but you're
not decrementing size, so they're out of sync and the resulting code
is hard to follow.
--Andy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V3 01/32] mm/slab: Fix broken stack trace storage
2019-04-14 16:16 ` Andy Lutomirski
@ 2019-04-14 16:34 ` Thomas Gleixner
2019-04-15 9:02 ` [patch V4 " Thomas Gleixner
2019-04-15 16:58 ` [patch V3 " Andy Lutomirski
0 siblings, 2 replies; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-14 16:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, X86 ML, Josh Poimboeuf, Sean Christopherson, Andrew Morton,
Pekka Enberg, Linux-MM
On Sun, 14 Apr 2019, Andy Lutomirski wrote:
> > + struct stack_trace trace = {
> > + .max_entries = size - 4;
> > + .entries = addr;
> > + .skip = 3;
> > + };
>
> This looks correct, but I think that it would have been clearer if you
> left the size -= 3 above. You're still incrementing addr, but you're
> not decrementing size, so they're out of sync and the resulting code
> is hard to follow.
What about the below?
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1480,10 +1480,12 @@ static void store_stackinfo(struct kmem_
*addr++ = 0x12345678;
*addr++ = caller;
*addr++ = smp_processor_id();
+ size -= 3;
#ifdef CONFIG_STACKTRACE
{
struct stack_trace trace = {
- .max_entries = size - 4;
+ /* Leave one for the end marker below */
+ .max_entries = size - 1;
.entries = addr;
.skip = 3;
};
^ permalink raw reply [flat|nested] 15+ messages in thread
* [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-14 16:34 ` Thomas Gleixner
@ 2019-04-15 9:02 ` Thomas Gleixner
2019-04-15 13:23 ` Josh Poimboeuf
2019-04-15 16:58 ` [patch V3 " Andy Lutomirski
1 sibling, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-15 9:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, X86 ML, Josh Poimboeuf, Sean Christopherson, Andrew Morton,
Pekka Enberg, Linux-MM
kstack_end() is broken on interrupt stacks as they are not guaranteed to be
sized THREAD_SIZE and THREAD_SIZE aligned.
Use the stack tracer instead. Remove the pointless pointer increment at the
end of the function while at it.
Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: linux-mm@kvack.org
---
V4: Made the code simpler to understand (Andy) and make it actually compile
---
mm/slab.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1470,33 +1470,31 @@ static bool is_debug_pagealloc_cache(str
static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
unsigned long caller)
{
- int size = cachep->object_size;
+ int size = cachep->object_size / sizeof(unsigned long);
addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
- if (size < 5 * sizeof(unsigned long))
+ if (size < 5)
return;
*addr++ = 0x12345678;
*addr++ = caller;
*addr++ = smp_processor_id();
- size -= 3 * sizeof(unsigned long);
+ size -= 3;
+#ifdef CONFIG_STACKTRACE
{
- unsigned long *sptr = &caller;
- unsigned long svalue;
-
- while (!kstack_end(sptr)) {
- svalue = *sptr++;
- if (kernel_text_address(svalue)) {
- *addr++ = svalue;
- size -= sizeof(unsigned long);
- if (size <= sizeof(unsigned long))
- break;
- }
- }
+ struct stack_trace trace = {
+ /* Leave one for the end marker below */
+ .max_entries = size - 1,
+ .entries = addr,
+ .skip = 3,
+ };
+ save_stack_trace(&trace);
+ addr += trace.nr_entries;
}
- *addr++ = 0x87654321;
+#endif
+ *addr = 0x87654321;
}
static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 9:02 ` [patch V4 " Thomas Gleixner
@ 2019-04-15 13:23 ` Josh Poimboeuf
2019-04-15 16:07 ` Thomas Gleixner
0 siblings, 1 reply; 15+ messages in thread
From: Josh Poimboeuf @ 2019-04-15 13:23 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andy Lutomirski, LKML, X86 ML, Sean Christopherson,
Andrew Morton, Pekka Enberg, Linux-MM
On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote:
> kstack_end() is broken on interrupt stacks as they are not guaranteed to be
> sized THREAD_SIZE and THREAD_SIZE aligned.
>
> Use the stack tracer instead. Remove the pointless pointer increment at the
> end of the function while at it.
>
> Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: linux-mm@kvack.org
> ---
> V4: Made the code simpler to understand (Andy) and make it actually compile
> ---
> mm/slab.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1470,33 +1470,31 @@ static bool is_debug_pagealloc_cache(str
> static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
> unsigned long caller)
> {
> - int size = cachep->object_size;
> + int size = cachep->object_size / sizeof(unsigned long);
>
> addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
>
> - if (size < 5 * sizeof(unsigned long))
> + if (size < 5)
> return;
>
> *addr++ = 0x12345678;
> *addr++ = caller;
> *addr++ = smp_processor_id();
> - size -= 3 * sizeof(unsigned long);
> + size -= 3;
> +#ifdef CONFIG_STACKTRACE
> {
> - unsigned long *sptr = &caller;
> - unsigned long svalue;
> -
> - while (!kstack_end(sptr)) {
> - svalue = *sptr++;
> - if (kernel_text_address(svalue)) {
> - *addr++ = svalue;
> - size -= sizeof(unsigned long);
> - if (size <= sizeof(unsigned long))
> - break;
> - }
> - }
> + struct stack_trace trace = {
> + /* Leave one for the end marker below */
> + .max_entries = size - 1,
> + .entries = addr,
> + .skip = 3,
> + };
>
> + save_stack_trace(&trace);
> + addr += trace.nr_entries;
> }
> - *addr++ = 0x87654321;
> +#endif
> + *addr = 0x87654321;
Looks like stack_trace.nr_entries isn't initialized? (though this code
gets eventually replaced by a later patch)
Who actually reads this stack trace? I couldn't find a consumer.
--
Josh
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 13:23 ` Josh Poimboeuf
@ 2019-04-15 16:07 ` Thomas Gleixner
2019-04-15 16:16 ` Josh Poimboeuf
2019-04-15 16:21 ` Peter Zijlstra
0 siblings, 2 replies; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-15 16:07 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andy Lutomirski, LKML, X86 ML, Sean Christopherson,
Andrew Morton, Pekka Enberg, Linux-MM
On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote:
> > addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
> >
> > - if (size < 5 * sizeof(unsigned long))
> > + if (size < 5)
> > return;
> >
> > *addr++ = 0x12345678;
> > *addr++ = caller;
> > *addr++ = smp_processor_id();
> > - size -= 3 * sizeof(unsigned long);
> > + size -= 3;
> > +#ifdef CONFIG_STACKTRACE
> > {
> > - unsigned long *sptr = &caller;
> > - unsigned long svalue;
> > -
> > - while (!kstack_end(sptr)) {
> > - svalue = *sptr++;
> > - if (kernel_text_address(svalue)) {
> > - *addr++ = svalue;
> > - size -= sizeof(unsigned long);
> > - if (size <= sizeof(unsigned long))
> > - break;
> > - }
> > - }
> > + struct stack_trace trace = {
> > + /* Leave one for the end marker below */
> > + .max_entries = size - 1,
> > + .entries = addr,
> > + .skip = 3,
> > + };
> >
> > + save_stack_trace(&trace);
> > + addr += trace.nr_entries;
> > }
> > - *addr++ = 0x87654321;
> > +#endif
> > + *addr = 0x87654321;
>
> Looks like stack_trace.nr_entries isn't initialized? (though this code
> gets eventually replaced by a later patch)
struct initializer initialized the non mentioned fields to 0, if I'm not
totally mistaken.
> Who actually reads this stack trace? I couldn't find a consumer.
It's stored directly in the memory pointed to by @addr and that's the freed
cache memory. If that is used later (UAF) then the stack trace can be
printed to see where it was freed.
Thanks,
tglx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 16:07 ` Thomas Gleixner
@ 2019-04-15 16:16 ` Josh Poimboeuf
2019-04-15 17:05 ` Andy Lutomirski
2019-04-15 21:20 ` [patch V4 01/32] mm/slab: Fix " Thomas Gleixner
2019-04-15 16:21 ` Peter Zijlstra
1 sibling, 2 replies; 15+ messages in thread
From: Josh Poimboeuf @ 2019-04-15 16:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andy Lutomirski, LKML, X86 ML, Sean Christopherson,
Andrew Morton, Pekka Enberg, Linux-MM
On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> > On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote:
> > > addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
> > >
> > > - if (size < 5 * sizeof(unsigned long))
> > > + if (size < 5)
> > > return;
> > >
> > > *addr++ = 0x12345678;
> > > *addr++ = caller;
> > > *addr++ = smp_processor_id();
> > > - size -= 3 * sizeof(unsigned long);
> > > + size -= 3;
> > > +#ifdef CONFIG_STACKTRACE
> > > {
> > > - unsigned long *sptr = &caller;
> > > - unsigned long svalue;
> > > -
> > > - while (!kstack_end(sptr)) {
> > > - svalue = *sptr++;
> > > - if (kernel_text_address(svalue)) {
> > > - *addr++ = svalue;
> > > - size -= sizeof(unsigned long);
> > > - if (size <= sizeof(unsigned long))
> > > - break;
> > > - }
> > > - }
> > > + struct stack_trace trace = {
> > > + /* Leave one for the end marker below */
> > > + .max_entries = size - 1,
> > > + .entries = addr,
> > > + .skip = 3,
> > > + };
> > >
> > > + save_stack_trace(&trace);
> > > + addr += trace.nr_entries;
> > > }
> > > - *addr++ = 0x87654321;
> > > +#endif
> > > + *addr = 0x87654321;
> >
> > Looks like stack_trace.nr_entries isn't initialized? (though this code
> > gets eventually replaced by a later patch)
>
> struct initializer initialized the non mentioned fields to 0, if I'm not
> totally mistaken.
Hm, it seems you are correct. And I thought I knew C.
> > Who actually reads this stack trace? I couldn't find a consumer.
>
> It's stored directly in the memory pointed to by @addr and that's the freed
> cache memory. If that is used later (UAF) then the stack trace can be
> printed to see where it was freed.
Right... but who reads it?
--
Josh
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 16:07 ` Thomas Gleixner
2019-04-15 16:16 ` Josh Poimboeuf
@ 2019-04-15 16:21 ` Peter Zijlstra
1 sibling, 0 replies; 15+ messages in thread
From: Peter Zijlstra @ 2019-04-15 16:21 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Josh Poimboeuf, Andy Lutomirski, LKML, X86 ML,
Sean Christopherson, Andrew Morton, Pekka Enberg, Linux-MM
On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> > > + struct stack_trace trace = {
> > > + /* Leave one for the end marker below */
> > > + .max_entries = size - 1,
> > > + .entries = addr,
> > > + .skip = 3,
> > > + };
> > Looks like stack_trace.nr_entries isn't initialized? (though this code
> > gets eventually replaced by a later patch)
>
> struct initializer initialized the non mentioned fields to 0, if I'm not
> totally mistaken.
Correct.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V3 01/32] mm/slab: Fix broken stack trace storage
2019-04-14 16:34 ` Thomas Gleixner
2019-04-15 9:02 ` [patch V4 " Thomas Gleixner
@ 2019-04-15 16:58 ` Andy Lutomirski
1 sibling, 0 replies; 15+ messages in thread
From: Andy Lutomirski @ 2019-04-15 16:58 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andy Lutomirski, LKML, X86 ML, Josh Poimboeuf,
Sean Christopherson, Andrew Morton, Pekka Enberg, Linux-MM
On Sun, Apr 14, 2019 at 9:34 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, 14 Apr 2019, Andy Lutomirski wrote:
> > > + struct stack_trace trace = {
> > > + .max_entries = size - 4;
> > > + .entries = addr;
> > > + .skip = 3;
> > > + };
> >
> > This looks correct, but I think that it would have been clearer if you
> > left the size -= 3 above. You're still incrementing addr, but you're
> > not decrementing size, so they're out of sync and the resulting code
> > is hard to follow.
>
> What about the below?
>
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1480,10 +1480,12 @@ static void store_stackinfo(struct kmem_
> *addr++ = 0x12345678;
> *addr++ = caller;
> *addr++ = smp_processor_id();
> + size -= 3;
> #ifdef CONFIG_STACKTRACE
> {
> struct stack_trace trace = {
> - .max_entries = size - 4;
> + /* Leave one for the end marker below */
> + .max_entries = size - 1;
> .entries = addr;
> .skip = 3;
> };
Looks good to me.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 16:16 ` Josh Poimboeuf
@ 2019-04-15 17:05 ` Andy Lutomirski
2019-04-15 21:22 ` Thomas Gleixner
2019-04-15 21:20 ` [patch V4 01/32] mm/slab: Fix " Thomas Gleixner
1 sibling, 1 reply; 15+ messages in thread
From: Andy Lutomirski @ 2019-04-15 17:05 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Thomas Gleixner, Andy Lutomirski, LKML, X86 ML,
Sean Christopherson, Andrew Morton, Pekka Enberg, Linux-MM
On Mon, Apr 15, 2019 at 9:17 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> > On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> > > On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote:
> > > > addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
> > > >
> > > > - if (size < 5 * sizeof(unsigned long))
> > > > + if (size < 5)
> > > > return;
> > > >
> > > > *addr++ = 0x12345678;
> > > > *addr++ = caller;
> > > > *addr++ = smp_processor_id();
> > > > - size -= 3 * sizeof(unsigned long);
> > > > + size -= 3;
> > > > +#ifdef CONFIG_STACKTRACE
> > > > {
> > > > - unsigned long *sptr = &caller;
> > > > - unsigned long svalue;
> > > > -
> > > > - while (!kstack_end(sptr)) {
> > > > - svalue = *sptr++;
> > > > - if (kernel_text_address(svalue)) {
> > > > - *addr++ = svalue;
> > > > - size -= sizeof(unsigned long);
> > > > - if (size <= sizeof(unsigned long))
> > > > - break;
> > > > - }
> > > > - }
> > > > + struct stack_trace trace = {
> > > > + /* Leave one for the end marker below */
> > > > + .max_entries = size - 1,
> > > > + .entries = addr,
> > > > + .skip = 3,
> > > > + };
> > > >
> > > > + save_stack_trace(&trace);
> > > > + addr += trace.nr_entries;
> > > > }
> > > > - *addr++ = 0x87654321;
> > > > +#endif
> > > > + *addr = 0x87654321;
> > >
> > > Looks like stack_trace.nr_entries isn't initialized? (though this code
> > > gets eventually replaced by a later patch)
> >
> > struct initializer initialized the non mentioned fields to 0, if I'm not
> > totally mistaken.
>
> Hm, it seems you are correct. And I thought I knew C.
>
> > > Who actually reads this stack trace? I couldn't find a consumer.
> >
> > It's stored directly in the memory pointed to by @addr and that's the freed
> > cache memory. If that is used later (UAF) then the stack trace can be
> > printed to see where it was freed.
>
> Right... but who reads it?
That seems like a reasonable question. After some grepping and some
git searching, it looks like there might not be any users. I found
SLAB_STORE_USER, but that seems to be independent.
So maybe the whole mess should just be deleted. If anyone ever
notices, they can re-add it better.
--Andy
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 16:16 ` Josh Poimboeuf
2019-04-15 17:05 ` Andy Lutomirski
@ 2019-04-15 21:20 ` Thomas Gleixner
1 sibling, 0 replies; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-15 21:20 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Andy Lutomirski, LKML, X86 ML, Sean Christopherson,
Andrew Morton, Pekka Enberg, Linux-MM
On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> > >
> > > Looks like stack_trace.nr_entries isn't initialized? (though this code
> > > gets eventually replaced by a later patch)
> >
> > struct initializer initialized the non mentioned fields to 0, if I'm not
> > totally mistaken.
>
> Hm, it seems you are correct. And I thought I knew C.
:)
> > > Who actually reads this stack trace? I couldn't find a consumer.
> >
> > It's stored directly in the memory pointed to by @addr and that's the freed
> > cache memory. If that is used later (UAF) then the stack trace can be
> > printed to see where it was freed.
>
> Right... but who reads it?
Indeed. I didn't check but I know that I saw that info printed at least a
decade ago. Looks like that debug magic in slab.c has seen major changes
since then.
Thanks,
tglx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 17:05 ` Andy Lutomirski
@ 2019-04-15 21:22 ` Thomas Gleixner
2019-04-16 11:37 ` Vlastimil Babka
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-15 21:22 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Josh Poimboeuf, LKML, X86 ML, Sean Christopherson, Andrew Morton,
Pekka Enberg, Linux-MM
On Mon, 15 Apr 2019, Andy Lutomirski wrote:
> On Mon, Apr 15, 2019 at 9:17 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> > > > Looks like stack_trace.nr_entries isn't initialized? (though this code
> > > > gets eventually replaced by a later patch)
> > >
> > > struct initializer initialized the non mentioned fields to 0, if I'm not
> > > totally mistaken.
> >
> > Hm, it seems you are correct. And I thought I knew C.
> >
> > > > Who actually reads this stack trace? I couldn't find a consumer.
> > >
> > > It's stored directly in the memory pointed to by @addr and that's the freed
> > > cache memory. If that is used later (UAF) then the stack trace can be
> > > printed to see where it was freed.
> >
> > Right... but who reads it?
>
> That seems like a reasonable question. After some grepping and some
> git searching, it looks like there might not be any users. I found
Anymore. There was something 10y+ ago...
> SLAB_STORE_USER, but that seems to be independent.
>
> So maybe the whole mess should just be deleted. If anyone ever
> notices, they can re-add it better.
No objections from my side, but the mm people might have opinions.
Thanks,
tglx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage
2019-04-15 21:22 ` Thomas Gleixner
@ 2019-04-16 11:37 ` Vlastimil Babka
2019-04-16 14:10 ` [patch V5 01/32] mm/slab: Remove " Thomas Gleixner
0 siblings, 1 reply; 15+ messages in thread
From: Vlastimil Babka @ 2019-04-16 11:37 UTC (permalink / raw)
To: Thomas Gleixner, Andy Lutomirski
Cc: Josh Poimboeuf, LKML, X86 ML, Sean Christopherson, Andrew Morton,
Pekka Enberg, Linux-MM, David Rientjes
On 4/15/19 11:22 PM, Thomas Gleixner wrote:
> On Mon, 15 Apr 2019, Andy Lutomirski wrote:
>> On Mon, Apr 15, 2019 at 9:17 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>> On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
>>>>> Looks like stack_trace.nr_entries isn't initialized? (though this code
>>>>> gets eventually replaced by a later patch)
>>>>
>>>> struct initializer initialized the non mentioned fields to 0, if I'm not
>>>> totally mistaken.
>>>
>>> Hm, it seems you are correct. And I thought I knew C.
>>>
>>>>> Who actually reads this stack trace? I couldn't find a consumer.
>>>>
>>>> It's stored directly in the memory pointed to by @addr and that's the freed
>>>> cache memory. If that is used later (UAF) then the stack trace can be
>>>> printed to see where it was freed.
>>>
>>> Right... but who reads it?
>>
>> That seems like a reasonable question. After some grepping and some
>> git searching, it looks like there might not be any users. I found
>
> Anymore. There was something 10y+ ago.
In theory it can be useful in a crash dump. But I don't see any related
debugging
check that would trigger a panic, in order to get one.
>> SLAB_STORE_USER, but that seems to be independent.
>>
>> So maybe the whole mess should just be deleted. If anyone ever
>> notices, they can re-add it better.
>
> No objections from my side, but the mm people might have opinions.
Anyone who wants to debug wrong slab usage probably uses SLUB anyway, so
I don't think it's a problem to remove broken SLAB debugging. Perhaps
even SLAB itself will be removed soon if there's performance data
supporting it [1].
[1]
https://lore.kernel.org/linux-mm/20190412112816.GD18914@techsingularity.net/T/#u
> Thanks,
>
> tglx
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [patch V5 01/32] mm/slab: Remove broken stack trace storage
2019-04-16 11:37 ` Vlastimil Babka
@ 2019-04-16 14:10 ` Thomas Gleixner
2019-04-16 15:16 ` Vlastimil Babka
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2019-04-16 14:10 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Andy Lutomirski, Josh Poimboeuf, LKML, X86 ML,
Sean Christopherson, Andrew Morton, Pekka Enberg, Linux-MM,
David Rientjes
kstack_end() is broken on interrupt stacks as they are not guaranteed to be
sized THREAD_SIZE and THREAD_SIZE aligned.
As SLAB seems not to be used much with debugging enabled and might just go
away completely according to:
https://lkml.kernel.org/r/612f9b99-a75b-6aeb-cf92-7dc5421cd950@suse.cz
just remove the bogus code instead of trying to fix it.
Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: linux-mm@kvack.org
---
V5: Remove the cruft.
V4: Make it actually work
V2: Made the code simpler to understand (Andy)
---
mm/slab.c | 22 +++-------------------
1 file changed, 3 insertions(+), 19 deletions(-)
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1470,33 +1470,17 @@ static bool is_debug_pagealloc_cache(str
static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
unsigned long caller)
{
- int size = cachep->object_size;
+ int size = cachep->object_size / sizeof(unsigned long);
addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
- if (size < 5 * sizeof(unsigned long))
+ if (size < 4)
return;
*addr++ = 0x12345678;
*addr++ = caller;
*addr++ = smp_processor_id();
- size -= 3 * sizeof(unsigned long);
- {
- unsigned long *sptr = &caller;
- unsigned long svalue;
-
- while (!kstack_end(sptr)) {
- svalue = *sptr++;
- if (kernel_text_address(svalue)) {
- *addr++ = svalue;
- size -= sizeof(unsigned long);
- if (size <= sizeof(unsigned long))
- break;
- }
- }
-
- }
- *addr++ = 0x87654321;
+ *addr = 0x87654321;
}
static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [patch V5 01/32] mm/slab: Remove broken stack trace storage
2019-04-16 14:10 ` [patch V5 01/32] mm/slab: Remove " Thomas Gleixner
@ 2019-04-16 15:16 ` Vlastimil Babka
0 siblings, 0 replies; 15+ messages in thread
From: Vlastimil Babka @ 2019-04-16 15:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andy Lutomirski, Josh Poimboeuf, LKML, X86 ML,
Sean Christopherson, Andrew Morton, Pekka Enberg, Linux-MM,
David Rientjes
On 4/16/19 4:10 PM, Thomas Gleixner wrote:
> kstack_end() is broken on interrupt stacks as they are not guaranteed to be
> sized THREAD_SIZE and THREAD_SIZE aligned.
>
> As SLAB seems not to be used much with debugging enabled and might just go
> away completely according to:
>
> https://lkml.kernel.org/r/612f9b99-a75b-6aeb-cf92-7dc5421cd950@suse.cz
>
> just remove the bogus code instead of trying to fix it.
>
> Fixes: 98eb235b7feb ("[PATCH] page unmapping debug") - History tree
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: linux-mm@kvack.org
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Thanks.
> ---
> V5: Remove the cruft.
> V4: Make it actually work
> V2: Made the code simpler to understand (Andy)
> ---
> mm/slab.c | 22 +++-------------------
> 1 file changed, 3 insertions(+), 19 deletions(-)
>
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -1470,33 +1470,17 @@ static bool is_debug_pagealloc_cache(str
> static void store_stackinfo(struct kmem_cache *cachep, unsigned long *addr,
> unsigned long caller)
> {
> - int size = cachep->object_size;
> + int size = cachep->object_size / sizeof(unsigned long);
>
> addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
>
> - if (size < 5 * sizeof(unsigned long))
> + if (size < 4)
> return;
>
> *addr++ = 0x12345678;
> *addr++ = caller;
> *addr++ = smp_processor_id();
> - size -= 3 * sizeof(unsigned long);
> - {
> - unsigned long *sptr = &caller;
> - unsigned long svalue;
> -
> - while (!kstack_end(sptr)) {
> - svalue = *sptr++;
> - if (kernel_text_address(svalue)) {
> - *addr++ = svalue;
> - size -= sizeof(unsigned long);
> - if (size <= sizeof(unsigned long))
> - break;
> - }
> - }
> -
> - }
> - *addr++ = 0x87654321;
> + *addr = 0x87654321;
> }
>
> static void slab_kernel_map(struct kmem_cache *cachep, void *objp,
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-04-16 15:19 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190414155936.679808307@linutronix.de>
2019-04-14 15:59 ` [patch V3 01/32] mm/slab: Fix broken stack trace storage Thomas Gleixner
2019-04-14 16:16 ` Andy Lutomirski
2019-04-14 16:34 ` Thomas Gleixner
2019-04-15 9:02 ` [patch V4 " Thomas Gleixner
2019-04-15 13:23 ` Josh Poimboeuf
2019-04-15 16:07 ` Thomas Gleixner
2019-04-15 16:16 ` Josh Poimboeuf
2019-04-15 17:05 ` Andy Lutomirski
2019-04-15 21:22 ` Thomas Gleixner
2019-04-16 11:37 ` Vlastimil Babka
2019-04-16 14:10 ` [patch V5 01/32] mm/slab: Remove " Thomas Gleixner
2019-04-16 15:16 ` Vlastimil Babka
2019-04-15 21:20 ` [patch V4 01/32] mm/slab: Fix " Thomas Gleixner
2019-04-15 16:21 ` Peter Zijlstra
2019-04-15 16:58 ` [patch V3 " Andy Lutomirski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).