If CONFIG_DEBUG_SLAB/CONFIG_DEBUG_SLAB_LEAK are enabled, the slab code
prints extra debug information when e.g. corruption is detected.
This includes pointers, which are not very useful when hashed.
Fix this by using %px to print unhashed pointers instead.
Fixes: ad67b74d2469d9b8 ("printk: hash addresses printed with %p")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
It's been ages I needed the above options. But of course I need them
just after the introduction of address hashing...
---
mm/slab.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index 183e996dde5ff37a..70be5823227dcb3e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1585,7 +1585,7 @@ static void print_objinfo(struct kmem_cache *cachep, void *objp, int lines)
}
if (cachep->flags & SLAB_STORE_USER) {
- pr_err("Last user: [<%p>](%pSR)\n",
+ pr_err("Last user: [<%px>](%pSR)\n",
*dbg_userword(cachep, objp),
*dbg_userword(cachep, objp));
}
@@ -1621,7 +1621,7 @@ static void check_poison_obj(struct kmem_cache *cachep, void *objp)
/* Mismatch ! */
/* Print header */
if (lines == 0) {
- pr_err("Slab corruption (%s): %s start=%p, len=%d\n",
+ pr_err("Slab corruption (%s): %s start=%px, len=%d\n",
print_tainted(), cachep->name,
realobj, size);
print_objinfo(cachep, objp, 0);
@@ -1650,13 +1650,13 @@ static void check_poison_obj(struct kmem_cache *cachep, void *objp)
if (objnr) {
objp = index_to_obj(cachep, page, objnr - 1);
realobj = (char *)objp + obj_offset(cachep);
- pr_err("Prev obj: start=%p, len=%d\n", realobj, size);
+ pr_err("Prev obj: start=%px, len=%d\n", realobj, size);
print_objinfo(cachep, objp, 2);
}
if (objnr + 1 < cachep->num) {
objp = index_to_obj(cachep, page, objnr + 1);
realobj = (char *)objp + obj_offset(cachep);
- pr_err("Next obj: start=%p, len=%d\n", realobj, size);
+ pr_err("Next obj: start=%px, len=%d\n", realobj, size);
print_objinfo(cachep, objp, 2);
}
}
@@ -2608,7 +2608,7 @@ static void slab_put_obj(struct kmem_cache *cachep,
/* Verify double free bug */
for (i = page->active; i < cachep->num; i++) {
if (get_free_obj(page, i) == objnr) {
- pr_err("slab: double free detected in cache '%s', objp %p\n",
+ pr_err("slab: double free detected in cache '%s', objp %px\n",
cachep->name, objp);
BUG();
}
@@ -2772,7 +2772,7 @@ static inline void verify_redzone_free(struct kmem_cache *cache, void *obj)
else
slab_error(cache, "memory outside object was overwritten");
- pr_err("%p: redzone 1:0x%llx, redzone 2:0x%llx\n",
+ pr_err("%px: redzone 1:0x%llx, redzone 2:0x%llx\n",
obj, redzone1, redzone2);
}
@@ -3078,7 +3078,7 @@ static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
if (*dbg_redzone1(cachep, objp) != RED_INACTIVE ||
*dbg_redzone2(cachep, objp) != RED_INACTIVE) {
slab_error(cachep, "double free, or memory outside object was overwritten");
- pr_err("%p: redzone 1:0x%llx, redzone 2:0x%llx\n",
+ pr_err("%px: redzone 1:0x%llx, redzone 2:0x%llx\n",
objp, *dbg_redzone1(cachep, objp),
*dbg_redzone2(cachep, objp));
}
@@ -3091,7 +3091,7 @@ static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
cachep->ctor(objp);
if (ARCH_SLAB_MINALIGN &&
((unsigned long)objp & (ARCH_SLAB_MINALIGN-1))) {
- pr_err("0x%p: not aligned to ARCH_SLAB_MINALIGN=%d\n",
+ pr_err("0x%px: not aligned to ARCH_SLAB_MINALIGN=%d\n",
objp, (int)ARCH_SLAB_MINALIGN);
}
return objp;
@@ -4283,7 +4283,7 @@ static void show_symbol(struct seq_file *m, unsigned long address)
return;
}
#endif
- seq_printf(m, "%p", (void *)address);
+ seq_printf(m, "%px", (void *)address);
}
static int leaks_show(struct seq_file *m, void *p)
--
2.7.4
On Thu, 7 Dec 2017, Geert Uytterhoeven wrote:
> If CONFIG_DEBUG_SLAB/CONFIG_DEBUG_SLAB_LEAK are enabled, the slab code
> prints extra debug information when e.g. corruption is detected.
> This includes pointers, which are not very useful when hashed.
>
> Fix this by using %px to print unhashed pointers instead.
Acked-by: Christoph Lameter <[email protected]>
These SLAB config options are only used for testing so this is ok.
On Thu, Dec 7, 2017 at 3:13 AM, Christopher Lameter <[email protected]> wrote:
> On Thu, 7 Dec 2017, Geert Uytterhoeven wrote:
>
>> If CONFIG_DEBUG_SLAB/CONFIG_DEBUG_SLAB_LEAK are enabled, the slab code
>> prints extra debug information when e.g. corruption is detected.
>> This includes pointers, which are not very useful when hashed.
>>
>> Fix this by using %px to print unhashed pointers instead.
>
> Acked-by: Christoph Lameter <[email protected]>
>
> These SLAB config options are only used for testing so this is ok.
Most systems use SLUB so I can't say how common CONFIG_DEBUG_SLAB is.
(Though, FWIW with SLUB, CONFIG_SLUB_DEBUG is very common.)
-Kees
--
Kees Cook
Pixel Security
On Thu, 7 Dec 2017, Kees Cook wrote:
> > These SLAB config options are only used for testing so this is ok.
>
> Most systems use SLUB so I can't say how common CONFIG_DEBUG_SLAB is.
> (Though, FWIW with SLUB, CONFIG_SLUB_DEBUG is very common.)
CONFIG_SLUB_DEBUG is on by default because it compiles into the kernel the
runtime configurable debugging framework. It does not activate any
debugging.
CONFIG_SLUB_DEBUG_ON is the equivalent to CONFIG_SLAB_DEBUG. The kernel
will boot with debugging on without any extra kernel options with these.
On Thu, Dec 7, 2017 at 2:17 AM, Geert Uytterhoeven
<[email protected]> wrote:
>
> if (cachep->flags & SLAB_STORE_USER) {
> - pr_err("Last user: [<%p>](%pSR)\n",
> + pr_err("Last user: [<%px>](%pSR)\n",
> *dbg_userword(cachep, objp),
> *dbg_userword(cachep, objp));
Is there actually any point to the %px at all?
Why not remove it? the _real_ information is printed out by %pSR, and
that's both sufficient and useful in ways %px isn't.
> - pr_err("Slab corruption (%s): %s start=%p, len=%d\n",
> + pr_err("Slab corruption (%s): %s start=%px, len=%d\n",
> print_tainted(), cachep->name,
> realobj, size);
and here, is the pointer actually interesting, or should we just give
the offset to the allocation?
But if the pointer is interesting, then ack.
Linus
Hi Linus,
On Sun, Dec 10, 2017 at 9:45 PM, Linus Torvalds
<[email protected]> wrote:
> On Thu, Dec 7, 2017 at 2:17 AM, Geert Uytterhoeven
> <[email protected]> wrote:
>> - pr_err("Slab corruption (%s): %s start=%p, len=%d\n",
>> + pr_err("Slab corruption (%s): %s start=%px, len=%d\n",
>> print_tainted(), cachep->name,
>> realobj, size);
>
> and here, is the pointer actually interesting, or should we just give
> the offset to the allocation?
The pointer may help to identify e.g. an empty list_head in the written data.
> But if the pointer is interesting, then ack.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Sun, 10 Dec 2017, Linus Torvalds wrote:
> On Thu, Dec 7, 2017 at 2:17 AM, Geert Uytterhoeven
> <[email protected]> wrote:
> >
> > if (cachep->flags & SLAB_STORE_USER) {
> > - pr_err("Last user: [<%p>](%pSR)\n",
> > + pr_err("Last user: [<%px>](%pSR)\n",
> > *dbg_userword(cachep, objp),
> > *dbg_userword(cachep, objp));
>
> Is there actually any point to the %px at all?
>
> Why not remove it? the _real_ information is printed out by %pSR, and
> that's both sufficient and useful in ways %px isn't.
This pointer refers to code so we can remove it.
>
> > - pr_err("Slab corruption (%s): %s start=%p, len=%d\n",
> > + pr_err("Slab corruption (%s): %s start=%px, len=%d\n",
> > print_tainted(), cachep->name,
> > realobj, size);
>
> and here, is the pointer actually interesting, or should we just give
> the offset to the allocation?
>
> But if the pointer is interesting, then ack.
The pointer here is to an slab object which could be important if one
wants to find the pointer value in a hexdump of another object (f.e.
listhead) or other pointer information that is being inspected
in a debugging session.