[PATCH 0/2] tracing: Fix an event field filter issue

[PATCH 0/2] tracing: Fix an event field filter issue

[Masami Hiramatsu (Google)]

Hi,

Here is a series to fix some issues in the tracing event filter.
The first patch removes the __attribute__ from the event format type
string, which can cause issues with parsing (detecting the string
type). The second patch optimizes memory allocation in
__trace_define_field() by only allocating field->type when it
needs to be sanitized.

Thank you,


---

Masami Hiramatsu (Google) (2):
      tracing: Remove "__attribute__()" from the type field of event format
      tracing: Allocate field->type only if it needs to be sanitized


 kernel/trace/trace.h        |    1 +
 kernel/trace/trace_events.c |   60 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 3 deletions(-)

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

With CONFIG_DEBUG_INFO_BTF=y and PAHOLE_HAS_BTF_TAG=y, `__user` is
converted to `__attribute__((btf_type_tag("user")))`. In this case,
some syscall events have it for __user data, like below;

/sys/kernel/tracing # cat events/syscalls/sys_enter_openat/format
name: sys_enter_openat
ID: 720
format:
        field:unsigned short common_type;       offset:0;       size:2; signed:0;
        field:unsigned char common_flags;       offset:2;       size:1; signed:0;
        field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
        field:int common_pid;   offset:4;       size:4; signed:1;

        field:int __syscall_nr; offset:8;       size:4; signed:1;
        field:int dfd;  offset:16;      size:8; signed:0;
        field:const char __attribute__((btf_type_tag("user"))) * filename;      offset:24;      size:8; signed:0;
        field:int flags;        offset:32;      size:8; signed:0;
        field:umode_t mode;     offset:40;      size:8; signed:0;


Then the trace event filter fails to set the string acceptable flag
(FILTER_PTR_STRING) to the field and rejects setting string filter;

 # echo 'filename.ustring ~ "*ftracetest-dir.wbx24v*"' \
    >> events/syscalls/sys_enter_openat/filter
 sh: write error: Invalid argument
 # cat error_log
 [  723.743637] event filter parse error: error: Expecting numeric field
   Command: filename.ustring ~ "*ftracetest-dir.wbx24v*"

Since this __attribute__ makes format parsing complicated and not
needed, remove the __attribute__(.*) from the type string.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_events.c |   44 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 120531268abf..2f950aceb783 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -112,18 +112,59 @@ trace_find_event_field(struct trace_event_call *call, char *name)
 	return __find_event_field(&ftrace_common_fields, name);
 }
 
