Re: [PATCH v4 2/2] tracing: fgraph: use scoped_guard() for the task list walk

[sashiko-bot@kernel.org]

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

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

tracing: fgraph: use scoped_guard() for the task list walk

This commit replaces the explicit rcu_read_lock() and rcu_read_unlock() pair
in alloc_retstack_tasklist() with scoped_guard(rcu) and removes the unlock
label by jumping to the free label on the -EAGAIN path.

> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 4c74f1a15ad48..bbf6e4e2da2d4 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1057,32 +1057,31 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>  		}
>  	}
>  
> -	rcu_read_lock();
> -	for_each_process_thread(g, t) {
> -		unsigned long *rs;
> +	scoped_guard (rcu) {
> +		for_each_process_thread(g, t) {
> +			unsigned long *rs;
>  
> -		if (t->ret_stack)
> -			continue;
> +			if (t->ret_stack)
> +				continue;
>  
> -		rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
> -		if (!rs) {
> -			if (start == end) {
> -				ret = -EAGAIN;
> -				goto unlock;
> +			rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
> +			if (!rs) {
> +				if (start == end) {
> +					ret = -EAGAIN;
> +					goto free;
> +				}
> +				rs = ret_stack_list[start++];
>  			}
> -			rs = ret_stack_list[start++];
> -		}
>  
> -		atomic_set(&t->trace_overrun, 0);
> -		ret_stack_init_task_vars(rs);
> -		t->curr_ret_stack = 0;
> -		t->curr_ret_depth = -1;
> -		/* Make sure the tasks see the 0 first: */
> -		smp_store_release(&t->ret_stack, rs);
> +			atomic_set(&t->trace_overrun, 0);
> +			ret_stack_init_task_vars(rs);
> +			t->curr_ret_stack = 0;
> +			t->curr_ret_depth = -1;
> +			/* Make sure the tasks see the 0 first: */
> +			smp_store_release(&t->ret_stack, rs);
> +		}
>  	}
>  
> -unlock:
> -	rcu_read_unlock();
>  free:
>  	for (i = start; i < end; i++)
>  		kmem_cache_free(fgraph_stack_cachep, ret_stack_list[i]);

[Severity: Low]
In kernel/trace/fgraph.c, alloc_retstack_tasklist() now uses
scoped_guard(rcu) while still jumping out of the guarded scope via goto free
when kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT) fails and start == end,
as well as jumping to free from the earlier pre-allocation loop:

kernel/trace/fgraph.c:alloc_retstack_tasklist() {
	for (i = 0; i < FTRACE_RETSTACK_ALLOC_SIZE; i++) {
		ret_stack_list[i] = kmem_cache_alloc(fgraph_stack_cachep, GFP_KERNEL);
		if (!ret_stack_list[i]) {
			start = 0;
			end = i;
			ret = -ENOMEM;
			goto free;
		}
	}
}

The documentation in include/linux/cleanup.h notes that goto-based cleanup
and scope-based cleanup helpers should not be mixed within the same function:

 * Lastly, given that the benefit of cleanup helpers is removal of
 * "goto", and that the "goto" statement can jump between scopes, the
 * expectation is that usage of "goto" and cleanup helpers is never
 * mixed in the same function. I.e. for a given routine, convert all
 * resources that need a "goto" cleanup to scope-based cleanup, or
 * convert none of them.

Also, on the -EAGAIN path inside scoped_guard(rcu), start == end is already
true because all pre-allocated entries in ret_stack_list[] have been consumed,
so the cleanup loop at the free label runs zero iterations on that path.

Should alloc_retstack_tasklist() avoid mixing scoped_guard(rcu) with goto
free?

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