linux-kernel - Re: [PATCH v4 1/4] mm/slub: enable debugging memory wasting of kmalloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxRzCX8WqOv3KESJ@feng-clx>
Date:   Sun, 4 Sep 2022 17:42:33 +0800
From:   Feng Tang <feng.tang@...el.com>
To:     Hyeonggon Yoo <42.hyeyoo@...il.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Christoph Lameter <cl@...ux.com>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        "Dmitry Vyukov" <dvyukov@...gle.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Robin Murphy <robin.murphy@....com>,
        "John Garry" <john.garry@...wei.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>
Subject: Re: [PATCH v4 1/4] mm/slub: enable debugging memory wasting of
 kmalloc

On Sun, Sep 04, 2022 at 05:03:34PM +0800, Hyeonggon Yoo wrote:
[...]
> > > 
> > > This patch is okay but with patch 4, init_object() initializes redzone/poison area
> > > using s->object_size, and init_kmalloc_object() fixes redzone/poison area using orig_size.
> > > Why not do it in init_object() in the first time?
> > > 
> > > Also, updating redzone/poison area after alloc_single_from_new_slab()
> > > (outside list_lock, after adding slab to list) will introduce races with validation.
> > > 
> > > So I think doing set_orig_size()/init_kmalloc_object() in alloc_debug_processing() would make more sense.
> > 
> > Yes, this makes sense, and in v3, kmalloc redzone/poison setup was
> > done in alloc_debug_processing() (through init_object()). When
> > rebasing to v4, I met the classical problem: how to pass 'orig_size'
> > parameter :)
> > 
> > In latest 'for-next' branch, one call path for alloc_debug_processing()
> > is
> >   ___slab_alloc
> >     get_partial
> >       get_any_partial
> >         get_partial_node
> >           alloc_debug_processing
> > 
> > Adding 'orig_size' paramter to all these function looks horrible, and
> > I couldn't figure out a good way and chosed to put those ops after
> > 'set_track()'
> 
> IMO adding a parameter to them isn't too horrible...
> I don't see better solution than adding a parameter with current implementation.
> (Yeah, the code is quite complicated...)
> 
> It won't affect performance to meaningful degree as most of
> allocations will be served from cpu slab or percpu partial list. 

Thanks for the suggestion! I'm fine with it and just afraid other
developers may dislike the extra parameter. 

The race condition you mentioned is a valid concern, and I have thought
about it, one way is moving the set_orig_size() after the redzone/poision
setup, and in 'check_object()' we can detect whether the 'orig_size' is
set, and skip that check if it's not set yet. As the manual validate_slab
triggered from sysfs interface is a rare debug activity, I think skipping
one object shouldn't hurt much.

Thanks,
Feng

> -- 
> Thanks,
> Hyeonggon
>