-static int __trace_define_field(struct list_head *head, const char *type,
+#define ATTRIBUTE_STR "__attribute__"
+
+/* Remove all __attribute__() from type */
+static void sanitize_field_type(char *type)
+{
+	static const int attr_len = strlen(ATTRIBUTE_STR);
+	char *attr, *tmp, *next;
+	int depth;
+
+	next = type;
+	while ((attr = strstr(next, ATTRIBUTE_STR))) {
+		next = attr + attr_len;
+
+		/* Retry if __attribute__ is a part of type name. */
+		if ((attr != type && !isspace(attr[-1])) ||
+		    attr[attr_len] != '(')
+			continue;
+
+		depth = 0;
+		while ((tmp = strpbrk(next, "()"))) {
+			if (*tmp == '(')
+				depth++;
+			else
+				depth--;
+			next = tmp + 1;
+			if (depth == 0)
+				break;
+		}
+		next = skip_spaces(next);
+		strcpy(attr, next);
+	}
+}
+
+static int __trace_define_field(struct list_head *head, const char *__type,
 				const char *name, int offset, int size,
 				int is_signed, int filter_type, int len,
 				int need_test)
 {
 	struct ftrace_event_field *field;
+	char *type;
 
 	field = kmem_cache_alloc(field_cachep, GFP_TRACE);
 	if (!field)
 		return -ENOMEM;
 
 	field->name = name;
+
+	type = kstrdup(__type, GFP_KERNEL);
+	if (!type) {
+		kfree(field);
+		return -ENOMEM;
+	}
+	sanitize_field_type(type);
 	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
@@ -225,6 +266,7 @@ static void trace_destroy_fields(struct trace_event_call *call)
 	head = trace_get_fields(call);
 	list_for_each_entry_safe(field, next, head, link) {
 		list_del(&field->link);
+		kfree(field->type);
 		kmem_cache_free(field_cachep, field);
 	}
 }

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[kernel test robot]

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

Hi Masami,

kernel test robot noticed the following build errors:

[auto build test ERROR on trace/for-next]
[also build test ERROR on linus/master v6.16-rc5 next-20250708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Masami-Hiramatsu-Google/tracing-Remove-__attribute__-from-the-type-field-of-event-format/20250708-195600
base:   https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link:    https://lore.kernel.org/r/175197568917.977073.2201559708302320631.stgit%40mhiramat.tok.corp.google.com
patch subject: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format
:::::: branch date: 13 hours ago
:::::: commit date: 13 hours ago
config: arc-randconfig-001-20250709 (attached as .config)
compiler: arc-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (attached as reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507090856.swUa0d8T-lkp@intel.com/

All errors (new ones prefixed by >>):

   kernel/trace/trace_events.c: In function 'sanitize_field_type':
>> kernel/trace/trace_events.c:120:30: error: initializer element is not constant
     static const int attr_len = strlen(ATTRIBUTE_STR);
                                 ^~~~~~


vim +120 kernel/trace/trace_events.c

c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  116) 
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  117) /* Remove all __attribute__() from type */
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  118) static void sanitize_field_type(char *type)
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  119) {
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08 @120) 	static const int attr_len = strlen(ATTRIBUTE_STR);
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  121) 	char *attr, *tmp, *next;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  122) 	int depth;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  123) 
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  124) 	next = type;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  125) 	while ((attr = strstr(next, ATTRIBUTE_STR))) {
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  126) 		next = attr + attr_len;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  127) 
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  128) 		/* Retry if __attribute__ is a part of type name. */
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  129) 		if ((attr != type && !isspace(attr[-1])) ||
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  130) 		    attr[attr_len] != '(')
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  131) 			continue;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  132) 
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  133) 		depth = 0;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  134) 		while ((tmp = strpbrk(next, "()"))) {
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  135) 			if (*tmp == '(')
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  136) 				depth++;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  137) 			else
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  138) 				depth--;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  139) 			next = tmp + 1;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  140) 			if (depth == 0)
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  141) 				break;
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  142) 		}
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  143) 		next = skip_spaces(next);
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  144) 		strcpy(attr, next);
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  145) 	}
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  146) }
c4415fcf8cbdb4 Masami Hiramatsu (Google  2025-07-08  147) 

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36116 bytes --]

[-- Attachment #3: reproduce --]
[-- Type: text/plain, Size: 778 bytes --]

reproduce (this is a W=1 build):
        git clone https://github.com/intel/lkp-tests.git ~/lkp-tests
        git remote add trace https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace
        git fetch trace for-next
        git checkout trace/for-next
        b4 shazam https://lore.kernel.org/r/175197568917.977073.2201559708302320631.stgit@mhiramat.tok.corp.google.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-8.5.0 ~/lkp-tests/kbuild/make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-8.5.0 ~/lkp-tests/kbuild/make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/iio/imu/inv_icm42600/ fs/ kernel/trace/

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Wed, 9 Jul 2025 09:06:10 +0800
kernel test robot <lkp@intel.com> wrote:

>    kernel/trace/trace_events.c: In function 'sanitize_field_type':
> >> kernel/trace/trace_events.c:120:30: error: initializer element is not constant  
>      static const int attr_len = strlen(ATTRIBUTE_STR);
>                                  ^~~~~~

I guess this could be fixed by making the above:

	static const int attr_len = sizeof(ATTRIBUTE_STR) - 1;

-- Steve

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu]

On Wed, 9 Jul 2025 12:51:18 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 9 Jul 2025 09:06:10 +0800
> kernel test robot <lkp@intel.com> wrote:
> 
> >    kernel/trace/trace_events.c: In function 'sanitize_field_type':
> > >> kernel/trace/trace_events.c:120:30: error: initializer element is not constant  
> >      static const int attr_len = strlen(ATTRIBUTE_STR);
> >                                  ^~~~~~
> 
> I guess this could be fixed by making the above:
> 
> 	static const int attr_len = sizeof(ATTRIBUTE_STR) - 1;

