On 10/21/26 10:53 AM, [email protected] wrote:
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 <[email protected]>
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?
This has been discussed before and it was agreed this is fine.
Thx,
-Vineet
