Re: [PATCH RFC v3 05/13] ftrace: Mark modules hosting direct-call trampolines for Tasks RCU

[bot+bpf-ci@kernel.org]

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

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 96cc98568eea5..28488687cb01f 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -521,6 +521,13 @@ struct module {
>  	unsigned int num_ftrace_callsites;
>  	unsigned long *ftrace_callsites;
>  #endif
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +	/*
> +	 * An ftrace direct-call trampoline lives in this module's text; see
> +	 * rcu_tasks_trampoline_text().  Sticky once set.
> +	 */
> +	bool ftrace_direct_tramp;
> +#endif

This isn't a bug, but could the field and ftrace_direct_mark_module()'s
body be gated on CONFIG_TASKS_RCU_TRAMPOLINE_READERS instead? The only
reader of module::ftrace_direct_tramp is rcu_tasks_trampoline_text(),
which lives inside kernel/rcu/tasks.h's CONFIG_TASKS_RCU_TRAMPOLINE_READERS
block:

kernel/rcu/tasks.h:rcu_tasks_trampoline_text() {
#ifdef CONFIG_MODULES
    scoped_guard(rcu) {
        struct module *mod = __module_text_address(ip);

        if (mod) {
            ret = kprobe_in_optimized_region(ip);
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
            ret = ret || READ_ONCE(mod->ftrace_direct_tramp);
#endif
        }
    }
#endif
}

On every configuration that does not select HAVE_RCU_TRAMPOLINE_READERS
the per-module bool and the four ftrace_direct_mark_module() call sites
that write it have no consumer.

> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 8667684628500..ec54a27e47fae 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h

[ ... ]

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 53d5db60bfa56..efc4a518658a3 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c

[ ... ]

> @@ -6090,6 +6113,17 @@ static void reset_direct(struct ftrace_ops *ops, unsigned long addr)
>   * and save the parameters of the function being traced, and restore them
>   * (or inject new ones if needed), before returning.
>   *
> + * Nothing but Tasks RCU keeps the trampoline at @addr alive while a task is
> + * executing it or is preempted in something it called.  On architectures that
> + * select HAVE_RCU_TRAMPOLINE_READERS, Tasks RCU only waits for such a task if
> + * it is a Tasks Trace RCU reader, so the trampoline must enter one
> + * (rcu_read_lock_trace() or its assembly equivalent, see
> + * samples/ftrace/ftrace-direct.h) before calling out and leave it before
> + * returning, as the ftrace and BPF trampolines do.  The few instructions

This isn't a bug, but would it read better to point at the sample header
only once it exists in the tree, or to word the ftrace/BPF trampoline
part as what the series establishes rather than as current state? The new
kernel-doc paragraph points readers at samples/ftrace/ftrace-direct.h,
which does not exist at this commit (it is added later in the series by
'samples: ftrace: Make the direct-call trampolines Tasks Trace readers'),
and states in the present tense that the ftrace and BPF trampolines
already take the reader, which also only becomes true in later commits of
the series (bpf/x86/arm64 reader patches). Anyone reading or bisecting at
this commit gets a dangling reference and a claim the tree does not yet
satisfy.

> + * before and after are covered by the irq-exit check: automatically for
> + * trampolines outside kernel and module text (e.g. BPF images), and via
> + * ftrace_direct_mark_module() for trampolines in module text.
> + *
>   * Returns:
>   *  0 on success
>   *  -EINVAL  - The @ops object was already registered with this call or

[ ... ]

commit ef57b52d2606546f6d483b113fc2202c13863c2c
Author: Josef Bacik <josef@toxicpanda.com>

>     ftrace: Mark modules hosting direct-call trampolines for Tasks RCU
>
>     An out-of-line direct trampoline registered with register_ftrace_direct()
>     is kept alive only by Tasks RCU while a task executes it or is preempted
>     in something it called; ftrace_shutdown()'s synchronize_rcu_tasks() is
>     what stops rmmod freeing it under such a task.  Where Tasks RCU is built
>     on reader-marked trampolines, such a trampoline must be a Tasks Trace
>     reader across its call-out like the ftrace and BPF trampolines are, so
>     document that in register_ftrace_direct().
>
>     That still leaves the few instructions before the reader is entered and
>     after it is left.  For BPF images those are in dynamically allocated
>     text that rcu_tasks_trampoline_text() already treats as unmarked
>     trampoline text, but the in-tree samples (and any similar user) place
>     their trampolines in module .text.  Add a sticky
>     module::ftrace_direct_tramp flag, set by every register/modify path when
>     the direct address is module text, and have rcu_tasks_trampoline_text()
>     treat a task interrupted anywhere in such a module as a potential
>     holdout.  Other modules' text is unaffected.
>
>     Assisted-by: LLM
>     Signed-off-by: Josef Bacik <josef@toxicpanda.com>

This looks like a bug fix for reader-marked trampoline support. Should
this include:

  Fixes: 00bd9b746f5e ("rcu-tasks: Add a Tasks RCU implementation for reader-marked trampolines")

The bug was introduced in commit 00bd9b746f5e which added
rcu_tasks_trampoline_text() but failed to properly handle module .text
containing direct trampolines. The function returned false for module
text addresses, meaning such trampolines were not protected by Tasks RCU,
potentially allowing module text to be freed while still executing.


---
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/34975311706