linux-kernel - Re: [PATCH v9 2/8] kbuild: add modules

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 21 Nov 2022 21:14:32 +0000
From:   Nick Alcock <nick.alcock@...cle.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
        arnd@...db.de, akpm@...ux-foundation.org, eugene.loh@...cle.com,
        kris.van.hees@...cle.com, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v9 2/8] kbuild: add modules_thick.builtin

On 21 Nov 2022, Luis Chamberlain spake thusly:

> On Mon, Nov 21, 2022 at 11:12:52AM -0800, Luis Chamberlain wrote:
>> On Mon, Nov 21, 2022 at 03:21:10PM +0000, Nick Alcock wrote:
>> > One question: do you think it's worthwhile me submitting patches to
>> > de-MODULE_* things that need it?
>> 
>> 100% yes.
>> 
>> Yes please remove all that module declration helpers for things that are
>> not modules, and after you add your helper which will nag at build time
>> when it finds new ones.
>> 
>> For justification just mention in the commit log that after commit
>> 8b41fc4454e ("kbuild: create modules.builtin without Makefile.modbuiltin or
>> tristate.conf") we rely on the module license tag to generate the
>> modules.builtin file and so built-in code which uses module helpers
>> just need to be removed.
>
> You should also mention what modules.builtin is used for as per our
> Documentation/kbuild/kbuild.rst, and that it is only used for
> modprobe to *not* fail when trying to load a module which is
> built-in.

Yep! (Which is an extremely damn useful improvement, I must say. I'm
relying on it already.)

Only two remaining problems in your patch that I can see (hacked around
in the checker, but consumers shouldn't have to hack around this sort of
thing). First, some very strange lines like this in modules.builtin.objs:

drivers/hid/hid-uclogic.o: drivers/hid/hid-uclogic-core.o drivers/hid/hid-uclogic-params.o drivers/hid/hid-uclogic-rdesc.o
drivers/hid/hid-uclogic
drivers/hid/hid-uclogic-test.o: 

(note the line with no .o or colon at all.)

This seems to be a consequence of lines like

hid-uclogic-objs                := hid-uclogic-core.o \
                                   hid-uclogic-rdesc.o \
                                   hid-uclogic-params.o
obj-$(CONFIG_HID_UCLOGIC)       += hid-uclogic.o

i.e. use of -objs as a completely random variable for holding object
files which might or might not be in a module. This seems a bit.. risky
to me. Looking for a fix... maybe we can just ignore *-objs on the
grounds that if it matters it will always land in some other variable
too? ... maybe?

One other definite problem: drivers/staging/media/atomisp/Makefile says:

obj-$(CONFIG_VIDEO_ATOMISP) += pci/atomisp_gmin_platform.o

This subdirectory is lost from KBUILD_MODOBJS, leading to the file entry
in modinfo and thus the resulting modules.builtin.objs pointing to a
file named drivers/staging/media/atomisp/atomisp_gmin_platform.o, which
does not exist. (This is also seen in some non-staging directories, e.g.
kernel/trace/rv.)

> How many of these are we talking about? I'm happy to take them
> via modules-next. I'd hope to not run accross many conflicts against
> other trees.

Going by an x86 allyesconfig run, 169 total (probably plus a few given
errors like the one above), plus no doubt a few more for other arches.
So not a vast number, but enough that hacking up a checker was clearly
not a waste of time.

If there turn out to be any conflicts that aren't spurious I'd be very
surprised. Hardly anyone ever even adjusts their email address in
MODULE_AUTHOR when they change it :P

-- 
NULL && (void)