Re: [PATCH v3 1/1] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu, 01 Oct 2026 10:37:51 +0000
sashiko-bot@kernel.org 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 <vineet.gupta@linux.dev>
> 
> 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/20261001102909.1152572-2-vineet.gupta@linux.dev?part=1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>