Message-ID: <87355qnt27.fsf@esperi.org.uk>
Date:   Mon, 27 Mar 2023 15:54:24 +0100
From:   Nick Alcock <nick.alcock@...cle.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Nick Alcock <nick.alcock@...cle.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Christoph Hellwig <hch@...radead.org>,
        linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
        Hitomi Hasegawa <hasegawa-hitomi@...itsu.com>,
        Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH 10/17] tty: remove MODULE_LICENSE in non-modules

On 27 Mar 2023, Greg Kroah-Hartman told this:

> On Mon, Mar 27, 2023 at 11:46:34AM +0100, Nick Alcock wrote:
>> On 24 Mar 2023, Luis Chamberlain spake thusly:
>> >                                      Please only re-submit only for
>> > files where the license is clear. The effort of clarifying licenses
>> > on files where one doesn't have an SPDX tag is welcomed but can take
>> > time and we'll need this anyway in the future to help later strive to
>> > see if we can automatically generate the MODULE_LICENSE() from the
>> > SPDX tags.
>> 
>> For now, I have an alternative that might be acceptable. I did a bit of
>> an audit and it's all a right mess (see below), with wild divergence
>> even when SPDX is present, GPL versus -only or GPL-2.0+ apparently
>> applied almost at random and some things being completely different (in
>> some cases they were both committed simultaneously and were inconsistent
>> from the moment the module was written). So many things are inconsistent
>> that kallmodsyms would call a lot of things modules that really aren't:
>> there is enough error that there probably be noticeable mistakes in
>> quite a high percentage of kernels.
>
> As you have found out, there is a difference that matters in the SPDX
> lines vs the MODULE_LICENSE lines when it comes to GPL vs GPLv2+ stuff.
> The SPDX lines are correct for the code itself, while the MODULE_LICENSE
> lines are correct from a "this is the license of this binary" point of
> view.

Oh right!

> So don't get confused here, if you all can figure out a way to generate
> the MODULE_LICENSE() lines from SPDX, that would be great, but in my
> quick look I think it's going to be very difficult (think about how
> multiple files make up a single module binary...)

In this specific case, 100% of the files I'm touching cannot be built as
modules at all (they are covered purely by booleans in Kconfig, no
tristate), so by this definition MODULE_LICENSE would seem to have no
meaning for them? If so, I don't see why you're objecting to ripping out
these meaningless lines. (I have already been told not to comment them
out but instead to just delete them.)

>> But... for our purposes, we don't actually *mind* if non-modules list
>> things like licenses inconsistently in two different places. Removing
>> MODULE_LICENSE was a means, not an end. What we're actually interested
>> in doing is removing .modinfo in things that can't possibly be modules,
>> and since a .modinfo in a guaranteed-non-module is at best entirely
>> useless I don't think anyone could reasonably be opposed to that end
>> goal (though they might reasonably be unhappy about all the churn
>> involved in getting there). They object to the removal of the visible
>> MODULE_LICENSE() argument text string, not to the useless compile-time
>> effect of a MODULE_LICENSE in a non-module.
>
> there are other things that create .modinfo lines, so I'm confused why
> you picked the license line to trigger all of this.

I didn't. Yamada did. MODULE_LICENSE is the only thing that generates
.modinfo *sections*, as of this commit:

commit 8b41fc4454e36fbfdbb23f940d023d4dece2de29
Author: Masahiro Yamada <masahiroy@...nel.org>
Date:   Thu Dec 19 17:33:29 2019 +0900

    kbuild: create modules.builtin without Makefile.modbuiltin or tristate.conf
[...]
    This commit simplifies the implementation. Let's exploit the fact
    that every module has MODULE_LICENSE(). (modpost shows a warning if
    MODULE_LICENSE is missing. If so, 0-day bot would already have blocked
    such a module.)
    
    I added MODULE_FILE to <linux/module.h>. When the code is being compiled
    as builtin, it will be filled with the file path of the module, and
    collected into modules.builtin.info. Then, scripts/link-vmlinux.sh
    extracts the list of builtin modules out of it.
    
    This new approach fixes the false-positives above, but adds another
    type of false-positives; non-modular code may have MODULE_LICENSE()
    by mistake. This is not a big deal, it is just the code is always
    orphan. We can clean it up if we like. [...]

It turns out that the presence of .modinfo is the *only* thing Yamada is
willing to use as an indicator that something can be a module: he's
vehemently refused any alternative, such as anything going back to the
tristate statements in Kconfig (using a "gave me nightmares" level of
vehemence, to be honest, and that is not a figure of speech).

But as a result of this, if we want there to be any way to tell whether
something can be a module, we now *have* to use .modinfo, which means we
*have* to ensure that only things that can be a module can contain
MODULE_LICENSEs. After the commit above, correct modules.builtin
construction requires this property. (Before that commit, all that was
necessary was Kconfig.)

>> So how about, for the first three groups below (the groups where
>> MODULE_LICENSE and SPDX are inconsistent, or where a SPDX simply doesn't
>> exist), instead of removing the MODULE_LICENSE we replace it with an
>> identical call to a new macro named perhaps NONMODULE_LICENSE(), which
>> is defined in module.h as simply expanding to nothing, except possibly
>> emitting a compile-time error if it's ever used in a module? This more
>> clearly denotes what's going on, keeps the license string in the source
>> file on a nearly identical line (for whatever purpose it serves), drops
>> the spurious .modinfo that's causing trouble, and probably makes fewer
>> people unhappy?
>
> Again, no, why aren't you just stubbing out MODULE_LICENSE() in the
> build if it's not being built as a module like the other MODULE_*()
> macros are?  Why is the license so special here yet the device list or
> module authors not?

This isn't "being built as a module": this is "can *ever* be built as a
module": i.e. "has a tristate line in Kconfig for at least one CONFIG_
variable governing the build for this object file".

> Don't treat this macro as somehow special where authors have to remember
> to not use it (or to use it), while the other MODULE_* macros do not
> have the issue?

The commit above should make things clearer, I hope? I think Yamada's
idea was that MODULE_LICENSE is something that must be present if module
loading is to work, so hanging other things that should be present in
all modules off it seems reasonable too: this seems pretty reasonable to
me as well, if directly driving via tristate statements is out.

This directly affects things like modules.builtin which are now driven
by .modinfo sections in vmlinux.o, dropped by the linker at final link
time. I'm just trying to make that .modinfo sections not contain entries
for things that are not tristate. Believe me, I'd much rather have done
this *without* a 140-patch series, but this was the first approach that
none of the module maintainers nacked.

> That's my main objection here.  Don't get confused by the license stuff,
> that's secondary.

Well, it seems to me that you're really objecting to the consequences of
a years-old commit that I didn't write. Since the person who did write
it seems to hate me for unclear reasons, I think you might have to ask
him for his rationale yourself (all his few communications with me for
the last several months have been various forms of insult, often for
doing things that he asked me to do years earlier: the remainder were
for doing things Luis asked me to do). If you could possibly get him to
stop attacking me I'd be grateful too. I'm only trying to improve the
kernel here.

(I am aware that saying this is probably going to make Yamada explode --
again -- so I removed him from Cc: out of an excess of hope. Nonetheless
it is true.)

(Not everyone has been awful. Luis in particular has been extremely
helpful even if I cursed at his requests sometimes. He even wrote a
replacement for the patch Yamada initially objected to, without my even
asking, and it was ever so much simpler than the thing I'd derived from
the old scripts/Makefile.modbuiltin. But that patch does rely on
modules.builtin being correct, hence needs all this MODULE_LICENSE
stuff...)

Powered by blists - more mailing lists