OK, let me fix it.

Thanks!

> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Tue,  8 Jul 2025 20:54:49 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> With CONFIG_DEBUG_INFO_BTF=y and PAHOLE_HAS_BTF_TAG=y, `__user` is
> converted to `__attribute__((btf_type_tag("user")))`. In this case,
> some syscall events have it for __user data, like below;
> 
> /sys/kernel/tracing # cat events/syscalls/sys_enter_openat/format
> name: sys_enter_openat
> ID: 720
> format:
>         field:unsigned short common_type;       offset:0;       size:2; signed:0;
>         field:unsigned char common_flags;       offset:2;       size:1; signed:0;
>         field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
>         field:int common_pid;   offset:4;       size:4; signed:1;
> 
>         field:int __syscall_nr; offset:8;       size:4; signed:1;
>         field:int dfd;  offset:16;      size:8; signed:0;
>         field:const char __attribute__((btf_type_tag("user"))) * filename;      offset:24;      size:8; signed:0;
>         field:int flags;        offset:32;      size:8; signed:0;
>         field:umode_t mode;     offset:40;      size:8; signed:0;
> 
> .
> Then the trace event filter fails to set the string acceptable flag
> (FILTER_PTR_STRING) to the field and rejects setting string filter;
> 
>  # echo 'filename.ustring ~ "*ftracetest-dir.wbx24v*"' \
>     >> events/syscalls/sys_enter_openat/filter  
>  sh: write error: Invalid argument
>  # cat error_log
>  [  723.743637] event filter parse error: error: Expecting numeric field
>    Command: filename.ustring ~ "*ftracetest-dir.wbx24v*"
> 
> Since this __attribute__ makes format parsing complicated and not
> needed, remove the __attribute__(.*) from the type string.

Actually, you can do this in update_event_fields() that already does
this magic for enums as the field length.

And it doesn't free after allocation because it only does the
allocation for events that will never be freed. For modules, it
registers the allocated string so it will be freed on unload.

-- Steve

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu]

On Wed, 9 Jul 2025 13:11:07 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue,  8 Jul 2025 20:54:49 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > With CONFIG_DEBUG_INFO_BTF=y and PAHOLE_HAS_BTF_TAG=y, `__user` is
> > converted to `__attribute__((btf_type_tag("user")))`. In this case,
> > some syscall events have it for __user data, like below;
> > 
> > /sys/kernel/tracing # cat events/syscalls/sys_enter_openat/format
> > name: sys_enter_openat
> > ID: 720
> > format:
> >         field:unsigned short common_type;       offset:0;       size:2; signed:0;
> >         field:unsigned char common_flags;       offset:2;       size:1; signed:0;
> >         field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
> >         field:int common_pid;   offset:4;       size:4; signed:1;
> > 
> >         field:int __syscall_nr; offset:8;       size:4; signed:1;
> >         field:int dfd;  offset:16;      size:8; signed:0;
> >         field:const char __attribute__((btf_type_tag("user"))) * filename;      offset:24;      size:8; signed:0;
> >         field:int flags;        offset:32;      size:8; signed:0;
> >         field:umode_t mode;     offset:40;      size:8; signed:0;
> > 
> > .
> > Then the trace event filter fails to set the string acceptable flag
> > (FILTER_PTR_STRING) to the field and rejects setting string filter;
> > 
> >  # echo 'filename.ustring ~ "*ftracetest-dir.wbx24v*"' \
> >     >> events/syscalls/sys_enter_openat/filter  
> >  sh: write error: Invalid argument
> >  # cat error_log
> >  [  723.743637] event filter parse error: error: Expecting numeric field
> >    Command: filename.ustring ~ "*ftracetest-dir.wbx24v*"
> > 
> > Since this __attribute__ makes format parsing complicated and not
> > needed, remove the __attribute__(.*) from the type string.
> 
> Actually, you can do this in update_event_fields() that already does
> this magic for enums as the field length.

