Message ID | 20230703143820.152479-1-matteorizzo@google.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | mm/slub: refactor freelist to use custom type | expand |
On Mon, 3 Jul 2023, Matteo Rizzo wrote: > From: Jann Horn <jannh@google.com> > > Currently the SLUB code represents encoded freelist entries as "void*". > That's misleading, those things are encoded under > CONFIG_SLAB_FREELIST_HARDENED so that they're not actually dereferencable. > > Give them their own type, and split freelist_ptr() into one function per > direction (one for encoding, one for decoding). > I don't feel strongly about this. > Signed-off-by: Jann Horn <jannh@google.com> > Co-developed-by: Matteo Rizzo <matteorizzo@google.com> > Signed-off-by: Matteo Rizzo <matteorizzo@google.com> > --- > include/linux/slub_def.h | 6 ++++++ > mm/slub.c | 37 ++++++++++++++++++++++++++----------- > 2 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h > index deb90cf4bffb..c747820a55b4 100644 > --- a/include/linux/slub_def.h > +++ b/include/linux/slub_def.h > @@ -43,6 +43,12 @@ enum stat_item { > }; > > #ifndef CONFIG_SLUB_TINY > +/* > + * freeptr_t represents a SLUB freelist pointer, which might be encoded > + * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled. > + */ > +typedef struct { unsigned long v; } freeptr_t; Seems strange this would only appear for configs without CONFIG_SLUB_TINY. Since lots of files include linux/slab.h I think this may start to be used in non-slab code. Not sure why it needs to be added to the header file? > + > /* > * When changing the layout, make sure freelist and tid are still compatible > * with this_cpu_cmpxchg_double() alignment requirements. > diff --git a/mm/slub.c b/mm/slub.c > index e3b5d5c0eb3a..26d0ca02b61d 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -365,8 +365,8 @@ static struct workqueue_struct *flushwq; > * with an XOR of the address where the pointer is held and a per-cache > * random number. > */ > -static inline void *freelist_ptr(const struct kmem_cache *s, void *ptr, > - unsigned long ptr_addr) > +static inline freeptr_t freelist_ptr_encode(const struct kmem_cache *s, > + void *ptr, unsigned long ptr_addr) > { > #ifdef CONFIG_SLAB_FREELIST_HARDENED > /* > @@ -379,25 +379,40 @@ static inline void *freelist_ptr(const struct kmem_cache *s, void *ptr, > * calls get_freepointer() with an untagged pointer, which causes the > * freepointer to be restored incorrectly. > */ > - return (void *)((unsigned long)ptr ^ s->random ^ > - swab((unsigned long)kasan_reset_tag((void *)ptr_addr))); > + return (freeptr_t){.v = (unsigned long)ptr ^ s->random ^ > + swab((unsigned long)kasan_reset_tag((void *)ptr_addr))}; > #else > - return ptr; > + return (freeptr_t){.v = (unsigned long)ptr}; > #endif > } > > +static inline void *freelist_ptr_decode(const struct kmem_cache *s, > + freeptr_t ptr, unsigned long ptr_addr) > +{ > + void *decoded; > + > +#ifdef CONFIG_SLAB_FREELIST_HARDENED > + /* See the comment in freelist_ptr_encode */ > + decoded = (void *)(ptr.v ^ s->random ^ > + swab((unsigned long)kasan_reset_tag((void *)ptr_addr))); > +#else > + decoded = (void *)ptr.v; > +#endif > + 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(s, (void *)*(unsigned long *)(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, object + s->offset); > + return freelist_dereference(s, (freeptr_t *)(object + s->offset)); > } > > #ifndef CONFIG_SLUB_TINY > @@ -421,15 +436,15 @@ __no_kmsan_checks > static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) > { > unsigned long freepointer_addr; > - void *p; > + freeptr_t p; > > if (!debug_pagealloc_enabled_static()) > return get_freepointer(s, object); > > object = kasan_reset_tag(object); > freepointer_addr = (unsigned long)object + s->offset; > - copy_from_kernel_nofault(&p, (void **)freepointer_addr, sizeof(p)); > - return freelist_ptr(s, p, freepointer_addr); > + copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); > + return freelist_ptr_decode(s, p, freepointer_addr); > } > > static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) > @@ -441,7 +456,7 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) > #endif > > freeptr_addr = (unsigned long)kasan_reset_tag((void *)freeptr_addr); > - *(void **)freeptr_addr = freelist_ptr(s, fp, freeptr_addr); > + *(freeptr_t *)freeptr_addr = freelist_ptr_encode(s, fp, freeptr_addr); > } > > /* Loop over all objects in a slab */ > > base-commit: a901a3568fd26ca9c4a82d8bc5ed5b3ed844d451 > -- > 2.41.0.255.g8b1d071c50-goog > >
Hi Matteo, kernel test robot noticed the following build errors: [auto build test ERROR on a901a3568fd26ca9c4a82d8bc5ed5b3ed844d451] url: https://github.com/intel-lab-lkp/linux/commits/Matteo-Rizzo/mm-slub-refactor-freelist-to-use-custom-type/20230703-223944 base: a901a3568fd26ca9c4a82d8bc5ed5b3ed844d451 patch link: https://lore.kernel.org/r/20230703143820.152479-1-matteorizzo%40google.com patch subject: [PATCH] mm/slub: refactor freelist to use custom type config: arm-randconfig-r001-20230703 (https://download.01.org/0day-ci/archive/20230704/202307041252.XCQcx0eb-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce: (https://download.01.org/0day-ci/archive/20230704/202307041252.XCQcx0eb-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202307041252.XCQcx0eb-lkp@intel.com/ All errors (new ones prefixed by >>): mm/slub.c:368:15: error: unknown type name 'freeptr_t' static inline freeptr_t freelist_ptr_encode(const struct kmem_cache *s, ^ >> mm/slub.c:385:10: error: use of undeclared identifier 'freeptr_t' return (freeptr_t){.v = (unsigned long)ptr}; ^ mm/slub.c:390:6: error: unknown type name 'freeptr_t' freeptr_t ptr, unsigned long ptr_addr) ^ >> mm/slub.c:408:45: error: expected expression return freelist_ptr_decode(s, *(freeptr_t *)(ptr_addr), ^ mm/slub.c:408:34: error: use of undeclared identifier 'freeptr_t' return freelist_ptr_decode(s, *(freeptr_t *)(ptr_addr), ^ mm/slub.c:415:45: error: expected expression return freelist_dereference(s, (freeptr_t *)(object + s->offset)); ^ mm/slub.c:415:34: error: use of undeclared identifier 'freeptr_t' return freelist_dereference(s, (freeptr_t *)(object + s->offset)); ^ mm/slub.c:439:2: error: use of undeclared identifier 'freeptr_t' freeptr_t p; ^ mm/slub.c:446:43: error: expected expression copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); ^ mm/slub.c:446:32: error: use of undeclared identifier 'freeptr_t' copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); ^ >> mm/slub.c:446:69: error: use of undeclared identifier 'p' copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); ^ mm/slub.c:446:28: error: use of undeclared identifier 'p' copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); ^ mm/slub.c:447:32: error: use of undeclared identifier 'p' return freelist_ptr_decode(s, p, freepointer_addr); ^ mm/slub.c:459:15: error: expected expression *(freeptr_t *)freeptr_addr = freelist_ptr_encode(s, fp, freeptr_addr); ^ mm/slub.c:459:4: error: use of undeclared identifier 'freeptr_t' *(freeptr_t *)freeptr_addr = freelist_ptr_encode(s, fp, freeptr_addr); ^ mm/slub.c:2285:15: warning: variable 'partial_slabs' set but not used [-Wunused-but-set-variable] unsigned int partial_slabs = 0; ^ 1 warning and 15 errors generated. vim +/freeptr_t +385 mm/slub.c 358 359 /******************************************************************** 360 * Core slab cache functions 361 *******************************************************************/ 362 363 /* 364 * Returns freelist pointer (ptr). With hardening, this is obfuscated 365 * with an XOR of the address where the pointer is held and a per-cache 366 * random number. 367 */ > 368 static inline freeptr_t freelist_ptr_encode(const struct kmem_cache *s, 369 void *ptr, unsigned long ptr_addr) 370 { 371 #ifdef CONFIG_SLAB_FREELIST_HARDENED 372 /* 373 * When CONFIG_KASAN_SW/HW_TAGS is enabled, ptr_addr might be tagged. 374 * Normally, this doesn't cause any issues, as both set_freepointer() 375 * and get_freepointer() are called with a pointer with the same tag. 376 * However, there are some issues with CONFIG_SLUB_DEBUG code. For 377 * example, when __free_slub() iterates over objects in a cache, it 378 * passes untagged pointers to check_object(). check_object() in turns 379 * calls get_freepointer() with an untagged pointer, which causes the 380 * freepointer to be restored incorrectly. 381 */ 382 return (freeptr_t){.v = (unsigned long)ptr ^ s->random ^ 383 swab((unsigned long)kasan_reset_tag((void *)ptr_addr))}; 384 #else > 385 return (freeptr_t){.v = (unsigned long)ptr}; 386 #endif 387 } 388 389 static inline void *freelist_ptr_decode(const struct kmem_cache *s, 390 freeptr_t ptr, unsigned long ptr_addr) 391 { 392 void *decoded; 393 394 #ifdef CONFIG_SLAB_FREELIST_HARDENED 395 /* See the comment in freelist_ptr_encode */ 396 decoded = (void *)(ptr.v ^ s->random ^ 397 swab((unsigned long)kasan_reset_tag((void *)ptr_addr))); 398 #else 399 decoded = (void *)ptr.v; 400 #endif 401 return decoded; 402 } 403 404 /* Returns the freelist pointer recorded at location ptr_addr. */ 405 static inline void *freelist_dereference(const struct kmem_cache *s, 406 void *ptr_addr) 407 { > 408 return freelist_ptr_decode(s, *(freeptr_t *)(ptr_addr), 409 (unsigned long)ptr_addr); 410 } 411 412 static inline void *get_freepointer(struct kmem_cache *s, void *object) 413 { 414 object = kasan_reset_tag(object); 415 return freelist_dereference(s, (freeptr_t *)(object + s->offset)); 416 } 417 418 #ifndef CONFIG_SLUB_TINY 419 static void prefetch_freepointer(const struct kmem_cache *s, void *object) 420 { 421 prefetchw(object + s->offset); 422 } 423 #endif 424 425 /* 426 * When running under KMSAN, get_freepointer_safe() may return an uninitialized 427 * pointer value in the case the current thread loses the race for the next 428 * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in 429 * slab_alloc_node() will fail, so the uninitialized value won't be used, but 430 * KMSAN will still check all arguments of cmpxchg because of imperfect 431 * handling of inline assembly. 432 * To work around this problem, we apply __no_kmsan_checks to ensure that 433 * get_freepointer_safe() returns initialized memory. 434 */ 435 __no_kmsan_checks 436 static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) 437 { 438 unsigned long freepointer_addr; > 439 freeptr_t p; 440 441 if (!debug_pagealloc_enabled_static()) 442 return get_freepointer(s, object); 443 444 object = kasan_reset_tag(object); 445 freepointer_addr = (unsigned long)object + s->offset; > 446 copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); 447 return freelist_ptr_decode(s, p, freepointer_addr); 448 } 449
On Mon, 3 Jul 2023 at 22:10, David Rientjes <rientjes@google.com> wrote: > Seems strange this would only appear for configs without CONFIG_SLUB_TINY. > > Since lots of files include linux/slab.h I think this may start to be used > in non-slab code. Not sure why it needs to be added to the header file? > Thanks for the comments! I think that makes sense. I will move the typedef to slub.c and move it out of the ifdef (since that also broke the build with SLUB_TINY). -- Matteo
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h index deb90cf4bffb..c747820a55b4 100644 --- a/include/linux/slub_def.h +++ b/include/linux/slub_def.h @@ -43,6 +43,12 @@ enum stat_item { }; #ifndef CONFIG_SLUB_TINY +/* + * freeptr_t represents a SLUB freelist pointer, which might be encoded + * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled. + */ +typedef struct { unsigned long v; } freeptr_t; + /* * When changing the layout, make sure freelist and tid are still compatible * with this_cpu_cmpxchg_double() alignment requirements. diff --git a/mm/slub.c b/mm/slub.c index e3b5d5c0eb3a..26d0ca02b61d 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -365,8 +365,8 @@ static struct workqueue_struct *flushwq; * with an XOR of the address where the pointer is held and a per-cache * random number. */ -static inline void *freelist_ptr(const struct kmem_cache *s, void *ptr, - unsigned long ptr_addr) +static inline freeptr_t freelist_ptr_encode(const struct kmem_cache *s, + void *ptr, unsigned long ptr_addr) { #ifdef CONFIG_SLAB_FREELIST_HARDENED /* @@ -379,25 +379,40 @@ static inline void *freelist_ptr(const struct kmem_cache *s, void *ptr, * calls get_freepointer() with an untagged pointer, which causes the * freepointer to be restored incorrectly. */ - return (void *)((unsigned long)ptr ^ s->random ^ - swab((unsigned long)kasan_reset_tag((void *)ptr_addr))); + return (freeptr_t){.v = (unsigned long)ptr ^ s->random ^ + swab((unsigned long)kasan_reset_tag((void *)ptr_addr))}; #else - return ptr; + return (freeptr_t){.v = (unsigned long)ptr}; #endif } +static inline void *freelist_ptr_decode(const struct kmem_cache *s, + freeptr_t ptr, unsigned long ptr_addr) +{ + void *decoded; + +#ifdef CONFIG_SLAB_FREELIST_HARDENED + /* See the comment in freelist_ptr_encode */ + decoded = (void *)(ptr.v ^ s->random ^ + swab((unsigned long)kasan_reset_tag((void *)ptr_addr))); +#else + decoded = (void *)ptr.v; +#endif + 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(s, (void *)*(unsigned long *)(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, object + s->offset); + return freelist_dereference(s, (freeptr_t *)(object + s->offset)); } #ifndef CONFIG_SLUB_TINY @@ -421,15 +436,15 @@ __no_kmsan_checks static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) { unsigned long freepointer_addr; - void *p; + freeptr_t p; if (!debug_pagealloc_enabled_static()) return get_freepointer(s, object); object = kasan_reset_tag(object); freepointer_addr = (unsigned long)object + s->offset; - copy_from_kernel_nofault(&p, (void **)freepointer_addr, sizeof(p)); - return freelist_ptr(s, p, freepointer_addr); + copy_from_kernel_nofault(&p, (freeptr_t *)freepointer_addr, sizeof(p)); + return freelist_ptr_decode(s, p, freepointer_addr); } static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) @@ -441,7 +456,7 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp) #endif freeptr_addr = (unsigned long)kasan_reset_tag((void *)freeptr_addr); - *(void **)freeptr_addr = freelist_ptr(s, fp, freeptr_addr); + *(freeptr_t *)freeptr_addr = freelist_ptr_encode(s, fp, freeptr_addr); } /* Loop over all objects in a slab */