Re: [RFC PATCH 3/3] fprobe: Switch fprobe and BPF kprobe-multi to Tasks-Rude RCU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] trace: Use-After-Free in fprobe_module_callback during rhashtable walk
- [Low] trace: sparse context imbalance due to missing RCU_SCHED annotation
--

commit b3e36a52547a2b577250e858535265a49a8ab8b5
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

fprobe: Switch fprobe and BPF kprobe-multi to Tasks-Rude RCU

This commit switches fprobe entry/exit callbacks and BPF kprobe-multi 
asynchronous unregistration from standard RCU to Tasks-Rude RCU. This 
prevents potential use-after-free races during RCU grace periods by 
waiting for preempt-disabled execution contexts to complete.

> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 9f2d98181779a..b7850df17575a 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c

[ ... ]

> @@ -76,8 +76,14 @@ static const struct rhashtable_params fprobe_rht_params = {
>  	.obj_hashfn		= fprobe_node_obj_hashfn,
>  	.obj_cmpfn		= fprobe_node_cmp,
>  	.automatic_shrinking	= true,
> +	.use_tasks_rude		= true,

[Severity: High]
Does setting .use_tasks_rude = true introduce a use-after-free race during
module unload?

Since deferred bucket table reclamation now uses call_rcu_tasks_rude(), the
table walk in fprobe_module_callback() might not have adequate reader
protection:

kernel/trace/fprobe.c:fprobe_module_callback() {
    ...
		rhashtable_walk_start(&iter);

		while ((node = rhashtable_walk_next(&iter)) && !IS_ERR(node))
			if (fprobe_remove_node_in_module(mod, node, &alist) < 0) {
    ...
}

rhashtable_walk_start() solely relies on rcu_read_lock(). Under
CONFIG_PREEMPT_RCU, rcu_read_lock() does not disable preemption, meaning
a preemption during the walk satisfies the Tasks-Rude RCU quiescent state.

If the walker is preempted, could a concurrent background resize complete its
grace period and free the bucket table, causing rhashtable_walk_next() to
dereference freed memory when the walker is scheduled back?

>  };
>  
> +DEFINE_LOCK_GUARD_0(rcu_sched_notrace, rcu_read_lock_sched_notrace(),
> +		    rcu_read_unlock_sched_notrace())
> +DECLARE_LOCK_GUARD_0_ATTRS(rcu_sched_notrace, __acquires_shared(RCU),
> +			   __releases_shared(RCU))

[Severity: Low]
Does the rcu_sched_notrace lock guard declaration omit the RCU_SCHED context?

rcu_read_lock_sched_notrace() acquires both RCU and RCU_SCHED, but this
DECLARE_LOCK_GUARD_0_ATTRS macro only declares __acquires_shared(RCU) and
__releases_shared(RCU).

Could this cause sparse context imbalance warnings due to the missing
__acquires_shared(RCU_SCHED) and __releases_shared(RCU_SCHED) annotations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179060575297.372125.10581329083761385407.stgit@devnote2?part=3