Re: [PATCH v7 10/10] selftests: ftrace: Add wprobe trigger testcase

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 15 Jul 2026 02:05:55 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [Critical] Deadlocks due to NMI-unsafe operations (schedule_work and raw_spin_lock_irqsave) in tracepoint context.
> - [High] Missing cancel_work_sync() causes a Use-After-Free of wprobe_trigger_data via the workqueue.
> - [High] Synchronous free of wprobe_data violates RCU removal ordering, risking concurrent use-after-free.
> - [High] Missing trace_event_try_get_ref() and imbalanced trace_event_put_ref() cause module refcount underflow.
> - [High] on_each_cpu is called with wait=false for an IPI that references wprobe_data.
> - [High] Immediate kfree() of RCU-protected trigger_data and wprobe_data on the error path.
> - [Medium] Permanent memory leak of wprobe trigger objects on successful registration.
> - [Medium] tw->addr is read and written concurrently by different triggers without a shared lock.
> --
> 

Thanks for the review comments.
BTW, why this only to linux-perf-users? (Maybe I need to modify Sashiko setting)

> --- Patch [9]: [PATCH v7 09/10] tracing: wprobe: Add wprobe event trigger ---
> The format of this report is altered due to recitation restrictions.
> I am providing a free-form summary of the findings rather than inline
> quoting the patch.
> 
> Commit: 4087513c82b11b98d27cfc4452df9a1a8e41ddd5
> Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Subject: tracing: wprobe: Add wprobe event trigger
> 
> This patch adds a wprobe event trigger to set and clear watch events
> dynamically, allowing watchpoints on local variables and slab objects.
> 
> [Severity: Critical]
> Is it safe to call raw_spin_lock_irqsave() and schedule_work() directly
> from wprobe_trigger()? Tracepoints can execute in arbitrary contexts,
> including NMI or with scheduler locks held. Calling schedule_work() is
> unsafe in these contexts. Additionally, acquiring wprobe_data->lock here
> can cause a hard deadlock if the tracepoint interrupts wprobe_smp_update_func()
> while it is holding the exact same lock in IPI context.

Hmm, good catch!
OK, use a pair of irq_work and work to call on_each_cpu() for some
interrupt context.
For NMI context, I think wprobe_trigger should check the original state
is in_nmi(). This can work for normal trace events or fprobe, but not
for kprobes, because kprobe handler itself is working inside NMI context.
Hmm, let me check how can I fix this. Until solving this problem, this
wprobe trigger may not work with kprobes.
Also, I found the lock should protect the wprobe itself, not data.
Let me move the irq_work and lock to trace_wprobe.

Hmm, we may need to pass to original context (calculated by preempt_count)
to trigger event so that it can evaluate the original context.
(BTW, )

> [Severity: High]
> Does free_wprobe_trigger_data() safely handle the embedded work struct?
> When a trigger is unregistered, this function frees wprobe_data via kfree().
> If wprobe_trigger() recently scheduled wprobe_data->work, the workqueue
> could execute wprobe_work_func() on the freed memory since there is no
> cancel_work_sync() before freeing.

OK, so move the worker to wprobe because anyway the worker will update
the wprobe itself.

> 
> [Severity: High]
> Is an RCU grace period needed in wprobe_trigger_free()?
> wprobe_unregister_trigger() unlinks the trigger and calls
> wprobe_trigger_free(), which synchronously frees wprobe_data. Since
> tracepoints execute under RCU, could a concurrent tracepoint execution
> still be accessing wprobe_data when it gets freed?

Indeed, let's use call_rcu().

> 
> [Severity: High]
> Can the error path in wprobe_trigger_cmd_parse() cause a use-after-free?
> If event_trigger_register() successfully exposes the trigger to RCU
> readers but a subsequent step fails, returning an error causes the
> __free(kfree) cleanup to immediately free trigger_data and wprobe_data.
> This bypasses any RCU grace period for objects that were just briefly
> exposed to concurrent tracepoint readers.

Ah, good catch. OK, let me call synchronize_rcu() before returning.

> 
> [Severity: High]
> Is it intentional that on_each_cpu() is called with wait=false in
> wprobe_work_func()? Because it doesn't wait, the work function completes
> immediately while IPIs are still pending on other CPUs. If the trigger
> is unregistered and freed right after, wprobe_smp_update_func() will
> access freed wprobe_data.

Ah, indeed. I will use a combination of irq_worker and worker and use
on_each_cpu() with wait=true for safety.

> 
> [Severity: High]
> Does wprobe_trigger_cmd_parse() acquire the target event's module
> reference? The code calls trace_event_enable_disable() directly without
> first calling trace_event_try_get_ref(). However, wprobe_trigger_free()
> unconditionally calls trace_event_put_ref(), which could lead to a module
> refcount underflow.

OK, let me fix it.

> 
> [Severity: Medium]
> Is there a memory leak on the success path of wprobe_trigger_cmd_parse()?
> The function increments the refcount via event_trigger_init(), and then
> event_trigger_register() increments it again. On success, trigger_data
> is set to NULL to bypass the cleanup macro, but the initial refcount is
> never dropped. When the trigger is later unregistered, the refcount only
> drops to 1, permanently leaking the objects.

OK, call trace_event_put_ref directly in wprobe_trigger_cmd_parse().


> 
> [Severity: Medium]
> How is concurrent access to tw->addr synchronized between different
> triggers? If a user attaches multiple triggers (e.g., set_wprobe and
> clear_wprobe) targeting the same wprobe event, they allocate separate
> wprobe_data instances with independent locks. When wprobe_trigger()
> executes, they will read and write the shared tw->addr simultaneously
> without a common lock, potentially corrupting the hardware breakpoint state.

I decided to use a lock in trace_wprobe side and use READ_ONCE/WRITE_ONCE()
which may be enough.

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>