[PATCH mm 1/4] kasan: clean up kasan_cache_create

[PATCH mm 1/4] kasan: clean up kasan_cache_create

[andrey.konovalov]

From: Andrey Konovalov <andreyknvl@google.com>

Reorganize the code to avoid nested if/else checks to improve the
readability.

Also drop the confusing comments about KMALLOC_MAX_SIZE checks: they
are relevant for both SLUB and SLAB (originally, the comments likely
confused KMALLOC_MAX_SIZE with KMALLOC_MAX_CACHE_SIZE).

Fixes: a5989d4ed40c ("kasan: improve free meta storage in Generic KASAN")
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/kasan/generic.c | 67 +++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 54e20b2bc3e1..769e43e05d0b 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -381,16 +381,11 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
 
 	ok_size = *size;
 
-	/* Add alloc meta into redzone. */
+	/* Add alloc meta into the redzone. */
 	cache->kasan_info.alloc_meta_offset = *size;
 	*size += sizeof(struct kasan_alloc_meta);
 
-	/*
-	 * If alloc meta doesn't fit, don't add it.
-	 * This can only happen with SLAB, as it has KMALLOC_MAX_SIZE equal
-	 * to KMALLOC_MAX_CACHE_SIZE and doesn't fall back to page_alloc for
-	 * larger sizes.
-	 */
+	/* If alloc meta doesn't fit, don't add it. */
 	if (*size > KMALLOC_MAX_SIZE) {
 		cache->kasan_info.alloc_meta_offset = 0;
 		*size = ok_size;
@@ -401,36 +396,52 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
 	orig_alloc_meta_offset = cache->kasan_info.alloc_meta_offset;
 
 	/*
-	 * Add free meta into redzone when it's not possible to store
+	 * Store free meta in the redzone when it's not possible to store
 	 * it in the object. This is the case when:
 	 * 1. Object is SLAB_TYPESAFE_BY_RCU, which means that it can
 	 *    be touched after it was freed, or
 	 * 2. Object has a constructor, which means it's expected to
-	 *    retain its content until the next allocation, or
-	 * 3. Object is too small and SLUB DEBUG is enabled. Avoid
-	 *    free meta that exceeds the object size corrupts the
-	 *    SLUB DEBUG metadata.
-	 * Otherwise cache->kasan_info.free_meta_offset = 0 is implied.
-	 * If the object is smaller than the free meta and SLUB DEBUG
-	 * is not enabled, it is still possible to store part of the
-	 * free meta in the object.
+	 *    retain its content until the next allocation.
 	 */
 	if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor) {
 		cache->kasan_info.free_meta_offset = *size;
 		*size += sizeof(struct kasan_free_meta);
-	} else if (cache->object_size < sizeof(struct kasan_free_meta)) {
-		if (__slub_debug_enabled()) {
-			cache->kasan_info.free_meta_offset = *size;
-			*size += sizeof(struct kasan_free_meta);
-		} else {
-			rem_free_meta_size = sizeof(struct kasan_free_meta) -
-									cache->object_size;
-			*size += rem_free_meta_size;
-			if (cache->kasan_info.alloc_meta_offset != 0)
-				cache->kasan_info.alloc_meta_offset += rem_free_meta_size;
-		}
+		goto free_meta_added;
+	}
+
+	/*
+	 * Otherwise, if the object is large enough to contain free meta,
+	 * store it within the object.
+	 */
+	if (sizeof(struct kasan_free_meta) <= cache->object_size) {
+		/* cache->kasan_info.free_meta_offset = 0 is implied. */
+		goto free_meta_added;
 	}
 
