Re: [PATCH v1] module: Fix prefix for module.sig_enforce module param

[Saravana Kannan]

On Thu, Jun 2, 2022 at 3:59 PM Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote:
>
> On Thu, Jun 02, 2022 at 02:47:04PM -0700, Saravana Kannan wrote:
> > On Thu, Jun 2, 2022 at 12:41 PM Linus Torvalds
> > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@xxxxxxxxxx> wrote:
> > > >
> > > > Linus want to take this in or should I just queue these up?
> > >
> > > I'll take it, and remove the unnecessary #ifdef/#endif. The #undef
> > > might as well be unconditional - simpler and doesn't hurt.
> >
> > Sounds good. I just copy-pasted how it was done elsewhere. Luis was
> > mentioning adding a wrapper to go this cleanly and I needed it in
> > another instance too. So I'll look into doing that in a future patch.
>
> Virtual hug, or something hippie like that.

Thanks :)

I gave this a shot.

+ #define set_module_param_prefix(prefix) \
+ #undef MODULE_PARAM_PREFIX              \
+ #define MODULE_PARAM_PREFIX prefix

I even wrote up a nice chunk of "function doc" before I tried
compiling it. And once I compiled it, I was smacked in the head by the
compiler for trying to put a #define inside a #define (Duh, the
preprocessing in a single pass).

Before I tried this, I looked up the current uses of the "hacky" snippet:

$ git grep -l "define.*MODULE_PARAM_PREFIX" -- :^include/
block/disk-events.c
drivers/misc/cxl/fault.c
drivers/mmc/core/block.c
drivers/pci/pcie/aspm.c
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
drivers/tty/serial/8250/8250_core.c
drivers/xen/balloon.c
drivers/xen/events/events_base.c
kernel/debug/kdb/kdb_main.c
kernel/kcsan/core.c
kernel/rcu/tree.c
kernel/rcu/update.c
mm/damon/reclaim.c
mm/kfence/core.c


In a lot of those files, there are a lot of module params that follow
this snippet. Going on a tangent, some of the uses of #define
MODULE_PARAM_PREFIX don't seem to have any obvious use or param
macros.

So adding a module_param_prefixed() doesn't make sense to me either,
because I'll have to repeat the same prefix in every usage of
module_param_prefixed() AND I'll have to create a _prefixed() variant
for other param macros too.

So, something like:
module_param_prefixed("module.", sig_enforce, bool, 644);
module_param_prefixed("module.", another_param1, bool, 644);
module_param_prefixed("module.", another_param2, bool, 644);

Or replace "module." with a MY_PREFIX so that it's easy to ensure the
string is the same across each use:
#define MY_PREFIX "module."
module_param_prefixed(MY_PREFIX, sig_enforce, bool, 644);
module_param_prefixed(MY_PREFIX, another_param1, bool, 644);
module_param_prefixed(MY_PREFIX, another_param2, bool, 644);

But at that point, all I'm avoiding is one #undef MODULE_PARAM_PREFIX
and a whole lot of code churn.

One other option is to do something like:
#ifndef MOD_PREFIX
#define MODULE_PARAM_PREFIX KBUILD_MODNAME "."
#else
#define MODULE_PARAM_PREFIX MOD_PREFIX "."
#endif

But that will have the limitation that MOD_PREFIX has to be defined
before any #includes is in a file (similar to pr_fmt())  and doesn't
allow you to redefine the prefix half way through a file -- which is
also a thing that happens in drivers/tty/serial/8250/8250_core.c.

So, long story short, none of these options sound especially appealing
that it's worth all the code churn across multiple maintainer trees.
Let me know if you have other ideas that might work or you think one
of the options I dismissed is actually worth doing.

Thanks,
Saravana