Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Jul 2023 13:10:46 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     Matteo Rizzo <matteorizzo@google.com>
cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org, cl@linux.com,
        penberg@kernel.org, iamjoonsoo.kim@lge.com,
        akpm@linux-foundation.org, vbabka@suse.cz,
        roman.gushchin@linux.dev, 42.hyeyoo@gmail.com, jannh@google.com
Subject: Re: [PATCH] mm/slub: refactor freelist to use custom type
In-Reply-To: <20230703143820.152479-1-matteorizzo@google.com>
Message-ID: <abd3f78b-5027-f859-27c8-296108ec8e92@google.com>
References: <20230703143820.152479-1-matteorizzo@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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
> 
>