linux-kernel - Re: [PATCH] tracing/user_events: Fix the order of the fields in the trace output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230525165329.GA82@W11-BEAU-MD.localdomain>
Date:   Thu, 25 May 2023 09:53:29 -0700
From:   Beau Belgrave <beaub@...ux.microsoft.com>
To:     sunliming <sunliming@...inos.cn>
Cc:     mhiramat@...nel.org, rostedt@...dmis.org,
        linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org,
        kelulanainsley@...il.com
Subject: Re: [PATCH] tracing/user_events: Fix the order of the fields in the
 trace output

On Thu, May 25, 2023 at 01:40:32PM +0800, sunliming wrote:
> Commit 4bec284cc0b9 ("tracing/user_events: Use print_format_fields() for
> trace output") use print_event_fields() as safe and gives user readable
> output. However, due to the insertion of the struct ftrace_event_field
> structure into the field linked list from the header, the trace output
> oder of fields of user events is reversed. Fix the problem by insertint
> to the tail of field linked list.
> 
> Signed-off-by: sunliming <sunliming@...inos.cn>
> ---
>  kernel/trace/trace_events_user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index aacd22c1e9f8..e9e2ec3c7613 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -972,7 +972,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
>  	if (filter_type == FILTER_OTHER)
>  		field->filter_type = filter_assign_type(type);
>  
> -	list_add(&field->link, &user->fields);
> +	list_add_tail(&field->link, &user->fields);
>  
>  	/*
>  	 * Min size from user writes that are required, this does not include
> -- 
> 2.25.1

Thanks for the patch, however, this breaks the tracefs format file. The
fields are required to be put in backwards since it walks them
backwards.

Example using this:
echo 'u:test u32 a; u32 b;' > dynamic_events
cat /sys/kernel/tracing/events/user_events/test/format

Before this change:
name: test
...
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:u32 a;    offset:8;       size:4; signed:0;
        field:u32 b;    offset:12;      size:4; signed:0;

print fmt: "a=%u b=%u", REC->a, REC->b

After this change:
name: test
...
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:u32 b;    offset:12;      size:4; signed:0;
        field:u32 a;    offset:8;       size:4; signed:0;

print fmt: "b=%u a=%u", REC->b, REC->a

I do agree though, that print_fields() is doing it backwards. Can you
please fix the print_fields() function instead? (It should walk the list
of fields backwards like tracefs format file does).

Steven can then Ack that work, since it's isolated there.

Thanks,
-Beau