| Message ID | 20220602035653.4167316-1-saravanak@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] module: Fix prefix for module.sig_enforce module param | expand |
On Wed, Jun 01, 2022 at 08:56:52PM -0700, Saravana Kannan wrote: > Commit cfc1d277891e ("module: Move all into module/") changed the prefix > of the module param by moving/renaming files. A later commit also moves > the module_param() into a different file, thereby changing the prefix > yet again. > > This would break kernel cmdline compatibility and also userspace > compatibility at /sys/module/module/parameters/sig_enforce. > > So, set the prefix back to "module.". > > Cc: Aaron Tomlin <atomlin@redhat.com> > Cc: mcgrof@kernel.org > Cc: christophe.leroy@csgroup.eu > Cc: cl@linux.com > Cc: mbenes@suse.cz > Cc: akpm@linux-foundation.org > Cc: jeyu@kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-modules@vger.kernel.org > Cc: void@manifault.com > Cc: atomlin@atomlin.com > Cc: allen.lkml@gmail.com > Cc: joe@perches.com > Cc: msuchanek@suse.de > Cc: oleksandr@natalenko.name > Cc: jason.wessel@windriver.com > Cc: pmladek@suse.com > Cc: daniel.thompson@linaro.org > Cc: hch@infradead.org > Fixes: cfc1d277891e ("module: Move all into module/") > Signed-off-by: Saravana Kannan <saravanak@google.com> Acked-by: Luis Chamberlain <mcgrof@kernel.org> Linus want to take this in or should I just queueu these up? Luis
On Wed 2022-06-01 20:56 -0700, Saravana Kannan wrote: > Commit cfc1d277891e ("module: Move all into module/") changed the prefix > of the module param by moving/renaming files. A later commit also moves > the module_param() into a different file, thereby changing the prefix > yet again. > > This would break kernel cmdline compatibility and also userspace > compatibility at /sys/module/module/parameters/sig_enforce. > > So, set the prefix back to "module.". > > Cc: Aaron Tomlin <atomlin@redhat.com> > Cc: mcgrof@kernel.org > Cc: christophe.leroy@csgroup.eu > Cc: cl@linux.com > Cc: mbenes@suse.cz > Cc: akpm@linux-foundation.org > Cc: jeyu@kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-modules@vger.kernel.org > Cc: void@manifault.com > Cc: atomlin@atomlin.com > Cc: allen.lkml@gmail.com > Cc: joe@perches.com > Cc: msuchanek@suse.de > Cc: oleksandr@natalenko.name > Cc: jason.wessel@windriver.com > Cc: pmladek@suse.com > Cc: daniel.thompson@linaro.org > Cc: hch@infradead.org > Fixes: cfc1d277891e ("module: Move all into module/") > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > Sending this patch in case my analysis in [1] was right. > > [1] - https://lore.kernel.org/lkml/20220602034111.4163292-1-saravanak@google.com/ > > -Saravana > > kernel/module/signing.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/kernel/module/signing.c b/kernel/module/signing.c > index 85c8999dfecf..6b0672e4417b 100644 > --- a/kernel/module/signing.c > +++ b/kernel/module/signing.c > @@ -16,6 +16,11 @@ > #include <uapi/linux/module.h> > #include "internal.h" > > +#ifdef MODULE_PARAM_PREFIX > +#undef MODULE_PARAM_PREFIX > +#endif > +#define MODULE_PARAM_PREFIX "module." > + > static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE); > module_param(sig_enforce, bool_enable_only, 0644); > > -- > 2.36.1.255.ge46751e96f-goog > Oops! Thanks Saravana. Reviewed-by: Aaron Tomlin <atomlin@redhat.com>
On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@kernel.org> 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. Linus
On Thu, Jun 2, 2022 at 12:41 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@kernel.org> 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. -Saravana
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@linux-foundation.org> wrote: > > > > On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@kernel.org> 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. Luis
On Thu, Jun 2, 2022 at 3:59 PM Luis Chamberlain <mcgrof@kernel.org> 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@linux-foundation.org> wrote: > > > > > > On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@kernel.org> 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
Hi-- On 6/2/22 20:48, Saravana Kannan wrote: > On Thu, Jun 2, 2022 at 3:59 PM Luis Chamberlain <mcgrof@kernel.org> 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@linux-foundation.org> wrote: >>>> >>>> On Thu, Jun 2, 2022 at 12:16 PM Luis Chamberlain <mcgrof@kernel.org> 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. I agree with your assessment. Nothing new is needed.
diff --git a/kernel/module/signing.c b/kernel/module/signing.c index 85c8999dfecf..6b0672e4417b 100644 --- a/kernel/module/signing.c +++ b/kernel/module/signing.c @@ -16,6 +16,11 @@ #include <uapi/linux/module.h> #include "internal.h" +#ifdef MODULE_PARAM_PREFIX +#undef MODULE_PARAM_PREFIX +#endif +#define MODULE_PARAM_PREFIX "module." + static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE); module_param(sig_enforce, bool_enable_only, 0644);
Commit cfc1d277891e ("module: Move all into module/") changed the prefix of the module param by moving/renaming files. A later commit also moves the module_param() into a different file, thereby changing the prefix yet again. This would break kernel cmdline compatibility and also userspace compatibility at /sys/module/module/parameters/sig_enforce. So, set the prefix back to "module.". Cc: Aaron Tomlin <atomlin@redhat.com> Cc: mcgrof@kernel.org Cc: christophe.leroy@csgroup.eu Cc: cl@linux.com Cc: mbenes@suse.cz Cc: akpm@linux-foundation.org Cc: jeyu@kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-modules@vger.kernel.org Cc: void@manifault.com Cc: atomlin@atomlin.com Cc: allen.lkml@gmail.com Cc: joe@perches.com Cc: msuchanek@suse.de Cc: oleksandr@natalenko.name Cc: jason.wessel@windriver.com Cc: pmladek@suse.com Cc: daniel.thompson@linaro.org Cc: hch@infradead.org Fixes: cfc1d277891e ("module: Move all into module/") Signed-off-by: Saravana Kannan <saravanak@google.com> --- Sending this patch in case my analysis in [1] was right. [1] - https://lore.kernel.org/lkml/20220602034111.4163292-1-saravanak@google.com/ -Saravana kernel/module/signing.c | 5 +++++ 1 file changed, 5 insertions(+)