Re: [PATCHv2] trace: allocate fields with elt struct

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

Hi,

Sashiko made 2 comments.

https://sashiko.dev/#/patchset/20260522212627.12406-1-rosenp%40gmail.com

On Fri, 22 May 2026 14:26:27 -0700
Rosen Penev <rosenp@gmail.com> wrote:

> > diff --git a/kernel/trace/tracing_map.c b/kernel/trace/tracing_map.c
> > index 0dd7927df22ac..196a7b0be99df 100644
> > --- a/kernel/trace/tracing_map.c
> > +++ b/kernel/trace/tracing_map.c
> [ ... ]
> > @@ -125,32 +125,32 @@ u64 tracing_map_read_var_once(struct tracing_map_elt *elt, unsigned int i)
> [ ... ]
> >  #define DEFINE_TRACING_MAP_CMP_FN(type)					\
> > -static int tracing_map_cmp_##type(void *val_a, void *val_b)		\
> > +static int tracing_map_cmp_##type(const void *val_a, const void *val_b)	\
> >  {									\
> > -	type a = (type)(*(u64 *)val_a);					\
> > -	type b = (type)(*(u64 *)val_b);					\
> > +	type a = (type)(*(const u64 *)val_a);				\
> > +	type b = (type)(*(const u64 *)val_b);				\
> >  									\
> >  	return (a > b) ? 1 : ((a < b) ? -1 : 0);			\
> >  }
> This is a pre-existing issue, but does unconditionally reading 8 bytes
> via the u64 cast cause unaligned access exceptions on architectures that
> do not support them?
> Additionally, for fields near the end of the dynamically allocated elt->key
> buffer, can this trigger KASAN slab-out-of-bounds reads?
> Also, on big-endian architectures, reading a smaller integer as a 64-bit
> value and casting it down extracts the least-significant bytes rather than
> the correct field value. Could this result in completely incorrect sorting
> for small types?

Steve, it seems this comes from your commit 106f41f5a302 ("tracing: Have
the histogram compare functions convert to u64 first").

I think neither of them is a problem, but could you check it?


> > diff --git a/kernel/trace/tracing_map.h b/kernel/trace/tracing_map.h
> > index 99c37eeebc161..ed64136782d80 100644
> > --- a/kernel/trace/tracing_map.h
> > +++ b/kernel/trace/tracing_map.h
> [ ... ]
> > @@ -137,11 +137,11 @@ struct tracing_map_field {
> >  
> >  struct tracing_map_elt {
> >  	struct tracing_map		*map;
> > -	struct tracing_map_field	*fields;
> >  	atomic64_t			*vars;
> >  	bool				*var_set;
> >  	void				*key;
> >  	void				*private_data;
> > +	struct tracing_map_field	fields[];
> >  };
>
> Is it safe to add a flexible array member to struct tracing_map_elt when it
> is embedded by value in the middle of struct track_data?
> kernel/trace/trace_events_hist.c:struct track_data {
>     ...
>     struct tracing_map_elt      elt;
>     ...
>     struct action_data          *action_data;
>     struct hist_trigger_data    *hist_data;
> };
> Embedding a struct with a flexible array member in the middle of another
> struct violates C standard constraints. Does this trigger compiler
> warnings (such as -Wflex-array-member-not-at-end on modern compilers) or
> break bounds computations for FORTIFY_SOURCE?

Yeah, from this reason, this is is not acceptable.
To fix this issue, we need to refactor the trace_events_hist.c,
because track_data only uses tracing_map_elt as a placeholder
of private_data.

Thank you,

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