[Crash-utility] Re: [PATCH] “kmem address” not working properly when redzone is enabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 4, 2024 at 7:41 PM Aureau, Georges (Kernel Tools ERT) <georges.aureau@xxxxxxx> wrote:

Hello Lianbo,

 

I wanted to keep the story short, but here is the long version.

 

This problem has been hiding for a long time due to a compiler alignment padding on 64bit machine.

 


Thanks for the explanation in detail, Georges. I have to say that is a good finding.
 

For example, on RHEL 8.0, “include/linux/slub_def.h?v=4.18.0-80”:

 

0101         void (*ctor)(void *);

0102         unsigned int inuse;             /* Offset to metadata */

0103         unsigned int align;             /* Alignment */

0104         unsigned int red_left_pad;      /* Left redzone padding size */

0105         const char *name;       /* Name (only for display!) */

 

We have 3 “unsigned int” followed by “char*name” (64bit aligned), hence the compiler will add a 32bit padding after red_left_pad.

With such padding (and a on little-endian machine, ie. x86_64), a bogus load_ULONG() instead of a load_UINT() can’t really be noticed if the padding is zero-filled.

 


Ah, looks like the current issue can not be *ALWAYS* reproduced.
  

Now, on releases where we don’t have such compiler padding after red_left_pad, a bogus load_ULONG() instead of a load_UINT()

will produce noticeable garbage.

 

For example, on RHEL 7.8, “include/linux/slub_def.h?v=3.10.0-1127”:

 

0084         void (*ctor)(void *);

0085         int inuse;              /* Offset to metadata */

0086         int align;              /* Alignment */

0087         int reserved;           /* Reserved bytes at the end of slabs */

0088         RH_KABI_FILL_HOLE(int red_left_pad)     /* Left redzone padding size */

0089         const char *name;       /* Name (only for display!) */

 

Here there won’t be any 32bit padding after red_left_pad as we have 4 “int” before “char*name”,

so here a load_ULONG() instead of load_UINT() we will get bogus data.

 

From a RHEL 7.8 crash dump, where the machine was booted with “slub_debug=FZP” (ie. Debugging all caches):

 

crash> sys | grep REL

     RELEASE: 3.10.0-1127.el7.x86_64

crash> kmem -S names_cache

CACHE             OBJSIZE  ALLOCATED     TOTAL  SLABS  SSIZE  NAME

ffff8838ffc2c040     4096          1       336     48    32k  names_cache

[…]

KMEM_CACHE_NODE   NODE  SLABS  PARTIAL  PER-CPU

ffff88017fdcfcc0     0      6        6        0

NODE 0 PARTIAL:

  SLAB              MEMORY            NODE  TOTAL  ALLOCATED  FREE

  ffffea0021799000  ffff88085e640000     0      7          0     7

  ffffea002179a600  ffff88085e698000     0      7          0     7

  ffffea00217fce00  ffff88085ff38000     0      7          0     7

  ffffea00216df000  ffff88085b7c0000     0      7          0     7

  ffffea00216dc400  ffff88085b710000     0      7          0     7

  ffffea0021799c00  ffff88085e670000     0      7          0     7

[…]

crash> struct kmem_cache ffff8838ffc2c040 | grep red

  red_left_pad = 64,

crash> kmem -s ffff88085e640040      <<< trying first object, should be free

CACHE             OBJSIZE  ALLOCATED     TOTAL  SLABS  SSIZE  NAME

ffff8838ffc2c040     4096          1       336     48    32k  names_cache

  SLAB              MEMORY            NODE  TOTAL  ALLOCATED  FREE

  ffffea0021799000  ffff88085e640000     0      7          0     7

  FREE / [ALLOCATED]

  [ffff88085e640000]     <<< getting expected red zone address, but not free !!

crash> set redzone off

redzone: off

crash> kmem -s ffff88085e640040    <<< trying again with redzone off

CACHE             OBJSIZE  ALLOCATED     TOTAL  SLABS  SSIZE  NAME

ffff8838ffc2c040     4096          1       336     48    32k  names_cache

  SLAB              MEMORY            NODE  TOTAL  ALLOCATED  FREE

  ffffea0021799000  ffff88085e640000     0      7          0     7

  FREE / [ALLOCATED]

  [ffc128105e640040]    <<< getting a bogus object address, still not free

crash> rd -32 0xffff8838ffc2c09c

ffff8838ffc2c09c:  00000040                              @...

crash> rd -64 0xffff8838ffc2c09c

ffff8838ffc2c09c:  ffc1a00800000040                    @.......

crash> p /x 0xffff88085e640000+0xffc1a00800000040

$1 = 0xffc128105e640040

 

Here above we can see that red_left_pad was read by crash as 0xffc1a00800000040 instead of 0x40 !!

 

With the patch:

 

crash> kmem -s ffff88085e640040

CACHE             OBJSIZE  ALLOCATED     TOTAL  SLABS  SSIZE  NAME

