linux-kernel - Re: [PATCH 1/6] hash.h: remove unused define directive

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: <CAAniXFT_C0kZ64uJWYwXsFR3KDyqmXdZH=vrWqmOvaioW7yLvw@mail.gmail.com>
Date:   Sun, 5 Sep 2021 19:31:31 -0300
From:   Isabella B do Amaral <isabellabdoamaral@....br>
To:     David Gow <davidgow@...gle.com>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Enzo Ferreira <ferreiraenzoa@...il.com>,
        Augusto Durães Camargo 
        <augusto.duraes33@...il.com>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        ~lkcamp/patches@...ts.sr.ht, rodrigosiqueiramelo@...il.com
Subject: Re: [PATCH 1/6] hash.h: remove unused define directive

Hi, David,

On Thu, Aug 26, 2021 at 1:01 AM David Gow <davidgow@...gle.com> wrote:
>
> On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <isabellabdoamaral@....br> wrote:
> >
> > The HAVE_ARCH_HASH_32 (single underscore) define hasn't been used for
> > any known supported architectures that have their own hash function
> > implementation (i.e. m68k, Microblaze, H8/300, pa-risc) since George's
> > patch [1], which introduced it.
> >
> > The supported 32-bit architectures from the list above have only been
> > making use of the (more general) HAVE_ARCH__HASH_32 define, which only
> > lacks the right shift operator, that wasn't targeted for optimizations
> > so far.
> >
> > [1] https://lore.kernel.org/lkml/20160525073311.5600.qmail@ns.sciencehorizons.net/
> >
> > Co-developed-by: Augusto Durães Camargo <augusto.duraes33@...il.com>
> > Signed-off-by: Augusto Durães Camargo <augusto.duraes33@...il.com>
> > Co-developed-by: Enzo Ferreira <ferreiraenzoa@...il.com>
> > Signed-off-by: Enzo Ferreira <ferreiraenzoa@...il.com>
> > Signed-off-by: Isabella Basso <isabellabdoamaral@....br>
> > ---
>
> I'm not familiar with the hash functions here, so take this with the
> appropriate heap of salt, but it took me a little while to understand
> exactly what this is doing.
>
> As I understand it:
> - There are separate __hash_32() and hash_32() functions.
> - Both of these have generic implementations, which can optionally be
> overridden by an architecture-specific optimised version.
> - There aren't any architectures which provide an optimised hash_32()
> implementation.
> - This patch therefore removes support for architecture-specific
> hash_32() implementations, and leaves only the generic implementation.
> - This generic implementation of hash_32() itself relies on
> __hash_32(), which may still be optimised.
>
> Could the commit description be updated to make this a bit clearer?

Sure, that makes perfect sense! Thank you very much for the feedback! Writing
those descriptions was quite a challenge for me, as I wasn't perfectly sure of
what the appropriate reasoning should be for the message. I'm also glad I was
able to get a grasp similar to yours. :)

> While we are getting rid of the HAVE_ARCH_HASH_32 #define, that seems
> to be a side-effect/implementation detail of removing support for
> architecture-specific hash_32() implementations...
>
> The other wild, out-there option would be to remove __hash_32()
> entirely and make everything use hash_32(), which then could have
> architecture-specific implementations. A quick grep reveals that
> there's only one use of __hash_32() outside of the hashing code itself
> (in fs/namei.c). This would be much more consistent with what
> hash_64() does, but also would be significantly more work, and
> potentially could have some implication (full_name_hash() performance
> maybe?) which I'm not aware of. So it's possibly not worth it.

I do agree with you that it seems a bit over the top, as I'm also really not
aware of the performance implications of such a change (and that seemed to be
what motivated most of the patch series that introduced the __hash_32() to
fs/namei.c), so I'd rather not mess with fs/namei.c based on consistency
reasons alone.

Thanks,
--
Isabella Basso