Got it. It should be done in the same place.

> 
> And it doesn't free after allocation because it only does the
> allocation for events that will never be freed. For modules, it
> registers the allocated string so it will be freed on unload.

What happen if the tracepoint in module has the __attribute__?
(or enum etc?)


Thanks,
> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Thu, 10 Jul 2025 22:45:20 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > And it doesn't free after allocation because it only does the
> > allocation for events that will never be freed. For modules, it
> > registers the allocated string so it will be freed on unload.  
> 
> What happen if the tracepoint in module has the __attribute__?
> (or enum etc?)

It adds the string to a linked list via add_str_to_module(). When the
module is unloaded, it searches the link list "module_strings" for all
the strings registered under that module and frees it.

See trace_module_remove_events().

-- Steve

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu]

On Thu, 10 Jul 2025 09:53:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 10 Jul 2025 22:45:20 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > And it doesn't free after allocation because it only does the
> > > allocation for events that will never be freed. For modules, it
> > > registers the allocated string so it will be freed on unload.  
> > 
> > What happen if the tracepoint in module has the __attribute__?
> > (or enum etc?)
> 
> It adds the string to a linked list via add_str_to_module(). When the
> module is unloaded, it searches the link list "module_strings" for all
> the strings registered under that module and frees it.

OK, thanks for the clarification!

> 
> See trace_module_remove_events().

Let me check it.

Thanks!

> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu]

On Wed, 9 Jul 2025 13:11:07 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue,  8 Jul 2025 20:54:49 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > With CONFIG_DEBUG_INFO_BTF=y and PAHOLE_HAS_BTF_TAG=y, `__user` is
> > converted to `__attribute__((btf_type_tag("user")))`. In this case,
> > some syscall events have it for __user data, like below;
> > 
> > /sys/kernel/tracing # cat events/syscalls/sys_enter_openat/format
> > name: sys_enter_openat
> > ID: 720
> > format:
> >         field:unsigned short common_type;       offset:0;       size:2; signed:0;
> >         field:unsigned char common_flags;       offset:2;       size:1; signed:0;
> >         field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
> >         field:int common_pid;   offset:4;       size:4; signed:1;
> > 
> >         field:int __syscall_nr; offset:8;       size:4; signed:1;
> >         field:int dfd;  offset:16;      size:8; signed:0;
> >         field:const char __attribute__((btf_type_tag("user"))) * filename;      offset:24;      size:8; signed:0;
> >         field:int flags;        offset:32;      size:8; signed:0;
> >         field:umode_t mode;     offset:40;      size:8; signed:0;
> > 
> > .
> > Then the trace event filter fails to set the string acceptable flag
> > (FILTER_PTR_STRING) to the field and rejects setting string filter;
> > 
> >  # echo 'filename.ustring ~ "*ftracetest-dir.wbx24v*"' \
> >     >> events/syscalls/sys_enter_openat/filter  
> >  sh: write error: Invalid argument
> >  # cat error_log
> >  [  723.743637] event filter parse error: error: Expecting numeric field
> >    Command: filename.ustring ~ "*ftracetest-dir.wbx24v*"
> > 
> > Since this __attribute__ makes format parsing complicated and not
> > needed, remove the __attribute__(.*) from the type string.
> 
> Actually, you can do this in update_event_fields() that already does
> this magic for enums as the field length.

I investigated this but it is not possible to use update_event_fields()
because that function is only used if the 
CONFIG_TRACE_EVAL_MAP_FILE=y.

But since update_event_fields() can replace the allocated
field->type, we need a special care for that case too.
E.g. if update_event_fields() finds field->alloc_type set,
it will reuse field->type instead of allocate new string.

Let me fix that. At lease we need to fold these 2 patches
into one.

Thank you,

> 
> And it doesn't free after allocation because it only does the
> allocation for events that will never be freed. For modules, it
> registers the allocated string so it will be freed on unload.
> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Fri, 11 Jul 2025 14:37:03 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> I investigated this but it is not possible to use update_event_fields()
> because that function is only used if the 
> CONFIG_TRACE_EVAL_MAP_FILE=y.

