[sl-b,1/6] mm: Add kmem_last_alloc() to return last allocation for memory block

Commit Message

Paul E. McKenney Dec. 5, 2020, 12:40 a.m. UTC

From: "Paul E. McKenney" <[e-mail]>

There are kernel facilities such as per-CPU reference counts that give
error messages in generic handlers or callbacks, whose messages are
unenlightening.  In the case of per-CPU reference-count underflow, this
is not a problem when creating a new use of this facility because in that
case the bug is almost certainly in the code implementing that new use.
However, trouble arises when deploying across many systems, which might
exercise corner cases that were not seen during development and testing.
Here, it would be really nice to get some kind of hint as to which of
several uses the underflow was caused by.

This commit therefore exposes a new kmem_last_alloc() function that
takes a pointer to dynamically allocated memory and returns the return
address of the call that allocated it.  This pointer can reference the
middle of the block as well as the beginning of the block, as needed
by things like RCU callback functions and timer handlers that might not
know where the beginning of the memory block is.  These functions and
handlers can use the return value from kmem_last_alloc() to give the
kernel hacker a better hint as to where the problem might lie.

This kmem_last_alloc() function returns NULL for slob and when the
necessary debug has not been enabled for slab and slub.  For slub,
build with CONFIG_SLUB_DEBUG=y and boot with slub_debug=U, or pass
SLAB_STORE_USER to kmem_cache_create() if more focused use is desired.

Cc: Christoph Lameter <[e-mail]>
Cc: Pekka Enberg <[e-mail]>
Cc: David Rientjes <[e-mail]>
Cc: Joonsoo Kim <[e-mail]>
Cc: Andrew Morton <[e-mail]>
Cc: <[e-mail]>
Reported-by: Andrii Nakryiko <[e-mail]>
Signed-off-by: Paul E. McKenney <[e-mail]>
---
 include/linux/slab.h |  2 ++
 mm/slab.c            | 19 +++++++++++++++++++
 mm/slab_common.c     | 20 ++++++++++++++++++++
 mm/slob.c            |  5 +++++
 mm/slub.c            | 26 ++++++++++++++++++++++++++
 5 files changed, 72 insertions(+)

Comments

Joonsoo Kim Dec. 7, 2020, 9:02 a.m. UTC | #1

Hello, Paul.

On Fri, Dec 04, 2020 at 04:40:52PM -0800, [e-mail] wrote:
> From: "Paul E. McKenney" <[e-mail]>
> 
> There are kernel facilities such as per-CPU reference counts that give
> error messages in generic handlers or callbacks, whose messages are
> unenlightening.  In the case of per-CPU reference-count underflow, this
> is not a problem when creating a new use of this facility because in that
> case the bug is almost certainly in the code implementing that new use.
> However, trouble arises when deploying across many systems, which might
> exercise corner cases that were not seen during development and testing.
> Here, it would be really nice to get some kind of hint as to which of
> several uses the underflow was caused by.
> 
> This commit therefore exposes a new kmem_last_alloc() function that
> takes a pointer to dynamically allocated memory and returns the return
> address of the call that allocated it.  This pointer can reference the
> middle of the block as well as the beginning of the block, as needed
> by things like RCU callback functions and timer handlers that might not
> know where the beginning of the memory block is.  These functions and
> handlers can use the return value from kmem_last_alloc() to give the
> kernel hacker a better hint as to where the problem might lie.

I agree with exposing allocation caller information to the other
subsystem to help the debugging. Some suggestions...

1. It's better to separate a slab object check (validity check) and
retrieving the allocation caller. Someone else would want to check
only a validity. And, it doesn't depend on the debug configuration so
it's not good to bind it to the debug function.

kmem_cache_valid_(obj|ptr)
kmalloc_valid_(obj|ptr)

2. rename kmem_last_alloc to ...

int kmem_cache_debug_alloc_caller(cache, obj, &ret_addr)
int kmalloc_debug_alloc_caller(obj, &ret_addr)

or debug_kmem_cache_alloc_caller()

I think that function name need to include the keyword 'debug' to show
itself as a debugging facility (enabled at the debugging). And, return
errno and get caller address by pointer argument.

3. If concrete error message is needed, please introduce more functions.

void *kmalloc_debug_error(errno)

Thanks.

Paul E. McKenney Dec. 7, 2020, 5:25 p.m. UTC | #2

