Re: [PATCH v3 1/1] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] tracing: mixing goto and scoped_guard in alloc_retstack_tasklist()
--

commit eacae2b1226bb2d8684604b5c37e3ec9f073cb80
Author: Vineet Gupta <vineet.gupta@linux.dev>

tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

This commit updates alloc_retstack_tasklist() to allocate return stacks
inline using GFP_NOWAIT within an RCU read-side critical section,
replacing the fixed-batch preallocation loop. It also replaces the
open-coded rcu_read_lock() and rcu_read_unlock() with scoped_guard(rcu)
to handle RCU protection implicitly.

> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 40d373d65f9b9..efb5d260334e0 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c

[ ... ]

> @@ -1057,26 +1060,37 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>  		}
>  	}
>  
> -	rcu_read_lock();
> -	for_each_process_thread(g, t) {
> -		if (start == end) {
> -			ret = -EAGAIN;
> -			goto unlock;
> -		}
> +	scoped_guard (rcu) {

[Severity: Low]
Does this code mix goto-based error handling and scope-based cleanup in the
same function?

The alloc_retstack_tasklist() function still contains a goto free error
handling path earlier in its body, and a free: cleanup label at the end.
Adding scoped_guard() here violates the kernel cleanup subsystem guidelines,
which state that for a given routine, usage of goto and cleanup helpers
should never be mixed.

Since returning directly from within the scoped_guard() skips the free: loop
at the end of the function, it forces manual reasoning about whether that is
safe. This is noted in the new comment about keeping the start == end check
in step with the exhaustion logic.

Is it possible to convert the initial return stack array allocation to use
scope-based cleanup as well, or revert to open-coded RCU read locks, to
avoid mixing the two patterns?

> +		for_each_process_thread(g, t) {
> +			unsigned long *rs;
> +
> +			if (t->ret_stack)
> +				continue;
> +
> +			rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
> +			if (!rs) {
> +				/*
> +				 * Returning from inside scoped_guard() drops
> +				 * the RCU read lock, but skips the free loop
> +				 * below. That is only safe because start ==
> +				 * end here, which leaves that loop nothing to
> +				 * free. Keep the two in step if this
> +				 * exhaustion check ever changes.
> +				 */
> +				if (start == end)
> +					return -EAGAIN;
> +				rs = ret_stack_list[start++];
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001102909.1152572-2-vineet.gupta@linux.dev?part=1