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

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

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head:   3123109284176b1532874591f7c81f3837bbdc17
commit: d31ed5d767c0452b4f49846d80a0bfeafa3a4ded kbuild: Fixup the IBT kbuild changes
date:   12 days ago
config: x86_64-randconfig-a011-20220404 (https://download.01.org/0day-ci/archive/20220404/202204041241.Hw855BWm-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
        # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
        git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
        git fetch --no-tags linus master
        git checkout d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> 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
   drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: gen6_alloc_va_range.cold()+0x1c6: relocation to !ENDBR: i915_vma_unpin.cold+0x0
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: fence_update.cold()+0x14a: relocation to !ENDBR: i915_vma_revoke_fence.cold+0x0
   drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: eb_move_to_gpu.cold()+0x52: relocation to !ENDBR: i915_reset_gen7_sol_offsets.cold+0x0
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __i915_gem_object_release_mmap_gtt.cold()+0xce: relocation to !ENDBR: i915_gem_mmap.cold+0x0
   drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: i915_ttm_io_mem_pfn.cold()+0x52: relocation to !ENDBR: i915_ttm_delayed_free.cold+0x0
>> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_close.cold()+0x52: relocation to !ENDBR: ttm_vm_open.cold+0x0
   drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_open.cold()+0x52: relocation to !ENDBR: i915_ttm_shrinker_release_pages.cold+0x0

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Peter Zijlstra]

