Re: [PATCH] lib/string: shrink lib/string.i via IWYU

From: Al Viro <viro@zeniv.linux.org.uk>
To: Greg KH <greg@kroah.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
	tanzirh@google.com, Kees Cook <keescook@chromium.org>,
	Andy Shevchenko <andy@kernel.org>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	Nick DeSaulniers <nnn@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	llvm@lists.linux.dev
Subject: Re: [PATCH] lib/string: shrink lib/string.i via IWYU
Date: Fri, 15 Dec 2023 21:03:44 +0000	[thread overview]
Message-ID: <20231215210344.GA3096493@ZenIV> (raw)
In-Reply-To: <20231214210400.GR1674809@ZenIV>

On Thu, Dec 14, 2023 at 09:04:00PM +0000, Al Viro wrote:

> drivers/firmware/arm_scmi/shmem.c:13:#include <asm-generic/bug.h>

Should just use linux/bug.h and be done with that.

> drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c:10:#include <asm-generic/posix_types.h>

Completely pointless; not to mention that none of the types defined
there are used anywhere in that file, the previous line pulls their
private header, which explicitly pulls linux/types.h.

> lib/trace_readwrite.c:10:#include <asm-generic/io.h>

Yeccchhh...  This is just plain wrong.

What happens here is that hooks are added to writeb() et.al., to
allow ftrace to play with those.  By default these are empty
inlines; with CONFIG_TRACE_MMIO_ACCESS they are real function
calls, the functions living in lib/trace_readwrite.c

asm-generic/io.h is pulled by all asm/io.h instances, so that's
where the externs went.  That would've made sense, except that
asm-generic/io.h is used as a backstop for architectures that
had not bothered to define e.g. readl() of their own.  And *that*
is where the calls of those hooks had gone.  IOW, if architecture
has readl()/writeb()/whatnot of its own, it won't get those hooks
at all.

It smells like a conversion in progress, stalled after the first
(and only) architecture where that thing is selectable.  In effect,
it's arm64-specific.

> net/sunrpc/xprtrdma/verbs.c:58:#include <asm-generic/barrier.h>

Bogus, same as the include of asm/bitops.h immediately before that
line (the latter would've blown up if we hadn't already pulled
linux/bitops.h - which needs asm/barrier.h and pulls it, TYVM).

> rust/uapi/uapi_helper.h:9:#include <uapi/asm-generic/ioctl.h>

To the rust crowd, that...  It's wrong for e.g. powerpc -
uapi/asm/ioctl.h in there does pull asm-generic/ioctl.h, but
only after
#define _IOC_SIZEBITS   13
#define _IOC_DIRBITS    3

#define _IOC_NONE       1U
#define _IOC_READ       2U
#define _IOC_WRITE      4U

which means that trying to use asm-generic/ioctl.h directly
will yield the wrong numbers out of _IOC() et.al.

So...
in arch headers (..../asm/.../*.h and similar):
	OK, that's what asm-generic is for
in asm-generic headers themselves (..../asm-generic/.../*.h):
	OK
in linker scripts (*/*.lds{,.S}):
	asm-generic/vmlinux.lds.h is fine in those (and nowhere else)
in */*audit*.c:
	asm-generic/audit_*.h is OK there (ugly, but...)
in drivers,fs,init,io_uring,ipc,kernel,mm,net,sound,virt and probably rust:
	100% bollocks, not a single valid use
in lib/trace_readwrite.c:
	asm-generic/io.h is an exception; complicated story.

That leaves several instances in arch/, tools/ and include/linux, plus
some oddities in makefiles, scripts, etc.  And include/linux ones are
also not obviously correct - e.g. linux/bug.h pulls asm/bug.h, then
(under ifdef CONFIG_BUG) asm-generic/bug.h.  AFAICS on all architectures
we have asm/bug.h pulling asm-generic/bug.h (the ones that don't have
asm/bug.h of their own get it generated with just such an include in
it).  So do we need that include in linux/bug.h these days?