Re: [PATCH bpf-next 2/2] uprobes: Switch uretprobes_srcu to SRCU-fast-updown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent hprobe_expire() and hprobe_consume() can cause a use-after-free on uprobe due to misunderstood SRCU semantics.
--

commit 86c857f0a70ec453b9e38c36e230b36b91bef460
Author: Puranjay Mohan <puranjay@kernel.org>

    uprobes: Switch uretprobes_srcu to SRCU-fast-updown
    
    uretprobes_srcu currently uses normal SRCU, which issues
    two smp_mb() per read lock/unlock pair. This overhead is
    paid on every uretprobe hit.
    
    Switch to SRCU-fast-updown, which eliminates the per-reader
    memory barriers by moving the ordering cost to the
    grace-period side.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4084e926e2844..afa491b0bd3f9 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
[ ... ]
> @@ -2045,7 +2046,7 @@ static void ri_timer(struct timer_list *timer)
>  	struct return_instance *ri;
>  
>  	/* SRCU protects uprobe from reuse for the cmpxchg() inside hprobe_expire(). */
> -	guard(srcu)(&uretprobes_srcu);
> +	guard(srcu_fast_updown)(&uretprobes_srcu);

[Severity: High]
This isn't a bug introduced by this patch, but does the new SRCU read lock
acquired here actually protect the uprobe from being freed?

If a uprobe is being unregistered, uprobe_free_deferred() calls
call_srcu(&uretprobes_srcu, ...) to free the uprobe. This starts an SRCU
grace period that waits only for pre-existing SRCU readers to finish.

Concurrently, if ri_timer() or dup_utask() takes this new SRCU read lock,
the grace period will not wait for it because the new lock started after the
grace period began.

If the traced task concurrently executes uprobe_handle_trampoline(), calls
hprobe_consume() and hprobe_finalize(), and releases the original SRCU read
lock, the grace period can complete and free the uprobe (via
call_rcu_tasks_trace()).

When ri_timer() then proceeds to call hprobe_expire(), which does:

kernel/events/uprobes.c:hprobe_expire() {
    ...
    case HPROBE_LEASED: {
        struct uprobe *uprobe = try_get_uprobe(hprobe->uprobe);
    ...
}

Could this lead to a use-after-free on the uprobe? The comment in
hprobe_expire() seems to assume the new lock prevents the free:

        /*
         * Even if hprobe_consume() or another hprobe_expire() wins
         * the state update race and unlocks SRCU from under us, we
         * still have a guarantee that underyling uprobe won't be
         * freed due to ongoing caller's SRCU lock region, so we can
         * return it regardless.
         */

Is this analysis accurate?

>  	/* RCU protects return_instance from freeing. */
>  	guard(rcu)();
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706172744.3920417-1-puranjay@kernel.org?part=2