Re: [mm/slub] 3616799128: BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten

[Feng Tang]

On Sun, Jul 31, 2022 at 04:16:53PM +0800, Tang, Feng wrote:
> Hi Oliver,
>
> On Sun, Jul 31, 2022 at 02:53:17PM +0800, Sang, Oliver wrote:
> >
> >
> > Greeting,
> >
> > FYI, we noticed the following commit (built with gcc-11):
> >
> > commit: 3616799128612e04ed919579e2c7b0dccf6bcb00 ("[PATCH v3 3/3] mm/slub: extend redzone check to cover extra allocated kmalloc space than requested")
> > url: https://github.com/intel-lab-lkp/linux/commits/Feng-Tang/mm-slub-some-debug-enhancements/20220727-151318
> > base: git://git.kernel.org/cgit/linux/kernel/git/vbabka/slab.git for-next
> > patch link: https://lore.kernel.org/linux-mm/20220727071042....@intel.com
> >
> > in testcase: boot
> >
> > on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
> >
> > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> >
> >
> > If you fix the issue, kindly add following tag
> > Reported-by: kernel test robot <olive...@intel.com>
> >
> >
> > [ 50.637839][ T154] =============================================================================
> > [ 50.639937][ T154] BUG kmalloc-16 (Not tainted): kmalloc Redzone overwritten
> > [ 50.641291][ T154] -----------------------------------------------------------------------------
> > [ 50.641291][ T154]
> > [ 50.643617][ T154] 0xffff88810018464c-0xffff88810018464f @offset=1612. First byte 0x7 instead of 0xcc
> > [ 50.645311][ T154] Allocated in __sdt_alloc+0x258/0x457 age=14287 cpu=0 pid=1
> > [ 50.646584][ T154] ___slab_alloc+0x52b/0x5b6
> > [ 50.647411][ T154] __slab_alloc+0x1a/0x22
> > [ 50.648374][ T154] __kmalloc_node+0x10c/0x1e1
> > [ 50.649237][ T154] __sdt_alloc+0x258/0x457
> > [ 50.650060][ T154] build_sched_domains+0xae/0x10e8
> > [ 50.650981][ T154] sched_init_smp+0x30/0xa5
> > [ 50.651805][ T154] kernel_init_freeable+0x1c6/0x23b
> > [ 50.652767][ T154] kernel_init+0x14/0x127
> > [ 50.653594][ T154] ret_from_fork+0x1f/0x30
> > [ 50.654414][ T154] Slab 0xffffea0004006100 objects=28 used=28 fp=0x0000000000000000 flags=0x1fffc0000000201(locked|slab|node=0|zone=1|lastcpupid=0x3fff)
> > [ 50.656866][ T154] Object 0xffff888100184640 @offset=1600 fp=0xffff888100184520
> > [ 50.656866][ T154]
> > [ 50.658410][ T154] Redzone ffff888100184630: cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc ................
> > [ 50.660047][ T154] Object ffff888100184640: 00 32 80 00 81 88 ff ff 01 00 00 00 07 00 80 8a .2..............
> > [ 50.661837][ T154] Redzone ffff888100184650: cc cc cc cc cc cc cc cc ........
> > [ 50.663454][ T154] Padding ffff8881001846b4: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZ
> > [ 50.665225][ T154] CPU: 0 PID: 154 Comm: systemd-udevd Not tainted 5.19.0-rc5-00010-g361679912861 #1
> > [ 50.666861][ T154] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
> > [ 50.668694][ T154] Call Trace:
> > [ 50.669331][ T154] <TASK>
> > [ 50.669832][ T154] dump_stack_lvl+0x57/0x7d
> > [ 50.670601][ T154] check_bytes_and_report+0xca/0xfe
> > [ 50.671436][ T154] check_object+0xdc/0x24d
> > [ 50.672163][ T154] free_debug_processing+0x98/0x210
> > [ 50.673904][ T154] __slab_free+0x46/0x198
> > [ 50.675746][ T154] qlist_free_all+0xae/0xde
> > [ 50.676552][ T154] kasan_quarantine_reduce+0x10d/0x145
> > [ 50.677507][ T154] __kasan_slab_alloc+0x1c/0x5a
> > [ 50.678327][ T154] slab_post_alloc_hook+0x5a/0xa2
> > [ 50.680069][ T154] kmem_cache_alloc+0x102/0x135
> > [ 50.680938][ T154] getname_flags+0x4b/0x314
> > [ 50.681781][ T154] do_sys_openat2+0x7a/0x15c
> > [ 50.706848][ T154] Disabling lock debugging due to kernel taint
> > [ 50.707913][ T154] FIX kmalloc-16: Restoring kmalloc Redzone 0xffff88810018464c-0xffff88810018464f=0xcc
>
> Thanks for the report!
>
> From the log it happened when kasan is enabled, and my first guess is
> the data processing from kmalloc redzone handling had some conflict
> with kasan's in allocation path (though I tested some kernel config
> with KASAN enabled)
>
> Will study more about kasan and reproduce/debug this. thanks