On Mon, Dec 07, 2020 at 06:02:53PM +0900, Joonsoo Kim wrote:
> Hello, Paul.
> 
> On Fri, Dec 04, 2020 at 04:40:52PM -0800, [e-mail] wrote:
> > From: "Paul E. McKenney" <[e-mail]>
> > 
> > There are kernel facilities such as per-CPU reference counts that give
> > error messages in generic handlers or callbacks, whose messages are
> > unenlightening.  In the case of per-CPU reference-count underflow, this
> > is not a problem when creating a new use of this facility because in that
> > case the bug is almost certainly in the code implementing that new use.
> > However, trouble arises when deploying across many systems, which might
> > exercise corner cases that were not seen during development and testing.
> > Here, it would be really nice to get some kind of hint as to which of
> > several uses the underflow was caused by.
> > 
> > This commit therefore exposes a new kmem_last_alloc() function that
> > takes a pointer to dynamically allocated memory and returns the return
> > address of the call that allocated it.  This pointer can reference the
> > middle of the block as well as the beginning of the block, as needed
> > by things like RCU callback functions and timer handlers that might not
> > know where the beginning of the memory block is.  These functions and
> > handlers can use the return value from kmem_last_alloc() to give the
> > kernel hacker a better hint as to where the problem might lie.
> 
> I agree with exposing allocation caller information to the other
> subsystem to help the debugging. Some suggestions...

Good to hear!  ;-)

> 1. It's better to separate a slab object check (validity check) and
> retrieving the allocation caller. Someone else would want to check
> only a validity. And, it doesn't depend on the debug configuration so
> it's not good to bind it to the debug function.
> 
> kmem_cache_valid_(obj|ptr)
> kmalloc_valid_(obj|ptr)

Here both functions would say "true" for a pointer from kmalloc()?
Or do I need to add a third function that is happy with a pointer from
either source?

I do understand that people who don't want to distinguish could just do
"kmem_cache_valid_ptr(p) || kmalloc_valid_ptr(p)".  However, the two
use cases in the series have no idea whether the pointer they have came
from kmalloc(), kmem_cache_alloc(), or somewhere else entirely, even an
on-stack variable.

Are you asking me to choose between the _obj() and _ptr() suffixes?
If not, please help me understand the distinction.

Do we want "debug" in these names as well?

> 2. rename kmem_last_alloc to ...
> 
> int kmem_cache_debug_alloc_caller(cache, obj, &ret_addr)
> int kmalloc_debug_alloc_caller(obj, &ret_addr)
> 
> or debug_kmem_cache_alloc_caller()
> 
> I think that function name need to include the keyword 'debug' to show
> itself as a debugging facility (enabled at the debugging). And, return
> errno and get caller address by pointer argument.

I am quite happy to add the "debug", but my use cases have no idea
how the pointer was allocated.  In fact, the next version of the
patch will also handle allocator return addresses from vmalloc().

And for kernels without sufficient debug enabled, I need to provide
the name of the slab cache, and this also is to be in the next version.

> 3. If concrete error message is needed, please introduce more functions.
> 
> void *kmalloc_debug_error(errno)

Agreed, in fact, I was planning to have a function that printed out
a suitable error-message continuation to the console for ease-of-use
reasons.  For example, why is the caller deciding how deep the stack
frame is?  ;-)

So something like this?

	void kmalloc_debug_print_provenance(void *ptr);

With the understanding that it will print something helpful regardless
of where ptr came from, within the constraints of the kernel build and
boot options?

							Thanx, Paul

Joonsoo Kim Dec. 8, 2020, 8:57 a.m. UTC | #3

On Mon, Dec 07, 2020 at 09:25:54AM -0800, Paul E. McKenney wrote:
> On Mon, Dec 07, 2020 at 06:02:53PM +0900, Joonsoo Kim wrote:
> > Hello, Paul.
> > 
> > On Fri, Dec 04, 2020 at 04:40:52PM -0800, [e-mail] wrote:
> > > From: "Paul E. McKenney" <[e-mail]>
> > > 
> > > There are kernel facilities such as per-CPU reference counts that give
> > > error messages in generic handlers or callbacks, whose messages are
> > > unenlightening.  In the case of per-CPU reference-count underflow, this
> > > is not a problem when creating a new use of this facility because in that
> > > case the bug is almost certainly in the code implementing that new use.
> > > However, trouble arises when deploying across many systems, which might
> > > exercise corner cases that were not seen during development and testing.
> > > Here, it would be really nice to get some kind of hint as to which of
> > > several uses the underflow was caused by.
> > > 
> > > This commit therefore exposes a new kmem_last_alloc() function that
> > > takes a pointer to dynamically allocated memory and returns the return
> > > address of the call that allocated it.  This pointer can reference the
> > > middle of the block as well as the beginning of the block, as needed
> > > by things like RCU callback functions and timer handlers that might not
> > > know where the beginning of the memory block is.  These functions and
> > > handlers can use the return value from kmem_last_alloc() to give the
> > > kernel hacker a better hint as to where the problem might lie.
> > 
> > I agree with exposing allocation caller information to the other
> > subsystem to help the debugging. Some suggestions...
> 
> Good to hear!  ;-)
> 
> > 1. It's better to separate a slab object check (validity check) and
> > retrieving the allocation caller. Someone else would want to check
> > only a validity. And, it doesn't depend on the debug configuration so
> > it's not good to bind it to the debug function.
> > 
> > kmem_cache_valid_(obj|ptr)
> > kmalloc_valid_(obj|ptr)
> 
> Here both functions would say "true" for a pointer from kmalloc()?
> Or do I need to add a third function that is happy with a pointer from
> either source?

