Re: [RESEND][PATCH] tracing/synthetic: Fix order of struct trace_dynamic_info

[Sven Schnelle]

Hi Steven,

Steven Rostedt <rostedt@xxxxxxxxxxx> writes:

> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
> To make handling BIG and LITTLE endian better the offset/len of dynamic
> fields of the synthetic events was changed into a structure of:
>
>  struct trace_dynamic_info {
>  #ifdef CONFIG_CPU_BIG_ENDIAN
> 	u16	offset;
> 	u16	len;
>  #else
> 	u16	len;
> 	u16	offset;
>  #endif
>  };
>
> to replace the manual changes of:
>
>  data_offset = offset & 0xffff;
>  data_offest = len << 16;
>
> But if you look closely, the above is:
>
>   <len> << 16 | offset
>
> Which in little endian would be in memory:
>
>  offset_lo offset_hi len_lo len_hi
>
> and in big endian:
>
>  len_hi len_lo offset_hi offset_lo
>
> Which if broken into a structure would be:
>
>  struct trace_dynamic_info {
>  #ifdef CONFIG_CPU_BIG_ENDIAN
> 	u16	len;
> 	u16	offset;
>  #else
> 	u16	offset;
> 	u16	len;
>  #endif
>  };
>
> Which is the opposite of what was defined.
>
> Fix this and just to be safe also add "__packed".
>
> Link: https://lore.kernel.org/all/20230908154417.5172e343@xxxxxxxxxxxxxxxxxx/
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: ddeea494a16f3 ("tracing/synthetic: Use union instead of casts")
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
>
>  [ Resending to the correct mailing list this time :-p ]
>
>  include/linux/trace_events.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 12f875e9e69a..21ae37e49319 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -62,13 +62,13 @@ void trace_event_printf(struct trace_iterator *iter, const char *fmt, ...);
>  /* Used to find the offset and length of dynamic fields in trace events */
>  struct trace_dynamic_info {
>  #ifdef CONFIG_CPU_BIG_ENDIAN
> -	u16	offset;
>  	u16	len;
> +	u16	offset;
>  #else
> -	u16	len;
>  	u16	offset;
> +	u16	len;
>  #endif
> -};
> +} __packed;
>  
>  /*
>   * The trace entry - the most basic unit of tracing. This is what

This issue was also present on BE, but as you noted "covered" by the
broken test case. With this patch everything works as expected. So:

Tested-by: Sven Schnelle <svens@xxxxxxxxxxxxx>