Re: [RFC PATCH 04/10] stacktrace: Don't unwind the user stack without an mm

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2649 bytes --]

> stacktrace: Don't unwind the user stack without an mm

TL;DR: Commit message only: the Fixes: tag may point at the wrong commit;
7e8e6816c649, which dropped the !current->mm check, looks like the real
culprit rather than 214d8ca6ee85.

> stacktrace: Don't unwind the user stack without an mm
>
> stack_trace_save_user() only refuses to unwind the user stack of tasks
> with PF_KTHREAD set. That doesn't cover all tasks without an mm:
>  - A user task that has already gone through exit_mm(), e.g. when an
>    event fires from exit_files(), exit_task_work() or the final context
>    switch in do_exit().
>  - PID 1 before it execs init, and usermode helper children before they
>    exec, which are created with user_mode_thread() and don't have
>    PF_KTHREAD set. E.g. booting with trace_options=userstacktrace and
>    an event enabled with trace_event= unwinds PID 1's "user stack"
>    during initcalls.
> With the ftrace userstacktrace option enabled, the architecture
> unwinders then read the user stack without an mm. That access does not
> reliably fail: the user half of the address space belongs to whatever
> mm happens to be lazily loaded, if any, so the unwinder may read
> another task's memory.
> Use is_user_task() instead, which also checks current->mm, as perf
> already does for the same reason since commit 76ed27608f7d ("perf:
> sched: Fix perf crash with new is_user_task() helper"). This also stops
> unwinding the user stack of PF_USER_WORKER threads, which never run in
> user space, matching perf.
> Fixes: 214d8ca6ee85 ("stacktrace: Provide common infrastructure")
> Signed-off-by: James Houghton <jthoughton@google.com>

Is the Fixes: tag pointing at the right commit?

When 214d8ca6ee85 added the ARCH_STACKWALK version of
stack_trace_save_user(), it guarded the unwind with:

	/* Trace user stack if not a kernel thread */
	if (!current->mm)
		return 0;

That check already covers both cases described above (a task past exit_mm(),
and PID 1 or usermode helpers before exec, which have mm == NULL).

The check was replaced by commit 7e8e6816c649 ("stacktrace: Use PF_KTHREAD to
check for kernel threads"), which switched it to:

	if (current->flags & PF_KTHREAD)
		return 0;

That dropped the no-mm protection, which looks like what this patch restores
via is_user_task().

Should this be:

  Fixes: 7e8e6816c649 ("stacktrace: Use PF_KTHREAD to check for kernel threads")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/40410166379