All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Marco Elver <elver@google.com>
Cc: jannh@google.com, x86@kernel.org, linux-kernel@vger.kernel.org,
	kasan-dev@googlegroups.com, linux-mm@kvack.org,
	glider@google.com, linux-arm-kernel@lists.infradead.org,
	akpm@linux-foundation.org, dvyukov@google.com
Subject: Re: [PATCH] kfence: Use pt_regs to generate stack trace on faults
Date: Thu, 5 Nov 2020 10:52:41 +0000	[thread overview]
Message-ID: <20201105105241.GC82102@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20201105092133.2075331-1-elver@google.com>

On Thu, Nov 05, 2020 at 10:21:33AM +0100, Marco Elver wrote:
> Instead of removing the fault handling portion of the stack trace based
> on the fault handler's name, just use struct pt_regs directly.
> 
> Change kfence_handle_page_fault() to take a struct pt_regs, and plumb it
> through to kfence_report_error() for out-of-bounds, use-after-free, or
> invalid access errors, where pt_regs is used to generate the stack
> trace.
> 
> If the kernel is a DEBUG_KERNEL, also show registers for more
> information.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Marco Elver <elver@google.com>

Wow; I wasn't expecting this to be put together so quickly, thanks for
doing this!

From a scan, this looks good to me -- just one question below.

> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> index ed2d48acdafe..98a97f9d43cd 100644
> --- a/include/linux/kfence.h
> +++ b/include/linux/kfence.h
> @@ -171,6 +171,7 @@ static __always_inline __must_check bool kfence_free(void *addr)
>  /**
>   * kfence_handle_page_fault() - perform page fault handling for KFENCE pages
>   * @addr: faulting address
> + * @regs: current struct pt_regs (can be NULL, but shows full stack trace)
>   *
>   * Return:
>   * * false - address outside KFENCE pool,

> @@ -44,8 +44,12 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
>  		case KFENCE_ERROR_UAF:
>  		case KFENCE_ERROR_OOB:
>  		case KFENCE_ERROR_INVALID:
> -			is_access_fault = true;
> -			break;
> +			/*
> +			 * kfence_handle_page_fault() may be called with pt_regs
> +			 * set to NULL; in that case we'll simply show the full
> +			 * stack trace.
> +			 */
> +			return 0;

For both the above comments, when/where is kfence_handle_page_fault()
called with regs set to NULL? I couldn't spot that in this patch, so
unless I mised it I'm guessing that's somewhere outside of the patch
context?

If this is a case we don't expect to happen, maybe add a WARN_ON_ONCE()?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Marco Elver <elver@google.com>
Cc: akpm@linux-foundation.org, glider@google.com, dvyukov@google.com,
	jannh@google.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, kasan-dev@googlegroups.com, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] kfence: Use pt_regs to generate stack trace on faults
Date: Thu, 5 Nov 2020 10:52:41 +0000	[thread overview]
Message-ID: <20201105105241.GC82102@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20201105092133.2075331-1-elver@google.com>

On Thu, Nov 05, 2020 at 10:21:33AM +0100, Marco Elver wrote:
> Instead of removing the fault handling portion of the stack trace based
> on the fault handler's name, just use struct pt_regs directly.
> 
> Change kfence_handle_page_fault() to take a struct pt_regs, and plumb it
> through to kfence_report_error() for out-of-bounds, use-after-free, or
> invalid access errors, where pt_regs is used to generate the stack
> trace.
> 
> If the kernel is a DEBUG_KERNEL, also show registers for more
> information.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Marco Elver <elver@google.com>

Wow; I wasn't expecting this to be put together so quickly, thanks for
doing this!

From a scan, this looks good to me -- just one question below.

> diff --git a/include/linux/kfence.h b/include/linux/kfence.h
> index ed2d48acdafe..98a97f9d43cd 100644
> --- a/include/linux/kfence.h
> +++ b/include/linux/kfence.h
> @@ -171,6 +171,7 @@ static __always_inline __must_check bool kfence_free(void *addr)
>  /**
>   * kfence_handle_page_fault() - perform page fault handling for KFENCE pages
>   * @addr: faulting address
> + * @regs: current struct pt_regs (can be NULL, but shows full stack trace)
>   *
>   * Return:
>   * * false - address outside KFENCE pool,

> @@ -44,8 +44,12 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
>  		case KFENCE_ERROR_UAF:
>  		case KFENCE_ERROR_OOB:
>  		case KFENCE_ERROR_INVALID:
> -			is_access_fault = true;
> -			break;
> +			/*
> +			 * kfence_handle_page_fault() may be called with pt_regs
> +			 * set to NULL; in that case we'll simply show the full
> +			 * stack trace.
> +			 */
> +			return 0;

For both the above comments, when/where is kfence_handle_page_fault()
called with regs set to NULL? I couldn't spot that in this patch, so
unless I mised it I'm guessing that's somewhere outside of the patch
context?

If this is a case we don't expect to happen, maybe add a WARN_ON_ONCE()?

Thanks,
Mark.


  reply	other threads:[~2020-11-05 10:53 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-05  9:21 [PATCH] kfence: Use pt_regs to generate stack trace on faults Marco Elver
2020-11-05  9:21 ` Marco Elver
2020-11-05 10:52 ` Mark Rutland [this message]
2020-11-05 10:52   ` Mark Rutland
2020-11-05 11:11   ` Marco Elver
2020-11-05 11:11     ` Marco Elver
2020-11-05 11:15     ` Mark Rutland
2020-11-05 11:15       ` Mark Rutland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201105105241.GC82102@C02TD0UTHF1T.local \
    --to=mark.rutland@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=dvyukov@google.com \
    --cc=elver@google.com \
    --cc=glider@google.com \
    --cc=jannh@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.