Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/4] mm/slub: factor user tracking metadata size calculation
       [not found] ` <20260616141410.52117-2-pengpeng@iscas.ac.cn>
@ 2026-06-17  4:26   ` Hao Li
  0 siblings, 0 replies; 2+ messages in thread
From: Hao Li @ 2026-06-17  4:26 UTC (permalink / raw)
  To: Pengpeng Hou
  Cc: Vlastimil Babka, Andrew Morton, linux-mm, Harry Yoo,
	Christoph Lameter, David Rientjes, Roman Gushchin,
	David Hildenbrand, Lorenzo Stoakes, liam, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Jonathan Corbet, Shuah Khan,
	linux-doc, linux-kernel

On Tue, Jun 16, 2026 at 10:14:07PM +0800, Pengpeng Hou wrote:
> SLAB_STORE_USER currently stores two struct track records in the per-object
> debug metadata.  The size is open-coded as 2 * sizeof(struct track) in
> several layout and offset calculations.
> 
> Add TRACK_NR and a small helper for the user-tracking metadata size.  This
> keeps all offset calculations tied to the number of track records and makes
> the following extension less error-prone.
> 
> No functional change.

Do we really need a user_tracking_size helper? This seems introducing some
redundant checking for SLAB_STORE_USER.

For example in set_orig_size(), obj_exts_offset_in_object() and
check_pad_bytes()

redundant checking may be confused...

> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  mm/slub.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index f87e693aca5d..43d4febd5bf2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -327,7 +327,15 @@ struct track {
>  	unsigned long when;	/* When did the operation occur */
>  };
>  
> -enum track_item { TRACK_ALLOC, TRACK_FREE };
> +enum track_item { TRACK_ALLOC, TRACK_FREE, TRACK_NR };
> +
> +static inline unsigned int user_tracking_size(slab_flags_t flags)
> +{
> +	if (flags & SLAB_STORE_USER)
> +		return TRACK_NR * sizeof(struct track);
> +
> +	return 0;
> +}
>  
>  #ifdef SLAB_SUPPORTS_SYSFS
>  static int sysfs_slab_add(struct kmem_cache *);
> @@ -751,7 +759,7 @@ static inline void set_orig_size(struct kmem_cache *s,
>  		return;
>  
>  	p += get_info_end(s);
> -	p += sizeof(struct track) * 2;
> +	p += user_tracking_size(s->flags);
>  
>  	*(unsigned long *)p = orig_size;
>  }
> @@ -767,7 +775,7 @@ static inline unsigned long get_orig_size(struct kmem_cache *s, void *object)
>  		return s->object_size;
>  
>  	p += get_info_end(s);
> -	p += sizeof(struct track) * 2;
> +	p += user_tracking_size(s->flags);
>  
>  	return *(unsigned long *)p;
>  }
> @@ -885,7 +893,7 @@ static unsigned int obj_exts_offset_in_object(struct kmem_cache *s)
>  	unsigned int offset = get_info_end(s);
>  
>  	if (kmem_cache_debug_flags(s, SLAB_STORE_USER))
> -		offset += sizeof(struct track) * 2;
> +		offset += user_tracking_size(s->flags);
>  
>  	if (slub_debug_orig_size(s))
>  		offset += sizeof(unsigned long);
> @@ -1088,7 +1096,7 @@ static void init_tracking(struct kmem_cache *s, void *object)
>  		return;
>  
>  	p = get_track(s, object, TRACK_ALLOC);
> -	memset(p, 0, 2*sizeof(struct track));
> +	memset(p, 0, user_tracking_size(s->flags));
>  }
>  
>  static void print_track(const char *s, struct track *t, unsigned long pr_time)
> @@ -1196,8 +1204,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p)
>  
>  	off = get_info_end(s);
>  
> -	if (s->flags & SLAB_STORE_USER)
> -		off += 2 * sizeof(struct track);
> +	off += user_tracking_size(s->flags);
>  
>  	if (slub_debug_orig_size(s))
>  		off += sizeof(unsigned long);
> @@ -1401,7 +1408,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p)
>  
>  	if (s->flags & SLAB_STORE_USER) {
>  		/* We also have user information there */
> -		off += 2 * sizeof(struct track);
> +		off += user_tracking_size(s->flags);
>  
>  		if (s->flags & SLAB_KMALLOC)
>  			off += sizeof(unsigned long);
> @@ -7820,7 +7827,7 @@ static int calculate_sizes(struct kmem_cache_args *args, struct kmem_cache *s)
>  		 * Need to store information about allocs and frees after
>  		 * the object.
>  		 */
> -		size += 2 * sizeof(struct track);
> +		size += user_tracking_size(flags);
>  
>  		/* Save the original kmalloc request size */
>  		if (flags & SLAB_KMALLOC)
> -- 
> 2.43.0
> 

-- 
Thanks,
Hao


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH 2/4] mm/slub: preserve previous object lifetime in user tracking
       [not found] ` <20260616141410.52117-3-pengpeng@iscas.ac.cn>
@ 2026-06-17  7:54   ` Hao Li
  0 siblings, 0 replies; 2+ messages in thread