Cc kansan mail list.

This is really related with KASAN debug, that in free path, some
kmalloc redzone ([orig_size+1, object_size]) area is written by
kasan to save free meta info.

The callstack is:

kfree
slab_free
slab_free_freelist_hook
slab_free_hook
__kasan_slab_free
____kasan_slab_free
kasan_set_free_info
kasan_set_track

And this issue only happens with "kmalloc-16" slab. Kasan has 2
tracks: alloc_track and free_track, for x86_64 test platform, most
of the slabs will reserve space for alloc_track, and reuse the
'object' area for free_track. The kasan free_track is 16 bytes
large, that it will occupy the whole 'kmalloc-16's object area,
so when kmalloc-redzone is enabled by this patch, the 'overwritten'
error is triggered.

But it won't hurt other kmalloc slabs, as kasan's free meta won't
conflict with kmalloc-redzone which stay in the latter part of
kmalloc area.

So the solution I can think of is:
* skip the kmalloc-redzone for kmalloc-16 only, or
* skip kmalloc-redzone if kasan is enabled, or
* let kasan reserve the free meta (16 bytes) outside of object
just like for alloc meta

I don't have way to test kasan's SW/HW tag configuration, which
is only enabled on arm64 now. And I don't know if there will
also be some conflict.

Thanks,
Feng

[Dmitry Vyukov]

/\/\/\

Please just not the last option. Memory consumption matters.

I would do whatever is the simplest, e.g. skip the check for
kmalloc-16 when KASAN is enabled.
Or does it make sense to prohibit KASAN+SLUB_DEBUG combination? Does
SLUB_DEBUG add anything on top of KASAN?

[Feng Tang]

Thanks for giving the suggestion. I'm fine with all these options,
and will also wait for Vlastimil and other developers opinion.

> Or does it make sense to prohibit KASAN+SLUB_DEBUG combination? Does
> SLUB_DEBUG add anything on top of KASAN?

I did a quick glance, seems the KASAN will select SLUB_DEBUG in
many cases, as shown in the lib/Kconfig.kasan:

config KASAN_GENERIC
...
select SLUB_DEBUG if SLUB

config KASAN_SW_TAGS
...
select SLUB_DEBUG if SLUB

Thanks,
Feng

[Christoph Lameter]

On Mon, 1 Aug 2022, Feng Tang wrote:

> > Or does it make sense to prohibit KASAN+SLUB_DEBUG combination? Does
> > SLUB_DEBUG add anything on top of KASAN?
>
> I did a quick glance, seems the KASAN will select SLUB_DEBUG in
> many cases, as shown in the lib/Kconfig.kasan:
>
> config KASAN_GENERIC
> ...
> select SLUB_DEBUG if SLUB
>
> config KASAN_SW_TAGS
> ...
> select SLUB_DEBUG if SLUB

SLUB_DEBUG is on by default on all distros. This just means that debugging
support is compiled in but not activated. Kasan etc could depend on
SLUB_DEBUG. Without SLUB_DEBUG the debugging infrastructure of SLUB that
is use by Kasan is not included.

If you want to enable debugging on bootup then you need to set
SLUB_DEBUG_ON.

[Vlastimil Babka]

Maybe we could add some hack that if both kasan and SLAB_STORE_USER is
enabled, we bump the stored orig_size from <16 to 16? Similar to what
__ksize() does.

[Feng Tang]

On Mon, Aug 01, 2022 at 10:23:23PM +0800, Vlastimil Babka wrote:
> On 8/1/22 08:21, Feng Tang wrote:

[snip]

How about the following patch:

