From: Jann Horn <[email protected]>
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).
Signed-off-by: Jann Horn <[email protected]>
Co-developed-by: Matteo Rizzo <[email protected]>
Signed-off-by: Matteo Rizzo <[email protected]>
---
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;
+
/*
* 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
On Mon, 3 Jul 2023, Matteo Rizzo wrote:
> From: Jann Horn <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Matteo Rizzo <[email protected]>
> Signed-off-by: Matteo Rizzo <[email protected]>
> ---
> 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/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230704/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, 3 Jul 2023 at 22:10, David Rientjes <[email protected]> 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