I focused on separation and missed this case that the user sometimes
cannot know the object source (kmalloc/kmem_cache). At first step,
just checking whether it is a slab-object or not looks enough.

int kmem_valid_obj()

> 
> I do understand that people who don't want to distinguish could just do
> "kmem_cache_valid_ptr(p) || kmalloc_valid_ptr(p)".  However, the two
> use cases in the series have no idea whether the pointer they have came
> from kmalloc(), kmem_cache_alloc(), or somewhere else entirely, even an
> on-stack variable.
> 
> Are you asking me to choose between the _obj() and _ptr() suffixes?

Yes, I prefer _obj().

> If not, please help me understand the distinction.
> 
> Do we want "debug" in these names as well?

I don't think so since it can be called without enabling the debug
option.

> 
> > 2. rename kmem_last_alloc to ...
> > 
> > int kmem_cache_debug_alloc_caller(cache, obj, &ret_addr)
> > int kmalloc_debug_alloc_caller(obj, &ret_addr)
> > 
> > or debug_kmem_cache_alloc_caller()
> > 
> > I think that function name need to include the keyword 'debug' to show
> > itself as a debugging facility (enabled at the debugging). And, return
> > errno and get caller address by pointer argument.
> 
> I am quite happy to add the "debug", but my use cases have no idea
> how the pointer was allocated.  In fact, the next version of the
> patch will also handle allocator return addresses from vmalloc().
> 
> And for kernels without sufficient debug enabled, I need to provide
> the name of the slab cache, and this also is to be in the next version.

Okay. So, your code would be...

if (kmem_valid_obj(ptr))
        kmalloc_debug_print_provenance(ptr)
else if (vmalloc_valid_obj(ptr))
        ....

> > 3. If concrete error message is needed, please introduce more functions.
> > 
> > void *kmalloc_debug_error(errno)
> 
> Agreed, in fact, I was planning to have a function that printed out
> a suitable error-message continuation to the console for ease-of-use
> reasons.  For example, why is the caller deciding how deep the stack
> frame is?  ;-)
> 
> So something like this?
> 
> 	void kmalloc_debug_print_provenance(void *ptr);
> 
> With the understanding that it will print something helpful regardless
> of where ptr came from, within the constraints of the kernel build and
> boot options?

Looks good idea. I suggest a name, kmem_dump_obj(), for this function.
In this case, I don't think that "debug" keyword is needed since it shows
something useful (slab cache info) even if debug option isn't enabled.

So, for summary, we need to introduce two functions to accomplish your
purpose. Please correct me if wrong.

int kmem_valid_obj(ptr)
void kmem_dump_obj(ptr)

Thanks.

Paul E. McKenney Dec. 8, 2020, 3:17 p.m. UTC | #4