+	/*
+	 * For smaller objects, store the beginning of free meta within the
+	 * object and the end in the redzone. And thus shift the location of
+	 * alloc meta to free up space for free meta.
+	 * This is only possible when slub_debug is disabled, as otherwise
+	 * the end of free meta will overlap with slub_debug metadata.
+	 */
+	if (!__slub_debug_enabled()) {
+		rem_free_meta_size = sizeof(struct kasan_free_meta) -
+							cache->object_size;
+		*size += rem_free_meta_size;
+		if (cache->kasan_info.alloc_meta_offset != 0)
+			cache->kasan_info.alloc_meta_offset += rem_free_meta_size;
+		goto free_meta_added;
+	}
+
+	/*
+	 * If the object is small and slub_debug is enabled, store free meta
+	 * in the redzone after alloc meta.
+	 */
+	cache->kasan_info.free_meta_offset = *size;
+	*size += sizeof(struct kasan_free_meta);
+
+free_meta_added:
 	/* If free meta doesn't fit, don't add it. */
 	if (*size > KMALLOC_MAX_SIZE) {
 		cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
@@ -440,7 +451,7 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
 
 	/* Calculate size with optimal redzone. */
 	optimal_size = cache->object_size + optimal_redzone(cache->object_size);
-	/* Limit it with KMALLOC_MAX_SIZE (relevant for SLAB only). */
+	/* Limit it with KMALLOC_MAX_SIZE. */
 	if (optimal_size > KMALLOC_MAX_SIZE)
 		optimal_size = KMALLOC_MAX_SIZE;
 	/* Use optimal size if the size with added metas is not large enough. */
-- 
2.25.1

[PATCH mm 2/4] kasan: reuse kasan_track in kasan_stack_ring_entry

[andrey.konovalov]

From: Andrey Konovalov <andreyknvl@google.com>

Avoid duplicating fields of kasan_track in kasan_stack_ring_entry:
reuse the structure.

Fixes: 5d4c6ac94694 ("kasan: record and report more information")
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/kasan/kasan.h       |  7 +------
 mm/kasan/report_tags.c | 12 ++++++------
 mm/kasan/tags.c        | 12 ++++++------
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 5e298e3ac909..9072ce4c1263 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -279,13 +279,8 @@ struct kasan_free_meta {
 struct kasan_stack_ring_entry {
 	void *ptr;
 	size_t size;
-	u32 pid;
-	depot_stack_handle_t stack;
+	struct kasan_track track;
 	bool is_free;
-#ifdef CONFIG_KASAN_EXTRA_INFO
-	u64 cpu:20;
-	u64 timestamp:44;
-#endif /* CONFIG_KASAN_EXTRA_INFO */
 };
 
 struct kasan_stack_ring {
diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c
index 979f284c2497..688b9d70b04a 100644
--- a/mm/kasan/report_tags.c
+++ b/mm/kasan/report_tags.c
@@ -31,8 +31,8 @@ static const char *get_common_bug_type(struct kasan_report_info *info)
 static void kasan_complete_extra_report_info(struct kasan_track *track,
 					 struct kasan_stack_ring_entry *entry)
 {
-	track->cpu = entry->cpu;
-	track->timestamp = entry->timestamp;
+	track->cpu = entry->track.cpu;
+	track->timestamp = entry->track.timestamp;
 }
 #endif /* CONFIG_KASAN_EXTRA_INFO */
 
@@ -80,8 +80,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
 			if (free_found)
 				break;
 
-			info->free_track.pid = entry->pid;
-			info->free_track.stack = entry->stack;
+			info->free_track.pid = entry->track.pid;
+			info->free_track.stack = entry->track.stack;
 #ifdef CONFIG_KASAN_EXTRA_INFO
 			kasan_complete_extra_report_info(&info->free_track, entry);
 #endif /* CONFIG_KASAN_EXTRA_INFO */
@@ -98,8 +98,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
 			if (alloc_found)
 				break;
 
-			info->alloc_track.pid = entry->pid;
-			info->alloc_track.stack = entry->stack;
+			info->alloc_track.pid = entry->track.pid;
+			info->alloc_track.stack = entry->track.stack;
 #ifdef CONFIG_KASAN_EXTRA_INFO
 			kasan_complete_extra_report_info(&info->alloc_track, entry);
 #endif /* CONFIG_KASAN_EXTRA_INFO */
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index c13b198b8302..c4d14dbf27c0 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -100,8 +100,8 @@ static void save_extra_info(struct kasan_stack_ring_entry *entry)
 	u32 cpu = raw_smp_processor_id();
 	u64 ts_nsec = local_clock();
 
-	entry->cpu = cpu;
-	entry->timestamp = ts_nsec >> 3;
+	entry->track.cpu = cpu;
+	entry->track.timestamp = ts_nsec >> 3;
 }
 #endif /* CONFIG_KASAN_EXTRA_INFO */
 
@@ -134,15 +134,15 @@ static void save_stack_info(struct kmem_cache *cache, void *object,
 	if (!try_cmpxchg(&entry->ptr, &old_ptr, STACK_RING_BUSY_PTR))
 		goto next; /* Busy slot. */
 
-	old_stack = entry->stack;
+	old_stack = entry->track.stack;
 
 	entry->size = cache->object_size;
-	entry->pid = current->pid;
-	entry->stack = stack;
-	entry->is_free = is_free;
+	entry->track.pid = current->pid;
+	entry->track.stack = stack;
 #ifdef CONFIG_KASAN_EXTRA_INFO
 	save_extra_info(entry);
 #endif /* CONFIG_KASAN_EXTRA_INFO */
+	entry->is_free = is_free;
 
 	entry->ptr = object;
 
-- 
2.25.1

[PATCH mm 3/4] kasan: simplify saving extra info into tracks

[andrey.konovalov]

From: Andrey Konovalov <andreyknvl@google.com>

Avoid duplicating code for saving extra info into tracks: reuse the
common function for this.

Fixes: 5d4c6ac94694 ("kasan: record and report more information")
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/kasan/common.c  | 12 ++++++++++--
 mm/kasan/generic.c |  4 ++--
 mm/kasan/kasan.h   |  3 ++-
 mm/kasan/tags.c    | 17 +----------------
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index fe6c4b43ad9f..d004a0f4406c 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -48,7 +48,7 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags, depot_flags_t depot_flags)
 	return stack_depot_save_flags(entries, nr_entries, flags, depot_flags);
 }
 
-void kasan_set_track(struct kasan_track *track, gfp_t flags)
+void kasan_set_track(struct kasan_track *track, depot_stack_handle_t stack)
 {
 #ifdef CONFIG_KASAN_EXTRA_INFO
 	u32 cpu = raw_smp_processor_id();
@@ -58,8 +58,16 @@ void kasan_set_track(struct kasan_track *track, gfp_t flags)
 	track->timestamp = ts_nsec >> 3;
 #endif /* CONFIG_KASAN_EXTRA_INFO */
 	track->pid = current->pid;
-	track->stack = kasan_save_stack(flags,
+	track->stack = stack;
+}
+
+void kasan_save_track(struct kasan_track *track, gfp_t flags)
+{
+	depot_stack_handle_t stack;
+
+	stack = kasan_save_stack(flags,
 			STACK_DEPOT_FLAG_CAN_ALLOC | STACK_DEPOT_FLAG_GET);
+	kasan_set_track(track, stack);
 }
 
 #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 769e43e05d0b..11b575707b05 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -553,7 +553,7 @@ void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t flags)
 	stack_depot_put(alloc_meta->aux_stack[1]);
 	__memset(alloc_meta, 0, sizeof(*alloc_meta));
 
-	kasan_set_track(&alloc_meta->alloc_track, flags);
+	kasan_save_track(&alloc_meta->alloc_track, flags);
 }
 
 void kasan_save_free_info(struct kmem_cache *cache, void *object)
@@ -564,7 +564,7 @@ void kasan_save_free_info(struct kmem_cache *cache, void *object)
 	if (!free_meta)
 		return;
 
-	kasan_set_track(&free_meta->free_track, 0);
+	kasan_save_track(&free_meta->free_track, 0);
 	/* The object was freed and has free track set. */
 	*(u8 *)kasan_mem_to_shadow(object) = KASAN_SLAB_FREETRACK;
 }
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 9072ce4c1263..31fb6bb26fed 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -372,7 +372,8 @@ static inline void kasan_init_object_meta(struct kmem_cache *cache, const void *
 #endif
 
 depot_stack_handle_t kasan_save_stack(gfp_t flags, depot_flags_t depot_flags);
-void kasan_set_track(struct kasan_track *track, gfp_t flags);
+void kasan_set_track(struct kasan_track *track, depot_stack_handle_t stack);
+void kasan_save_track(struct kasan_track *track, gfp_t flags);
 void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t flags);
 void kasan_save_free_info(struct kmem_cache *cache, void *object);
 
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index c4d14dbf27c0..d65d48b85f90 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -94,17 +94,6 @@ void __init kasan_init_tags(void)
 	}
 }
 
