* [PATCH 0/3] slub: Print non-hashed pointers in slub debugging @ 2021-05-20 1:35 Stephen Boyd 2021-05-20 1:35 ` [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for " Stephen Boyd ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Stephen Boyd @ 2021-05-20 1:35 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm I was doing some debugging recently and noticed that my pointers were being hashed while slub_debug was on the kernel commandline. Let's make the prints in here meaningful in that case by pushing %px throughout. Alternatively, we could force on no_hash_pointers if slub_debug is on the commandline. Maybe that would be better? The final patch is just something else I noticed while looking at the code. The message argument is never used so the debugging messages are not as clear as they could be. Stephen Boyd (3): lib/hexdump: Add a raw pointer printing format for slub debugging slub: Print raw pointer addresses when debugging slub: Actually use 'message' in restore_bytes() include/linux/printk.h | 1 + lib/hexdump.c | 12 ++++++++++-- mm/slub.c | 24 ++++++++++++------------ 3 files changed, 23 insertions(+), 14 deletions(-) base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 -- https://chromeos.dev ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for slub debugging 2021-05-20 1:35 [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Stephen Boyd @ 2021-05-20 1:35 ` Stephen Boyd 2021-05-24 5:11 ` David Rientjes 2021-05-24 11:36 ` Petr Mladek 2021-05-20 1:35 ` [PATCH 2/3] slub: Print raw pointer addresses when debugging Stephen Boyd ` (2 subsequent siblings) 3 siblings, 2 replies; 14+ messages in thread From: Stephen Boyd @ 2021-05-20 1:35 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm We want to get actual pointer addresses when we're looking at slub debugging reports. Add another prefix format specifier that says we want raw pointer addresses, i.e. %px, in the printk format. Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- include/linux/printk.h | 1 + lib/hexdump.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/include/linux/printk.h b/include/linux/printk.h index fe7eb2351610..a7b0b620982d 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -567,6 +567,7 @@ extern const struct file_operations kmsg_fops; enum { DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS, + DUMP_PREFIX_RAW_ADDRESS, DUMP_PREFIX_OFFSET }; extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, diff --git a/lib/hexdump.c b/lib/hexdump.c index 9301578f98e8..87af5755563f 100644 --- a/lib/hexdump.c +++ b/lib/hexdump.c @@ -211,8 +211,12 @@ EXPORT_SYMBOL(hex_dump_to_buffer); * @level: kernel log level (e.g. KERN_DEBUG) * @prefix_str: string to prefix each line with; * caller supplies trailing spaces for alignment if desired - * @prefix_type: controls whether prefix of an offset, address, or none - * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE) + * @prefix_type: controls how prefix is printed + * %DUMP_PREFIX_OFFSET - offset prefix + * %DUMP_PREFIX_ADDRESS - hashed address prefix + * %DUMP_PREFIX_RAW_ADDRESS - non-hashed address prefix + * %DUMP_PREFIX_NONE - no prefix + * * @rowsize: number of bytes to print per line; must be 16 or 32 * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1) * @buf: data blob to dump @@ -260,6 +264,10 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type, printk("%s%s%p: %s\n", level, prefix_str, ptr + i, linebuf); break; + case DUMP_PREFIX_RAW_ADDRESS: + printk("%s%s%px: %s\n", + level, prefix_str, ptr + i, linebuf); + break; case DUMP_PREFIX_OFFSET: printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf); break; -- https://chromeos.dev ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for slub debugging 2021-05-20 1:35 ` [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for " Stephen Boyd @ 2021-05-24 5:11 ` David Rientjes 2021-05-24 11:36 ` Petr Mladek 1 sibling, 0 replies; 14+ messages in thread From: David Rientjes @ 2021-05-24 5:11 UTC (permalink / raw) To: Stephen Boyd Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Vlastimil Babka, linux-mm On Wed, 19 May 2021, Stephen Boyd wrote: > We want to get actual pointer addresses when we're looking at slub > debugging reports. Add another prefix format specifier that says we want > raw pointer addresses, i.e. %px, in the printk format. > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Acked-by: David Rientjes <rientjes@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for slub debugging 2021-05-20 1:35 ` [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for " Stephen Boyd 2021-05-24 5:11 ` David Rientjes @ 2021-05-24 11:36 ` Petr Mladek 1 sibling, 0 replies; 14+ messages in thread From: Petr Mladek @ 2021-05-24 11:36 UTC (permalink / raw) To: Stephen Boyd Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm On Wed 2021-05-19 18:35:37, Stephen Boyd wrote: > We want to get actual pointer addresses when we're looking at slub > debugging reports. Add another prefix format specifier that says we want > raw pointer addresses, i.e. %px, in the printk format. This patch makes sense only with the 2nd patch. Please, do not do this! Raw pointers might be printed safely only in panic(). Users should be warned by the fat warning triggered by "no_hash_pointers" in other use-cases. And this patch is not needed when "no_hash_pointers" are enabled. Best Regards, Petr ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] slub: Print raw pointer addresses when debugging 2021-05-20 1:35 [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Stephen Boyd 2021-05-20 1:35 ` [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for " Stephen Boyd @ 2021-05-20 1:35 ` Stephen Boyd 2021-05-24 11:32 ` Petr Mladek 2021-05-20 1:35 ` [PATCH 3/3] slub: Actually use 'message' in restore_bytes() Stephen Boyd 2021-05-20 10:01 ` [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Vlastimil Babka 3 siblings, 1 reply; 14+ messages in thread From: Stephen Boyd @ 2021-05-20 1:35 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm Obscuring the pointers that slub shows when debugging makes for some confusing slub debug messages: Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17 Those addresses are hashed for kernel security reasons. If we're trying to be secure with slub_debug on the commandline we have some big problems given that we dump whole chunks of kernel memory to the kernel logs. Let's use %px here and dump buffers with the actual address for the buffer instead of the hashed version so that the logs are meaningful. This also helps if a kernel address is in some slub debug report so we can figure out that the object is referencing itself. Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- mm/slub.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index feda53ae62ba..87eeeed1f369 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -549,7 +549,7 @@ static void print_section(char *level, char *text, u8 *addr, unsigned int length) { metadata_access_enable(); - print_hex_dump(level, kasan_reset_tag(text), DUMP_PREFIX_ADDRESS, + print_hex_dump(level, kasan_reset_tag(text), DUMP_PREFIX_RAW_ADDRESS, 16, 1, addr, length, 1); metadata_access_disable(); } @@ -650,7 +650,7 @@ void print_tracking(struct kmem_cache *s, void *object) static void print_page_info(struct page *page) { - pr_err("Slab 0x%p objects=%u used=%u fp=0x%p flags=%#lx(%pGp)\n", + pr_err("Slab 0x%px objects=%u used=%u fp=0x%px flags=%#lx(%pGp)\n", page, page->objects, page->inuse, page->freelist, page->flags, &page->flags); @@ -707,7 +707,7 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p) print_page_info(page); - pr_err("Object 0x%p @offset=%tu fp=0x%p\n\n", + pr_err("Object 0x%px @offset=%tu fp=0x%px\n\n", p, p - addr, get_freepointer(s, p)); if (s->flags & SLAB_RED_ZONE) @@ -777,7 +777,7 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) static void restore_bytes(struct kmem_cache *s, char *message, u8 data, void *from, void *to) { - slab_fix(s, "Restoring 0x%p-0x%p=0x%x\n", from, to - 1, data); + slab_fix(s, "Restoring 0x%px-0x%px=0x%x\n", from, to - 1, data); memset(from, data, to - from); } @@ -800,7 +800,7 @@ static int check_bytes_and_report(struct kmem_cache *s, struct page *page, end--; slab_bug(s, "%s overwritten", what); - pr_err("0x%p-0x%p @offset=%tu. First byte 0x%x instead of 0x%x\n", + pr_err("0x%px-0x%px @offset=%tu. First byte 0x%x instead of 0x%x\n", fault, end - 1, fault - addr, fault[0], value); print_trailer(s, page, object); @@ -893,7 +893,7 @@ static int slab_pad_check(struct kmem_cache *s, struct page *page) while (end > fault && end[-1] == POISON_INUSE) end--; - slab_err(s, page, "Padding overwritten. 0x%p-0x%p @offset=%tu", + slab_err(s, page, "Padding overwritten. 0x%px-0x%px @offset=%tu", fault, end - 1, fault - start); print_section(KERN_ERR, "Padding ", pad, remainder); @@ -1041,7 +1041,7 @@ static void trace(struct kmem_cache *s, struct page *page, void *object, int alloc) { if (s->flags & SLAB_TRACE) { - pr_info("TRACE %s %s 0x%p inuse=%d fp=0x%p\n", + pr_info("TRACE %s %s 0x%px inuse=%d fp=0x%px\n", s->name, alloc ? "alloc" : "free", object, page->inuse, @@ -1186,7 +1186,7 @@ static inline int free_consistency_checks(struct kmem_cache *s, struct page *page, void *object, unsigned long addr) { if (!check_valid_pointer(s, page, object)) { - slab_err(s, page, "Invalid object pointer 0x%p", object); + slab_err(s, page, "Invalid object pointer 0x%px", object); return 0; } @@ -1200,10 +1200,10 @@ static inline int free_consistency_checks(struct kmem_cache *s, if (unlikely(s != page->slab_cache)) { if (!PageSlab(page)) { - slab_err(s, page, "Attempt to free object(0x%p) outside of slab", + slab_err(s, page, "Attempt to free object(0x%px) outside of slab", object); } else if (!page->slab_cache) { - pr_err("SLUB <none>: no slab for object 0x%p.\n", + pr_err("SLUB <none>: no slab for object 0x%px.\n", object); dump_stack(); } else @@ -1263,7 +1263,7 @@ static noinline int free_debug_processing( slab_unlock(page); spin_unlock_irqrestore(&n->list_lock, flags); if (!ret) - slab_fix(s, "Object at 0x%p not freed", object); + slab_fix(s, "Object at 0x%px not freed", object); return ret; } @@ -3908,7 +3908,7 @@ static void list_slab_objects(struct kmem_cache *s, struct page *page, for_each_object(p, s, addr, page->objects) { if (!test_bit(__obj_to_index(s, addr, p), map)) { - pr_err("Object 0x%p @offset=%tu\n", p, p - addr); + pr_err("Object 0x%px @offset=%tu\n", p, p - addr); print_tracking(s, p); } } -- https://chromeos.dev ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] slub: Print raw pointer addresses when debugging 2021-05-20 1:35 ` [PATCH 2/3] slub: Print raw pointer addresses when debugging Stephen Boyd @ 2021-05-24 11:32 ` Petr Mladek 2021-05-25 6:21 ` Stephen Boyd 0 siblings, 1 reply; 14+ messages in thread From: Petr Mladek @ 2021-05-24 11:32 UTC (permalink / raw) To: Stephen Boyd Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm On Wed 2021-05-19 18:35:38, Stephen Boyd wrote: > Obscuring the pointers that slub shows when debugging makes for some > confusing slub debug messages: > > Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17 > > Those addresses are hashed for kernel security reasons. If we're trying > to be secure with slub_debug on the commandline we have some big > problems given that we dump whole chunks of kernel memory to the kernel > logs. Let's use %px here and dump buffers with the actual address for > the buffer instead of the hashed version so that the logs are > meaningful. This also helps if a kernel address is in some slub debug > report so we can figure out that the object is referencing itself. Please, do not do this! Use "no_hash_pointers" commandling option when you want to see raw pointers. It will make it clear when the kernel logs are save and when not. If "slub_debug" is useless with hashed pointers then it might enable "no_hash_pointers". But make sure that it prints the fat warning. This patch is the worst approach. We have to keep the number of "%px" callers at minimum to keep it maintainable. The only safe use-case is when the system is in panic() [*]. If the pointers might be printed at any time then users should be warned by the fat message printed by "no_hash_pointers". [*] Raw pointers are currently printed also by Oops/WARN messages. It is from historic reasons. Anyway, they are fat warnings on its own. The system often need to get reported anyway. Best Regards, Petr ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] slub: Print raw pointer addresses when debugging 2021-05-24 11:32 ` Petr Mladek @ 2021-05-25 6:21 ` Stephen Boyd 0 siblings, 0 replies; 14+ messages in thread From: Stephen Boyd @ 2021-05-25 6:21 UTC (permalink / raw) To: Petr Mladek Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm Quoting Petr Mladek (2021-05-24 04:32:13) > On Wed 2021-05-19 18:35:38, Stephen Boyd wrote: > > Obscuring the pointers that slub shows when debugging makes for some > > confusing slub debug messages: > > > > Padding overwritten. 0x0000000079f0674a-0x000000000d4dce17 > > > > Those addresses are hashed for kernel security reasons. If we're trying > > to be secure with slub_debug on the commandline we have some big > > problems given that we dump whole chunks of kernel memory to the kernel > > logs. Let's use %px here and dump buffers with the actual address for > > the buffer instead of the hashed version so that the logs are > > meaningful. This also helps if a kernel address is in some slub debug > > report so we can figure out that the object is referencing itself. > > Please, do not do this! > > Use "no_hash_pointers" commandling option when you want to see > raw pointers. It will make it clear when the kernel logs are save > and when not. > > If "slub_debug" is useless with hashed pointers then it might enable > "no_hash_pointers". But make sure that it prints the fat warning. Ok I'll try to make slub_debug force on no_hash_pointers. > > This patch is the worst approach. We have to keep the number of "%px" > callers at minimum to keep it maintainable. The only safe use-case is > when the system is in panic() [*]. If the pointers might be printed > at any time then users should be warned by the fat message printed > by "no_hash_pointers". > > > [*] Raw pointers are currently printed also by Oops/WARN messages. > It is from historic reasons. Anyway, they are fat warnings > on its own. The system often need to get reported anyway. > Got it. The slub debug messages are usually followed by stuff blowing up and the system crashing but it's possible that the automatic fixup code will save us. When you have things scribbling all over the place it doesn't end well. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] slub: Actually use 'message' in restore_bytes() 2021-05-20 1:35 [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Stephen Boyd 2021-05-20 1:35 ` [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for " Stephen Boyd 2021-05-20 1:35 ` [PATCH 2/3] slub: Print raw pointer addresses when debugging Stephen Boyd @ 2021-05-20 1:35 ` Stephen Boyd 2021-05-20 10:02 ` Vlastimil Babka 2021-05-24 5:12 ` David Rientjes 2021-05-20 10:01 ` [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Vlastimil Babka 3 siblings, 2 replies; 14+ messages in thread From: Stephen Boyd @ 2021-05-20 1:35 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Vlastimil Babka, linux-mm The message argument isn't used here. Let's pass the string to the printk message so that the developer can figure out what's happening, instead of guessing that a redzone is being restored, etc. Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- mm/slub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/slub.c b/mm/slub.c index 87eeeed1f369..16e8e8f8dc0c 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -777,7 +777,7 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) static void restore_bytes(struct kmem_cache *s, char *message, u8 data, void *from, void *to) { - slab_fix(s, "Restoring 0x%px-0x%px=0x%x\n", from, to - 1, data); + slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x\n", message, from, to - 1, data); memset(from, data, to - from); } -- https://chromeos.dev ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] slub: Actually use 'message' in restore_bytes() 2021-05-20 1:35 ` [PATCH 3/3] slub: Actually use 'message' in restore_bytes() Stephen Boyd @ 2021-05-20 10:02 ` Vlastimil Babka 2021-05-24 5:12 ` David Rientjes 1 sibling, 0 replies; 14+ messages in thread From: Vlastimil Babka @ 2021-05-20 10:02 UTC (permalink / raw) To: Stephen Boyd, Andrew Morton Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, linux-mm On 5/20/21 3:35 AM, Stephen Boyd wrote: > The message argument isn't used here. Let's pass the string to the > printk message so that the developer can figure out what's happening, > instead of guessing that a redzone is being restored, etc. > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slub.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 87eeeed1f369..16e8e8f8dc0c 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -777,7 +777,7 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) > static void restore_bytes(struct kmem_cache *s, char *message, u8 data, > void *from, void *to) > { > - slab_fix(s, "Restoring 0x%px-0x%px=0x%x\n", from, to - 1, data); > + slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x\n", message, from, to - 1, data); > memset(from, data, to - from); > } > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] slub: Actually use 'message' in restore_bytes() 2021-05-20 1:35 ` [PATCH 3/3] slub: Actually use 'message' in restore_bytes() Stephen Boyd 2021-05-20 10:02 ` Vlastimil Babka @ 2021-05-24 5:12 ` David Rientjes 2021-05-25 7:37 ` Joe Perches 1 sibling, 1 reply; 14+ messages in thread From: David Rientjes @ 2021-05-24 5:12 UTC (permalink / raw) To: Stephen Boyd Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Vlastimil Babka, linux-mm On Wed, 19 May 2021, Stephen Boyd wrote: > The message argument isn't used here. Let's pass the string to the > printk message so that the developer can figure out what's happening, > instead of guessing that a redzone is being restored, etc. > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Acked-by: David Rientjes <rientjes@google.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] slub: Actually use 'message' in restore_bytes() 2021-05-24 5:12 ` David Rientjes @ 2021-05-25 7:37 ` Joe Perches 2021-05-26 2:32 ` Stephen Boyd 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2021-05-25 7:37 UTC (permalink / raw) To: David Rientjes, Stephen Boyd Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Vlastimil Babka, linux-mm On Sun, 2021-05-23 at 22:12 -0700, David Rientjes wrote: > On Wed, 19 May 2021, Stephen Boyd wrote: > > > The message argument isn't used here. Let's pass the string to the > > printk message so that the developer can figure out what's happening, > > instead of guessing that a redzone is being restored, etc. > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > Acked-by: David Rientjes <rientjes@google.com> Ideally, the slab_fix function would be marked with __printf and the format here would not use \n as that's emitted by the slab_fix. --- mm/slub.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index ee51857d8e9bc..46f9b043089b6 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -702,6 +702,7 @@ static void slab_bug(struct kmem_cache *s, char *fmt, ...) va_end(args); } +__printf(2, 3) static void slab_fix(struct kmem_cache *s, char *fmt, ...) { struct va_format vaf; @@ -816,7 +817,8 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) static void restore_bytes(struct kmem_cache *s, char *message, u8 data, void *from, void *to) { - slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x\n", message, from, to - 1, data); + slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x", + message, from, to - 1, data); memset(from, data, to - from); } @@ -1069,13 +1071,13 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search) slab_err(s, page, "Wrong number of objects. Found %d but should be %d", page->objects, max_objects); page->objects = max_objects; - slab_fix(s, "Number of objects adjusted."); + slab_fix(s, "Number of objects adjusted"); } if (page->inuse != page->objects - nr) { slab_err(s, page, "Wrong object count. Counter is %d but counted were %d", page->inuse, page->objects - nr); page->inuse = page->objects - nr; - slab_fix(s, "Object count adjusted."); + slab_fix(s, "Object count adjusted"); } return search == NULL; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] slub: Actually use 'message' in restore_bytes() 2021-05-25 7:37 ` Joe Perches @ 2021-05-26 2:32 ` Stephen Boyd 2021-05-26 2:38 ` Joe Perches 0 siblings, 1 reply; 14+ messages in thread From: Stephen Boyd @ 2021-05-26 2:32 UTC (permalink / raw) To: David Rientjes, Joe Perches Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Vlastimil Babka, linux-mm Quoting Joe Perches (2021-05-25 00:37:45) > On Sun, 2021-05-23 at 22:12 -0700, David Rientjes wrote: > > On Wed, 19 May 2021, Stephen Boyd wrote: > > > > > The message argument isn't used here. Let's pass the string to the > > > printk message so that the developer can figure out what's happening, > > > instead of guessing that a redzone is being restored, etc. > > > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > > > Acked-by: David Rientjes <rientjes@google.com> > > Ideally, the slab_fix function would be marked with __printf and the > format here would not use \n as that's emitted by the slab_fix. Thanks. I can make this into a proper patch and author it from you. Can you provide a signed-off-by? The restore_bytes() hunk is slightly different but I can fix that up. > --- > mm/slub.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index ee51857d8e9bc..46f9b043089b6 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -702,6 +702,7 @@ static void slab_bug(struct kmem_cache *s, char *fmt, ...) > va_end(args); > } > > +__printf(2, 3) > static void slab_fix(struct kmem_cache *s, char *fmt, ...) > { > struct va_format vaf; > @@ -816,7 +817,8 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) > static void restore_bytes(struct kmem_cache *s, char *message, u8 data, > void *from, void *to) > { > - slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x\n", message, from, to - 1, data); > + slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x", > + message, from, to - 1, data); > memset(from, data, to - from); > } > > @@ -1069,13 +1071,13 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search) > slab_err(s, page, "Wrong number of objects. Found %d but should be %d", > page->objects, max_objects); > page->objects = max_objects; > - slab_fix(s, "Number of objects adjusted."); > + slab_fix(s, "Number of objects adjusted"); > } > if (page->inuse != page->objects - nr) { > slab_err(s, page, "Wrong object count. Counter is %d but counted were %d", > page->inuse, page->objects - nr); > page->inuse = page->objects - nr; > - slab_fix(s, "Object count adjusted."); > + slab_fix(s, "Object count adjusted"); > } > return search == NULL; > } > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] slub: Actually use 'message' in restore_bytes() 2021-05-26 2:32 ` Stephen Boyd @ 2021-05-26 2:38 ` Joe Perches 0 siblings, 0 replies; 14+ messages in thread From: Joe Perches @ 2021-05-26 2:38 UTC (permalink / raw) To: Stephen Boyd, David Rientjes Cc: Andrew Morton, linux-kernel, Christoph Lameter, Pekka Enberg, Joonsoo Kim, Vlastimil Babka, linux-mm On Tue, 2021-05-25 at 22:32 -0400, Stephen Boyd wrote: > Quoting Joe Perches (2021-05-25 00:37:45) > > On Sun, 2021-05-23 at 22:12 -0700, David Rientjes wrote: > > > On Wed, 19 May 2021, Stephen Boyd wrote: > > > > > > > The message argument isn't used here. Let's pass the string to the > > > > printk message so that the developer can figure out what's happening, > > > > instead of guessing that a redzone is being restored, etc. > > > > > > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > > > > > Acked-by: David Rientjes <rientjes@google.com> > > > > Ideally, the slab_fix function would be marked with __printf and the > > format here would not use \n as that's emitted by the slab_fix. > > Thanks. I can make this into a proper patch and author it from you. Can > you provide a signed-off-by? The restore_bytes() hunk is slightly > different but I can fix that up. If you want... Signed-off-by: Joe Perches <joe@perches.com> > > > --- > > mm/slub.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index ee51857d8e9bc..46f9b043089b6 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -702,6 +702,7 @@ static void slab_bug(struct kmem_cache *s, char *fmt, ...) > > va_end(args); > > } > > > > +__printf(2, 3) > > static void slab_fix(struct kmem_cache *s, char *fmt, ...) > > { > > struct va_format vaf; > > @@ -816,7 +817,8 @@ static void init_object(struct kmem_cache *s, void *object, u8 val) > > static void restore_bytes(struct kmem_cache *s, char *message, u8 data, > > void *from, void *to) > > { > > - slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x\n", message, from, to - 1, data); > > + slab_fix(s, "Restoring %s 0x%px-0x%px=0x%x", > > + message, from, to - 1, data); > > memset(from, data, to - from); > > } > > > > @@ -1069,13 +1071,13 @@ static int on_freelist(struct kmem_cache *s, struct page *page, void *search) > > slab_err(s, page, "Wrong number of objects. Found %d but should be %d", > > page->objects, max_objects); > > page->objects = max_objects; > > - slab_fix(s, "Number of objects adjusted."); > > + slab_fix(s, "Number of objects adjusted"); > > } > > if (page->inuse != page->objects - nr) { > > slab_err(s, page, "Wrong object count. Counter is %d but counted were %d", > > page->inuse, page->objects - nr); > > page->inuse = page->objects - nr; > > - slab_fix(s, "Object count adjusted."); > > + slab_fix(s, "Object count adjusted"); > > } > > return search == NULL; > > } > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] slub: Print non-hashed pointers in slub debugging 2021-05-20 1:35 [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Stephen Boyd ` (2 preceding siblings ...) 2021-05-20 1:35 ` [PATCH 3/3] slub: Actually use 'message' in restore_bytes() Stephen Boyd @ 2021-05-20 10:01 ` Vlastimil Babka 3 siblings, 0 replies; 14+ messages in thread From: Vlastimil Babka @ 2021-05-20 10:01 UTC (permalink / raw) To: Stephen Boyd, Andrew Morton Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, linux-mm, Timur Tabi, Petr Mladek, Steven Rostedt, Sergey Senozhatsky, Kees Cook, Marco Elver, Linus Torvalds +CC folks from the no_hash_pointers debate Full thread with patches here: https://lore.kernel.org/linux-mm/20210520013539.3733631-1-swboyd@chromium.org/ On 5/20/21 3:35 AM, Stephen Boyd wrote: > I was doing some debugging recently and noticed that my pointers were > being hashed while slub_debug was on the kernel commandline. Let's make > the prints in here meaningful in that case by pushing %px throughout. But we actually added no_hash_pointers exactly so that we don't push %px in hexdump and others. So I don't think this will be accepted. > Alternatively, we could force on no_hash_pointers if slub_debug is on > the commandline. Maybe that would be better? That would certainly be more acceptable, but maybe still too much, dunno. I'm neutral on this approach, let's see what others think. > The final patch is just something else I noticed while looking at the > code. The message argument is never used so the debugging messages are > not as clear as they could be. > > Stephen Boyd (3): > lib/hexdump: Add a raw pointer printing format for slub debugging > slub: Print raw pointer addresses when debugging > slub: Actually use 'message' in restore_bytes() > > include/linux/printk.h | 1 + > lib/hexdump.c | 12 ++++++++++-- > mm/slub.c | 24 ++++++++++++------------ > 3 files changed, 23 insertions(+), 14 deletions(-) > > > base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-05-26 2:38 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-20 1:35 [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Stephen Boyd 2021-05-20 1:35 ` [PATCH 1/3] lib/hexdump: Add a raw pointer printing format for " Stephen Boyd 2021-05-24 5:11 ` David Rientjes 2021-05-24 11:36 ` Petr Mladek 2021-05-20 1:35 ` [PATCH 2/3] slub: Print raw pointer addresses when debugging Stephen Boyd 2021-05-24 11:32 ` Petr Mladek 2021-05-25 6:21 ` Stephen Boyd 2021-05-20 1:35 ` [PATCH 3/3] slub: Actually use 'message' in restore_bytes() Stephen Boyd 2021-05-20 10:02 ` Vlastimil Babka 2021-05-24 5:12 ` David Rientjes 2021-05-25 7:37 ` Joe Perches 2021-05-26 2:32 ` Stephen Boyd 2021-05-26 2:38 ` Joe Perches 2021-05-20 10:01 ` [PATCH 0/3] slub: Print non-hashed pointers in slub debugging Vlastimil Babka
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).