| Message ID | 20240823062415.3632189-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V1] mm, slub: avoid zeroing kmalloc redzone | expand |
On Fri, Aug 23, 2024 at 02:24:15PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > With commit 946fa0dbf2d8 > ("mm/slub: extend redzone check to extra allocated kmalloc space than requested"), > setting orig_size treats the wasted space (object_size - orig_size) as > redzones. But (in check_object()) when orig_size is set to zero, the entire > object is perceived as a redzone. To a valid allocated kmalloc space, > when init_on_free=1, the wasted space and the orig_size should > not be cleared to 0, otherwise there will be kernel dump: > > [ 0.000000] ============================================================================= > [ 0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone overwritten > [ 0.000000] ----------------------------------------------------------------------------- > [ 0.000000] > [ 0.000000] 0xffff000010032858-0xffff00001003285f @offset=2136. First byte 0x0 instead of 0xcc > [ 0.000000] FIX kmalloc-8: Restoring kmalloc Redzone 0xffff000010032858-0xffff00001003285f=0xcc > [ 0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23 fp=0xffff000010032a18 flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1ffff) > [ 0.000000] Object 0xffff000010032858 @offset=2136 fp=0xffff0000100328c8 > [ 0.000000] > [ 0.000000] Redzone ffff000010032850: cc cc cc cc cc cc cc cc ........ > [ 0.000000] Object ffff000010032858: cc cc cc cc cc cc cc cc ........ > [ 0.000000] Redzone ffff000010032860: cc cc cc cc cc cc cc cc ........ > [ 0.000000] Padding ffff0000100328b4: 00 00 00 00 00 00 00 00 00 00 00 00 ............ > [ 0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144 > [ 0.000000] Hardware name: NXP i.MX95 19X19 board (DT) > [ 0.000000] Call trace: > [ 0.000000] dump_backtrace+0x90/0xe8 > [ 0.000000] show_stack+0x18/0x24 > [ 0.000000] dump_stack_lvl+0x74/0x8c > [ 0.000000] dump_stack+0x18/0x24 > [ 0.000000] print_trailer+0x150/0x218 > [ 0.000000] check_object+0xe4/0x454 > [ 0.000000] free_to_partial_list+0x2f8/0x5ec > > To address the issue, use orig_size to clear the used area. And restore > the value of orig_size after clear the remaining area. > > When CONFIG_SLUB_DEBUG not defined, (get_orig_size()' directly returns > s->object_size. So when using memset to init the area, the size can simply > be orig_size, as orig_size returns object_size when CONFIG_SLUB_DEBUG not > enabled. And orig_size can never be bigger than object_size. > > Fixes: 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") Thanks for the fix! I missed to test the 'init_on_free' case back then. Reviewed-by: Feng Tang <feng.tang@intel.com> with one small nit below > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > > RFC->V1: > Update commit log (Per Hyeonggon) > Use orig_size to do memset(Per Hyeonggon) > Add get_orig_size and set_orig_size when CONFIG_SLUB_DEBUG not enabled(kernel test robot) > https://lore.kernel.org/all/20240819064115.385086-1-peng.fan@oss.nxp.com/ > > mm/slub.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 94f5a4143825..a5fbeb2835b1 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -1895,6 +1895,15 @@ static inline void inc_slabs_node(struct kmem_cache *s, int node, > static inline void dec_slabs_node(struct kmem_cache *s, int node, > int objects) {} > > +static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) > +{ > + return s->object_size; > +} > + > +static inline void set_orig_size(struct kmem_cache *s, void *object, > + unsigned int orig_size) > +{} Current get_orig_size() and set_orig_size() are protected by CONFIG_SLUB_DEUG=y macro, and with this patch, they will be called in both ON and OFF case. Maybe we can just lift those existing functions out of the "#ifdef CONFIG_SLUB_DEBUG" protection? Thanks, Feng
> Subject: Re: [PATCH V1] mm, slub: avoid zeroing kmalloc redzone > > On Fri, Aug 23, 2024 at 02:24:15PM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > With commit 946fa0dbf2d8 > > ("mm/slub: extend redzone check to extra allocated kmalloc space > than > > requested"), setting orig_size treats the wasted space (object_size - > > orig_size) as redzones. But (in check_object()) when orig_size is set > > to zero, the entire object is perceived as a redzone. To a valid > > allocated kmalloc space, when init_on_free=1, the wasted space and > the > > orig_size should not be cleared to 0, otherwise there will be kernel > dump: > > > > [ 0.000000] > ============================================================ > ================= > > [ 0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone > overwritten > > [ 0.000000] ----------------------------------------------------------------------------- > > [ 0.000000] > > [ 0.000000] 0xffff000010032858-0xffff00001003285f > @offset=2136. First byte 0x0 instead of 0xcc > > [ 0.000000] FIX kmalloc-8: Restoring kmalloc Redzone > 0xffff000010032858-0xffff00001003285f=0xcc > > [ 0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23 > fp=0xffff000010032a18 > flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1f > fff) > > [ 0.000000] Object 0xffff000010032858 @offset=2136 > fp=0xffff0000100328c8 > > [ 0.000000] > > [ 0.000000] Redzone ffff000010032850: cc cc cc cc cc cc cc > cc ........ > > [ 0.000000] Object ffff000010032858: cc cc cc cc cc cc cc > cc ........ > > [ 0.000000] Redzone ffff000010032860: cc cc cc cc cc cc cc > cc ........ > > [ 0.000000] Padding ffff0000100328b4: 00 00 00 00 00 00 00 00 > 00 00 00 00 ............ > > [ 0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted > 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144 > > [ 0.000000] Hardware name: NXP i.MX95 19X19 board (DT) > > [ 0.000000] Call trace: > > [ 0.000000] dump_backtrace+0x90/0xe8 > > [ 0.000000] show_stack+0x18/0x24 > > [ 0.000000] dump_stack_lvl+0x74/0x8c > > [ 0.000000] dump_stack+0x18/0x24 > > [ 0.000000] print_trailer+0x150/0x218 > > [ 0.000000] check_object+0xe4/0x454 > > [ 0.000000] free_to_partial_list+0x2f8/0x5ec > > > > To address the issue, use orig_size to clear the used area. And > > restore the value of orig_size after clear the remaining area. > > > > When CONFIG_SLUB_DEBUG not defined, (get_orig_size()' directly > returns > > s->object_size. So when using memset to init the area, the size can > > s->simply > > be orig_size, as orig_size returns object_size when > CONFIG_SLUB_DEBUG > > not enabled. And orig_size can never be bigger than object_size. > > > > Fixes: 946fa0dbf2d8 ("mm/slub: extend redzone check to extra > allocated > > kmalloc space than requested") > > Thanks for the fix! I missed to test the 'init_on_free' case back then. > > Reviewed-by: Feng Tang <feng.tang@intel.com> > > with one small nit below > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > > > RFC->V1: > > Update commit log (Per Hyeonggon) > > Use orig_size to do memset(Per Hyeonggon) Add get_orig_size and > > set_orig_size when CONFIG_SLUB_DEBUG not enabled(kernel test > robot) > > > > > > > > mm/slub.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index 94f5a4143825..a5fbeb2835b1 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -1895,6 +1895,15 @@ static inline void inc_slabs_node(struct > > kmem_cache *s, int node, static inline void dec_slabs_node(struct > kmem_cache *s, int node, > > int objects) > {} > > > > +static inline unsigned int get_orig_size(struct kmem_cache *s, void > > +*object) { > > + return s->object_size; > > +} > > + > > +static inline void set_orig_size(struct kmem_cache *s, void *object, > > + unsigned int orig_size) > > +{} > > Current get_orig_size() and set_orig_size() are protected by > CONFIG_SLUB_DEUG=y macro, and with this patch, they will be called > in both ON and OFF case. Maybe we can just lift those existing > functions out of the "#ifdef CONFIG_SLUB_DEBUG" protection? This will build a bit more code when CONFIG_SLUB_DEBUG is not defined. But it should be fine. I will wait to see if other people have comments on this. Thanks, Peng. > > Thanks, > Feng
On 8/25/24 15:05, Feng Tang wrote: > On Fri, Aug 23, 2024 at 02:24:15PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> With commit 946fa0dbf2d8 >> ("mm/slub: extend redzone check to extra allocated kmalloc space than requested"), >> setting orig_size treats the wasted space (object_size - orig_size) as >> redzones. But (in check_object()) when orig_size is set to zero, the entire >> object is perceived as a redzone. To a valid allocated kmalloc space, >> when init_on_free=1, the wasted space and the orig_size should >> not be cleared to 0, otherwise there will be kernel dump: Could we make it more clear? Since commit 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested"), setting orig_size treats the wasted space (object_size - orig_size) as a redzone. However with init_on_free=1 we clear the full object->size, including the redzone. Additionally we clear the object metadata, including the stored orig_size, making it zero, which makes check_object() treat the the whole object as a redzone. These issues lead to the following BUG report with "slub_debug=FUZ init_on_free=1": >> >> [ 0.000000] ============================================================================= >> [ 0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone overwritten >> [ 0.000000] ----------------------------------------------------------------------------- >> [ 0.000000] >> [ 0.000000] 0xffff000010032858-0xffff00001003285f @offset=2136. First byte 0x0 instead of 0xcc >> [ 0.000000] FIX kmalloc-8: Restoring kmalloc Redzone 0xffff000010032858-0xffff00001003285f=0xcc >> [ 0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23 fp=0xffff000010032a18 flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1ffff) >> [ 0.000000] Object 0xffff000010032858 @offset=2136 fp=0xffff0000100328c8 >> [ 0.000000] >> [ 0.000000] Redzone ffff000010032850: cc cc cc cc cc cc cc cc ........ >> [ 0.000000] Object ffff000010032858: cc cc cc cc cc cc cc cc ........ >> [ 0.000000] Redzone ffff000010032860: cc cc cc cc cc cc cc cc ........ >> [ 0.000000] Padding ffff0000100328b4: 00 00 00 00 00 00 00 00 00 00 00 00 ............ >> [ 0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144 >> [ 0.000000] Hardware name: NXP i.MX95 19X19 board (DT) >> [ 0.000000] Call trace: >> [ 0.000000] dump_backtrace+0x90/0xe8 >> [ 0.000000] show_stack+0x18/0x24 >> [ 0.000000] dump_stack_lvl+0x74/0x8c >> [ 0.000000] dump_stack+0x18/0x24 >> [ 0.000000] print_trailer+0x150/0x218 >> [ 0.000000] check_object+0xe4/0x454 >> [ 0.000000] free_to_partial_list+0x2f8/0x5ec >> >> To address the issue, use orig_size to clear the used area. And restore >> the value of orig_size after clear the remaining area. >> >> When CONFIG_SLUB_DEBUG not defined, (get_orig_size()' directly returns >> s->object_size. So when using memset to init the area, the size can simply >> be orig_size, as orig_size returns object_size when CONFIG_SLUB_DEBUG not >> enabled. And orig_size can never be bigger than object_size. >> >> Fixes: 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") > > Thanks for the fix! I missed to test the 'init_on_free' case back then. > > Reviewed-by: Feng Tang <feng.tang@intel.com> By the way Feng, have you noticed the other issue we have, with krealloc() and redzoning? Want to look at that? Thanks. https://lore.kernel.org/all/44fa564b-9c8f-4ac2-bce3-f6d2c99b73b7@suse.cz/ > with one small nit below > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> --- >> >> RFC->V1: >> Update commit log (Per Hyeonggon) >> Use orig_size to do memset(Per Hyeonggon) >> Add get_orig_size and set_orig_size when CONFIG_SLUB_DEBUG not enabled(kernel test robot) >> https://lore.kernel.org/all/20240819064115.385086-1-peng.fan@oss.nxp.com/ >> >> mm/slub.c | 20 ++++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 94f5a4143825..a5fbeb2835b1 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -1895,6 +1895,15 @@ static inline void inc_slabs_node(struct kmem_cache *s, int node, >> static inline void dec_slabs_node(struct kmem_cache *s, int node, >> int objects) {} >> >> +static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) >> +{ >> + return s->object_size; >> +} >> + >> +static inline void set_orig_size(struct kmem_cache *s, void *object, >> + unsigned int orig_size) >> +{} > > Current get_orig_size() and set_orig_size() are protected by > CONFIG_SLUB_DEUG=y macro, and with this patch, they will be called > in both ON and OFF case. Maybe we can just lift those existing > functions out of the "#ifdef CONFIG_SLUB_DEBUG" protection? I agree. As for the "code is larger for !CONFIG_SLUB_DEBUG" concern I think the code will be eliminated anyway, because of the if (!slub_debug_orig_size(s)) return; and slub_debug_orig_size() does kmem_cache_debug_flags() which is a compile-time false for !CONFIG_SLUB_DEBUG. > Thanks, > Feng
On Wed, Aug 28, 2024 at 06:53:11PM +0200, Vlastimil Babka wrote: > On 8/25/24 15:05, Feng Tang wrote: > > On Fri, Aug 23, 2024 at 02:24:15PM +0800, Peng Fan (OSS) wrote: > >> From: Peng Fan <peng.fan@nxp.com> > >> > >> With commit 946fa0dbf2d8 > >> ("mm/slub: extend redzone check to extra allocated kmalloc space than requested"), > >> setting orig_size treats the wasted space (object_size - orig_size) as > >> redzones. But (in check_object()) when orig_size is set to zero, the entire > >> object is perceived as a redzone. To a valid allocated kmalloc space, > >> when init_on_free=1, the wasted space and the orig_size should > >> not be cleared to 0, otherwise there will be kernel dump: > > Could we make it more clear? > > Since commit 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated > kmalloc space than requested"), setting orig_size treats the wasted space > (object_size - orig_size) as a redzone. However with init_on_free=1 we clear > the full object->size, including the redzone. > > Additionally we clear the object metadata, including the stored orig_size, > making it zero, which makes check_object() treat the the whole object as a > redzone. > > These issues lead to the following BUG report with "slub_debug=FUZ > init_on_free=1": > > >> > >> [ 0.000000] ============================================================================= > >> [ 0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone overwritten > >> [ 0.000000] ----------------------------------------------------------------------------- > >> [ 0.000000] > >> [ 0.000000] 0xffff000010032858-0xffff00001003285f @offset=2136. First byte 0x0 instead of 0xcc > >> [ 0.000000] FIX kmalloc-8: Restoring kmalloc Redzone 0xffff000010032858-0xffff00001003285f=0xcc > >> [ 0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23 fp=0xffff000010032a18 flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1ffff) > >> [ 0.000000] Object 0xffff000010032858 @offset=2136 fp=0xffff0000100328c8 > >> [ 0.000000] > >> [ 0.000000] Redzone ffff000010032850: cc cc cc cc cc cc cc cc ........ > >> [ 0.000000] Object ffff000010032858: cc cc cc cc cc cc cc cc ........ > >> [ 0.000000] Redzone ffff000010032860: cc cc cc cc cc cc cc cc ........ > >> [ 0.000000] Padding ffff0000100328b4: 00 00 00 00 00 00 00 00 00 00 00 00 ............ > >> [ 0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144 > >> [ 0.000000] Hardware name: NXP i.MX95 19X19 board (DT) > >> [ 0.000000] Call trace: > >> [ 0.000000] dump_backtrace+0x90/0xe8 > >> [ 0.000000] show_stack+0x18/0x24 > >> [ 0.000000] dump_stack_lvl+0x74/0x8c > >> [ 0.000000] dump_stack+0x18/0x24 > >> [ 0.000000] print_trailer+0x150/0x218 > >> [ 0.000000] check_object+0xe4/0x454 > >> [ 0.000000] free_to_partial_list+0x2f8/0x5ec > >> > >> To address the issue, use orig_size to clear the used area. And restore > >> the value of orig_size after clear the remaining area. > >> > >> When CONFIG_SLUB_DEBUG not defined, (get_orig_size()' directly returns > >> s->object_size. So when using memset to init the area, the size can simply > >> be orig_size, as orig_size returns object_size when CONFIG_SLUB_DEBUG not > >> enabled. And orig_size can never be bigger than object_size. > >> > >> Fixes: 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") > > > > Thanks for the fix! I missed to test the 'init_on_free' case back then. > > > > Reviewed-by: Feng Tang <feng.tang@intel.com> > > By the way Feng, have you noticed the other issue we have, with krealloc() > and redzoning? Want to look at that? Thanks. > > https://lore.kernel.org/all/44fa564b-9c8f-4ac2-bce3-f6d2c99b73b7@suse.cz/ Sure. Will check that thread (it might take a while as I just came back from vacation). thanks - Feng
diff --git a/mm/slub.c b/mm/slub.c index 94f5a4143825..a5fbeb2835b1 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1895,6 +1895,15 @@ static inline void inc_slabs_node(struct kmem_cache *s, int node, static inline void dec_slabs_node(struct kmem_cache *s, int node, int objects) {} +static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) +{ + return s->object_size; +} + +static inline void set_orig_size(struct kmem_cache *s, void *object, + unsigned int orig_size) +{} + #ifndef CONFIG_SLUB_TINY static bool freelist_corrupted(struct kmem_cache *s, struct slab *slab, void **freelist, void *nextfree) @@ -2282,14 +2291,21 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init, */ if (unlikely(init)) { int rsize; - unsigned int inuse; + unsigned int inuse, orig_size; inuse = get_info_end(s); + orig_size = get_orig_size(s, x); if (!kasan_has_integrated_init()) - memset(kasan_reset_tag(x), 0, s->object_size); + memset(kasan_reset_tag(x), 0, orig_size); rsize = (s->flags & SLAB_RED_ZONE) ? s->red_left_pad : 0; memset((char *)kasan_reset_tag(x) + inuse, 0, s->size - inuse - rsize); + /* + * Restore orig_size, otherwize kmalloc redzone overwritten + * would be reported + */ + set_orig_size(s, x, orig_size); + } /* KASAN might put x into memory quarantine, delaying its reuse. */ return !kasan_slab_free(s, x, init, still_accessible);