Received-SPF: pass (google.com: domain of linux-kernel+bounces-189231-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 24 May 2024 21:32:08 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Florent Revest
 <revest@chromium.org>, linux-trace-kernel@vger.kernel.org, LKML
 <linux-kernel@vger.kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>,
 bpf <bpf@vger.kernel.org>, Sven Schnelle <svens@linux.ibm.com>, Alexei
 Starovoitov <ast@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Arnaldo
 Carvalho de Melo <acme@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
 Alan Maguire <alan.maguire@oracle.com>, Mark Rutland
 <mark.rutland@arm.com>, Peter Zijlstra <peterz@infradead.org>, Thomas
 Gleixner <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Subject: Re: [PATCH v10 07/36] function_graph: Allow multiple users to
 attach to function graph
Message-ID: <20240524213208.36f274c8@gandalf.local.home>
In-Reply-To: <171509096221.162236.8806372072523195752.stgit@devnote2>
References: <171509088006.162236.7227326999861366050.stgit@devnote2>
	<171509096221.162236.8806372072523195752.stgit@devnote2>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue,  7 May 2024 23:09:22 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> @@ -109,6 +244,21 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func,
>  	if (!current->ret_stack)
>  		return -EBUSY;
>  
> +	/*
> +	 * At first, check whether the previous fgraph callback is pushed by
> +	 * the fgraph on the same function entry.
> +	 * But if @func is the self tail-call function, we also need to ensure
> +	 * the ret_stack is not for the previous call by checking whether the
> +	 * bit of @fgraph_idx is set or not.
> +	 */
> +	ret_stack = get_ret_stack(current, current->curr_ret_stack, &offset);
> +	if (ret_stack && ret_stack->func == func &&
> +	    get_fgraph_type(current, offset + FGRAPH_FRAME_OFFSET) == FGRAPH_TYPE_BITMAP &&
> +	    !is_fgraph_index_set(current, offset + FGRAPH_FRAME_OFFSET, fgraph_idx))
> +		return offset + FGRAPH_FRAME_OFFSET;
> +
> +	val = (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_FRAME_OFFSET;
> +
>  	BUILD_BUG_ON(SHADOW_STACK_SIZE % sizeof(long));

I'm trying to figure out what the above is trying to do. This gets called
once in function_graph_enter() (or function_graph_enter_ops()). What
exactly are you trying to catch here?

Is it from this email:

  https://lore.kernel.org/all/20231110105154.df937bf9f200a0c16806c522@kernel.org/

As that's the last version before you added the above code.

But you also noticed it may not be needed, but triggered a crash without it
in v3:

  https://lore.kernel.org/all/20231205234511.3839128259dfec153ea7da81@kernel.org/

I removed this code in my version and it runs just fine. Perhaps there was
another bug that this was hiding that you fixed in later versions?

-- Steve