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] [day] [month] [year] [list]

Message-ID: <CAJncD7SnEr9FSz7VwDx3k9jeamDqAe0e1Ce+9TubjDEO3S0eJg@mail.gmail.com>
Date:   Fri, 26 May 2023 09:05:49 +0800
From:   sunliming <kelulanainsley@...il.com>
To:     Beau Belgrave <beaub@...ux.microsoft.com>
Cc:     mhiramat@...nel.org, rostedt@...dmis.org,
        linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tracing/user_events: Fix the order of the fields in the
 trace output

OK，I got it.

Beau Belgrave <beaub@...ux.microsoft.com> 于2023年5月26日周五 00:53写道：
>
> 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