On Mon, Apr 04, 2022 at 12:33:19PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   3123109284176b1532874591f7c81f3837bbdc17
> commit: d31ed5d767c0452b4f49846d80a0bfeafa3a4ded kbuild: Fixup the IBT kbuild changes
> date:   12 days ago
> config: x86_64-randconfig-a011-20220404 (https://download.01.org/0day-ci/archive/20220404/202204041241.Hw855BWm-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
> reproduce (this is a W=1 build):
>         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
>         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>         git fetch --no-tags linus master
>         git checkout d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
>         # save the config file to linux build tree
>         mkdir build_dir
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
> >> 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
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: gen6_alloc_va_range.cold()+0x1c6: relocation to !ENDBR: i915_vma_unpin.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: fence_update.cold()+0x14a: relocation to !ENDBR: i915_vma_revoke_fence.cold+0x0
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: eb_move_to_gpu.cold()+0x52: relocation to !ENDBR: i915_reset_gen7_sol_offsets.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __i915_gem_object_release_mmap_gtt.cold()+0xce: relocation to !ENDBR: i915_gem_mmap.cold+0x0
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: i915_ttm_io_mem_pfn.cold()+0x52: relocation to !ENDBR: i915_ttm_delayed_free.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_close.cold()+0x52: relocation to !ENDBR: ttm_vm_open.cold+0x0
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_open.cold()+0x52: relocation to !ENDBR: i915_ttm_shrinker_release_pages.cold+0x0

---
Subject: objtool/ibt: Allow _THIS_IP_ at: sym+len
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Apr  5 15:54:41 CEST 2022

0day robot reported:

  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

Which turns out to be GCC placing a _THIS_IP_ past the end of the
function:

0000000000001d00 <__intel_wait_for_register_fw.cold>:
    ...
    1dce:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi        1dd1: R_X86_64_32S      .text.unlikely+0x1df8
    1dd5:       e8 00 00 00 00          call   1dda <__intel_wait_for_register_fw.cold+0xda>    1dd6: R_X86_64_PLT32    __trace_bprintk-0x4
    ...
    1df6:       0f 0b                   ud2

Add an exception for this one weird case...

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3198,6 +3198,8 @@ static void warn_noendbr(const char *msg
 static void validate_ibt_dest(struct objtool_file *file, struct instruction *insn,
 			      struct instruction *dest)
 {
+	struct instruction *last, *next;
+
 	if (dest->func && dest->func == insn->func) {
 		/*
 		 * Anything from->to self is either _THIS_IP_ or IRET-to-self.
@@ -3217,6 +3219,21 @@ static void validate_ibt_dest(struct obj
 	if (dest->noendbr)
 		return;
 
+	/*
+	 * Occasionally, when the last instruction of the function is UD2, GCC
+	 * manages to generate a text reference to the instruction after it.
+	 */
+	last = next = insn;
+	for (;;) {
+		next = next_insn_same_sec(file, next);
+		if (!next || next->func != insn->func)
+			break;
+		last = next;
+	}
+	if (last->type == INSN_BUG &&
+	    last->offset + last->len == dest->offset)
+		return;
+
 	warn_noendbr("", insn->sec, insn->offset, dest);
 }
 

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

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 4363 bytes --]

On Mon, Apr 04, 2022 at 12:33:19PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   3123109284176b1532874591f7c81f3837bbdc17
> commit: d31ed5d767c0452b4f49846d80a0bfeafa3a4ded kbuild: Fixup the IBT kbuild changes
> date:   12 days ago
> config: x86_64-randconfig-a011-20220404 (https://download.01.org/0day-ci/archive/20220404/202204041241.Hw855BWm-lkp(a)intel.com/config)
> compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
> reproduce (this is a W=1 build):
>         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
>         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>         git fetch --no-tags linus master
>         git checkout d31ed5d767c0452b4f49846d80a0bfeafa3a4ded
>         # save the config file to linux build tree
>         mkdir build_dir
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
> >> 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
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: gen6_alloc_va_range.cold()+0x1c6: relocation to !ENDBR: i915_vma_unpin.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: fence_update.cold()+0x14a: relocation to !ENDBR: i915_vma_revoke_fence.cold+0x0
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: eb_move_to_gpu.cold()+0x52: relocation to !ENDBR: i915_reset_gen7_sol_offsets.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: __i915_gem_object_release_mmap_gtt.cold()+0xce: relocation to !ENDBR: i915_gem_mmap.cold+0x0
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: i915_ttm_io_mem_pfn.cold()+0x52: relocation to !ENDBR: i915_ttm_delayed_free.cold+0x0
> >> drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_close.cold()+0x52: relocation to !ENDBR: ttm_vm_open.cold+0x0
>    drivers/gpu/drm/i915/i915.prelink.o: warning: objtool: ttm_vm_open.cold()+0x52: relocation to !ENDBR: i915_ttm_shrinker_release_pages.cold+0x0

---
Subject: objtool/ibt: Allow _THIS_IP_ at: sym+len
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Apr  5 15:54:41 CEST 2022

0day robot reported:

  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

Which turns out to be GCC placing a _THIS_IP_ past the end of the
function:

0000000000001d00 <__intel_wait_for_register_fw.cold>:
    ...
    1dce:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi        1dd1: R_X86_64_32S      .text.unlikely+0x1df8
    1dd5:       e8 00 00 00 00          call   1dda <__intel_wait_for_register_fw.cold+0xda>    1dd6: R_X86_64_PLT32    __trace_bprintk-0x4
    ...
    1df6:       0f 0b                   ud2

Add an exception for this one weird case...

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3198,6 +3198,8 @@ static void warn_noendbr(const char *msg
 static void validate_ibt_dest(struct objtool_file *file, struct instruction *insn,
 			      struct instruction *dest)
 {
+	struct instruction *last, *next;
+
 	if (dest->func && dest->func == insn->func) {
 		/*
 		 * Anything from->to self is either _THIS_IP_ or IRET-to-self.
@@ -3217,6 +3219,21 @@ static void validate_ibt_dest(struct obj
 	if (dest->noendbr)
 		return;
 
+	/*
+	 * Occasionally, when the last instruction of the function is UD2, GCC
+	 * manages to generate a text reference to the instruction after it.
+	 */
+	last = next = insn;
+	for (;;) {
+		next = next_insn_same_sec(file, next);
+		if (!next || next->func != insn->func)
+			break;
+		last = next;
+	}
+	if (last->type == INSN_BUG &&
+	    last->offset + last->len == dest->offset)
+		return;
+
 	warn_noendbr("", insn->sec, insn->offset, dest);
 }
 

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 Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
> Subject: objtool/ibt: Allow _THIS_IP_ at: sym+len
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Apr  5 15:54:41 CEST 2022
> 
> 0day robot reported:
> 
>   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
> 
> Which turns out to be GCC placing a _THIS_IP_ past the end of the
> function:
> 
> 0000000000001d00 <__intel_wait_for_register_fw.cold>:
>     ...
>     1dce:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi        1dd1: R_X86_64_32S      .text.unlikely+0x1df8
>     1dd5:       e8 00 00 00 00          call   1dda <__intel_wait_for_register_fw.cold+0xda>    1dd6: R_X86_64_PLT32    __trace_bprintk-0x4
>     ...
>     1df6:       0f 0b                   ud2
> 
> Add an exception for this one weird case...
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

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)
-- 
2.34.1

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]

[-- Attachment #1: Type: text/plain, Size: 4162 bytes --]

On Tue, Apr 05, 2022 at 04:01:15PM +0200, Peter Zijlstra wrote:
> Subject: objtool/ibt: Allow _THIS_IP_ at: sym+len
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Apr  5 15:54:41 CEST 2022
> 
> 0day robot reported:
> 
>   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
> 
> Which turns out to be GCC placing a _THIS_IP_ past the end of the
> function:
> 
> 0000000000001d00 <__intel_wait_for_register_fw.cold>:
>     ...
>     1dce:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi        1dd1: R_X86_64_32S      .text.unlikely+0x1df8
>     1dd5:       e8 00 00 00 00          call   1dda <__intel_wait_for_register_fw.cold+0xda>    1dd6: R_X86_64_PLT32    __trace_bprintk-0x4
>     ...
>     1df6:       0f 0b                   ud2
> 
> Add an exception for this one weird case...
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

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)
-- 
2.34.1

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 3852 bytes --]

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

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

[Steven Rostedt]

On Wed, 06 Apr 2022 02:46:22 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

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

Hmm, I'm not even sure why ftrace_vprintk() is there. It seems redundant.

Arnaldo,

Was there a reason for it. The commit that added it isn't very descriptive.

commit 9011262a37cb438f0fa9394b5e83840db8f9680a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Fri Jan 23 12:06:23 2009 -0200

    ftrace: add ftrace_vprintk
    
    Impact: new helper function
    
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>


-- Steve

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

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

On Wed, 06 Apr 2022 02:46:22 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

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

Hmm, I'm not even sure why ftrace_vprintk() is there. It seems redundant.

Arnaldo,

Was there a reason for it. The commit that added it isn't very descriptive.

commit 9011262a37cb438f0fa9394b5e83840db8f9680a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Fri Jan 23 12:06:23 2009 -0200

    ftrace: add ftrace_vprintk
    
    Impact: new helper function
    
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>


-- Steve

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_;
+}

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]

[-- Attachment #1: Type: text/plain, Size: 2913 bytes --]

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_;
+}

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

[Peter Zijlstra]

On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> 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?  

Best would be for Steve to send a patch removing them all:


arch/powerpc/kvm/book3s_xics.c:#define XICS_DBG(fmt...) trace_printk(fmt)
drivers/gpu/drm/i915/gt/intel_gtt.h:#define DBG(...) trace_printk(__VA_ARGS__)
drivers/gpu/drm/i915/i915_gem.h:#define GEM_TRACE(...) trace_printk(__VA_ARGS__)
drivers/gpu/drm/i915/i915_gem.h:        trace_printk(__VA_ARGS__);                                      \
drivers/hwtracing/stm/dummy_stm.c:      trace_printk("[%u:%u] [pkt: %x/%x] (%llx)\n", master, channel,
drivers/infiniband/hw/hfi1/trace_dbg.h: trace_printk(fmt, ##__VA_ARGS__)
drivers/tty/n_tty.c:# define n_tty_trace(f, args...)    trace_printk(f, ##args)
drivers/usb/early/xhci-dbc.c:#define    xdbc_trace      trace_printk
fs/ext4/inline.c:       trace_printk("inode %lu\n", dir->i_ino);
fs/ext4/inline.c:               trace_printk("de: off %u rlen %u name %.*s nlen %u ino %u\n",


All except i915 use CPP tokens that don't exist, local developer really
has to take effort to enable it. i915 crud depends on a CONFIG_ symbols
that all{yes,mod}config will happily set for you,

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

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]

On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> 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?  

Best would be for Steve to send a patch removing them all:


arch/powerpc/kvm/book3s_xics.c:#define XICS_DBG(fmt...) trace_printk(fmt)
drivers/gpu/drm/i915/gt/intel_gtt.h:#define DBG(...) trace_printk(__VA_ARGS__)
drivers/gpu/drm/i915/i915_gem.h:#define GEM_TRACE(...) trace_printk(__VA_ARGS__)
drivers/gpu/drm/i915/i915_gem.h:        trace_printk(__VA_ARGS__);                                      \
drivers/hwtracing/stm/dummy_stm.c:      trace_printk("[%u:%u] [pkt: %x/%x] (%llx)\n", master, channel,
drivers/infiniband/hw/hfi1/trace_dbg.h: trace_printk(fmt, ##__VA_ARGS__)
drivers/tty/n_tty.c:# define n_tty_trace(f, args...)    trace_printk(f, ##args)
drivers/usb/early/xhci-dbc.c:#define    xdbc_trace      trace_printk
fs/ext4/inline.c:       trace_printk("inode %lu\n", dir->i_ino);
fs/ext4/inline.c:               trace_printk("de: off %u rlen %u name %.*s nlen %u ino %u\n",


All except i915 use CPP tokens that don't exist, local developer really
has to take effort to enable it. i915 crud depends on a CONFIG_ symbols
that all{yes,mod}config will happily set for you,

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

[Peter Zijlstra]

On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> 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...

Well, there's a ton of _THIS_IP_ instances all around, and it would be
unfortunate to have them grow into actual code :/

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

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> 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...

Well, there's a ton of _THIS_IP_ instances all around, and it would be
unfortunate to have them grow into actual code :/

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

[Steven Rostedt]

On Tue, 5 Apr 2022 22:32:51 -0700
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

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

We already do. When anything with a trace_printk() is loaded, the dmesg
will display:

 **********************************************************
 **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
 **                                                      **
 ** trace_printk() being used. Allocating extra memory.  **
 **                                                      **
 ** This means that this is a DEBUG kernel and it is     **
 ** unsafe for production use.                           **
 **                                                      **
 ** If you see this message and you are not debugging    **
 ** the kernel, report this immediately to your vendor!  **
 **                                                      **
 **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
 **********************************************************

:-)

-- Steve

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

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On Tue, 5 Apr 2022 22:32:51 -0700
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

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

We already do. When anything with a trace_printk() is loaded, the dmesg
will display:

 **********************************************************
 **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
 **                                                      **
 ** trace_printk() being used. Allocating extra memory.  **
 **                                                      **
 ** This means that this is a DEBUG kernel and it is     **
 ** unsafe for production use.                           **
 **                                                      **
 ** If you see this message and you are not debugging    **
 ** the kernel, report this immediately to your vendor!  **
 **                                                      **
 **   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **
 **********************************************************

:-)

-- Steve

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

[Steven Rostedt]

On Wed, 6 Apr 2022 09:29:13 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Best would be for Steve to send a patch removing them all:
> 
> 
> arch/powerpc/kvm/book3s_xics.c:#define XICS_DBG(fmt...) trace_printk(fmt)
> drivers/gpu/drm/i915/gt/intel_gtt.h:#define DBG(...) trace_printk(__VA_ARGS__)
> drivers/gpu/drm/i915/i915_gem.h:#define GEM_TRACE(...) trace_printk(__VA_ARGS__)
> drivers/gpu/drm/i915/i915_gem.h:        trace_printk(__VA_ARGS__);                                      \
> drivers/hwtracing/stm/dummy_stm.c:      trace_printk("[%u:%u] [pkt: %x/%x] (%llx)\n", master, channel,
> drivers/infiniband/hw/hfi1/trace_dbg.h: trace_printk(fmt, ##__VA_ARGS__)
> drivers/tty/n_tty.c:# define n_tty_trace(f, args...)    trace_printk(f, ##args)
> drivers/usb/early/xhci-dbc.c:#define    xdbc_trace      trace_printk
> fs/ext4/inline.c:       trace_printk("inode %lu\n", dir->i_ino);
> fs/ext4/inline.c:               trace_printk("de: off %u rlen %u name %.*s nlen %u ino %u\n",
> 
> 
> All except i915 use CPP tokens that don't exist, local developer really
> has to take effort to enable it. i915 crud depends on a CONFIG_ symbols
> that all{yes,mod}config will happily set for you,

As I replied to Josh, when trace_printk() is used, you get nasty dmesg
reports, and those do show up in bug reports to vendors (I laugh every time
I see them.)

I'm not too worried if trace_printk() is called for debugging, and even
with CONFIG_ options.

I even have trace_printk() used for things like my ring_buffer benchmark
(which should never be run on production!)

Thus, if the code that has trace_printk() is truly for debugging, and not
something that would normally get applied in other people's trees, then
that actually falls into its use case.

-- Steve

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

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

On Wed, 6 Apr 2022 09:29:13 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Best would be for Steve to send a patch removing them all:
> 
> 
> arch/powerpc/kvm/book3s_xics.c:#define XICS_DBG(fmt...) trace_printk(fmt)
> drivers/gpu/drm/i915/gt/intel_gtt.h:#define DBG(...) trace_printk(__VA_ARGS__)
> drivers/gpu/drm/i915/i915_gem.h:#define GEM_TRACE(...) trace_printk(__VA_ARGS__)
> drivers/gpu/drm/i915/i915_gem.h:        trace_printk(__VA_ARGS__);                                      \
> drivers/hwtracing/stm/dummy_stm.c:      trace_printk("[%u:%u] [pkt: %x/%x] (%llx)\n", master, channel,
> drivers/infiniband/hw/hfi1/trace_dbg.h: trace_printk(fmt, ##__VA_ARGS__)
> drivers/tty/n_tty.c:# define n_tty_trace(f, args...)    trace_printk(f, ##args)
> drivers/usb/early/xhci-dbc.c:#define    xdbc_trace      trace_printk
> fs/ext4/inline.c:       trace_printk("inode %lu\n", dir->i_ino);
> fs/ext4/inline.c:               trace_printk("de: off %u rlen %u name %.*s nlen %u ino %u\n",
> 
> 
> All except i915 use CPP tokens that don't exist, local developer really
> has to take effort to enable it. i915 crud depends on a CONFIG_ symbols
> that all{yes,mod}config will happily set for you,

As I replied to Josh, when trace_printk() is used, you get nasty dmesg
reports, and those do show up in bug reports to vendors (I laugh every time
I see them.)

I'm not too worried if trace_printk() is called for debugging, and even
with CONFIG_ options.

I even have trace_printk() used for things like my ring_buffer benchmark
(which should never be run on production!)

Thus, if the code that has trace_printk() is truly for debugging, and not
something that would normally get applied in other people's trees, then
that actually falls into its use case.

-- Steve

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 09:43:30AM +0200, Peter Zijlstra wrote:
> On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> > 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...
> 
> Well, there's a ton of _THIS_IP_ instances all around, and it would be
> unfortunate to have them grow into actual code :/

What do you mean by growing into actual code?  It's still just a single
instruction, as was the immediate load before.

Though, you pasted this on irc:

  #define _THIS_IP_  ({ __label__ __here; __here: asm_volatile_goto ("":::: __here); (unsigned long)&&__here; })

which seems decent to me, though less than ideal because it grows an
ENDBR.  But I like its arch-independence, so yeah, LGTM.

-- 
Josh

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]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Wed, Apr 06, 2022 at 09:43:30AM +0200, Peter Zijlstra wrote:
> On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> > 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...
> 
> Well, there's a ton of _THIS_IP_ instances all around, and it would be
> unfortunate to have them grow into actual code :/

What do you mean by growing into actual code?  It's still just a single
instruction, as was the immediate load before.

Though, you pasted this on irc:

  #define _THIS_IP_  ({ __label__ __here; __here: asm_volatile_goto ("":::: __here); (unsigned long)&&__here; })

which seems decent to me, though less than ideal because it grows an
ENDBR.  But I like its arch-independence, so yeah, LGTM.

-- 
Josh

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

[Arnaldo Carvalho de Melo]

Em Tue, Apr 05, 2022 at 09:20:32PM -0400, Steven Rostedt escreveu:
> On Wed, 06 Apr 2022 02:46:22 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > This covers the trace_printk() case which uses do_trace_printk(), but
> > the same problem exists in trace_puts() and ftrace_vprintk()...., no?
> 
> Hmm, I'm not even sure why ftrace_vprintk() is there. It seems redundant.
> 
> Arnaldo,
> 
> Was there a reason for it. The commit that added it isn't very descriptive.

Yeah, I was young and in a hurry.

- Arnaldo
 
> commit 9011262a37cb438f0fa9394b5e83840db8f9680a
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Fri Jan 23 12:06:23 2009 -0200
> 
>     ftrace: add ftrace_vprintk
>     
>     Impact: new helper function
>     
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> -- Steve

-- 

- Arnaldo

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

[Arnaldo Carvalho de Melo]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

Em Tue, Apr 05, 2022 at 09:20:32PM -0400, Steven Rostedt escreveu:
> On Wed, 06 Apr 2022 02:46:22 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > This covers the trace_printk() case which uses do_trace_printk(), but
> > the same problem exists in trace_puts() and ftrace_vprintk()...., no?
> 
> Hmm, I'm not even sure why ftrace_vprintk() is there. It seems redundant.
> 
> Arnaldo,
> 
> Was there a reason for it. The commit that added it isn't very descriptive.

Yeah, I was young and in a hurry.

- Arnaldo
 
> commit 9011262a37cb438f0fa9394b5e83840db8f9680a
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Fri Jan 23 12:06:23 2009 -0200
> 
>     ftrace: add ftrace_vprintk
>     
>     Impact: new helper function
>     
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> -- Steve

-- 

- Arnaldo

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

[Peter Zijlstra]

On Wed, Apr 06, 2022 at 09:37:03AM -0700, Josh Poimboeuf wrote:
> On Wed, Apr 06, 2022 at 09:43:30AM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> > > 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...
> > 
> > Well, there's a ton of _THIS_IP_ instances all around, and it would be
> > unfortunate to have them grow into actual code :/
> 
> What do you mean by growing into actual code?  It's still just a single
> instruction, as was the immediate load before.

Aah, indeed. I was somehow thinking we'd get extra instructions.

> Though, you pasted this on irc:
> 
>   #define _THIS_IP_  ({ __label__ __here; __here: asm_volatile_goto ("":::: __here); (unsigned long)&&__here; })
> 
> which seems decent to me, though less than ideal because it grows an
> ENDBR.  But I like its arch-independence, so yeah, LGTM.

I did send hjl an email about that extra endbr, because I really don't
like that. And jump_label (also using asm-goto) doesn't grow those endbr
instructions, so something is weird.

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

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

On Wed, Apr 06, 2022 at 09:37:03AM -0700, Josh Poimboeuf wrote:
> On Wed, Apr 06, 2022 at 09:43:30AM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 05, 2022 at 10:32:51PM -0700, Josh Poimboeuf wrote:
> > > 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...
> > 
> > Well, there's a ton of _THIS_IP_ instances all around, and it would be
> > unfortunate to have them grow into actual code :/
> 
> What do you mean by growing into actual code?  It's still just a single
> instruction, as was the immediate load before.

Aah, indeed. I was somehow thinking we'd get extra instructions.

> Though, you pasted this on irc:
> 
>   #define _THIS_IP_  ({ __label__ __here; __here: asm_volatile_goto ("":::: __here); (unsigned long)&&__here; })
> 
> which seems decent to me, though less than ideal because it grows an
> ENDBR.  But I like its arch-independence, so yeah, LGTM.

I did send hjl an email about that extra endbr, because I really don't
like that. And jump_label (also using asm-goto) doesn't grow those endbr
instructions, so something is weird.