On Tue, Dec 08, 2020 at 05:57:07PM +0900, Joonsoo Kim wrote:
> On Mon, Dec 07, 2020 at 09:25:54AM -0800, Paul E. McKenney wrote:
> > On Mon, Dec 07, 2020 at 06:02:53PM +0900, Joonsoo Kim wrote:
> > > Hello, Paul.
> > > 
> > > On Fri, Dec 04, 2020 at 04:40:52PM -0800, [e-mail] wrote:
> > > > From: "Paul E. McKenney" <[e-mail]>
> > > > 
> > > > There are kernel facilities such as per-CPU reference counts that give
> > > > error messages in generic handlers or callbacks, whose messages are
> > > > unenlightening.  In the case of per-CPU reference-count underflow, this
> > > > is not a problem when creating a new use of this facility because in that
> > > > case the bug is almost certainly in the code implementing that new use.
> > > > However, trouble arises when deploying across many systems, which might
> > > > exercise corner cases that were not seen during development and testing.
> > > > Here, it would be really nice to get some kind of hint as to which of
> > > > several uses the underflow was caused by.
> > > > 
> > > > This commit therefore exposes a new kmem_last_alloc() function that
> > > > takes a pointer to dynamically allocated memory and returns the return
> > > > address of the call that allocated it.  This pointer can reference the
> > > > middle of the block as well as the beginning of the block, as needed
> > > > by things like RCU callback functions and timer handlers that might not
> > > > know where the beginning of the memory block is.  These functions and
> > > > handlers can use the return value from kmem_last_alloc() to give the
> > > > kernel hacker a better hint as to where the problem might lie.
> > > 
> > > I agree with exposing allocation caller information to the other
> > > subsystem to help the debugging. Some suggestions...
> > 
> > Good to hear!  ;-)
> > 
> > > 1. It's better to separate a slab object check (validity check) and
> > > retrieving the allocation caller. Someone else would want to check
> > > only a validity. And, it doesn't depend on the debug configuration so
> > > it's not good to bind it to the debug function.
> > > 
> > > kmem_cache_valid_(obj|ptr)
> > > kmalloc_valid_(obj|ptr)
> > 
> > Here both functions would say "true" for a pointer from kmalloc()?
> > Or do I need to add a third function that is happy with a pointer from
> > either source?
> 
> I focused on separation and missed this case that the user sometimes
> cannot know the object source (kmalloc/kmem_cache). At first step,
> just checking whether it is a slab-object or not looks enough.
> 
> int kmem_valid_obj()

OK, I will update my current kmalloc_valid_obj() to kmem_valid_obj(),
thank you!

> > I do understand that people who don't want to distinguish could just do
> > "kmem_cache_valid_ptr(p) || kmalloc_valid_ptr(p)".  However, the two
> > use cases in the series have no idea whether the pointer they have came
> > from kmalloc(), kmem_cache_alloc(), or somewhere else entirely, even an
> > on-stack variable.
> > 
> > Are you asking me to choose between the _obj() and _ptr() suffixes?
> 
> Yes, I prefer _obj().

Then _obj() it is.

> > If not, please help me understand the distinction.
> > 
> > Do we want "debug" in these names as well?
> 
> I don't think so since it can be called without enabling the debug
> option.

OK, understood.

> > > 2. rename kmem_last_alloc to ...
> > > 
> > > int kmem_cache_debug_alloc_caller(cache, obj, &ret_addr)
> > > int kmalloc_debug_alloc_caller(obj, &ret_addr)
> > > 
> > > or debug_kmem_cache_alloc_caller()
> > > 
> > > I think that function name need to include the keyword 'debug' to show
> > > itself as a debugging facility (enabled at the debugging). And, return
> > > errno and get caller address by pointer argument.
> > 
> > I am quite happy to add the "debug", but my use cases have no idea
> > how the pointer was allocated.  In fact, the next version of the
> > patch will also handle allocator return addresses from vmalloc().
> > 
> > And for kernels without sufficient debug enabled, I need to provide
> > the name of the slab cache, and this also is to be in the next version.
> 
> Okay. So, your code would be...
> 
> if (kmem_valid_obj(ptr))
>         kmalloc_debug_print_provenance(ptr)
> else if (vmalloc_valid_obj(ptr))
>         ....

Suggestions on where to put the mem_dump_obj() or whatever name that
executes this code?  Left to myself, I will pick a likely on the theory
that it can always be moved later.

This structuring does cause double work, but this should be OK because
all of the uses I know of are on error paths.

> > > 3. If concrete error message is needed, please introduce more functions.
> > > 
> > > void *kmalloc_debug_error(errno)
> > 
> > Agreed, in fact, I was planning to have a function that printed out
> > a suitable error-message continuation to the console for ease-of-use
> > reasons.  For example, why is the caller deciding how deep the stack
> > frame is?  ;-)
> > 
> > So something like this?
> > 
> > 	void kmalloc_debug_print_provenance(void *ptr);
> > 
> > With the understanding that it will print something helpful regardless
> > of where ptr came from, within the constraints of the kernel build and
> > boot options?
> 
> Looks good idea. I suggest a name, kmem_dump_obj(), for this function.
> In this case, I don't think that "debug" keyword is needed since it shows
> something useful (slab cache info) even if debug option isn't enabled.
> 
> So, for summary, we need to introduce two functions to accomplish your
> purpose. Please correct me if wrong.
> 
> int kmem_valid_obj(ptr)
> void kmem_dump_obj(ptr)