From: Hao Li @ 2026-06-17  7:54 UTC (permalink / raw)
  To: Pengpeng Hou
  Cc: Vlastimil Babka, Andrew Morton, linux-mm, Harry Yoo,
	Christoph Lameter, David Rientjes, Roman Gushchin,
	David Hildenbrand, Lorenzo Stoakes, liam, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Jonathan Corbet, Shuah Khan,
	linux-doc, linux-kernel

On Tue, Jun 16, 2026 at 10:14:08PM +0800, Pengpeng Hou wrote:
> SLAB_STORE_USER stores one allocation track and one free track for an
> object.  When that object is reused, the next allocation overwrites the
> allocation track.  If a stale pointer from the previous lifetime is later
> freed or otherwise reported, the free/check report can contain the victim
> allocation and the stale operation while the previous completed alloc/free
> pair has already been overwritten.
> 
> Keep one previous completed lifetime in the existing user tracking
> metadata.  When an object is allocated and the current allocation/free
> tracks both exist, copy that completed lifetime to the previous-lifetime
> slots before recording the new allocation.  Clear the current free track
> when the new allocation begins so the current lifetime does not continue
> to display a free from the old lifetime.
> 
> Print the previous object lifetime when it is available.  This is
> diagnostic information only; it does not infer semantic ownership or
> identify the root cause of a use-after-free.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  mm/slub.c | 66 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 43d4febd5bf2..358f42e92207 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -327,7 +327,13 @@ struct track {
>  	unsigned long when;	/* When did the operation occur */
>  };
>  
> -enum track_item { TRACK_ALLOC, TRACK_FREE, TRACK_NR };
> +enum track_item {
> +	TRACK_ALLOC,
> +	TRACK_FREE,
> +	TRACK_PREV_ALLOC,
> +	TRACK_PREV_FREE,
> +	TRACK_NR,
> +};
>  
>  static inline unsigned int user_tracking_size(slab_flags_t flags)
>  {
> @@ -1080,12 +1086,37 @@ static void set_track_update(struct kmem_cache *s, void *object,
>  	p->when = jiffies;
>  }
>  
> -static __always_inline void set_track(struct kmem_cache *s, void *object,
> -				      enum track_item alloc, unsigned long addr, gfp_t gfp_flags)
> +static bool track_has_record(const struct track *t)
> +{
> +	return t->addr;
> +}

how about inline it

> +
> +static void clear_track(struct kmem_cache *s, void *object,
> +			enum track_item track)
> +{
> +	memset(get_track(s, object, track), 0, sizeof(struct track));
> +}
> +
> +static void save_previous_lifetime(struct kmem_cache *s, void *object)
> +{
> +	struct track *alloc = get_track(s, object, TRACK_ALLOC);
> +	struct track *free = get_track(s, object, TRACK_FREE);
> +
> +	if (!track_has_record(alloc) || !track_has_record(free))
> +		return;
> +
> +	*get_track(s, object, TRACK_PREV_ALLOC) = *alloc;
> +	*get_track(s, object, TRACK_PREV_FREE) = *free;

Maybe we can use memcpy instead of copying them one by one.

> +}
> +
> +static __always_inline void set_alloc_track(struct kmem_cache *s, void *object,
> +					    unsigned long addr, gfp_t gfp_flags)
>  {
>  	depot_stack_handle_t handle = set_track_prepare(gfp_flags);
>  
> -	set_track_update(s, object, alloc, addr, handle);
> +	save_previous_lifetime(s, object);
> +	set_track_update(s, object, TRACK_ALLOC, addr, handle);
> +	clear_track(s, object, TRACK_FREE);

sashiko has a comment:

https://sashiko.dev/#/patchset/20260616141410.52117-1-pengpeng%40iscas.ac.cn

It seems a simple fix could be removing clear_track() and allow the stale free
track.

>  }
>  
>  static void init_tracking(struct kmem_cache *s, void *object)
> @@ -1120,11 +1151,22 @@ static void print_track(const char *s, struct track *t, unsigned long pr_time)
>  void print_tracking(struct kmem_cache *s, void *object)
>  {
>  	unsigned long pr_time = jiffies;
> +	struct track *prev_alloc;
> +	struct track *prev_free;
> +
>  	if (!(s->flags & SLAB_STORE_USER))
>  		return;
>  
>  	print_track("Allocated", get_track(s, object, TRACK_ALLOC), pr_time);
>  	print_track("Freed", get_track(s, object, TRACK_FREE), pr_time);
> +
> +	prev_alloc = get_track(s, object, TRACK_PREV_ALLOC);
> +	prev_free = get_track(s, object, TRACK_PREV_FREE);
> +	if (track_has_record(prev_alloc) || track_has_record(prev_free)) {
> +		pr_err("Previous object lifetime:\n");
> +		print_track("Previously allocated", prev_alloc, pr_time);
> +		print_track("Previously freed", prev_free, pr_time);
> +	}
>  }
>  
>  static void print_slab_info(const struct slab *slab)
> @@ -1371,10 +1413,12 @@ check_bytes_and_report(struct kmem_cache *s, struct slab *slab,
>   *
>   * [Metadata starts at object + s->inuse]
>   *   - A. freelist pointer (if freeptr_outside_object)
> - *   - B. alloc tracking (SLAB_STORE_USER)
> - *   - C. free tracking (SLAB_STORE_USER)
> - *   - D. original request size (SLAB_KMALLOC && SLAB_STORE_USER)
> - *   - E. KASAN metadata (if enabled)
> + *   - B. current alloc tracking (SLAB_STORE_USER)
> + *   - C. current free tracking (SLAB_STORE_USER)
> + *   - D. previous alloc tracking (SLAB_STORE_USER)
> + *   - E. previous free tracking (SLAB_STORE_USER)
> + *   - F. original request size (SLAB_KMALLOC && SLAB_STORE_USER)
> + *   - G. KASAN metadata (if enabled)
>   *
>   * [Mandatory padding] (if CONFIG_SLUB_DEBUG && SLAB_RED_ZONE)
>   *   - One mandatory debug word to guarantee a minimum poisoned gap
> @@ -2029,8 +2073,8 @@ static inline void slab_pad_check(struct kmem_cache *s, struct slab *slab) {}
>  static inline int check_object(struct kmem_cache *s, struct slab *slab,
>  			void *object, u8 val) { return 1; }
>  static inline depot_stack_handle_t set_track_prepare(gfp_t gfp_flags) { return 0; }
> -static inline void set_track(struct kmem_cache *s, void *object,
> -			     enum track_item alloc, unsigned long addr, gfp_t gfp_flags) {}
> +static inline void set_alloc_track(struct kmem_cache *s, void *object,
> +				   unsigned long addr, gfp_t gfp_flags) {}
>  static inline void add_full(struct kmem_cache *s, struct kmem_cache_node *n,
>  					struct slab *slab) {}
>  static inline void remove_full(struct kmem_cache *s, struct kmem_cache_node *n,
> @@ -4522,7 +4566,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>  
>  success:
>  	if (kmem_cache_debug_flags(s, SLAB_STORE_USER))
> -		set_track(s, object, TRACK_ALLOC, addr, gfpflags);
> +		set_alloc_track(s, object, addr, gfpflags);
>  
>  	return object;
>  }
> -- 
> 2.43.0
> 
-- 
Thanks,
Hao


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2026-06-17  7:54 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260616141410.52117-1-pengpeng@iscas.ac.cn>
     [not found] ` <20260616141410.52117-2-pengpeng@iscas.ac.cn>
2026-06-17  4:26   ` [PATCH 1/4] mm/slub: factor user tracking metadata size calculation Hao Li
     [not found] ` <20260616141410.52117-3-pengpeng@iscas.ac.cn>
2026-06-17  7:54   ` [PATCH 2/4] mm/slub: preserve previous object lifetime in user tracking Hao Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox