[PATCH v2 0/3] kprobes: notrace enhancements

[PATCH v2 0/3] kprobes: notrace enhancements

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

There are inconsistencies and some issues in marking functions as
notrace. On one hand, all inline functions are marked as "notrace" and
some libraries cannot be traced. As more users and tools try to make use
of the tracing functionality, it is beneficial to allow their tracing as
possible.

At the same time, some functions should not be traced but are not marked
as notrace.

These patch address issues that I encountered during work on an
automatic tracing tool.

---

v1->v2:
* Add find_bit to tracable libraries
* Improve the change log to explain the reasons for inline->notrace
* Switch the order of patch 2 and patch 3


Nadav Amit (3):
  kprobes: Mark descendents of core_kernel_text as notrace
  compiler: inline does not imply notrace
  lib: Allow traceing of usercopy, xarray, iov_iter, find_bit

 arch/arm/kernel/process.c             | 2 +-
 arch/ia64/mm/init.c                   | 2 +-
 arch/x86/entry/vsyscall/vsyscall_64.c | 2 +-
 arch/x86/um/mem_32.c                  | 2 +-
 include/asm-generic/sections.h        | 6 +++---
 include/linux/compiler_types.h        | 2 +-
 include/linux/kallsyms.h              | 6 +++---
 include/linux/mm.h                    | 2 +-
 lib/Makefile                          | 5 +++++
 9 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] kprobes: Mark descendents of core_kernel_text as notrace

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Commit c0d80ddab899 ("kernel/extable.c: mark core_kernel_text notrace")
disabled the tracing of core_kernel_text to avoid recursive calls. For
the same reasons, all the functions in the dynamic extents of
core_kernel_text should be marked as notrace.

Cc: Marcin Nowakowski <marcin.nowakowski@mips.com>
Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/arm/kernel/process.c             | 2 +-
 arch/ia64/mm/init.c                   | 2 +-
 arch/x86/entry/vsyscall/vsyscall_64.c | 2 +-
 arch/x86/um/mem_32.c                  | 2 +-
 include/asm-generic/sections.h        | 6 +++---
 include/linux/kallsyms.h              | 6 +++---
 include/linux/mm.h                    | 2 +-
 7 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 0e8ff85890ad..a8c0d0a06664 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -330,7 +330,7 @@ int in_gate_area(struct mm_struct *mm, unsigned long addr)
 	return (addr >= gate_vma.vm_start) && (addr < gate_vma.vm_end);
 }
 
-int in_gate_area_no_mm(unsigned long addr)
+notrace int in_gate_area_no_mm(unsigned long addr)
 {
 	return in_gate_area(NULL, addr);
 }
diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
index 7f5353e28516..6dbd3acbe837 100644
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -284,7 +284,7 @@ struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
 	return &gate_vma;
 }
 
-int in_gate_area_no_mm(unsigned long addr)
+notrace int in_gate_area_no_mm(unsigned long addr)
 {
 	if ((addr >= FIXADDR_USER_START) && (addr < FIXADDR_USER_END))
 		return 1;
diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
index e0ca8120aea8..2d1d09877f0c 100644
--- a/arch/x86/entry/vsyscall/vsyscall_64.c
+++ b/arch/x86/entry/vsyscall/vsyscall_64.c
@@ -340,7 +340,7 @@ int in_gate_area(struct mm_struct *mm, unsigned long addr)
  * context. It is less reliable than using a task's mm and may give
  * false positives.
  */
-int in_gate_area_no_mm(unsigned long addr)
+notrace int in_gate_area_no_mm(unsigned long addr)
 {
 	return vsyscall_mode != NONE && (addr & PAGE_MASK) == VSYSCALL_ADDR;
 }
diff --git a/arch/x86/um/mem_32.c b/arch/x86/um/mem_32.c
index 29b2203bc82c..1f92840af2f3 100644
--- a/arch/x86/um/mem_32.c
+++ b/arch/x86/um/mem_32.c
@@ -28,7 +28,7 @@ struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
 	return FIXADDR_USER_START ? &gate_vma : NULL;
 }
 