---
diff --git a/mm/slub.c b/mm/slub.c
index added2653bb0..33bbac2afaef 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -830,6 +830,16 @@ static inline void set_orig_size(struct kmem_cache *s,
if (!slub_debug_orig_size(s))
return;

+#ifdef CONFIG_KASAN
+ /*
+ * When kasan is enabled, it could save its free meta data in the
+ * start part of object area, so skip the kmalloc redzone check
+ * for small kmalloc slabs to avoid the data conflict.
+ */
+ if (s->object_size <= 32)
+ orig_size = s->object_size;
+#endif
+
p += get_info_end(s);
p += sizeof(struct track) * 2;

I extend the size to 32 for potential's kasan meta data size increase.
This is tested locally, if people are OK with it, I can ask for 0Day's
help to verify this.

Thanks,
Feng

[Dmitry Vyukov]

This looks like the simplest workaround. And with a proper comment I
think it's fine.

[Dmitry Vyukov]

Where is set_orig_size() function defined? Don't see it upstream nor
in linux-next.
This looks fine but my only concern is that this should not increase
memory consumption when slub debug tracking is not enabled, which
should be the main operation mode when KASAN is enabled. But I can't
figure this out w/o context.

> Thanks,
> Feng
>
> >
> > > I don't have way to test kasan's SW/HW tag configuration, which
> > > is only enabled on arm64 now. And I don't know if there will
> > > also be some conflict.
> > >
> > > Thanks,
> > > Feng
> > >
> >
>

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/YujKCxu2lJJFm73P%40feng-skl.

[Feng Tang]

Yes, the patchset was only posted on LKML, and not in any tree now.
The link to the original patches is:

https://lore.kernel.org/lkml/20220727071042....@intel.com/t/

Thanks,
Feng

[Dmitry Vyukov]

I think this can be done only when CONFIG_KASAN_GENERIC.
Only CONFIG_KASAN_GENERIC stores free meta info in objects:
https://elixir.bootlin.com/linux/latest/source/mm/kasan/common.c#L176

And KASAN_HW_TAGS has chances of being enabled with DEBUG_SLUB in
real-world uses (with Arm MTE).

> > > +
> > > p += get_info_end(s);
> > > p += sizeof(struct track) * 2;
> > >
> > > I extend the size to 32 for potential's kasan meta data size increase.
> > > This is tested locally, if people are OK with it, I can ask for 0Day's
> > > help to verify this.
> >
> > Where is set_orig_size() function defined? Don't see it upstream nor
> > in linux-next.
> > This looks fine but my only concern is that this should not increase
> > memory consumption when slub debug tracking is not enabled, which
> > should be the main operation mode when KASAN is enabled. But I can't
> > figure this out w/o context.
>
> Yes, the patchset was only posted on LKML, and not in any tree now.
> The link to the original patches is:
>
> https://lore.kernel.org/lkml/20220727071042....@intel.com/t/

Lots of code...

This SLAB_STORE_USER seems to be set on all kmalloc slabs by default
when CONFIG_SLUB_DEBUG is enabled, right?
And KASAN enables CONFIG_SLUB_DEBUG, this means that this is stored
always when KASAN is enabled? Looks wrong.

[Feng Tang]

Thanks for the catch! will change.

> And KASAN_HW_TAGS has chances of being enabled with DEBUG_SLUB in
> real-world uses (with Arm MTE).

I only have device to test the kasan-generic mode, and not SW/HW tag.
But if there is conflict, we may have to apply the similar solution :)

>
> > > > +
> > > > p += get_info_end(s);
> > > > p += sizeof(struct track) * 2;
> > > >
> > > > I extend the size to 32 for potential's kasan meta data size increase.
> > > > This is tested locally, if people are OK with it, I can ask for 0Day's
> > > > help to verify this.
> > >
> > > Where is set_orig_size() function defined? Don't see it upstream nor
> > > in linux-next.
> > > This looks fine but my only concern is that this should not increase
> > > memory consumption when slub debug tracking is not enabled, which
> > > should be the main operation mode when KASAN is enabled. But I can't
> > > figure this out w/o context.
> >
> > Yes, the patchset was only posted on LKML, and not in any tree now.
> > The link to the original patches is:
> >
> > https://lore.kernel.org/lkml/20220727071042....@intel.com/t/
>
> Lots of code...
>
> This SLAB_STORE_USER seems to be set on all kmalloc slabs by default
> when CONFIG_SLUB_DEBUG is enabled, right?

Christoph has explained in one earlier mail that CONFIG_SLUB_DEBUG only
compile in the debug support but not activate it. Option CONFIG_SLUB_DEBUG_ON
will enable it, and each slub debug flag bits can also be enabled
by changing kernel cmdline for some or all slabs.

> And KASAN enables CONFIG_SLUB_DEBUG, this means that this is stored
> always when KASAN is enabled? Looks wrong.

Thanks,
Feng

[Vlastimil Babka]

Is there maybe some KASAN macro we can use instead of hardcoding 32?

>
> Where is set_orig_size() function defined? Don't see it upstream nor
> in linux-next.
> This looks fine but my only concern is that this should not increase
> memory consumption when slub debug tracking is not enabled, which
> should be the main operation mode when KASAN is enabled. But I can't
> figure this out w/o context.

