linux-kernel - Re: [PATCH v2 1/2] sh: Added kernel.h to word-at-a-time

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: <CAKwvOdmMqJacYRfwohY-DXBbmNmz_M4EKUL1KuTv=tT2dO_p1g@mail.gmail.com>
Date: Mon, 18 Dec 2023 08:57:59 -0800
From: Nick Desaulniers <ndesaulniers@...gle.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Tanzir Hasan <tanzirh@...gle.com>, Andy Shevchenko <andy@...nel.org>, 
	Kees Cook <keescook@...omium.org>, linux-hardening@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Nick DeSaulniers <nnn@...gle.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, llvm@...ts.linux.dev, 
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v2 1/2] sh: Added kernel.h to word-at-a-time

On Fri, Dec 15, 2023 at 11:09 AM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Fri, Dec 15, 2023 at 8:31 PM Tanzir Hasan <tanzirh@...gle.com> wrote:
> > On Fri, Dec 15, 2023 at 8:04 AM Andy Shevchenko <andy@...nel.org> wrote:
> >> On Thu, Dec 14, 2023 at 09:06:12PM +0000, tanzirh@...gle.com wrote:
>
> ...
>
> >> > +#include <linux/kernel.h>
> >>
> >> I highly discourage from doing that. Instead, split what is needed to
> >> the separate (new) header and include that one.
> >
> >
> > I think it would make the most sense to do this in a separate patch.
> > What word-at-a-time.h needs from kernel.h is REPEAT_BYTE and to my knowledge,
> > almost every other version of word-at-a-time.h includes kernel.h gets this by
> > including kernel.h. A future change could be removing REPEAT_BYTE
> > out of kernel.h
>
> Just create a patch that either moves that macro (along with upper_*()
> and lower_*() APIs) to a more distinguishable header
> (maybe bytes.h or words.h or wordpart.h, etc) and use it in your case
> and fix others.

Andy,
These are good suggestions and we should do them...

...and Tanzir only has 3 weeks left of his internship.  I don't want
him to get bogged down chasing build regressions from modifying the
headers themselves.  I think what's best for him from here through the
remainder of his internship is to stay focused on applying suggestions
from IWYU to just modify the #include list of .c files, and not start
splitting .h files.  Splitting the .h files will be the next step, and
is made easier by having the codebase not have so many indirect
includes (via IWYU), but we need time to soak header changes, and time
Tanzir does not have.  Please can we keep the suggestions focused on
whether the modifications to the header includes (and the tangential
cleanups) are correct?

While REPEAT_BYTE has a manageable number of users, upper_* and
lower_* have significantly more; I worry about moving those causing
regressions.  We can move them, but such changes would need
significantly more soak time than this series IMO.  Tanzir is also
working on statistical analysis; I suspect if he analyzes
include/linux/kernel.h, he can comment on whether the usage of
REPEAT_BYTE is correlated with the usage of upper_* and lower_* in
order to inform whether they should be grouped together or not.
-- 
Thanks,
~Nick Desaulniers