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

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: <20231214210400.GR1674809@ZenIV>
Date: Thu, 14 Dec 2023 21:04:00 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Greg KH <greg@...ah.com>
Cc: Nick Desaulniers <ndesaulniers@...gle.com>, tanzirh@...gle.com,
	Kees Cook <keescook@...omium.org>,
	Andy Shevchenko <andy@...nel.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
Subject: Re: [PATCH] lib/string: shrink lib/string.i via IWYU

On Wed, Dec 06, 2023 at 12:09:17PM +0900, Greg KH wrote:
> > slap #include "unaligned.h" into their traps.c and unaligned.c
> > (callers and definitions resp.) and strip those from asm/unaligned.h?
> > At that point we can remove arch/{arc,parisc}/asm/unaligned.h - everything
> > will pick include/asm-generic/unaligned.h.
> > 
> > Then the next cycle we ask Linus to run the following:
> > for i in `git grep -l -w asm/unaligned.h`; do
> > 	sed -i -e "s/asm\/unaligned.h/linux\/unaligned.h/" $i
> > done
> > git mv include/asm-generic/unaligned.h include/linux/unaligned.h
> > sed -i -e "/unaligned.h/d" include/asm-generic/Kbuild
> > right before releasing -rc1 and asm/unaligned.h is gone...
> 
> Please do, it's really annoying and would be great to fix up.

	FWIW, turns out that we have several places in drivers/* that pull
asm-generic/unaligned.h.  IMO that's completely wrong - not just in this
case (it's a matter of trivially adjusting the script), but I think that
as a matter of policy we should *NOT* have includes of asm-generic/*.h
anywhere in drivers/ fs/ io_uring/ kernel/ mm/ net/ sound/

	Current situation (outside of arch, include, scripts and tools,
with asm-generic/unaligned.h cases already taken out):

drivers/android/binderfs.c:32:#include <uapi/asm-generic/errno-base.h>
drivers/clk/microchip/clk-mpfs-ccc.c:7:#include "asm-generic/errno-base.h"
drivers/firmware/arm_scmi/shmem.c:13:#include <asm-generic/bug.h>
drivers/irqchip/irq-ti-sci-inta.c:24:#include <asm-generic/msi.h>
drivers/mtd/nand/raw/ingenic/ingenic_ecc.h:9:#include <uapi/asm-generic/errno-base.h>
drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c:10:#include <asm-generic/posix_types.h>
io_uring/uring_cmd.c:10:#include <uapi/asm-generic/ioctls.h>
lib/trace_readwrite.c:10:#include <asm-generic/io.h>
mm/damon/vaddr.c:10:#include <asm-generic/mman-common.h>
net/sunrpc/xprtrdma/verbs.c:58:#include <asm-generic/barrier.h>
rust/uapi/uapi_helper.h:9:#include <uapi/asm-generic/ioctl.h>

That's it.  asm-generic/errno-base.h cases - just use linux/errno.h and
be done with it.  Anyone who goes "I've got ENOENT and ENOMEM in my driver,
but those two are covered by errno-base.h and I can be clever and use
just that" is wrong.

asm-generic/mman-common.h - and linux/mman.h would not serve why, exactly?
uapi/linux/linux/mman.h, if one really feels like it...

asm-generic/ioctls.h in io_uring - definitely wrong; SIOCINQ is what
it's brought for, but that's in sockios.h.  It expands to FIONREAD, which
*is* in ioctls.h, but...

arch/alpha/include/uapi/asm/ioctls.h:11:#define FIONREAD        _IOR('f', 127, int)
arch/mips/include/uapi/asm/ioctls.h:64:#define FIONREAD 0x467f
arch/parisc/include/uapi/asm/ioctls.h:35:#define FIONREAD       0x541B
arch/powerpc/include/uapi/asm/ioctls.h:11:#define FIONREAD      _IOR('f', 127, int)
arch/sh/include/uapi/asm/ioctls.h:11:#define FIONREAD   _IOR('f', 127, int)
arch/sparc/include/uapi/asm/ioctls.h:101:#define FIONREAD       _IOR('f', 127, int)
arch/xtensa/include/uapi/asm/ioctls.h:23:#define FIONREAD       _IOR('f', 127, int)
include/uapi/asm-generic/ioctls.h:46:#define FIONREAD   0x541B

See the problem?  On mips the value is not 0x541B - it's 0x467F; on
alpha, powerpc and sparc it's 0x4004667F, on sh and xtensa - 0x8004667F
(different expansions of _IOR).

The only thing that has any business including asm-generic/ioctls.h is
arch/*/include/uapi/asm/ioctls.h - if it feels like doing so.  Anywhere
else it ought to be asm/ioctls.h.

asm-generic/msi.h - looks bogus; there's an include of linux/msi.h several lines
above, and that pulls asm/msi.h.  Which makes it either a no-op or a build bug
(the latter - if that driver can be built on x86 with CONFIG_GENERIC_MSI_IRQ
defined;
typedef struct irq_alloc_info msi_alloc_info_t;
in asm/msi.h vs.
typedef struct msi_alloc_info {
        struct msi_desc                 *desc;
        irq_hw_number_t                 hwirq;
        unsigned long                   flags;
        union {
                unsigned long           ul;
                void                    *ptr;
        } scratchpad[NUM_MSI_ALLOC_SCRATCHPAD_REGS];
} msi_alloc_info_t;
in asm-generic/msi.h).

Hell knows about the rest, but they all look very dubious...