From: Andrey Konovalov <[email protected]>
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 <[email protected]>
---
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
From: Andrey Konovalov <[email protected]>
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 <[email protected]>
---
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
From: Andrey Konovalov <[email protected]>
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 <[email protected]>
---
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
From: Andrey Konovalov <[email protected]>
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 <[email protected]>
---
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
On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
> ---
> 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
>
On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
> ---
> 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
>
On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
> ---
> 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
>
On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
>
> From: Andrey Konovalov <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Marco Elver <[email protected]>
> ---
> 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
>
On Thu, Dec 21, 2023 at 9:11 PM Marco Elver <[email protected]> wrote:
>
> On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
> >
> > From: Andrey Konovalov <[email protected]>
> >
> > 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 <[email protected]>
>
> Reviewed-by: Marco Elver <[email protected]>
Thank you!
On Thu, Dec 21, 2023 at 9:12 PM Marco Elver <[email protected]> wrote:
>
> On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
> >
> > From: Andrey Konovalov <[email protected]>
> >
> > 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.
On Thu, Dec 21, 2023 at 9:14 PM Marco Elver <[email protected]> wrote:
>
> On Thu, 21 Dec 2023 at 19:35, <[email protected]> wrote:
> >
> > From: Andrey Konovalov <[email protected]>
> >
> > 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 <[email protected]>
>
> Reviewed-by: Marco Elver <[email protected]>
>
> > ---
> > 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!