That had better *not* be true, otherwise all the TRACE_DEFINE_ENUM() in
include/trace/events/*.h would be useless and those events would not
parse. Note, I usually run without that config enabled, so it would
most definitely break on me if this was true.

In the code we have:

----------------------8<----------------------
#else /* CONFIG_TRACE_EVAL_MAP_FILE */
static inline void trace_create_eval_file(struct dentry *d_tracer) { }
static inline void trace_insert_eval_map_file(struct module *mod,
			      struct trace_eval_map **start, int len) { }
#endif /* !CONFIG_TRACE_EVAL_MAP_FILE */

static void trace_insert_eval_map(struct module *mod,
				  struct trace_eval_map **start, int len)
{
	struct trace_eval_map **map;

	if (len <= 0)
		return;

	map = start;

	trace_event_eval_update(map, len);

	trace_insert_eval_map_file(mod, start, len);
}
---------------------->8----------------------

Notice the "#endif". The trace_insert_eval_map_file() is a nop, but the
trace_event_eval_update() is not. That has the call to
update_event_printk() and update_event_fields().

So it can most definitely be used when that config is not defined. That
config only creates a file to show you *what* was replaced. It doesn't
stop the replacing.

-- Steve

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu]

On Fri, 11 Jul 2025 12:03:22 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 11 Jul 2025 14:37:03 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > I investigated this but it is not possible to use update_event_fields()
> > because that function is only used if the 
> > CONFIG_TRACE_EVAL_MAP_FILE=y.
> 
> That had better *not* be true, otherwise all the TRACE_DEFINE_ENUM() in
> include/trace/events/*.h would be useless and those events would not
> parse. Note, I usually run without that config enabled, so it would
> most definitely break on me if this was true.
> 
> In the code we have:
> 
> ----------------------8<----------------------
> #else /* CONFIG_TRACE_EVAL_MAP_FILE */
> static inline void trace_create_eval_file(struct dentry *d_tracer) { }
> static inline void trace_insert_eval_map_file(struct module *mod,
> 			      struct trace_eval_map **start, int len) { }
> #endif /* !CONFIG_TRACE_EVAL_MAP_FILE */
> 
> static void trace_insert_eval_map(struct module *mod,
> 				  struct trace_eval_map **start, int len)
> {
> 	struct trace_eval_map **map;
> 
> 	if (len <= 0)
> 		return;
> 
> 	map = start;
> 
> 	trace_event_eval_update(map, len);
> 
> 	trace_insert_eval_map_file(mod, start, len);
> }
> ---------------------->8----------------------
> 
> Notice the "#endif". The trace_insert_eval_map_file() is a nop, but the
> trace_event_eval_update() is not. That has the call to
> update_event_printk() and update_event_fields().
> 
> So it can most definitely be used when that config is not defined. That
> config only creates a file to show you *what* was replaced. It doesn't
> stop the replacing.

Hmm, Ok. But when I sanitized the field->type in
update_event_fields(), it did not work. So something
we missed.

Thanksm

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Sat, 12 Jul 2025 20:45:24 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Hmm, Ok. But when I sanitized the field->type in
> update_event_fields(), it did not work. So something
> we missed.

Ah, it's because we test to see if the event has enums or not before
calling update_event_fields. We need something like this:


diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 120531268abf..52829b950022 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -3349,6 +3349,8 @@ void trace_event_eval_update(struct trace_eval_map **map, int len)
 				}
 				update_event_printk(call, map[i]);
 				update_event_fields(call, map[i]);
+			} else if (need_sanitize_field_type(__type)) {
+				sanitize_fields(call);
 			}
 		}
 		cond_resched();


And have the attribute fixed in both update_event_fields() and have
your own sanitize_fields() that just does the attribute update.

-- Steve

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Sat, 12 Jul 2025 10:37:32 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> +			} else if (need_sanitize_field_type(__type)) {
> +				sanitize_fields(call);
>  			}

Note, I just cut and pasted what you had and added "sanitize_fields()".
Obviously this isn't going to compile, but I just did this to show you
where the changes had to be made.

