Re: [PATCH v4 11/15] tracing/fgraph: convert process/thread iterator to for_each_process_thread_rculock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] trace/fgraph: mixed goto-based error handling with scope-based cleanup
--

commit 6422a16035934ad7c858389b51cb5a7d9fecc15f
Author: Ye Liu <liuye@kylinos.cn>
tracing/fgraph: convert process/thread iterator to for_each_process_thread_rculock

This commit converts the manual rcu_read_lock() and rcu_read_unlock() pair
around the for_each_process_thread() loop in alloc_retstack_tasklist() to use
the for_each_process_thread_rculock() macro. This scopes the RCU read lock to
the loop body using a scoped guard, and removes the manual unlock label in the
error handling path.

> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 40d373d65f9b9..f797ce1192233 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1057,11 +1057,10 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>  		}
>  	}
>  
> -	rcu_read_lock();
> -	for_each_process_thread(g, t) {
> +	for_each_process_thread_rculock(g, t) {
>  		if (start == end) {
>  			ret = -EAGAIN;
> -			goto unlock;
> +			goto free;

[Severity: Low]
This doesn't cause a direct runtime bug here, but does this change mix
goto-based cleanup with scope-based cleanup?

The for_each_process_thread_rculock() macro initializes a scope-based RCU
guard via scoped_guard(rcu). By using goto free; to jump out of the
scoped guard to a manual cleanup label, this explicitly violates the
strict kernel cleanup rule in include/linux/cleanup.h meant to prevent
double-free and resource leak bugs. That rule states that goto-based
cleanup and scope-based cleanup must never be mixed in the same function.

While jumping out of scoped_guard() here triggers the RCU unlock safely
before executing the manual free label, should alloc_retstack_tasklist()
be fully converted to use scope-based cleanup for the ret_stack_list
allocations as well, or should it stick to manual RCU locks to avoid
mixing paradigms?

>  		}
>  
>  		if (t->ret_stack == NULL) {

[ ... ]

> @@ -1074,9 +1073,6 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>  			t->ret_stack = ret_stack_list[start++];
>  		}
>  	}
> -
> -unlock:
> -	rcu_read_unlock();
>  free:
>  	for (i = start; i < end; i++)
>  		kmem_cache_free(fgraph_stack_cachep, ret_stack_list[i]);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921092101.89285-1-ye.liu@linux.dev?part=11