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 >