It won't increase memory consumption even if slub_debug tracking is enabled.
It just fakes a bit the size that was passed to kmalloc() and which we newly
store (thanks to Feng's patches) for statistics and debugging purposes.

[Dmitry Vyukov]

kasan_free_meta is placed in the object data after freeing, so it can
be sizeof(kasan_free_meta)

[Dmitry Vyukov]

On Tue, 2 Aug 2022 at 11:43, Vlastimil Babka <vba...@suse.cz> wrote:
>

Then it looks good to me. Thanks for double checking.

[Feng Tang]

'kasan_free_meta' is defined in mm/kasan/kasan.h, to use it we need to
include "../kasan/kasan.h" in slub.c, or move its definition to
"include/linux/kasan.h"

Another idea is to save the info in kasan_info, like:

---
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b092277bf48d..97e899948d0b 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -100,6 +100,7 @@ static inline bool kasan_has_integrated_init(void)
struct kasan_cache {
int alloc_meta_offset;
int free_meta_offset;
+ int free_meta_size;
bool is_kmalloc;
};

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index c40c0e7b3b5f..7bd82c5ec264 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -178,6 +178,8 @@ void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
return;
}

+ cache->kasan_info.free_meta_size = sizeof(struct free_meta_offset);
+
/*
* Add free meta into redzone when it's not possible to store
* it in the object. This is the case when:

Thanks,
Feng

[Dmitry Vyukov]

Storing it here looks fine to me.
But I would name it based on the meaning for external users (i.e. that
many bytes are occupied by kasan in freed objects). For some caches
KASAN does not store anything in freed objects at all.

[Feng Tang]

OK, please review the below patch, thanks!

- Feng

---8<---
From c4fc739ea4d5222f0aba4b42b59668d64a010082 Mon Sep 17 00:00:00 2001
From: Feng Tang <feng...@intel.com>
Date: Thu, 4 Aug 2022 13:25:35 +0800
Subject: [PATCH] mm: kasan: Add free_meta size info in struct kasan_cache

When kasan is enabled for slab/slub, it may save kasan' free_meta
data in the former part of slab object data area in slab object
free path, which works fine.

There is ongoing effort to extend slub's debug function which will
redzone the latter part of kmalloc object area, and when both of
the debug are enabled, there is possible conflict, especially when
the kmalloc object has small size, as caught by 0Day bot [1]

For better information for slab/slub, add free_meta's data size
info 'kasan_cache', so that its users can take right action to
avoid data conflict.

[1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/

Reported-by: kernel test robot <olive...@intel.com>

Signed-off-by: Feng Tang <feng...@intel.com>
---
include/linux/kasan.h | 2 ++
mm/kasan/common.c | 2 ++
2 files changed, 4 insertions(+)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b092277bf48d..293bdaa0ba09 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -100,6 +100,8 @@ static inline bool kasan_has_integrated_init(void)

struct kasan_cache {
int alloc_meta_offset;
int free_meta_offset;

+ /* size of free_meta data saved in object's data area */
+ int free_meta_size_in_object;

bool is_kmalloc;
};

diff --git a/mm/kasan/common.c b/mm/kasan/common.c

index 78be2beb7453..a627efa267d1 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -201,6 +201,8 @@ void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
*size = ok_size;
}
+ } else {
+ cache->kasan_info.free_meta_size_in_object = sizeof(struct kasan_free_meta);
}

/* Calculate size with optimal redzone. */
--
2.27.0

[Dmitry Vyukov]

Acked-by: Dmitry Vyukov <dvy...@google.com>

I assume there will be a second patch that uses
free_meta_size_in_object in slub debug code.

[Feng Tang]

On Thu, Aug 04, 2022 at 06:47:58PM +0800, Dmitry Vyukov wrote:
> On Thu, 4 Aug 2022 at 08:29, Feng Tang <feng...@intel.com> wrote:

[...]

