Re: [PATCH v4 28/39] unwind_user/deferred: Add deferred unwinding interface

Mathieu Desnoyers
2025-01-22T20:13:17Z

On 2025-01-21 21:31, Josh Poimboeuf wrote:
> Add an interface for scheduling task work to unwind the user space stack
> before returning to user space. This solves several problems for its
> callers:
>
> - Ensure the unwind happens in task context even if the caller may be
> running in NMI or interrupt context.
>
> - Avoid duplicate unwinds, whether called multiple times by the same
> caller or by different callers.
>
> - Create a "context cookie" which allows trace post-processing to
> correlate kernel unwinds/traces with the user unwind.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
> include/linux/entry-common.h | 2 +
> include/linux/sched.h | 5 +
> include/linux/unwind_deferred.h | 46 +++++++
> include/linux/unwind_deferred_types.h | 10 ++
> kernel/fork.c | 4 +
> kernel/unwind/Makefile | 2 +-
> kernel/unwind/deferred.c | 178 ++++++++++++++++++++++++++
> 7 files changed, 246 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/unwind_deferred.h
> create mode 100644 include/linux/unwind_deferred_types.h
> create mode 100644 kernel/unwind/deferred.c
>
> diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
> index fc61d0205c97..fb2b27154fee 100644
> --- a/include/linux/entry-common.h
> +++ b/include/linux/entry-common.h
> @@ -12,6 +12,7 @@
> #include <linux/resume_user_mode.h>
> #include <linux/tick.h>
> #include <linux/kmsan.h>
> +#include <linux/unwind_deferred.h>
>
> #include <asm/entry-common.h>
>
> @@ -111,6 +112,7 @@ static __always_inline void enter_from_user_mode(struct pt_regs *regs)
>
> CT_WARN_ON(__ct_state() != CT_STATE_USER);
> user_exit_irqoff();
> + unwind_enter_from_user_mode();
>
> instrumentation_begin();
> kmsan_unpoison_entry_regs(regs);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 64934e0830af..042a95f4f6e6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -46,6 +46,7 @@
> #include <linux/rv.h>
> #include <linux/livepatch_sched.h>
> #include <linux/uidgid_types.h>
> +#include <linux/unwind_deferred_types.h>
> #include <asm/kmap_size.h>
>
> /* task_struct member predeclarations (sorted alphabetically): */
> @@ -1603,6 +1604,10 @@ struct task_struct {
> struct user_event_mm *user_event_mm;
> #endif
>
> +#ifdef CONFIG_UNWIND_USER
> + struct unwind_task_info unwind_info;
> +#endif
> +
> /*
> * New fields for task_struct should be added above here, so that
> * they are included in the randomized portion of task_struct.
> diff --git a/include/linux/unwind_deferred.h b/include/linux/unwind_deferred.h
> new file mode 100644
> index 000000000000..741f409f0d1f
> --- /dev/null
> +++ b/include/linux/unwind_deferred.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_UNWIND_USER_DEFERRED_H
> +#define _LINUX_UNWIND_USER_DEFERRED_H
> +
> +#include <linux/task_work.h>
> +#include <linux/unwind_user.h>
> +#include <linux/unwind_deferred_types.h>
> +
> +struct unwind_work;
> +
> +typedef void (*unwind_callback_t)(struct unwind_work *work, struct unwind_stacktrace *trace, u64 cookie);
> +
> +struct unwind_work {
> + struct callback_head work;
> + unwind_callback_t func;
> + int pending;
> +};

This is a lot of information to keep around per instance.

I'm not sure it would be OK to have a single unwind_work per perf-event
for perf. I suspect it may need to be per perf-event X per-task if a
perf-event can be associated to more than a single task (not sure ?).

For LTTng, we'd have to consider something similar because of multi-session
support. Either we'd have one unwind_work per-session X per-task, or we'd
need to multiplex this internally within LTTng-modules. None of this is
ideal in terms of memory footprint.

We should look at what part of this information can be made static/global
and what part is task-local, so we minimize the amount of redundant data
per-task (memory footprint).

AFAIU, most of that unwind_work information is global:

- work,
- func,

And could be registered dynamically by the tracer when it enables
tracing with an interest on stack walking.

At registration, we can allocate a descriptor ID (with a limited bounded
max number, configurable). This would associate a work+func to a given
ID, and keep track of this in a global table (indexed by ID).

I suspect that the only thing we really want to keep track of per-task
is the pending bit, and what is the ID of the unwind_work associated.
This could be kept, per-task, in either:

- a bitmap of pending bits, indexed by ID, or
- an array of pending IDs.

Unregistration of unwind_work could iterate on all tasks and clear the
pending bit or ID associated with the unregistered work, to make sure
we don't trigger unrelated work after a re-use.

> +
> +#ifdef CONFIG_UNWIND_USER
> +
> +void unwind_task_init(struct task_struct *task);
> +void unwind_task_free(struct task_struct *task);
> +
> +void unwind_deferred_init(struct unwind_work *work, unwind_callback_t func);
> +int unwind_deferred_request(struct unwind_work *work, u64 *cookie);
> +bool unwind_deferred_cancel(struct task_struct *task, struct unwind_work *work);
> +
> +static __always_inline void unwind_enter_from_user_mode(void)
> +{
> + current->unwind_info.cookie = 0;
> +}
> +
> +#else /* !CONFIG_UNWIND_USER */
> +
> +static inline void unwind_task_init(struct task_struct *task) {}
> +static inline void unwind_task_free(struct task_struct *task) {}
> +
> +static inline void unwind_deferred_init(struct unwind_work *work, unwind_callback_t func) {}
> +static inline int unwind_deferred_request(struct task_struct *task, struct unwind_work *work, u64 *cookie) { return -ENOSYS; }
> +static inline bool unwind_deferred_cancel(struct task_struct *task, struct unwind_work *work) { return false; }
> +
> +static inline void unwind_enter_from_user_mode(void) {}
> +
> +#endif /* !CONFIG_UNWIND_USER */
> +
> +#endif /* _LINUX_UNWIND_USER_DEFERRED_H */
> diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
> new file mode 100644
> index 000000000000..9749824aea09
> --- /dev/null
> +++ b/include/linux/unwind_deferred_types.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
> +#define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
> +
> +struct unwind_task_info {
> + unsigned long *entries;
> + u64 cookie;
> +};
> +
> +#endif /* _LINUX_UNWIND_USER_DEFERRED_TYPES_H */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 88753f8bbdd3..c9a954af72a1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -106,6 +106,7 @@
> #include <linux/pidfs.h>
> #include <linux/tick.h>
> #include <linux/sframe.h>
> +#include <linux/unwind_deferred.h>
>
> #include <asm/pgalloc.h>
> #include <linux/uaccess.h>
> @@ -973,6 +974,7 @@ void __put_task_struct(struct task_struct *tsk)
> WARN_ON(refcount_read(&tsk->usage));
> WARN_ON(tsk == current);
>
> + unwind_task_free(tsk);
> sched_ext_free(tsk);
> io_uring_free(tsk);
> cgroup_free(tsk);
> @@ -2370,6 +2372,8 @@ __latent_entropy struct task_struct *copy_process(
> p->bpf_ctx = NULL;
> #endif
>
> + unwind_task_init(p);
> +
> /* Perform scheduler related setup. Assign this task to a CPU. */
> retval = sched_fork(clone_flags, p);
> if (retval)
> diff --git a/kernel/unwind/Makefile b/kernel/unwind/Makefile
> index f70380d7a6a6..146038165865 100644
> --- a/kernel/unwind/Makefile
> +++ b/kernel/unwind/Makefile
> @@ -1,2 +1,2 @@
> - obj-$(CONFIG_UNWIND_USER) += user.o
> + obj-$(CONFIG_UNWIND_USER) += user.o deferred.o
> obj-$(CONFIG_HAVE_UNWIND_USER_SFRAME) += sframe.o
> diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
> new file mode 100644
> index 000000000000..f0dbe4069247
> --- /dev/null
> +++ b/kernel/unwind/deferred.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> +* Deferred user space unwinding
> +*/
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/sched/task_stack.h>
> +#include <linux/sframe.h>
> +#include <linux/slab.h>
> +#include <linux/task_work.h>
> +#include <linux/mm.h>
> +#include <linux/unwind_deferred.h>
> +
> +#define UNWIND_MAX_ENTRIES 512
> +
> +/* entry-from-user counter */
> +static DEFINE_PER_CPU(u64, unwind_ctx_ctr);
> +
> +/*
> + * The context cookie is a unique identifier which allows post-processing to
> + * correlate kernel trace(s) with user unwinds. The high 12 bits are the CPU
> + * id; the lower 48 bits are a per-CPU entry counter.
> + */
> +static u64 ctx_to_cookie(u64 cpu, u64 ctx)
> +{
> + BUILD_BUG_ON(NR_CPUS > 65535);

2^12 = 4k, not 64k. Perhaps you mean to reserve 16 bits
for cpu numbers ?

> + return (ctx & ((1UL << 48) - 1)) | (cpu << 48);

Perhaps use ilog2(NR_CPUS) instead for the number of bits to use
rather than hard code 12 ?

> +}
> +
> +/*
> + * Read the task context cookie, first initializing it if this is the first
> + * call to get_cookie() since the most recent entry from user.
> + */
> +static u64 get_cookie(struct unwind_task_info *info)
> +{
> + u64 ctx_ctr;
> + u64 cookie;
> + u64 cpu;
> +
> + guard(irqsave)();
> +
> + cookie = info->cookie;
> + if (cookie)
> + return cookie;
> +
> +
> + cpu = raw_smp_processor_id();
> + ctx_ctr = __this_cpu_inc_return(unwind_ctx_ctr);
> + info->cookie = ctx_to_cookie(cpu, ctx_ctr);
> +
> + return cookie;
> +
> +}
> +
> +static void unwind_deferred_task_work(struct callback_head *head)
> +{
> + struct unwind_work *work = container_of(head, struct unwind_work, work);
> + struct unwind_task_info *info = &current->unwind_info;
> + struct unwind_stacktrace trace;
> + u64 cookie;
> +
> + if (WARN_ON_ONCE(!work->pending))
> + return;
> +
> + /*
> + * From here on out, the callback must always be called, even if it's
> + * just an empty trace.
> + */
> +
> + cookie = get_cookie(info);
> +
> + /* Check for task exit path. */
> + if (!current->mm)
> + goto do_callback;
> +
> + if (!info->entries) {
> + info->entries = kmalloc(UNWIND_MAX_ENTRIES * sizeof(long),
> + GFP_KERNEL);
> + if (!info->entries)
> + goto do_callback;
> + }
> +
> + trace.entries = info->entries;
> + trace.nr = 0;
> + unwind_user(&trace, UNWIND_MAX_ENTRIES);
> +
> +do_callback:
> + work->func(work, &trace, cookie);
> + work->pending = 0;
> +}
> +
> +/*
> + * Schedule a user space unwind to be done in task work before exiting the
> + * kernel.
> + *
> + * The returned cookie output is a unique identifer for the current task entry

identifier

Thanks,

Mathieu

> + * context. Its value will also be passed to the callback function. It can be
> + * used to stitch kernel and user stack traces together in post-processing.
> + *
> + * It's valid to call this function multiple times for the same @work within
> + * the same task entry context. Each call will return the same cookie. If the
> + * callback is already pending, an error will be returned along with the
> + * cookie. If the callback is not pending because it has already been
> + * previously called for the same entry context, it will be called again with
> + * the same stack trace and cookie.
> + *
> + * Thus are three possible return scenarios:
> + *
> + * * return != 0, *cookie == 0: the operation failed, no pending callback.
> + *
> + * * return != 0, *cookie != 0: the callback is already pending. The cookie
> + * can still be used to correlate with the pending callback.
> + *
> + * * return == 0, *cookie != 0: the callback queued successfully. The
> + * callback is guaranteed to be called with the given cookie.
> + */
> +int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
> +{
> + struct unwind_task_info *info = &current->unwind_info;
> + int ret;
> +
> + *cookie = 0;
> +
> + if (WARN_ON_ONCE(in_nmi()))
> + return -EINVAL;
> +
> + if (!current->mm || !user_mode(task_pt_regs(current)))
> + return -EINVAL;
> +
> + guard(irqsave)();
> +
> + *cookie = get_cookie(info);
> +
> + /* callback already pending? */
> + if (work->pending)
> + return -EEXIST;
> +
> + ret = task_work_add(current, &work->work, TWA_RESUME);
> + if (WARN_ON_ONCE(ret))
> + return ret;
> +
> + work->pending = 1;
> +
> + return 0;
> +}
> +
> +bool unwind_deferred_cancel(struct task_struct *task, struct unwind_work *work)
> +{
> + bool ret;
> +
> + ret = task_work_cancel(task, &work->work);
> + if (ret)
> + work->pending = 0;
> +
> + return ret;
> +}
> +
> +void unwind_deferred_init(struct unwind_work *work, unwind_callback_t func)
> +{
> + memset(work, 0, sizeof(*work));
> +
> + init_task_work(&work->work, unwind_deferred_task_work);
> + work->func = func;
> +}
> +
> +void unwind_task_init(struct task_struct *task)
> +{
> + struct unwind_task_info *info = &task->unwind_info;
> +
> + memset(info, 0, sizeof(*info));
> +}
> +
> +void unwind_task_free(struct task_struct *task)
> +{
> + struct unwind_task_info *info = &task->unwind_info;
> +
> + kfree(info->entries);
> +}

--
Mathieu Desnoyers
EfficiOS Inc.