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

[Josh Poimboeuf]

On Wed, Apr 06, 2022 at 02:46:22AM +0200, Thomas Gleixner wrote:
> 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.)

So what do we do ... send a nastygram?  

> > +	__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?

Yes, though objtool didn't seem to complain about those yet.  They
probably don't have the perfect storm required for the label to end up
at the end of the function.  It might also need something like being
invoked from within a macro which then does BUG() (see GEM_BUG_ON).

More broadly, this issue could theoretically happen in some other places
throughout the kernel tree, since _THIS_IP_ is fundamentally unreliable
as currently written.

So we could look at making _THIS_IP_ more predictable.

Inline asm would work better ("lea 0(%rip), %[rip]"), but then you need
an arch-dependent implementation...

Or we could add a control dependency like the below ugliness...

Thoughts?


diff --git a/include/linux/instruction_pointer.h b/include/linux/instruction_pointer.h
index cda1f706eaeb..3f2f0ebecca0 100644
--- a/include/linux/instruction_pointer.h
+++ b/include/linux/instruction_pointer.h
@@ -2,7 +2,9 @@
 #ifndef _LINUX_INSTRUCTION_POINTER_H
 #define _LINUX_INSTRUCTION_POINTER_H
 
+unsigned long __this_ip(void);
+
 #define _RET_IP_		(unsigned long)__builtin_return_address(0)
-#define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })
+#define _THIS_IP_  __this_ip()
 
 #endif /* _LINUX_INSTRUCTION_POINTER_H */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index da03c15ecc89..8674c7434ead 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3781,3 +3781,8 @@ void __printk_cpu_unlock(void)
 }
 EXPORT_SYMBOL(__printk_cpu_unlock);
 #endif /* CONFIG_SMP */
+
+unsigned long __this_ip(void)
+{
+	return _RET_IP_;
+}