> >
> > ---8<---
> > From c4fc739ea4d5222f0aba4b42b59668d64a010082 Mon Sep 17 00:00:00 2001
> > From: Feng Tang <feng...@intel.com>
> > Date: Thu, 4 Aug 2022 13:25:35 +0800
> > Subject: [PATCH] mm: kasan: Add free_meta size info in struct kasan_cache
> >
> > When kasan is enabled for slab/slub, it may save kasan' free_meta
> > data in the former part of slab object data area in slab object
> > free path, which works fine.
> >
> > There is ongoing effort to extend slub's debug function which will
> > redzone the latter part of kmalloc object area, and when both of
> > the debug are enabled, there is possible conflict, especially when
> > the kmalloc object has small size, as caught by 0Day bot [1]
> >
> > For better information for slab/slub, add free_meta's data size
> > info 'kasan_cache', so that its users can take right action to
> > avoid data conflict.
> >
> > [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/
> > Reported-by: kernel test robot <olive...@intel.com>
> > Signed-off-by: Feng Tang <feng...@intel.com>
>
> Acked-by: Dmitry Vyukov <dvy...@google.com>

Thanks for your suggestion and review!

> I assume there will be a second patch that uses
> free_meta_size_in_object in slub debug code.

Yes, it will be called in the slub kmalloc object redzone debug code.

Thanks,
Feng

[Feng Tang]

Hi Oliver,

Could you help to check if the below combined patch fix the problem
you reported? thanks!

- Feng

---

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b092277bf48d6..293bdaa0ba09c 100644

--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -100,6 +100,8 @@ static inline bool kasan_has_integrated_init(void)
struct kasan_cache {
int alloc_meta_offset;
int free_meta_offset;
+ /* size of free_meta data saved in object's data area */
+ int free_meta_size_in_object;
bool is_kmalloc;
};

diff --git a/mm/kasan/common.c b/mm/kasan/common.c

index c40c0e7b3b5f1..9d2994dbe4e7a 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -200,6 +200,8 @@ void __kasan_cache_create(struct kmem_cache *cache, unsigned int *size,

cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
*size = ok_size;
}
+ } else {
+ cache->kasan_info.free_meta_size_in_object = sizeof(struct kasan_free_meta);
}

/* Calculate size with optimal redzone. */

diff --git a/mm/slub.c b/mm/slub.c
index added2653bb03..272dcdbaaa03b 100644

--- a/mm/slub.c
+++ b/mm/slub.c
@@ -830,6 +830,16 @@ static inline void set_orig_size(struct kmem_cache *s,
if (!slub_debug_orig_size(s))
return;

+#ifdef CONFIG_KASAN_GENERIC
+ /*
+ * kasn could save its free meta data in the start part of object
+ * area, so skip the redzone check if kasan's meta data size is
+ * bigger enough to possibly overlap with kmalloc redzone
+ */
+ if (s->kasan_info.free_meta_size_in_object * 2 > s->object_size)

+ orig_size = s->object_size;
+#endif
+
p += get_info_end(s);
p += sizeof(struct track) * 2;

[Oliver Sang]

Hi Feng,

On Mon, Aug 15, 2022 at 03:27:43PM +0800, Feng Tang wrote:
> Hi Oliver,
>
> Could you help to check if the below combined patch fix the problem
> you reported? thanks!

I applied below patch upon 3616799128:
28b34693c816e9 (linux-devel/fixup-3616799128) fix for 3616799128: BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten
3616799128612e (linux-review/Feng-Tang/mm-slub-some-debug-enhancements/20220727-151318) mm/slub: extend redzone check to cover extra allocated kmalloc space than requested
acc77d62f91ccc mm/slub: only zero the requested size of buffer for kzalloc


confirmed the issue gone:

=========================================================================================
compiler/kconfig/rootfs/sleep/tbox_group/testcase:
gcc-11/x86_64-randconfig-a005-20220117/debian-11.1-x86_64-20220510.cgz/300/vm-snb/boot


acc77d62f91ccca2 3616799128612e04ed919579e2c 28b34693c816e9fcbe42bdd341e
---------------- --------------------------- ---------------------------
fail:runs %reproduction fail:runs %reproduction fail:runs
| | | | |
:20 95% 19:20 0% :22 dmesg.BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten
:20 95% 19:20 0% :22 dmesg.BUG_kmalloc-#(Tainted:G_B):kmalloc_Redzone_overwritten

[Feng Tang]

On Tue, Aug 16, 2022 at 09:27:39PM +0800, Sang, Oliver wrote:
> Hi Feng,
>
> On Mon, Aug 15, 2022 at 03:27:43PM +0800, Feng Tang wrote:
> > Hi Oliver,
> >
> > Could you help to check if the below combined patch fix the problem
> > you reported? thanks!
>
> I applied below patch upon 3616799128:
> 28b34693c816e9 (linux-devel/fixup-3616799128) fix for 3616799128: BUG_kmalloc-#(Not_tainted):kmalloc_Redzone_overwritten
> 3616799128612e (linux-review/Feng-Tang/mm-slub-some-debug-enhancements/20220727-151318) mm/slub: extend redzone check to cover extra allocated kmalloc space than requested
> acc77d62f91ccc mm/slub: only zero the requested size of buffer for kzalloc
>
>
> confirmed the issue gone:

Many thanks for helping testing!

- Feng