-#ifdef CONFIG_KASAN_EXTRA_INFO
-static void save_extra_info(struct kasan_stack_ring_entry *entry)
-{
-	u32 cpu = raw_smp_processor_id();
-	u64 ts_nsec = local_clock();
-
-	entry->track.cpu = cpu;
-	entry->track.timestamp = ts_nsec >> 3;
-}
-#endif /* CONFIG_KASAN_EXTRA_INFO */
-
 static void save_stack_info(struct kmem_cache *cache, void *object,
 			gfp_t gfp_flags, bool is_free)
 {
@@ -137,11 +126,7 @@ static void save_stack_info(struct kmem_cache *cache, void *object,
 	old_stack = entry->track.stack;
 
 	entry->size = cache->object_size;
-	entry->track.pid = current->pid;
-	entry->track.stack = stack;
-#ifdef CONFIG_KASAN_EXTRA_INFO
-	save_extra_info(entry);
-#endif /* CONFIG_KASAN_EXTRA_INFO */
+	kasan_set_track(&entry->track, stack);
 	entry->is_free = is_free;
 
 	entry->ptr = object;
-- 
2.25.1

[PATCH mm 4/4] kasan: simplify kasan_complete_mode_report_info for tag-based modes

[andrey.konovalov]

From: Andrey Konovalov <andreyknvl@google.com>

memcpy the alloc/free tracks when collecting the information about a bad
access instead of copying fields one by one.

Fixes: 5d4c6ac94694 ("kasan: record and report more information")
Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/kasan/report_tags.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c
index 688b9d70b04a..d15f8f580e2c 100644
--- a/mm/kasan/report_tags.c
+++ b/mm/kasan/report_tags.c
@@ -27,15 +27,6 @@ static const char *get_common_bug_type(struct kasan_report_info *info)
 	return "invalid-access";
 }
 
-#ifdef CONFIG_KASAN_EXTRA_INFO
-static void kasan_complete_extra_report_info(struct kasan_track *track,
-					 struct kasan_stack_ring_entry *entry)
-{
-	track->cpu = entry->track.cpu;
-	track->timestamp = entry->track.timestamp;
-}
-#endif /* CONFIG_KASAN_EXTRA_INFO */
-
 void kasan_complete_mode_report_info(struct kasan_report_info *info)
 {
 	unsigned long flags;
@@ -80,11 +71,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
 			if (free_found)
 				break;
 
-			info->free_track.pid = entry->track.pid;
-			info->free_track.stack = entry->track.stack;
-#ifdef CONFIG_KASAN_EXTRA_INFO
-			kasan_complete_extra_report_info(&info->free_track, entry);
-#endif /* CONFIG_KASAN_EXTRA_INFO */
+			memcpy(&info->free_track, &entry->track,
+			       sizeof(info->free_track));
 			free_found = true;
 
 			/*
@@ -98,11 +86,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
 			if (alloc_found)
 				break;
 
-			info->alloc_track.pid = entry->track.pid;
-			info->alloc_track.stack = entry->track.stack;
-#ifdef CONFIG_KASAN_EXTRA_INFO
-			kasan_complete_extra_report_info(&info->alloc_track, entry);
-#endif /* CONFIG_KASAN_EXTRA_INFO */
+			memcpy(&info->alloc_track, &entry->track,
+			       sizeof(info->alloc_track));
 			alloc_found = true;
 
 			/*
-- 
2.25.1

Re: [PATCH mm 1/4] kasan: clean up kasan_cache_create

[Marco Elver]

On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Reorganize the code to avoid nested if/else checks to improve the
> readability.
>
> Also drop the confusing comments about KMALLOC_MAX_SIZE checks: they
> are relevant for both SLUB and SLAB (originally, the comments likely
> confused KMALLOC_MAX_SIZE with KMALLOC_MAX_CACHE_SIZE).
>
> Fixes: a5989d4ed40c ("kasan: improve free meta storage in Generic KASAN")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>  mm/kasan/generic.c | 67 +++++++++++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 54e20b2bc3e1..769e43e05d0b 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -381,16 +381,11 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>
>         ok_size = *size;
>
> -       /* Add alloc meta into redzone. */
> +       /* Add alloc meta into the redzone. */
>         cache->kasan_info.alloc_meta_offset = *size;
>         *size += sizeof(struct kasan_alloc_meta);
>
> -       /*
> -        * If alloc meta doesn't fit, don't add it.
> -        * This can only happen with SLAB, as it has KMALLOC_MAX_SIZE equal
> -        * to KMALLOC_MAX_CACHE_SIZE and doesn't fall back to page_alloc for
> -        * larger sizes.
> -        */
> +       /* If alloc meta doesn't fit, don't add it. */
>         if (*size > KMALLOC_MAX_SIZE) {
>                 cache->kasan_info.alloc_meta_offset = 0;
>                 *size = ok_size;
> @@ -401,36 +396,52 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>         orig_alloc_meta_offset = cache->kasan_info.alloc_meta_offset;
>
>         /*
> -        * Add free meta into redzone when it's not possible to store
> +        * Store free meta in the redzone when it's not possible to store
>          * it in the object. This is the case when:
>          * 1. Object is SLAB_TYPESAFE_BY_RCU, which means that it can
>          *    be touched after it was freed, or
>          * 2. Object has a constructor, which means it's expected to
> -        *    retain its content until the next allocation, or
> -        * 3. Object is too small and SLUB DEBUG is enabled. Avoid
> -        *    free meta that exceeds the object size corrupts the
> -        *    SLUB DEBUG metadata.
> -        * Otherwise cache->kasan_info.free_meta_offset = 0 is implied.
> -        * If the object is smaller than the free meta and SLUB DEBUG
> -        * is not enabled, it is still possible to store part of the
> -        * free meta in the object.
> +        *    retain its content until the next allocation.
>          */
>         if ((cache->flags & SLAB_TYPESAFE_BY_RCU) || cache->ctor) {
>                 cache->kasan_info.free_meta_offset = *size;
>                 *size += sizeof(struct kasan_free_meta);
> -       } else if (cache->object_size < sizeof(struct kasan_free_meta)) {
> -               if (__slub_debug_enabled()) {
> -                       cache->kasan_info.free_meta_offset = *size;
> -                       *size += sizeof(struct kasan_free_meta);
> -               } else {
> -                       rem_free_meta_size = sizeof(struct kasan_free_meta) -
> -                                                                       cache->object_size;
> -                       *size += rem_free_meta_size;
> -                       if (cache->kasan_info.alloc_meta_offset != 0)
> -                               cache->kasan_info.alloc_meta_offset += rem_free_meta_size;
> -               }
> +               goto free_meta_added;
> +       }
> +
> +       /*
> +        * Otherwise, if the object is large enough to contain free meta,
> +        * store it within the object.
> +        */
> +       if (sizeof(struct kasan_free_meta) <= cache->object_size) {
> +               /* cache->kasan_info.free_meta_offset = 0 is implied. */
> +               goto free_meta_added;
>         }
>
> +       /*
> +        * For smaller objects, store the beginning of free meta within the
> +        * object and the end in the redzone. And thus shift the location of
> +        * alloc meta to free up space for free meta.
> +        * This is only possible when slub_debug is disabled, as otherwise
> +        * the end of free meta will overlap with slub_debug metadata.
> +        */
> +       if (!__slub_debug_enabled()) {
> +               rem_free_meta_size = sizeof(struct kasan_free_meta) -
> +                                                       cache->object_size;
> +               *size += rem_free_meta_size;
> +               if (cache->kasan_info.alloc_meta_offset != 0)
> +                       cache->kasan_info.alloc_meta_offset += rem_free_meta_size;
> +               goto free_meta_added;
> +       }
> +
> +       /*
> +        * If the object is small and slub_debug is enabled, store free meta
> +        * in the redzone after alloc meta.
> +        */
> +       cache->kasan_info.free_meta_offset = *size;
> +       *size += sizeof(struct kasan_free_meta);
> +
> +free_meta_added:
>         /* If free meta doesn't fit, don't add it. */
>         if (*size > KMALLOC_MAX_SIZE) {
>                 cache->kasan_info.free_meta_offset = KASAN_NO_FREE_META;
> @@ -440,7 +451,7 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>
>         /* Calculate size with optimal redzone. */
>         optimal_size = cache->object_size + optimal_redzone(cache->object_size);
> -       /* Limit it with KMALLOC_MAX_SIZE (relevant for SLAB only). */
> +       /* Limit it with KMALLOC_MAX_SIZE. */
>         if (optimal_size > KMALLOC_MAX_SIZE)
>                 optimal_size = KMALLOC_MAX_SIZE;
>         /* Use optimal size if the size with added metas is not large enough. */
> --
> 2.25.1
>

Re: [PATCH mm 2/4] kasan: reuse kasan_track in kasan_stack_ring_entry

[Marco Elver]

On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Avoid duplicating fields of kasan_track in kasan_stack_ring_entry:
> reuse the structure.

No functional change?

> Fixes: 5d4c6ac94694 ("kasan: record and report more information")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>  mm/kasan/kasan.h       |  7 +------
>  mm/kasan/report_tags.c | 12 ++++++------
>  mm/kasan/tags.c        | 12 ++++++------
>  3 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 5e298e3ac909..9072ce4c1263 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -279,13 +279,8 @@ struct kasan_free_meta {
>  struct kasan_stack_ring_entry {
>         void *ptr;
>         size_t size;
> -       u32 pid;
> -       depot_stack_handle_t stack;
> +       struct kasan_track track;
>         bool is_free;
> -#ifdef CONFIG_KASAN_EXTRA_INFO
> -       u64 cpu:20;
> -       u64 timestamp:44;
> -#endif /* CONFIG_KASAN_EXTRA_INFO */
>  };
>
>  struct kasan_stack_ring {
> diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c
> index 979f284c2497..688b9d70b04a 100644
> --- a/mm/kasan/report_tags.c
> +++ b/mm/kasan/report_tags.c
> @@ -31,8 +31,8 @@ static const char *get_common_bug_type(struct kasan_report_info *info)
>  static void kasan_complete_extra_report_info(struct kasan_track *track,
>                                          struct kasan_stack_ring_entry *entry)
>  {
> -       track->cpu = entry->cpu;
> -       track->timestamp = entry->timestamp;
> +       track->cpu = entry->track.cpu;
> +       track->timestamp = entry->track.timestamp;
>  }
>  #endif /* CONFIG_KASAN_EXTRA_INFO */
>
> @@ -80,8 +80,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
>                         if (free_found)
>                                 break;
>
> -                       info->free_track.pid = entry->pid;
> -                       info->free_track.stack = entry->stack;
> +                       info->free_track.pid = entry->track.pid;
> +                       info->free_track.stack = entry->track.stack;
>  #ifdef CONFIG_KASAN_EXTRA_INFO
>                         kasan_complete_extra_report_info(&info->free_track, entry);
>  #endif /* CONFIG_KASAN_EXTRA_INFO */
> @@ -98,8 +98,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
>                         if (alloc_found)
>                                 break;
>
> -                       info->alloc_track.pid = entry->pid;
> -                       info->alloc_track.stack = entry->stack;
> +                       info->alloc_track.pid = entry->track.pid;
> +                       info->alloc_track.stack = entry->track.stack;
>  #ifdef CONFIG_KASAN_EXTRA_INFO
>                         kasan_complete_extra_report_info(&info->alloc_track, entry);
>  #endif /* CONFIG_KASAN_EXTRA_INFO */
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> index c13b198b8302..c4d14dbf27c0 100644
> --- a/mm/kasan/tags.c
> +++ b/mm/kasan/tags.c
> @@ -100,8 +100,8 @@ static void save_extra_info(struct kasan_stack_ring_entry *entry)
>         u32 cpu = raw_smp_processor_id();
>         u64 ts_nsec = local_clock();
>
> -       entry->cpu = cpu;
> -       entry->timestamp = ts_nsec >> 3;
> +       entry->track.cpu = cpu;
> +       entry->track.timestamp = ts_nsec >> 3;
>  }
>  #endif /* CONFIG_KASAN_EXTRA_INFO */
>
> @@ -134,15 +134,15 @@ static void save_stack_info(struct kmem_cache *cache, void *object,
>         if (!try_cmpxchg(&entry->ptr, &old_ptr, STACK_RING_BUSY_PTR))
>                 goto next; /* Busy slot. */
>
> -       old_stack = entry->stack;
> +       old_stack = entry->track.stack;
>
>         entry->size = cache->object_size;
> -       entry->pid = current->pid;
> -       entry->stack = stack;
> -       entry->is_free = is_free;
> +       entry->track.pid = current->pid;
> +       entry->track.stack = stack;
>  #ifdef CONFIG_KASAN_EXTRA_INFO
>         save_extra_info(entry);
>  #endif /* CONFIG_KASAN_EXTRA_INFO */
> +       entry->is_free = is_free;
>
>         entry->ptr = object;
>
> --
> 2.25.1
>

Re: [PATCH mm 3/4] kasan: simplify saving extra info into tracks

[Marco Elver]

On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> Avoid duplicating code for saving extra info into tracks: reuse the
> common function for this.
>
> Fixes: 5d4c6ac94694 ("kasan: record and report more information")

Looking at this patch and the previous ones, is this Fixes really
needed? I.e. was the previous patch broken?

> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>  mm/kasan/common.c  | 12 ++++++++++--
>  mm/kasan/generic.c |  4 ++--
>  mm/kasan/kasan.h   |  3 ++-
>  mm/kasan/tags.c    | 17 +----------------
>  4 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index fe6c4b43ad9f..d004a0f4406c 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -48,7 +48,7 @@ depot_stack_handle_t kasan_save_stack(gfp_t flags, depot_flags_t depot_flags)
>         return stack_depot_save_flags(entries, nr_entries, flags, depot_flags);
>  }
>
> -void kasan_set_track(struct kasan_track *track, gfp_t flags)
> +void kasan_set_track(struct kasan_track *track, depot_stack_handle_t stack)
>  {
>  #ifdef CONFIG_KASAN_EXTRA_INFO
>         u32 cpu = raw_smp_processor_id();
> @@ -58,8 +58,16 @@ void kasan_set_track(struct kasan_track *track, gfp_t flags)
>         track->timestamp = ts_nsec >> 3;
>  #endif /* CONFIG_KASAN_EXTRA_INFO */
>         track->pid = current->pid;
> -       track->stack = kasan_save_stack(flags,
> +       track->stack = stack;
> +}
> +
> +void kasan_save_track(struct kasan_track *track, gfp_t flags)
> +{
> +       depot_stack_handle_t stack;
> +
> +       stack = kasan_save_stack(flags,
>                         STACK_DEPOT_FLAG_CAN_ALLOC | STACK_DEPOT_FLAG_GET);
> +       kasan_set_track(track, stack);
>  }
>
>  #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 769e43e05d0b..11b575707b05 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -553,7 +553,7 @@ void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t flags)
>         stack_depot_put(alloc_meta->aux_stack[1]);
>         __memset(alloc_meta, 0, sizeof(*alloc_meta));
>
> -       kasan_set_track(&alloc_meta->alloc_track, flags);
> +       kasan_save_track(&alloc_meta->alloc_track, flags);
>  }
>
>  void kasan_save_free_info(struct kmem_cache *cache, void *object)
> @@ -564,7 +564,7 @@ void kasan_save_free_info(struct kmem_cache *cache, void *object)
>         if (!free_meta)
>                 return;
>
> -       kasan_set_track(&free_meta->free_track, 0);
> +       kasan_save_track(&free_meta->free_track, 0);
>         /* The object was freed and has free track set. */
>         *(u8 *)kasan_mem_to_shadow(object) = KASAN_SLAB_FREETRACK;
>  }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 9072ce4c1263..31fb6bb26fed 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -372,7 +372,8 @@ static inline void kasan_init_object_meta(struct kmem_cache *cache, const void *
>  #endif
>
>  depot_stack_handle_t kasan_save_stack(gfp_t flags, depot_flags_t depot_flags);
> -void kasan_set_track(struct kasan_track *track, gfp_t flags);
> +void kasan_set_track(struct kasan_track *track, depot_stack_handle_t stack);
> +void kasan_save_track(struct kasan_track *track, gfp_t flags);
>  void kasan_save_alloc_info(struct kmem_cache *cache, void *object, gfp_t flags);
>  void kasan_save_free_info(struct kmem_cache *cache, void *object);
>
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> index c4d14dbf27c0..d65d48b85f90 100644
> --- a/mm/kasan/tags.c
> +++ b/mm/kasan/tags.c
> @@ -94,17 +94,6 @@ void __init kasan_init_tags(void)
>         }
>  }
>
> -#ifdef CONFIG_KASAN_EXTRA_INFO
> -static void save_extra_info(struct kasan_stack_ring_entry *entry)
> -{
> -       u32 cpu = raw_smp_processor_id();
> -       u64 ts_nsec = local_clock();
> -
> -       entry->track.cpu = cpu;
> -       entry->track.timestamp = ts_nsec >> 3;
> -}
> -#endif /* CONFIG_KASAN_EXTRA_INFO */
> -
>  static void save_stack_info(struct kmem_cache *cache, void *object,
>                         gfp_t gfp_flags, bool is_free)
>  {
> @@ -137,11 +126,7 @@ static void save_stack_info(struct kmem_cache *cache, void *object,
>         old_stack = entry->track.stack;
>
>         entry->size = cache->object_size;
> -       entry->track.pid = current->pid;
> -       entry->track.stack = stack;
> -#ifdef CONFIG_KASAN_EXTRA_INFO
> -       save_extra_info(entry);
> -#endif /* CONFIG_KASAN_EXTRA_INFO */
> +       kasan_set_track(&entry->track, stack);
>         entry->is_free = is_free;
>
>         entry->ptr = object;
> --
> 2.25.1
>

Re: [PATCH mm 4/4] kasan: simplify kasan_complete_mode_report_info for tag-based modes

[Marco Elver]

On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
>
> From: Andrey Konovalov <andreyknvl@google.com>
>
> memcpy the alloc/free tracks when collecting the information about a bad
> access instead of copying fields one by one.
>
> Fixes: 5d4c6ac94694 ("kasan: record and report more information")
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>  mm/kasan/report_tags.c | 23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c
> index 688b9d70b04a..d15f8f580e2c 100644
> --- a/mm/kasan/report_tags.c
> +++ b/mm/kasan/report_tags.c
> @@ -27,15 +27,6 @@ static const char *get_common_bug_type(struct kasan_report_info *info)
>         return "invalid-access";
>  }
>
> -#ifdef CONFIG_KASAN_EXTRA_INFO
> -static void kasan_complete_extra_report_info(struct kasan_track *track,
> -                                        struct kasan_stack_ring_entry *entry)
> -{
> -       track->cpu = entry->track.cpu;
> -       track->timestamp = entry->track.timestamp;
> -}
> -#endif /* CONFIG_KASAN_EXTRA_INFO */
> -
>  void kasan_complete_mode_report_info(struct kasan_report_info *info)
>  {
>         unsigned long flags;
> @@ -80,11 +71,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
>                         if (free_found)
>                                 break;
>
> -                       info->free_track.pid = entry->track.pid;
> -                       info->free_track.stack = entry->track.stack;
> -#ifdef CONFIG_KASAN_EXTRA_INFO
> -                       kasan_complete_extra_report_info(&info->free_track, entry);
> -#endif /* CONFIG_KASAN_EXTRA_INFO */
> +                       memcpy(&info->free_track, &entry->track,
> +                              sizeof(info->free_track));

Not sure why the line break is necessary.

>                         free_found = true;
>
>                         /*
> @@ -98,11 +86,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
>                         if (alloc_found)
>                                 break;
>
> -                       info->alloc_track.pid = entry->track.pid;
> -                       info->alloc_track.stack = entry->track.stack;
> -#ifdef CONFIG_KASAN_EXTRA_INFO
> -                       kasan_complete_extra_report_info(&info->alloc_track, entry);
> -#endif /* CONFIG_KASAN_EXTRA_INFO */
> +                       memcpy(&info->alloc_track, &entry->track,
> +                              sizeof(info->alloc_track));
>                         alloc_found = true;
>
>                         /*
> --
> 2.25.1
>

Re: [PATCH mm 2/4] kasan: reuse kasan_track in kasan_stack_ring_entry

[Andrey Konovalov]

On Thu, Dec 21, 2023 at 9:11 PM Marco Elver <elver@google.com> wrote:
>
> On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
> >
> > From: Andrey Konovalov <andreyknvl@google.com>
> >
> > Avoid duplicating fields of kasan_track in kasan_stack_ring_entry:
> > reuse the structure.
>
> No functional change?

Yes, no functional changes in this and the following patches in the series.

> > Fixes: 5d4c6ac94694 ("kasan: record and report more information")
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>
> Reviewed-by: Marco Elver <elver@google.com>

Thank you!

Re: [PATCH mm 3/4] kasan: simplify saving extra info into tracks

[Andrey Konovalov]

On Thu, Dec 21, 2023 at 9:12 PM Marco Elver <elver@google.com> wrote:
>
> On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
> >
> > From: Andrey Konovalov <andreyknvl@google.com>
> >
> > Avoid duplicating code for saving extra info into tracks: reuse the
> > common function for this.
> >
> > Fixes: 5d4c6ac94694 ("kasan: record and report more information")
>
> Looking at this patch and the previous ones, is this Fixes really
> needed? I.e. was the previous patch broken?

Yeah, maybe it's not needed in these 3 patches. The original patch
works, these are just clean-ups.

Re: [PATCH mm 4/4] kasan: simplify kasan_complete_mode_report_info for tag-based modes

[Andrey Konovalov]

On Thu, Dec 21, 2023 at 9:14 PM Marco Elver <elver@google.com> wrote:
>
> On Thu, 21 Dec 2023 at 19:35, <andrey.konovalov@linux.dev> wrote:
> >
> > From: Andrey Konovalov <andreyknvl@google.com>
> >
> > memcpy the alloc/free tracks when collecting the information about a bad
> > access instead of copying fields one by one.
> >
> > Fixes: 5d4c6ac94694 ("kasan: record and report more information")
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> > ---
> >  mm/kasan/report_tags.c | 23 ++++-------------------
> >  1 file changed, 4 insertions(+), 19 deletions(-)
> >
> > diff --git a/mm/kasan/report_tags.c b/mm/kasan/report_tags.c
> > index 688b9d70b04a..d15f8f580e2c 100644
> > --- a/mm/kasan/report_tags.c
> > +++ b/mm/kasan/report_tags.c
> > @@ -27,15 +27,6 @@ static const char *get_common_bug_type(struct kasan_report_info *info)
> >         return "invalid-access";
> >  }
> >
> > -#ifdef CONFIG_KASAN_EXTRA_INFO
> > -static void kasan_complete_extra_report_info(struct kasan_track *track,
> > -                                        struct kasan_stack_ring_entry *entry)
> > -{
> > -       track->cpu = entry->track.cpu;
> > -       track->timestamp = entry->track.timestamp;
> > -}
> > -#endif /* CONFIG_KASAN_EXTRA_INFO */
> > -
> >  void kasan_complete_mode_report_info(struct kasan_report_info *info)
> >  {
> >         unsigned long flags;
> > @@ -80,11 +71,8 @@ void kasan_complete_mode_report_info(struct kasan_report_info *info)
> >                         if (free_found)
> >                                 break;
> >
> > -                       info->free_track.pid = entry->track.pid;
> > -                       info->free_track.stack = entry->track.stack;
> > -#ifdef CONFIG_KASAN_EXTRA_INFO
> > -                       kasan_complete_extra_report_info(&info->free_track, entry);
> > -#endif /* CONFIG_KASAN_EXTRA_INFO */
> > +                       memcpy(&info->free_track, &entry->track,
> > +                              sizeof(info->free_track));
>
> Not sure why the line break is necessary.

Ah, just the old desire to use 80-column line limit :)

Let's keep this as this for now, but I'll fix it if I end up sending
v2 of this series.

Thanks!