Message-ID: <ZAuGE2ay3q0MT4Yi@bombadil.infradead.org>
Date:   Fri, 10 Mar 2023 11:33:39 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Christoph Hellwig <hch@...radead.org>,
        Nick Alcock <nick.alcock@...cle.com>,
        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 Fri, Mar 10, 2023 at 08:31:30AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 02:38:10PM -0800, Luis Chamberlain wrote:
> > On Thu, Mar 09, 2023 at 05:15:42PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 02, 2023 at 09:17:52PM +0000, Nick Alcock wrote:
> > > > Since commit 8b41fc4454e ("kbuild: create modules.builtin without
> > > > Makefile.modbuiltin or tristate.conf"), MODULE_LICENSE declarations
> > > > are used to identify modules. As a consequence, uses of the macro
> > > > in non-modules will cause modprobe to misidentify their containing
> > > > object file as a module when it is not (false positives), and modprobe
> > > > might succeed rather than failing with a suitable error message.
> > > > 
> > > > So remove it in the files in this commit, none of which can be built as
> > > > modules.
> > > > 
> > > > Signed-off-by: Nick Alcock <nick.alcock@...cle.com>
> > > > Suggested-by: Luis Chamberlain <mcgrof@...nel.org>
> > > > Cc: Luis Chamberlain <mcgrof@...nel.org>
> > > > Cc: linux-modules@...r.kernel.org
> > > > Cc: linux-kernel@...r.kernel.org
> > > > Cc: Hitomi Hasegawa <hasegawa-hitomi@...itsu.com>
> > > > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > > Cc: Jiri Slaby <jirislaby@...nel.org>
> > > > ---
> > > >  drivers/tty/n_null.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
> > > > index f913b665af725..c24f75942c49d 100644
> > > > --- a/drivers/tty/n_null.c
> > > > +++ b/drivers/tty/n_null.c
> > > > @@ -63,7 +63,6 @@ static void __exit n_null_exit(void)
> > > >  module_init(n_null_init);
> > > >  module_exit(n_null_exit);
> > > >  
> > > > -MODULE_LICENSE("GPL");
> > > >  MODULE_AUTHOR("Alan Cox");
> > > >  MODULE_ALIAS_LDISC(N_NULL);
> > > >  MODULE_DESCRIPTION("Null ldisc driver");
> > > > -- 
> > > > 2.39.1.268.g9de2f9a303
> > > > 
> > > 
> > > Nope, sorry, this is not good to do, please fix kbuild instead of
> > > forcing a tree-wide change like this.
> > 
> > Masahiro Yamada already NACK'd it such effort:
> > 
> > https://lkml.kernel.org/r/CAK7LNAQLttPD=Ae==e0CYeQtS78=o_JZFK+zxa29JnUYio52Ug@mail.gmail.com
> > 
> > And his descriptiuon of the reasoning and logic is explained here:              
> > 
> > https://lore.kernel.org/all/CAK7LNASL7_RgfASstBvN6AzhR=nMU=HsQvODf5q13Xud8tBWRQ@mail.gmail.com/
> > 
> > Let me summarize it though with a few quotes from him:
> > 
> > "Having false-positives in modules.builtin should be OK"
> > "In this sense, having always-builtin entries in module.builtin is OK."
> 
> None of that matters, sorry.
> 
> Again, all I am saying is that you can not have some MODULE_() macros
> that are ok for code that is built in, and some that are not, for
> "reasons" that have to do how you all are treating the build system
> infrastructure as you are now putting arbritrary requirements for all
> driver authors (of which there are thousands) to know this.

As noted once again, it is not putting hard requirement. Future tooling
not yet added would just not benefit from distinguishing symbols for
your modules.

I'm happy to live with module authors not wanting to remove the module
license tag from their modules if they can never actually be modules
from not benefitting from the above tooling gains as its just cherry
on top tooling gains.

Solving this in way that does not impeed on current build system
improvements is a challenge and I'm happy to punt that out to future
work.

> Just change the macros to work properly in both cases, I can't believe
> this is all that hard as obviously all of the other macros work both
> ways, right?  That should not require any kbuild changes.

Patches are welcomed.

> > The reason Nick wants to do this work is that his future patches
> > (which have been under review for years and I'm starting to chew on
> > it and provide guidance on now) extend our ability to have more
> > elaborate symbol to address mapping with more metdata, which does
> > include information such as if something came from a module. So
> > long term *I* certainly am interested in a deterministic way to
> > determine if something could be a module.
> > 
> > For a more elaborate attempt on my part to try to describe the problem
> > and some side ideas I had if we wanted an alternative:
> > 
> > https://lore.kernel.org/all/Y/kXDqW+7d71C4wz@bombadil.infradead.org/
> > 
> > I should also mention Christoph has also suggested we eventually move
> > towards automatically generating the module license tag from the SPDX
> > tag:
> > 
> > https://lore.kernel.org/all/Y5BNCbFyvNA1Xp%2FX@infradead.org
> 
> That too would be wonderful, and I would love to see that, but it does
> not remove the base problem here that you are somehow forcing all driver
> authors to change their code for the build system changes which should
> not be affecting them at all at this point in time.

By you, you mean Masahiro Yamada's patch. That is just collateral for
tooling being built upon by Nick.  And as Masahiro has pointed out
more than once already, this is not a regression / fatal issue.

> If you all do trigger off of the SPDX tags, then the removal of all
> MODULE_LICENSE() instances would be great too, but I don't think you are
> there yet (and it also wouldn't require removal all at once as you could
> just stub out that macro to be nothing.)  But this is not the real issue
> here...

That's future work.

> Maybe the solution is to stop triggering on MODULE_LICENSE() as
> something magic for the build, as obviously that is the root problem
> here.  Or something else, I don't know, but what you all are doing here
> does not seem correct at all, sorry, and is only going to cause more
> long-term problems with maintenance and headaches for driver authors.

I already suggested what I think *could* be done as an alternative and
the problem is not easy to resolve.

Feel free to ignore the patches for your drivers which remove the module
macros even if they are not modules. In lieu of alternative suggestions,
that's what we have now, and it is much better than what Nick was suggesting
before which Masahiro NACK'd.

Most other module developers seem happy with the change, and this has also
helped fix SPDX tags where there wasn't some too.

So if you don't take the patches it is not the end of the world and Nick
can move on with that effort for folks who do want to clarify this.

  Luis

Powered by blists - more mailing lists