Within slab, agreed.

We course also need something like mem_dump_obj() to handle a pointer
with unknown provenance, along with the vmalloc_valid_obj() and the
vmalloc_dump_obj().  And similar functions should other allocation
sources become important.

							Thanx, Paul

Patch

diff --git a/include/linux/slab.h b/include/linux/slab.h
index dd6897f..06dd56b 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -186,6 +186,8 @@  void kfree(const void *);
 void kfree_sensitive(const void *);
 size_t __ksize(const void *);
 size_t ksize(const void *);
+void *kmem_cache_last_alloc(struct kmem_cache *s, void *object);
+void *kmem_last_alloc(void *object);
 
 #ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR
 void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
diff --git a/mm/slab.c b/mm/slab.c
index b111356..2ab93b8 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3602,6 +3602,25 @@  void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
 EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
 #endif
 
+void *kmem_cache_last_alloc(struct kmem_cache *cachep, void *object)
+{
+#ifdef DEBUG
+	unsigned int objnr;
+	void *objp;
+	struct page *page;
+
+	if (!(cachep->flags & SLAB_STORE_USER))
+		return NULL;
+	objp = object - obj_offset(cachep);
+	page = virt_to_head_page(objp);
+	objnr = obj_to_index(cachep, page, objp);
+	objp = index_to_obj(cachep, page, objnr);
+	return *dbg_userword(cachep, objp);
+#else
+	return NULL;
+#endif
+}
+
 static __always_inline void *
 __do_kmalloc_node(size_t size, gfp_t flags, int node, unsigned long caller)
 {
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f9ccd5d..3f647982 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -536,6 +536,26 @@  bool slab_is_available(void)
 	return slab_state >= UP;
 }
 
+/*
+ * If the pointer references a slab-allocated object and if sufficient
+ * debugging is enabled, return the returrn address for the corresponding
+ * allocation.  Otherwise, return NULL.  Note that passing random pointers
+ * to this function (including addresses of on-stack variables) is likely
+ * to result in panics.
+ */
+void *kmem_last_alloc(void *object)
+{
+	struct page *page;
+
+	if (!virt_addr_valid(object))
+		return NULL;
+	page = virt_to_head_page(object);
+	if (!PageSlab(page))
+		return NULL;
+	return kmem_cache_last_alloc(page->slab_cache, object);
+}
+EXPORT_SYMBOL_GPL(kmem_last_alloc);
+
 #ifndef CONFIG_SLOB
 /* Create a cache during boot when no slab services are available yet */
 void __init create_boot_cache(struct kmem_cache *s, const char *name,
diff --git a/mm/slob.c b/mm/slob.c
index 7cc9805..c1f8ed7 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -461,6 +461,11 @@  static void slob_free(void *block, int size)
 	spin_unlock_irqrestore(&slob_lock, flags);
 }
 
+void *kmem_cache_last_alloc(struct kmem_cache *s, void *object)
+{
+	return NULL;
+}
+
 /*
  * End of slob allocator proper. Begin kmem_cache_alloc and kmalloc frontend.
  */
diff --git a/mm/slub.c b/mm/slub.c
index b30be23..8ed3ba2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3918,6 +3918,32 @@  int __kmem_cache_shutdown(struct kmem_cache *s)
 	return 0;
 }
 
+void *kmem_cache_last_alloc(struct kmem_cache *s, void *object)
+{
+#ifdef CONFIG_SLUB_DEBUG
+	void *base;
+	unsigned int objnr;
+	void *objp;
+	struct page *page;
+	struct track *trackp;
+
+	if (!(s->flags & SLAB_STORE_USER))
+		return NULL;
+	page = virt_to_head_page(object);
+	base = page_address(page);
+	objp = kasan_reset_tag(object);
+	objp = restore_red_left(s, objp);
+	objnr = obj_to_index(s, page, objp);
+	objp = base + s->size * objnr;
+	if (objp < base || objp >= base + page->objects * s->size || (objp - base) % s->size)
+		return NULL;
+	trackp = get_track(s, objp, TRACK_ALLOC);
+	return (void *)trackp->addr;
+#else
+	return NULL;
+#endif
+}
+
 /********************************************************************
  *		Kmalloc subsystem
  *******************************************************************/