-- Steve

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Masami Hiramatsu]

On Sat, 12 Jul 2025 10:37:32 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 12 Jul 2025 20:45:24 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > Hmm, Ok. But when I sanitized the field->type in
> > update_event_fields(), it did not work. So something
> > we missed.
> 
> Ah, it's because we test to see if the event has enums or not before
> calling update_event_fields. We need something like this:
> 
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 120531268abf..52829b950022 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -3349,6 +3349,8 @@ void trace_event_eval_update(struct trace_eval_map **map, int len)
>  				}
>  				update_event_printk(call, map[i]);
>  				update_event_fields(call, map[i]);
> +			} else if (need_sanitize_field_type(__type)) {
> +				sanitize_fields(call);
>  			}
>  		}
>  		cond_resched();
> 
> 
> And have the attribute fixed in both update_event_fields() and have
> your own sanitize_fields() that just does the attribute update.

Hmm, is this called unless loading modules? It seems that the
function is only kicked from trace_module_notify() -> trace_module_add_evals() -> trace_insert_eval_map() (but if mod has any trace_evals)

This sanitizing must be done with/without the module loading even if it
had no trace_evals. Thus I think it is hard to be done in
trace_event_eval_update().

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 1/2] tracing: Remove "__attribute__()" from the type field of event format

[Steven Rostedt]

On Mon, 14 Jul 2025 23:14:12 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Hmm, is this called unless loading modules? It seems that the
> function is only kicked from trace_module_notify() -> trace_module_add_evals() -> trace_insert_eval_map() (but if mod has any trace_evals)

As it has to work on non-module events, yes it is called.

late_init_syscall() -> trace_eval_init() -> queue_work(eval_map_work)

  eval_map_work_func() -> trace_insert_eval_map() -> ...

-- Steve

[PATCH 2/2] tracing: Allocate field->type only if it needs to be sanitized

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

__trace_define_field() always allocate field->type for sanitize
the type string, but almost all cases it does not contain the
string to be sanitized. To reduce such memory usage, prevent
to allocate field->type unless it actually has the string which
needs to be sanitized.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace.h        |    1 +
 kernel/trace/trace_events.c |   28 ++++++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index bd084953a98b..cd7be4ce6ee9 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1468,6 +1468,7 @@ struct ftrace_event_field {
 	int			size;
 	unsigned int		is_signed:1;
 	unsigned int		needs_test:1;
+	unsigned int		alloc_type:1;
 	int			len;
 };
 
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 2f950aceb783..d95f24d61875 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -145,6 +145,11 @@ static void sanitize_field_type(char *type)
 	}
 }
 
+static bool need_sanitize_field_type(const char *type)
+{
+	return !!strstr(type, ATTRIBUTE_STR);
+}
+
 static int __trace_define_field(struct list_head *head, const char *__type,
 				const char *name, int offset, int size,
 				int is_signed, int filter_type, int len,
@@ -159,16 +164,22 @@ static int __trace_define_field(struct list_head *head, const char *__type,
 
 	field->name = name;
 
-	type = kstrdup(__type, GFP_KERNEL);
-	if (!type) {
-		kfree(field);
-		return -ENOMEM;
+	if (need_sanitize_field_type(__type)) {
+		type = kstrdup(__type, GFP_KERNEL);
+		if (!type) {
+			kfree(field);
+			return -ENOMEM;
+		}
+		sanitize_field_type(type);
+		field->type = type;
+		field->alloc_type = 1;
+	} else {
+		field->type = __type;
+		field->alloc_type = 0;
 	}
-	sanitize_field_type(type);
-	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
-		field->filter_type = filter_assign_type(type);
+		field->filter_type = filter_assign_type(field->type);
 	else
 		field->filter_type = filter_type;
 
@@ -266,7 +277,8 @@ static void trace_destroy_fields(struct trace_event_call *call)
 	head = trace_get_fields(call);
 	list_for_each_entry_safe(field, next, head, link) {
 		list_del(&field->link);
-		kfree(field->type);
+		if (field->alloc_type)
+			kfree(field->type);
 		kmem_cache_free(field_cachep, field);
 	}
 }