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 <[email protected]> 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/[email protected]?part=1
