| Message ID | 20230711134623.12695-4-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm/slub: remove redundant kasan_reset_tag() from freelist_ptr calculations | expand |
On Tue, Jul 11, 2023 at 03:46:25PM +0200, Vlastimil Babka wrote: > freelist_dereference() is a one-liner only used from get_freepointer(). > Remove it and make get_freepointer() call freelist_ptr_decode() > directly to make the code easier to follow. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slub.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 07edad305512..c4556a5dab4b 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -397,18 +397,14 @@ static inline void *freelist_ptr_decode(const struct kmem_cache *s, > return decoded; > } > > -/* Returns the freelist pointer recorded at location ptr_addr. */ > -static inline void *freelist_dereference(const struct kmem_cache *s, > - void *ptr_addr) > -{ > - return freelist_ptr_decode(s, *(freeptr_t *)(ptr_addr), > - (unsigned long)ptr_addr); > -} > - > static inline void *get_freepointer(struct kmem_cache *s, void *object) > { > - object = kasan_reset_tag(object); > - return freelist_dereference(s, (freeptr_t *)(object + s->offset)); > + unsigned long ptr_addr; > + freeptr_t p; > + > + ptr_addr = ((unsigned long)kasan_reset_tag(object)) + s->offset; > + p = *(freeptr_t *)(ptr_addr); > + return freelist_ptr_decode(s, p, ptr_addr); > } > > #ifndef CONFIG_SLUB_TINY > -- > 2.41.0 > I like reducing the complexity here, but I find dropping the "object" reassignment makes this a bit harder to read. What about: object = kasan_reset_tag(object); unsigned long ptr_addr = (unsigned long)object + s->offset; freeptr_t p = *(freeptr_t *)(ptr_addr); return freelist_ptr_decode(s, p, ptr_addr); ? They're the same result, so either way: Acked-by: Kees Cook <keescook@chromium.org>
On 7/11/23 18:21, Kees Cook wrote: > On Tue, Jul 11, 2023 at 03:46:25PM +0200, Vlastimil Babka wrote: >> >> #ifndef CONFIG_SLUB_TINY >> -- >> 2.41.0 >> > > I like reducing the complexity here, but I find dropping the "object" > reassignment makes this a bit harder to read. What about: Alright. > object = kasan_reset_tag(object); > unsigned long ptr_addr = (unsigned long)object + s->offset; > freeptr_t p = *(freeptr_t *)(ptr_addr); Are we really so benevolent with declaration-after-statement now? :) > return freelist_ptr_decode(s, p, ptr_addr); > > ? > > They're the same result, so either way: > > Acked-by: Kees Cook <keescook@chromium.org> >
On 7/13/23 09:44, Vlastimil Babka wrote: > On 7/11/23 18:21, Kees Cook wrote: >> On Tue, Jul 11, 2023 at 03:46:25PM +0200, Vlastimil Babka wrote: >>> >>> #ifndef CONFIG_SLUB_TINY >>> -- >>> 2.41.0 >>> >> >> I like reducing the complexity here, but I find dropping the "object" >> reassignment makes this a bit harder to read. What about: > > Alright. > >> object = kasan_reset_tag(object); >> unsigned long ptr_addr = (unsigned long)object + s->offset; >> freeptr_t p = *(freeptr_t *)(ptr_addr); > > Are we really so benevolent with declaration-after-statement now? :) I've left the declarations separate for now so it's similar to get_freepointer_safe(). Pushed the result to slab/for-6.6/cleanup and for-next. Thanks for the reviews! >> return freelist_ptr_decode(s, p, ptr_addr); >> >> ? >> >> They're the same result, so either way: >> >> Acked-by: Kees Cook <keescook@chromium.org> >> >
diff --git a/mm/slub.c b/mm/slub.c index 07edad305512..c4556a5dab4b 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -397,18 +397,14 @@ static inline void *freelist_ptr_decode(const struct kmem_cache *s, return decoded; } -/* Returns the freelist pointer recorded at location ptr_addr. */ -static inline void *freelist_dereference(const struct kmem_cache *s, - void *ptr_addr) -{ - return freelist_ptr_decode(s, *(freeptr_t *)(ptr_addr), - (unsigned long)ptr_addr); -} - static inline void *get_freepointer(struct kmem_cache *s, void *object) { - object = kasan_reset_tag(object); - return freelist_dereference(s, (freeptr_t *)(object + s->offset)); + unsigned long ptr_addr; + freeptr_t p; + + ptr_addr = ((unsigned long)kasan_reset_tag(object)) + s->offset; + p = *(freeptr_t *)(ptr_addr); + return freelist_ptr_decode(s, p, ptr_addr); } #ifndef CONFIG_SLUB_TINY
freelist_dereference() is a one-liner only used from get_freepointer(). Remove it and make get_freepointer() call freelist_ptr_decode() directly to make the code easier to follow. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)