ffff8838ffc2c040     4096          1       336     48    32k  names_cache

  SLAB              MEMORY            NODE  TOTAL  ALLOCATED  FREE

  ffffea0021799000  ffff88085e640000     0      7          0     7

  FREE / [ALLOCATED]

   ffff88085e640000   <<< getting expected red zone address, and object free

crash> set redzone off

redzone: off

crash> kmem -s ffff88085e640040

CACHE             OBJSIZE  ALLOCATED     TOTAL  SLABS  SSIZE  NAME

ffff8838ffc2c040     4096          1       336     48    32k  names_cache

  SLAB              MEMORY            NODE  TOTAL  ALLOCATED  FREE

  ffffea0021799000  ffff88085e640000     0      7          0     7

  FREE / [ALLOCATED]

   ffff88085e640040  <<< getting expected object address, and object free


Got it, wonderful.
 

 

Now, with respect to:

> And also please add the kernel commit to patch log:

> kernel commit 2ca6d39b3102 ("slub: make ->red_left_pad unsigned int")

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2ca6d39b31022bb9e1dda77109e292517f701261

 

I don’t agree with this suggestion, this problem has nothing to do with “int” versus “unsigned int”.

It is only related to doing a load_ULONG() (64bit) instead of a load_INT() or load_UINT() (32bit).

For some releases, where we have a 32bit padding after red_left_pad, the problem won’t be noticed.

But without a 32bit padding after red_left_pad, crash will get a bogus red_left_pad causing all objects

to be reported as [ALLOCATED], and it will print bogus object addresses when using “set redzone off”.


It's true that the current issue is irrelevant to the kernel commit changes. I misunderstood your patch log:

"...The "red_left_pad" member within "struct kmem_cache" is currently an "unsigned int",
it used to be an "int", but it never was a "long...""

I have no other issues, so for the patch: Ack.

Thanks
Lianbo
 

 

Cheers,

Georges

 

 

 

From: lijiang <lijiang@xxxxxxxxxx>
Sent: Wednesday, September 4, 2024 5:13 AM
To: devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
Cc: Aureau, Georges (Kernel Tools ERT) <georges.aureau@xxxxxxx>
Subject: Re: [PATCH] “kmem address” not working properly when redzone is enabled

 

Hi, Aureau

Thank you for the fix.

 

On Thu, Aug 29, 2024 at 5:56PM <devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

Date: Thu, 29 Aug 2024 09:15:36 +0000
From: "Aureau, Georges (Kernel Tools ERT)" <georges.aureau@xxxxxxx>
Subject: [PATCH] “kmem address” not working properly
        when redzone is enabled
To: "devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx"
        <devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
Message-ID: <SJ0PR84MB1482E72F9E168B3B0CE885C89F962@xxxxxxxxxxxxxxxxxx
        RD84.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset="Windows-1252"

Crash “kmem address” not working properly when redzone is enabled.

When "slub_debug" is enabled with redzoning, "kmem address" does not work properly.
The "red_left_pad" member within "struct kmem_cache" is currently an "unsigned int",
it used to be an "int", but it never was a "long", hence "red_left_pad" in do_slab_slub()
was not initialized properly. This "red_left_pad" issue resulted in reporting free objects
as "[ALLOCATED]", and in reporting bogus object addresses when using "set redzone off".

 

Can you help add the result of the 'kmem address' command here? We can clearly see what error it is.

 

And also please add the kernel commit to patch log:

kernel commit 2ca6d39b3102 ("slub: make ->red_left_pad unsigned int")

 

Signed-off-by: Georges Aureau <georges.aureau@xxxxxxx>
--
diff --git a/memory.c b/memory.c
index a74ebaf..967a9cf 100644
--- a/memory.c
+++ b/memory.c
@@ -19637,7 +19637,8 @@ do_slab_slub(struct meminfo *si, int verbose)
        int i, free_objects, cpu_slab, is_free, node;
        ulong p, q;
 #define SLAB_RED_ZONE 0x00000400UL
-       ulong flags, red_left_pad;
+       ulong flags;
+       uint red_left_pad;

        if (!si->slab) {
                if (CRASHDEBUG(1))
@@ -19727,7 +19728,7 @@ do_slab_slub(struct meminfo *si, int verbose)
        if (VALID_MEMBER(kmem_cache_red_left_pad)) {
                flags = ULONG(si->cache_buf + OFFSET(kmem_cache_flags));
                if (flags & SLAB_RED_ZONE)
-                       red_left_pad = ULONG(si->cache_buf + OFFSET(kmem_cache_red_left_pad));
+                       red_left_pad = UINT(si->cache_buf + OFFSET(kmem_cache_red_left_pad));
        }

 

This change looks good to me, but I still have a question:

I can not reproduce the current issue, how did you reproduce this one? Can you help list the steps to reproduce?

 

 

Thanks

Lianbo

 

        for (p = vaddr; p < vaddr + objects * si->size; p += si->size) {

------------------------------

--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki

[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux