The reports of KASAN include KASAN related stack frames which are not
the point of interest in the stack-trace. KCSAN report filters out such
internal frames providing relevant stack trace. Currently, KASAN reports
are generated by dump_stack_lvl() which prints the entire stack.

Add functionality to KASAN reports to save the stack entries and filter
out the kasan related stack frames in place of dump_stack_lvl() and
stack_depot_print().

Within this new functionality:
	- A function kasan_dump_stack_lvl() in place of dump_stack_lvl() is
	  created which contains functionality for saving, filtering and
	  printing the stack-trace.
	- A function kasan_stack_depot_print() in place of
	  stack_depot_print() is created which contains functionality for
	  filtering and printing the stack-trace.
	- The get_stack_skipnr() function which employs pattern based stack
	  filtering is included.
	- The replace_stack_entry() function which uses ip value based
	  stack filtering is included.

Signed-off-by: Nihar Chaithanya <niharchaithanya@gmail.com>
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=215756
---
Changes in v2:
        - Changed the function name from save_stack_lvl_kasan() to
          kasan_dump_stack_lvl().
        - Added filtering of stack frames for print_track() with
          kasan_stack_depot_print().
        - Removed redundant print_stack_trace(), and instead using
          stack_trace_print() directly.
        - Removed sanitize_stack_entries() and replace_stack_entry()
          functions.
        - Increased the buffer size in get_stack_skipnr to 128.

Changes in v3:
	- Included an additional criteria for pattern based filtering
	  in get_stack_skipnr().
	- Included ip value based stack filtering with the functions
	  sanitize_stack_entries() and replace_stack_entry().
	- Corrected the comments and name of the newly added functions
	  kasan_dump_stack() and kasan_stack_depot_print().

 mm/kasan/report.c | 111 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 4 deletions(-)

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 3e48668c3e40..648a89fea3e7 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -261,6 +261,110 @@ static void print_error_description(struct kasan_report_info *info)
 			info->access_addr, current->comm, task_pid_nr(current));
 }
 
+/* Helper to skip KASAN-related functions in stack-trace. */
+static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries)
+{
+	char buf[64];
+	int len, skip;
+
+	for (skip = 0; skip < num_entries; ++skip) {
+		len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skip]);
+
+		/* Never show  kasan_* or __kasan_* functions. */
+		if ((strnstr(buf, "kasan_", len) == buf) ||
+			(strnstr(buf, "__kasan_", len) == buf))
+			continue;
+		/*
+		 * No match for runtime functions -- @skip entries to skip to
+		 * get to first frame of interest.
+		 */
+		break;
+	}
+
+	return skip;
+}
+
+/*
+ * Skips to the first entry that matches the function of @ip, and then replaces
+ * that entry with @ip, returning the entries to skip with @replaced containing
+ * the replaced entry.
+ */
+static int
+replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned long ip,
+		    unsigned long *replaced)
+{
+	unsigned long symbolsize, offset;
+	unsigned long target_func;
+	int skip;
+
+	if (kallsyms_lookup_size_offset(ip, &symbolsize, &offset))
+		target_func = ip - offset;
+	else
+		goto fallback;
+
+	for (skip = 0; skip < num_entries; ++skip) {
+		unsigned long func = stack_entries[skip];
+
+		if (!kallsyms_lookup_size_offset(func, &symbolsize, &offset))
+			goto fallback;
+		func -= offset;
+
+		if (func == target_func) {
+			*replaced = stack_entries[skip];
+			stack_entries[skip] = ip;
+			return skip;
+		}
+	}
+
+fallback:
+	/* Should not happen; the resulting stack trace is likely misleading. */
+	WARN_ONCE(1, "Cannot find frame for %pS in stack trace", (void *)ip);
+	return get_stack_skipnr(stack_entries, num_entries);
+}
+
+static int
+sanitize_stack_entries(unsigned long stack_entries[], int num_entries, unsigned long ip,
+		       unsigned long *replaced)
+{
+	return ip ? replace_stack_entry(stack_entries, num_entries, ip, replaced) :
+			  get_stack_skipnr(stack_entries, num_entries);
+}
+
+/*
+ * Use in place of dump_stack() to filter out KASAN-related frames in
+ * the stack trace.
+ */
+static void kasan_dump_stack(unsigned long ip)
+{
+	unsigned long reordered_to = 0;
+	unsigned long stack_entries[KASAN_STACK_DEPTH] = { 0 };
+
+	int num_stack_entries = stack_trace_save(stack_entries, KASAN_STACK_DEPTH, 0);
+	int skipnr = sanitize_stack_entries(stack_entries, num_stack_entries,
+										ip, &reordered_to);
+
+	dump_stack_print_info(KERN_ERR);
+	stack_trace_print(stack_entries + skipnr, num_stack_entries - skipnr, 0);
+}
+
+/*
+ * Use in place of stack_depot_print() to filter out KASAN-related frames
+ * in the stack trace.
+ */
+static void kasan_stack_depot_print(depot_stack_handle_t stack)
+{
+	unsigned int nr_entries;
+	unsigned long *entries = NULL;
+	unsigned int skipnr;
+
+	nr_entries = stack_depot_fetch(stack, &entries);
+	if (nr_entries) {
+		skipnr = get_stack_skipnr(entries, nr_entries);
+		stack_trace_print(entries + skipnr, nr_entries - skipnr, 0);
+	} else
+		pr_err("(stack is not available)\n");
+}
+
 static void print_track(struct kasan_track *track, const char *prefix)
 {
 #ifdef CONFIG_KASAN_EXTRA_INFO
@@ -277,7 +381,7 @@ static void print_track(struct kasan_track *track, const char *prefix)
 	pr_err("%s by task %u:\n", prefix, track->pid);
 #endif /* CONFIG_KASAN_EXTRA_INFO */
 	if (track->stack)
-		stack_depot_print(track->stack);
+		kasan_stack_depot_print(track->stack);
 	else
 		pr_err("(stack is not available)\n");
 }
