linux-kernel - Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after export.h included

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: <CAK7LNASPBBHz8duZa4OkxbFP3tCsF29ctj5E9GKC-MxwoU+1pA@mail.gmail.com>
Date: Mon, 30 Dec 2024 20:06:59 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: David Laight <david.laight.linux@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, andi.shyti@...nel.org, 
	andriy.shevchenko@...ux.intel.com, u.kleine-koenig@...libre.com, 
	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH] module: Allow DEFAULT_SYMBOL_NAMESPACE be set after
 export.h included

On Sun, Dec 29, 2024 at 9:59 AM David Laight
<david.laight.linux@...il.com> wrote:
>
> On Sat, 28 Dec 2024 15:29:24 -0800
> Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>
> > On Sat, 28 Dec 2024 at 10:43, David Laight <david.laight.linux@...il.com> wrote:
> > >
> > > Instead just default DEFAULT_SYMBOL_NAMESPACE to "" and remove the
> > > extra _EXPORT_SYMBOL() wrapper.
> > >
> > > This lets DEFAULT_SYMBOL_NAMESPACE be defined after export.h is included.
> >
> > Grr. This is horribly ugly.
>
> I thought it was a neater 'ugly' than the current definitions in export.h
>
> > I think the i2c code should just be fixed to use the proper "define
> > namespace early".
>
> The i2c changes were needed because I found the code wouldn't compile.
> It is pretty easy mistake to make and will happen again.


Agree.

Currently, the compilation still succeeds, and the empty string ""
is used instead of the specified namespace, silently.
It is difficult to notice this mistake.

So, I like the change for include/linux/export.h
since it causes a compile error if
DEFAULT_SYMBOL_NAMESPACE is defined after
the header inclusion.

Perhaps, we can add a comment about how
to fix the issue.


/*
 * If you override DEFAULT_SYMBOL_NAMESPACE, please define it at the very top
 * of the source file, before any header inclusion.
 */
#ifndef DEFAULT_SYMBOL_NAMESPACE
#define DEFAULT_SYMBOL_NAMESPACE ""
#endif









>
> and does - I missed drivers/pwm/pwm-lpss.c drivers/hwmon/nct6775-core.c
> and drivers/pwm/pwm-lpss.c

OK, this patch breaks the compilation, and we can notice the mistake.
This is good.



> I guess those files could be fixed by moving the definition 'early'.
>
> > I will also note that 'sparse' has a notion of a "weak define", where
> > you can set a default value for a preprocessor symbol, but if it gets
> > redefined by the user (or already has a definition), sparse won't
> > complain about it, and just use the strong one.
> >
> > That would have been lovely, and we could have had a
> >
> >    #weak_define DEFAULT_SYMBOL_NAMESPACE ""
> >
> > and this wouldn't be the ugly mess it is.
> >
> > I wish the regular C preprocessor could do the same. Oh well. Since it
> > doesn't, I really think i2c should just be fixed, and we shouldn't try
> > to deal with i2c having done things wrong.
>
> What you really need is the preprocessor to support a ?: type operator
> in an expansion. Then you can have (DEFAULT_SYMBOL_NAMESPACE ?: "") in
> the expansion of EXPORT_SYMBOL().
>
> >
> >                   Linus
>


-- 
Best Regards
Masahiro Yamada