Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Josh Poimboeuf <jpoimboe@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     kernel test robot <lkp@intel.com>, kbuild-all@lists.01.org,
        linux-kernel@vger.kernel.org, mbenes@suse.cz, x86@kernel.org,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: drivers/gpu/drm/i915/i915.prelink.o: warning: objtool:
 __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR:
 vlv_allow_gt_wake.cold+0x0
In-Reply-To: <20220406000500.5hlaqy5zrdqsg5mg@treble>
References: <202204041241.Hw855BWm-lkp@intel.com>
 <YkxLqznOz0ldTz5a@hirez.programming.kicks-ass.net>
 <20220406000500.5hlaqy5zrdqsg5mg@treble>
Date:   Wed, 06 Apr 2022 02:46:22 +0200
Message-ID: <87czhv11k1.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Tue, Apr 05 2022 at 17:05, Josh Poimboeuf wrote:
> On Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
>
> But objtool is complaining about a real problem (albeit with a cryptic
> warning).  I don't think we want to paper over that. See patch.
>
> Also, are in-tree users of trace_printk() even allowed??

See the comment in the header file you are patching:

 * This is intended as a debugging tool for the developer only.
 * Please refrain from leaving trace_printks scattered around in
 * your code. (Extra memory is used for special buffers that are
 * allocated when trace_printk() is used.)

....

> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] tracing: Fix _THIS_IP_ usage in trace_printk()
>
> do_trace_printk() uses the _THIS_IP_ macro to save the current
> instruction pointer as an argument to a called function.  However,
> because _THIS_IP_ relies on an empty label hack to get the IP, the
> compiler is actually free to place the label anywhere in the function,
> including at the very end -- which, since the label doesn't actually
> have any code, is technically at the beginning of whatever function
> happens to come next.
>
> For example:
>
>       1d89:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
>   			1d8c: R_X86_64_32S	.text.unlikely+0x1d3a
>       1d90:	e8 00 00 00 00       	callq  1d95 <__intel_wait_for_register_fw.cold+0xd4>
>   			1d91: R_X86_64_PLT32	__trace_bprintk-0x4
>       1d95:	e8 00 00 00 00       	callq  1d9a <__intel_wait_for_register_fw.cold+0xd9>
>   			1d96: R_X86_64_PLT32	__sanitizer_cov_trace_pc-0x4
>       1d9a:	bf 01 00 00 00       	mov    $0x1,%edi
>       1d9f:	e8 00 00 00 00       	callq  1da4 <__intel_wait_for_register_fw.cold+0xe3>
>   			1da0: R_X86_64_PLT32	ftrace_dump-0x4
>       1da4:	31 f6                	xor    %esi,%esi
>       1da6:	bf 09 00 00 00       	mov    $0x9,%edi
>       1dab:	e8 00 00 00 00       	callq  1db0 <__intel_wait_for_register_fw.cold+0xef>
>   			1dac: R_X86_64_PLT32	add_taint-0x4
>       1db0:	90                   	nop
>       1db1:	0f 0b                	ud2
>
>   0000000000001db3 <vlv_allow_gt_wake.cold>:
>
> In this case _THIS_IP_ causes the instruction at 0x1d89 to reference the
> next function.  This results in a semi-cryptic objtool warning:
>
>   warning: objtool: __intel_wait_for_register_fw.cold()+0xce: relocation to !ENDBR: vlv_allow_gt_wake.cold+0x
>
> While _THIS_IP_ is inherently imprecise, we can at least coddle the
> compiler into putting the label *before* the call by using _THIS_IP_
> immediately before the call instead of as an argument to the call.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  include/linux/kernel.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 08ba5995aa8b..c399b29840eb 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -390,13 +390,15 @@ do {									\
>  	static const char *trace_printk_fmt __used			\
>  		__section("__trace_printk_fmt") =			\
>  		__builtin_constant_p(fmt) ? fmt : NULL;			\
> +	unsigned long __ip;						\
>  									\
>  	__trace_printk_check_format(fmt, ##args);			\
>  									\
> +	__ip = _THIS_IP_;						\
>  	if (__builtin_constant_p(fmt))					\
> -		__trace_bprintk(_THIS_IP_, trace_printk_fmt, ##args);	\
> +		__trace_bprintk(__ip, trace_printk_fmt, ##args);	\
>  	else								\
> -		__trace_printk(_THIS_IP_, fmt, ##args);			\
> +		__trace_printk(__ip, fmt, ##args);			\
>  } while (0)
>  
>  extern __printf(2, 3)

This covers the trace_printk() case which uses do_trace_printk(), but
the same problem exists in trace_puts() and ftrace_vprintk()...., no?

Thanks,

        tglx