@@ -374,7 +478,6 @@ static void print_address_description(void *addr, u8 tag,
 {
 	struct page *page = addr_to_page(addr);
 
-	dump_stack_lvl(KERN_ERR);
 	pr_err("\n");
 
 	if (info->cache && info->object) {
@@ -484,11 +587,11 @@ static void print_report(struct kasan_report_info *info)
 		kasan_print_tags(tag, info->first_bad_addr);
 	pr_err("\n");
 
+	kasan_dump_stack(info->ip);
+
 	if (addr_has_metadata(addr)) {
 		print_address_description(addr, tag, info);
 		print_memory_metadata(info->first_bad_addr);
-	} else {
-		dump_stack_lvl(KERN_ERR);
 	}
 }
 
-- 
2.34.1

On Sat, 26 Oct 2024 at 18:17, Nihar Chaithanya
<niharchaithanya@gmail.com> wrote:
>
> The reports of KASAN include KASAN related stack frames which are not
> the point of interest in the stack-trace. KCSAN report filters out such
> internal frames providing relevant stack trace. Currently, KASAN reports
> are generated by dump_stack_lvl() which prints the entire stack.
>
> Add functionality to KASAN reports to save the stack entries and filter
> out the kasan related stack frames in place of dump_stack_lvl() and
> stack_depot_print().
>
> Within this new functionality:
>         - A function kasan_dump_stack_lvl() in place of dump_stack_lvl() is
>           created which contains functionality for saving, filtering and
>           printing the stack-trace.
>         - A function kasan_stack_depot_print() in place of
>           stack_depot_print() is created which contains functionality for
>           filtering and printing the stack-trace.
>         - The get_stack_skipnr() function which employs pattern based stack
>           filtering is included.
>         - The replace_stack_entry() function which uses ip value based
>           stack filtering is included.
>
> Signed-off-by: Nihar Chaithanya <niharchaithanya@gmail.com>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=215756
> ---
> Changes in v2:
>         - Changed the function name from save_stack_lvl_kasan() to
>           kasan_dump_stack_lvl().
>         - Added filtering of stack frames for print_track() with
>           kasan_stack_depot_print().
>         - Removed redundant print_stack_trace(), and instead using
>           stack_trace_print() directly.
>         - Removed sanitize_stack_entries() and replace_stack_entry()
>           functions.
>         - Increased the buffer size in get_stack_skipnr to 128.
>
> Changes in v3:
>         - Included an additional criteria for pattern based filtering
>           in get_stack_skipnr().
>         - Included ip value based stack filtering with the functions
>           sanitize_stack_entries() and replace_stack_entry().
>         - Corrected the comments and name of the newly added functions
>           kasan_dump_stack() and kasan_stack_depot_print().
>
>  mm/kasan/report.c | 111 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 107 insertions(+), 4 deletions(-)
>
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index 3e48668c3e40..648a89fea3e7 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -261,6 +261,110 @@ static void print_error_description(struct kasan_report_info *info)
>                         info->access_addr, current->comm, task_pid_nr(current));
>  }
>
> +/* Helper to skip KASAN-related functions in stack-trace. */
> +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries)
> +{
> +       char buf[64];
> +       int len, skip;
> +
> +       for (skip = 0; skip < num_entries; ++skip) {
> +               len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skip]);
> +
> +               /* Never show  kasan_* or __kasan_* functions. */
> +               if ((strnstr(buf, "kasan_", len) == buf) ||
> +                       (strnstr(buf, "__kasan_", len) == buf))
> +                       continue;
> +               /*
> +                * No match for runtime functions -- @skip entries to skip to
> +                * get to first frame of interest.
> +                */
> +               break;
> +       }
> +
> +       return skip;
> +}
> +
> +/*
> + * Skips to the first entry that matches the function of @ip, and then replaces
> + * that entry with @ip, returning the entries to skip with @replaced containing
> + * the replaced entry.
> + */
> +static int
> +replace_stack_entry(unsigned long stack_entries[], int num_entries, unsigned long ip,
> +                   unsigned long *replaced)
> +{
> +       unsigned long symbolsize, offset;
> +       unsigned long target_func;
> +       int skip;
> +
> +       if (kallsyms_lookup_size_offset(ip, &symbolsize, &offset))
> +               target_func = ip - offset;
> +       else
> +               goto fallback;
> +
> +       for (skip = 0; skip < num_entries; ++skip) {
> +               unsigned long func = stack_entries[skip];
> +
> +               if (!kallsyms_lookup_size_offset(func, &symbolsize, &offset))
> +                       goto fallback;
> +               func -= offset;
> +
> +               if (func == target_func) {
> +                       *replaced = stack_entries[skip];

All this replaced logic is not needed for KASAN.

> +                       stack_entries[skip] = ip;
> +                       return skip;
> +               }
> +       }
> +
> +fallback:
> +       /* Should not happen; the resulting stack trace is likely misleading. */
> +       WARN_ONCE(1, "Cannot find frame for %pS in stack trace", (void *)ip);
> +       return get_stack_skipnr(stack_entries, num_entries);
> +}
> +
> +static int
> +sanitize_stack_entries(unsigned long stack_entries[], int num_entries, unsigned long ip,
> +                      unsigned long *replaced)
> +{
> +       return ip ? replace_stack_entry(stack_entries, num_entries, ip, replaced) :
> +                         get_stack_skipnr(stack_entries, num_entries);
> +}
> +
> +/*
> + * Use in place of dump_stack() to filter out KASAN-related frames in
> + * the stack trace.
> + */
> +static void kasan_dump_stack(unsigned long ip)
> +{
> +       unsigned long reordered_to = 0;

Do you understand what this code is doing? The whole "reordered_to"
logic (along with the "replaced" logic in replace_stack_entry()) is
very specific to KCSAN and not at all needed for KASAN. See the commit
that introduced it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/kernel/kcsan/report.c?id=be3f6967ec5947dd7b2f23bf9d42bb2729889618

You've copied and pasted the code from KCSAN, in the hopes it more or
less does what we want. However, the code as-is is more complex than
needed.

Please try to really understand what the most optimal way to do this
might be. As additional inspiration, please look at how KFENCE does
it: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/kfence/report.c#n49

In the end what I'd like to see is the simplest possible way to do
this for KASAN.