Re: [POC 5/5] x86_64: invoke SFrame based stack tracer for user space

[Peter Zijlstra <peterz@infradead.org>]

On Mon, May 01, 2023 at 01:04:10PM -0700, Indu Bhagat wrote:
> The task's sframe_state is allocated and initialized if a phdr with type
> PT_GNU_SFRAME is encountered for the binary.
> 
> perf_callchain_user() will fall back on the frame pointer based stack
> trace approach if:
>   - SFrame section for the main program is not found.
>   - SFrame state for the task is either not setup or stale and cannot
>   be refreshed.
> 
> Finally, the sframe_state is cleaned up in release_task().
> 
> Signed-off-by: Indu Bhagat <indu.bhagat@oracle.com>
> ---
>  arch/x86/events/core.c | 51 ++++++++++++++++++++++++++++++++++++++++++
>  fs/binfmt_elf.c        | 39 ++++++++++++++++++++++++++++++++
>  kernel/exit.c          |  9 ++++++++
>  3 files changed, 99 insertions(+)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index d096b04bf80e..4be9e826714a 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2860,11 +2860,54 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry_ctx *ent
>  }
>  #endif
>  
> +#ifdef CONFIG_USER_UNWINDER_SFRAME
> +
> +#include <sframe/sframe_unwind.h>
> +
> +/* Check if the specified task has SFrame unwind state set up.  */
> +static inline bool check_sframe_state_p(struct task_struct *task)
> +{
> +	bool sframe_ok = false;
> +
> +	/* FIXME TODO - only current task can be unwinded at this time.
> +	 * Even for current tasks, following unknowns remain and hence, not
> +	 * handled:
> +	 *    - dlopen / dlclose detection and update of sframe_state,
> +	 *    - in general, any change in memory mappings.
> +	 */
> +	if (task != current)
> +		return false;
> +
> +	if (!task->sframe_state)
> +		return false;
> +
> +	sframe_ok = (unwind_sframe_state_valid_p(task->sframe_state)
> +		     || unwind_sframe_state_ready_p(task->sframe_state));

Please; forget this style is even possible, it is horrific :/

> +
> +	return sframe_ok;
> +}
> +
> +#else
> +/* Check if the specified task has SFrame unwind state set up. */
> +static inline bool check_sframe_state_p(struct task_struct *task)
> +{
> +	  return false;
> +}
> +
> +static inline int sframe_callchain_user(struct task_struct *task,
> +					struct perf_callchain_entry_ctx *entry,
> +					struct pt_regs *regs)
> +{
> +	  return 0;
> +}
> +#endif
> +
>  void
>  perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
>  {
>  	struct stack_frame frame;
>  	const struct stack_frame __user *fp;
> +	bool sframe_avail;
>  
>  	if (perf_guest_state()) {
>  		/* TODO: We don't support guest os callchain now */
> @@ -2887,7 +2930,15 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs
>  	if (perf_callchain_user32(regs, entry))
>  		return;
>  
> +	sframe_avail = check_sframe_state_p (current);
> +
>  	pagefault_disable();
> +
> +	if (sframe_avail && !sframe_callchain_user (current, entry, regs)) {
> +		pagefault_enable();
> +		return;
> +	}
> +
>  	while (entry->nr < entry->max_stack) {
>  		if (!valid_user_frame(fp, sizeof(frame)))
>  			break;

This is broken, the sframe stuff is not NMI safe. First you need to
change perf to emit a forward reference to a 'next' user trace and then
emit the user trace on return-to-user.

As is, perf does everything in-place from NMI context.