Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Jun 2023 12:34:07 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Sven Schnelle <svens@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tracing: fix memcpy size when copying stack entries
Message-ID: <20230612123407.5ebcabdf@gandalf.local.home>
In-Reply-To: <20230612160748.4082850-1-svens@linux.ibm.com>
References: <20230612160748.4082850-1-svens@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 12 Jun 2023 18:07:48 +0200
Sven Schnelle <svens@linux.ibm.com> wrote:

> Noticed the following warning during boot:
> 
> [    2.316341] Testing tracer wakeup:
> [    2.383512] ------------[ cut here ]------------
> [    2.383517] memcpy: detected field-spanning write (size 104) of single field "&entry->caller" at kernel/trace/trace.c:3167 (size 64)
> 
> The reason seems to be that the maximum number of entries is calculated
> from the size of the fstack->calls array which is 128. But later the same
> size is used to memcpy() the entries to entry->callers, which has only
> room for eight elements. Therefore use the minimum of both arrays as limit.
> 
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 64a4dde073ef..988d664c13ec 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3146,7 +3146,7 @@ static void __ftrace_trace_stack(struct trace_buffer *buffer,
>  	barrier();
>  
>  	fstack = this_cpu_ptr(ftrace_stacks.stacks) + stackidx;
> -	size = ARRAY_SIZE(fstack->calls);
> +	size = min(ARRAY_SIZE(entry->caller), ARRAY_SIZE(fstack->calls));

No, this is not how it works, and this breaks the stack tracing code.

>  
>  	if (regs) {
>  		nr_entries = stack_trace_save_regs(regs, fstack->calls,

I guess we need to add some type of annotation to make the memcpy()
checking happy.

Let me explain what is happening. By default the stack trace has a minimum
of 8 entries (defined by struct stack_entry, which is used to show to user
space the default size - for backward compatibility).

Let's take a look at the code in more detail:

/* What is the size of the temp buffer to use to find the stack? */
	size = ARRAY_SIZE(fstack->calls);

	if (regs) {
/* Fills in the stack into the temp buffer */
		nr_entries = stack_trace_save_regs(regs, fstack->calls,
						   size, skip);
	} else {
/* Also fills in the stack into the temp buffer */
		nr_entries = stack_trace_save(fstack->calls, size, skip);
	}

/* Calculate the size from the number of entries stored in the temp buffer */

	size = nr_entries * sizeof(unsigned long);

/* Now reserve space on the ring buffer */
	event = __trace_buffer_lock_reserve(buffer, TRACE_STACK,

/*
 * Notice how it calculates the size! It subtracts the sizeof
 *  entry->caller and then adds size again!
 */
				    (sizeof(*entry) - sizeof(entry->caller)) + size,
				    trace_ctx);
	if (!event)
		goto out;

/* Point entry to the ring buffer data */
	entry = ring_buffer_event_data(event);

/* Now copy the stack to the location for the data on the ftrace ring buffer */
	memcpy(&entry->caller, fstack->calls, size);
	entry->size = nr_entries;

The old way use to just record the 8 entries, but that was not very useful
in real world analysis. Your patch takes that away. Might as well just
record directly into the ring buffer again like it use to.

Yes the above may be special, but your patch breaks it.

NAK on the patch, but I'm willing to update this to make your tooling
handle this special case.

-- Steve