-int in_gate_area_no_mm(unsigned long addr)
+notrace int in_gate_area_no_mm(unsigned long addr)
 {
 	if (!FIXADDR_USER_START)
 		return 0;
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index db13bb620f52..d519965b67bf 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -188,7 +188,7 @@ static inline bool is_kernel_rodata(unsigned long addr)
  *
  * Returns: true if the address is located in .init.text, false otherwise.
  */
-static inline bool is_kernel_inittext(unsigned long addr)
+static notrace inline bool is_kernel_inittext(unsigned long addr)
 {
 	return addr >= (unsigned long)_sinittext &&
 	       addr < (unsigned long)_einittext;
@@ -203,7 +203,7 @@ static inline bool is_kernel_inittext(unsigned long addr)
  * Returns: true if the address is located in .text, false otherwise.
  * Note: an internal helper, only check the range of _stext to _etext.
  */
-static inline bool __is_kernel_text(unsigned long addr)
+static notrace inline bool __is_kernel_text(unsigned long addr)
 {
 	return addr >= (unsigned long)_stext &&
 	       addr < (unsigned long)_etext;
@@ -219,7 +219,7 @@ static inline bool __is_kernel_text(unsigned long addr)
  *       and range from __init_begin to __init_end, which can be outside
  *       of the _stext to _end range.
  */
-static inline bool __is_kernel(unsigned long addr)
+static notrace inline bool __is_kernel(unsigned long addr)
 {
 	return ((addr >= (unsigned long)_stext &&
 	         addr < (unsigned long)_end) ||
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index fe3c9993b5bf..e11743e68124 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -24,21 +24,21 @@
 struct cred;
 struct module;
 
-static inline int is_kernel_text(unsigned long addr)
+static notrace inline int is_kernel_text(unsigned long addr)
 {
 	if (__is_kernel_text(addr))
 		return 1;
 	return in_gate_area_no_mm(addr);
 }
 
-static inline int is_kernel(unsigned long addr)
+static notrace inline int is_kernel(unsigned long addr)
 {
 	if (__is_kernel(addr))
 		return 1;
 	return in_gate_area_no_mm(addr);
 }
 
-static inline int is_ksym_addr(unsigned long addr)
+static notrace inline int is_ksym_addr(unsigned long addr)
 {
 	if (IS_ENABLED(CONFIG_KALLSYMS_ALL))
 		return is_kernel(addr);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 27ce77080c79..e71ea764659c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3485,7 +3485,7 @@ static inline struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
 {
 	return NULL;
 }
-static inline int in_gate_area_no_mm(unsigned long addr) { return 0; }
+static notrace inline int in_gate_area_no_mm(unsigned long addr) { return 0; }
 static inline int in_gate_area(struct mm_struct *mm, unsigned long addr)
 {
 	return 0;
-- 
2.25.1

[PATCH v2 2/3] compiler: inline does not imply notrace

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Functions that are marked as "inline" are currently also not tracable.
This limits tracing functionality for many functions for no reason.
Apparently, this has been done for two reasons.

First, as described in commit 5963e317b1e9d2a ("ftrace/x86: Do not
change stacks in DEBUG when calling lockdep"), it was intended to
prevent some functions that cannot be traced from being traced as these
functions were marked as inline (among others).

Yet, this change has been done a decade ago, and according to Steven
Rostedt, ftrace should have improved and hopefully resolved nested
tracing issues by now. Arguably, if functions that should be traced -
for instance since they are used during tracing - still exist, they
should be marked as notrace explicitly.

The second reason, which Steven raised, is that attaching "notrace" to
"inline" prevented tracing differences between different configs, which
caused various problem. This consideration is not very strong, and tying
"inline" and "notrace" does not seem very beneficial. The "inline"
keyword is just a hint, and many functions are currently not tracable
due to this reason.

Disconnect "inline" from "notrace".

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 include/linux/compiler_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 547ea1ff806e..bab3e25bbe3f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -184,7 +184,7 @@ struct ftrace_likely_data {
  * of extern inline functions at link time.
  * A lot of inline functions can cause havoc with function tracing.
  */
-#define inline inline __gnu_inline __inline_maybe_unused notrace
+#define inline inline __gnu_inline __inline_maybe_unused
 
 /*
  * gcc provides both __inline__ and __inline as alternate spellings of
-- 
2.25.1

Re: [PATCH v2 2/3] compiler: inline does not imply notrace

[Steven Rostedt]

On Thu, 25 May 2023 14:00:39 -0700
Nadav Amit <nadav.amit@gmail.com> wrote:

> From: Nadav Amit <namit@vmware.com>
> 
> Functions that are marked as "inline" are currently also not tracable.
> This limits tracing functionality for many functions for no reason.
> Apparently, this has been done for two reasons.
> 
> First, as described in commit 5963e317b1e9d2a ("ftrace/x86: Do not
> change stacks in DEBUG when calling lockdep"), it was intended to
> prevent some functions that cannot be traced from being traced as these
> functions were marked as inline (among others).
> 
> Yet, this change has been done a decade ago, and according to Steven
> Rostedt, ftrace should have improved and hopefully resolved nested
> tracing issues by now. Arguably, if functions that should be traced -
> for instance since they are used during tracing - still exist, they
> should be marked as notrace explicitly.
> 
> The second reason, which Steven raised, is that attaching "notrace" to
> "inline" prevented tracing differences between different configs, which
> caused various problem. This consideration is not very strong, and tying
> "inline" and "notrace" does not seem very beneficial. The "inline"
> keyword is just a hint, and many functions are currently not tracable
> due to this reason.
> 
> Disconnect "inline" from "notrace".

FYI, I have a patch queued (still needs to go through testing) that
already does this ;-)

https://lore.kernel.org/all/20230502164102.1a51cdb4@gandalf.local.home/

-- Steve

Re: [PATCH v2 2/3] compiler: inline does not imply notrace

[Nadav Amit]

> On May 25, 2023, at 7:28 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Thu, 25 May 2023 14:00:39 -0700
> Nadav Amit <nadav.amit@gmail.com> wrote:
> 
>> From: Nadav Amit <namit@vmware.com>
>> 
>> Functions that are marked as "inline" are currently also not tracable.
>> This limits tracing functionality for many functions for no reason.
>> Apparently, this has been done for two reasons.
>> 
>> First, as described in commit 5963e317b1e9d2a ("ftrace/x86: Do not
>> change stacks in DEBUG when calling lockdep"), it was intended to
>> prevent some functions that cannot be traced from being traced as these
>> functions were marked as inline (among others).
>> 
>> Yet, this change has been done a decade ago, and according to Steven
>> Rostedt, ftrace should have improved and hopefully resolved nested
>> tracing issues by now. Arguably, if functions that should be traced -
>> for instance since they are used during tracing - still exist, they
>> should be marked as notrace explicitly.
>> 
>> The second reason, which Steven raised, is that attaching "notrace" to
>> "inline" prevented tracing differences between different configs, which
>> caused various problem. This consideration is not very strong, and tying
>> "inline" and "notrace" does not seem very beneficial. The "inline"
>> keyword is just a hint, and many functions are currently not tracable
>> due to this reason.
>> 
>> Disconnect "inline" from "notrace".
> 
> FYI, I have a patch queued (still needs to go through testing) that
> already does this ;-)
> 
> https://lore.kernel.org/all/20230502164102.1a51cdb4@gandalf.local.home/

Ugh. If you cc’d me, I wouldn’t bother you during your vacation. :)

I think you may like the first patch in my series to precede this patch
though as some of the function I marked as “notrace" are currently “inline”.

Let me know how you want to proceed, so I would know how to break this
series.

Re: [PATCH v2 2/3] compiler: inline does not imply notrace

[Steven Rostedt]

On Thu, 25 May 2023 22:17:33 -0700
Nadav Amit <nadav.amit@gmail.com> wrote:


> > FYI, I have a patch queued (still needs to go through testing) that
> > already does this ;-)
> > 
> > https://lore.kernel.org/all/20230502164102.1a51cdb4@gandalf.local.home/  
> 
> Ugh. If you cc’d me, I wouldn’t bother you during your vacation. :)

I'm currently passed the vacation part and now in Taiwan for work.

> 
> I think you may like the first patch in my series to precede this patch
> though as some of the function I marked as “notrace" are currently “inline”.
> 
> Let me know how you want to proceed, so I would know how to break this
> series.

Currently there's a nasty bug in v6.4-rc3 I'm fighting where I can't
proceed on anything until it's resolved. But I could also just pull
your first and third patch too. I'll let you know when I'm finished
debugging.

-- Steve

Re: [PATCH v2 2/3] compiler: inline does not imply notrace

[Steven Rostedt]

On Thu, 25 May 2023 22:17:33 -0700
Nadav Amit <nadav.amit@gmail.com> wrote:

> Ugh. If you cc’d me, I wouldn’t bother you during your vacation. :)

Oh, and if you are interested in tracing patches, just subscribe to
linux-trace-kernel@vger.kernel.org.

-- Steve

[PATCH v2 3/3] lib: Allow traceing of usercopy, xarray, iov_iter, find_bit

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

There is no reason not to allow the use of ftrace for usercopy, xarray
and iov_iter.  Enable tracing for these compilation unit.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 lib/Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/Makefile b/lib/Makefile
index 876fcdeae34e..00450e1cc97d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -27,6 +27,9 @@ KASAN_SANITIZE_string.o := n
 CFLAGS_string.o += -fno-stack-protector
 endif
 
+CFLAGS_xarray.o += $(CC_FLAGS_FTRACE)
+CFLAGS_iov_iter.o += $(CC_FLAGS_FTRACE)
+
 lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 rbtree.o radix-tree.o timerqueue.o xarray.o \
 	 maple_tree.o idr.o extable.o irq_regs.o argv_split.o \
@@ -42,6 +45,8 @@ lib-$(CONFIG_SMP) += cpumask.o
 lib-y	+= kobject.o klist.o
 obj-y	+= lockref.o
 
+CFLAGS_usercopy.o += $(CC_FLAGS_FTRACE)
+CFLAGS_find_bit.o += $(CC_FLAGS_FTRACE)
 obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
 	 bust_spinlocks.o kasprintf.o bitmap.o scatterlist.o \
 	 list_sort.o uuid.o iov_iter.o clz_ctz.o \
-- 
2.25.1