On Thu, 01 Oct 2026 10:37:51 +0000
[email protected] wrote:
> 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?
If this sends to stable kernel, I think it is better to keep using
goto in this patch, and send another cleanup later. It is allow us
to backport the patch easier.
Thank you,
>
> > + 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
>
--
Masami Hiramatsu (Google) <[email protected]>
