linux-kernel - Re: [PATCH] gendwarfksyms: order -T symtypes output by name

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: <CAK7LNAQo0CyMdsVSg4dfWjVU+uYUSMdmwgLEdpRfTVcgOTuwzg@mail.gmail.com>
Date: Tue, 1 Jul 2025 00:40:57 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Giuliano Procida <gprocida@...gle.com>
Cc: Sami Tolvanen <samitolvanen@...gle.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-modules@...r.kernel.org, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gendwarfksyms: order -T symtypes output by name

On Mon, Jun 30, 2025 at 10:46 PM Giuliano Procida <gprocida@...gle.com> wrote:
>
> On Mon, 30 Jun 2025 at 14:24, Masahiro Yamada <masahiroy@...nel.org> wrote:
> >
> > On Mon, Jun 30, 2025 at 7:05 PM Giuliano Procida <gprocida@...gle.com> wrote:
> > >
> > > Hi.
> > >
> > > On Sun, 29 Jun 2025 at 18:51, Masahiro Yamada <masahiroy@...nel.org> wrote:
> > > >
> > > > On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@...gle.com> wrote:
> > > > >
> > > > > When writing symtypes information, we iterate through the entire hash
> > > > > table containing type expansions. The key order varies unpredictably
> > > > > as new entries are added, making it harder to compare symtypes between
> > > > > builds.
> > > > >
> > > > > Resolve this by sorting the type expansions by name before output.
> > > > >
> > > > > Signed-off-by: Giuliano Procida <gprocida@...gle.com>
> > > > > Acked-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > > > Reviewed-by: Sami Tolvanen <samitolvanen@...gle.com>
> > > > > ---
> > > > >  scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> > > > >  1 file changed, 26 insertions(+), 3 deletions(-)
> > > > >
> > > > > [Adjusted the first line of the description. Added reviewer tags.
> > > > >  Added missing CC to linux-modules.]
> > > > >
> > > > > diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> > > > > index 7bd459ea6c59..51c1471e8684 100644
> > > > > --- a/scripts/gendwarfksyms/types.c
> > > > > +++ b/scripts/gendwarfksyms/types.c
> > > > > @@ -6,6 +6,8 @@
> > > > >  #define _GNU_SOURCE
> > > > >  #include <inttypes.h>
> > > > >  #include <stdio.h>
> > > > > +#include <stdlib.h>
> > > > > +#include <string.h>
> > > > >  #include <zlib.h>
> > > > >
> > > > >  #include "gendwarfksyms.h"
> > > > > @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> > > > >         return -1;
> > > > >  }
> > > > >
> > > > > +static int cmp_expansion_name(const void *p1, const void *p2)
> > > > > +{
> > > > > +       struct type_expansion *const *e1 = p1;
> > > > > +       struct type_expansion *const *e2 = p2;
> > > > > +
> > > > > +       return strcmp((*e1)->name, (*e2)->name);
> > > > > +}
> > > > > +
> > > > >  static void type_map_write(FILE *file)
> > > > >  {
> > > > >         struct type_expansion *e;
> > > > >         struct hlist_node *tmp;
> > > > > +       struct type_expansion **es;
> > > > > +       size_t count = 0;
> > > > > +       size_t i = 0;
> > > > >
> > > > >         if (!file)
> > > > >                 return;
> > > > >
> > > > > -       hash_for_each_safe(type_map, e, tmp, hash) {
> > > > > -               checkp(fputs(e->name, file));
> > > > > +       hash_for_each_safe(type_map, e, tmp, hash)
> > > > > +               ++count;
> > > > > +       es = xmalloc(count * sizeof(struct type_expansion *));
> > > >
> > > > Just a nit:
> > > >
> > > >            es = xmalloc(count * sizeof(*es));
> > > >
> > > > is better?
> > > >
> > > > > +       hash_for_each_safe(type_map, e, tmp, hash)
> > > > > +               es[i++] = e;
> > > > > +
> > > > > +       qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
> > > >
> > > > qsort(es, count, sizeof(*es), cmp_expansion_name);
> > > >
> > >
> > > That's a fair point.
> > >
> > > However, in the gendwarfksyms code, all but one of the sizeofs uses an
> > > explicit type name. The exception is sizeof(stats) where stats is an array.
> > >
> > > I'll leave Sami's code as it is.
> >
> >
> > This rule is clearly documented with rationale.
> >
> > See this:
> > https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
> >
> >
>
> I can follow up with a change that adjusts all occurrences. That
> shouldn't take long at all.

I expected a new patch version (I do not know whether it is v2 or v3 since
you do not add such a prefix),
instead of breaking the style, and fixing it in a follow-up patch.







--
Best Regards
Masahiro Yamada