linux-kernel - Re: [PATCH v5 01/19] scripts: move genksyms crc32 implementation to a common include

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: <CABCJKue+n2V5vua2=Hwc1SXBdkmdLBD7ac8imt5HO1bsy7s9dw@mail.gmail.com>
Date: Tue, 19 Nov 2024 13:37:35 -0800
From: Sami Tolvanen <samitolvanen@...gle.com>
To: "Darrick J. Wong" <djwong@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Matthew Maurer <mmaurer@...gle.com>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Petr Pavlu <petr.pavlu@...e.com>, Daniel Gomez <da.gomez@...sung.com>, Neal Gompa <neal@...pa.dev>, 
	Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Miroslav Benes <mbenes@...e.cz>, 
	Asahi Linux <asahi@...ts.linux.dev>, Sedat Dilek <sedat.dilek@...il.com>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 01/19] scripts: move genksyms crc32 implementation to a
 common include

Hi Darrick,

On Tue, Nov 19, 2024 at 12:48 PM Darrick J. Wong <djwong@...nel.org> wrote:
>
> On Mon, Nov 18, 2024 at 09:58:09PM +0000, Sami Tolvanen wrote:
> > Hi,
> >
> > On Sat, Nov 16, 2024 at 9:09 AM Masahiro Yamada <masahiroy@...nel.org> wrote:
> > >
> > > On Thu, Nov 14, 2024 at 2:54 AM Sami Tolvanen <samitolvanen@...gle.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Mon, Nov 11, 2024 at 8:06 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
> > > > >
> > > > > On Thu, Oct 31, 2024 at 2:01 AM Sami Tolvanen <samitolvanen@...gle.com> wrote:
> > > > > >
> > > > > > To avoid duplication between host programs, move the crc32 code to a
> > > > > > shared header file.
> > > > >
> > > > >
> > > > > Only the motivation to use this long table is to keep compatibility
> > > > > between genksyms and gendwarfksyms.
> > > > > I do not think this should be exposed to other programs.
> > > > >
> > > > >
> > > > > If you avoid the code duplication, you can do
> > > > >
> > > > > // scripts/gendwarfksyms/crc.c
> > > > > #include "../genksyms/crc.c"
> > > >
> > > > Sure, that sounds reasonable. I'll change this in the next version.
> > >
> > >
> > > BTW, is it necessary to share the same crc function
> > > between genksyms and gendwarfksyms?
> > >
> > > If CONFIG_GENKSYMS and CONFIG_GENDWARFKSYMS
> > > were able to produce the same CRC, it would be a good motivation
> > > to share the same function.
> > > However, as far as I tested, gendwarfksyms generates different CRC values.
>
> crc32() is operating on different data, right?  CONFIG_GENDWARFKSYMS
> computes a crc of the DWARF data, whereas CONFIG_GENKSYMS computes a crc
> of a magic string from ... the source code, right?  Hence the crcs will
> never match?

Correct, they will never match.

> > > > > > Suggested-by: Petr Pavlu <petr.pavlu@...e.com>
> > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@...gle.com>
> > > > > > Acked-by: Neal Gompa <neal@...pa.dev>
> > > > >
> > > > > Does this Ack add any value?
> > > > >
> > > > > Acked-by is meaningful only when it is given by someone who
> > > > > maintains the relevant area or has established a reputation.
> > > > >
> > > > > $ git grep "Neal Gompa"
> > > > > $ git shortlog -n -s | grep "Neal Gompa"
> > > > >      2 Neal Gompa
> > > > >
> > > > > His Ack feels more like "I like it" rather than a qualified endorsement.
> > > >
> > > > Like Neal explained, an Ack from a potential user of this feature
> > > > seemed relevant, but if you don't think it's meaningful, I can
> > > > certainly drop it.
> > >
> > > Tested-by is more suitable if he wants to leave something.
> >
> > Ack. Neal, I'll drop the acks from v6, but if you end up testing that
> > series, please feel free to add your Tested-by.
>
> Just my 2 cents, but it seems rude to me to *remove* an Ack from an
> existing patchset on the grounds that person doesn't appear often in the
> kernel log.  "We won't hire you for this entry level job because you
> don't have experience" etc.
>
> Also, wouldn't Neal be one of the people shepherding this change into
> distro kernels?  He seems to show up somewhat frequently in the Fedora
> and SUSE ecosystems.
>
> Is the problem here that you all think "Acked-by" isn't appropriate from
> someone who isn't a subsystem maintainer, but the kernel doesn't seem to
> have a tag for "downstream consumer of this change says they're willing
> to put their name on the line for this"?

I certainly appreciate Neal's input, but I don't have a strong opinion
about which tag is appropriate. The documentation seems to suggest
that Acked-by is _often_ used by maintainers and focuses on that use
case, but doesn't explicitly rule out other folks acking patches
either:

https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Perhaps Greg, or someone else with more experience with the nuances of
acking, can clarify the policy in this situation?

Sami