| Message ID | 3a99048a52aeee356d01dbf7f2f06e6e0826ed78.1733245406.git.ukleinek@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pwm: lpss: module namespace fixes | expand |
On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote: > Each user of the exported symbols related to the pwm-lpss driver needs > to import the matching namespace. So this can just be done in the header > together with the prototypes. > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import > that namespace before. (However this didn't hurt because the pwm-lpss > module namespace isn't used; see the next commit.) I disagree on this change, I think it had been discussed already. The header must not provide any module importing features as it effectively diminishes the point of namespace. Any (ab)user can include just a header and be okay with that. Besides that, you should have based this on recent changes in the NS area of the module symbols, i.e. module namespaces needs to be provided as string literals. See last few commits in Linus' tree as of today.
Hello Andy, On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote: > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote: > > Each user of the exported symbols related to the pwm-lpss driver needs > > to import the matching namespace. So this can just be done in the header > > together with the prototypes. > > > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import > > that namespace before. (However this didn't hurt because the pwm-lpss > > module namespace isn't used; see the next commit.) > > I disagree on this change, I think it had been discussed already. Who discussed this? If I was involved, I don't remember. So if you have a link, that would be great. > The header must not provide any module importing features as it effectively > diminishes the point of namespace. Any (ab)user can include just a header and > be okay with that. Huh? Any abuser can also just do the IMPORT_NS statement? Module namespaces are not a safeguard against bad code? So I don't see why making it simple for the regular users should be the wrong choice. Actually I think this is very elegant because this way all needs to use these symbols (i.e. prototype and namespace) are in a single place and users just do the #include and get all the preconditions. > Besides that, you should have based this on recent changes in the NS area of > the module symbols, i.e. module namespaces needs to be provided as string > literals. I coordinated my patch set with the pwm maintainer and he is ok with it :-) That's why I wrote "This conflicts with ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is currently in Linus' tree. I'll take care about that." in the cover letter. Best regards Uwe
On Tue, Dec 03, 2024 at 10:32:17PM +0100, Uwe Kleine-König wrote: > On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote: > > > Each user of the exported symbols related to the pwm-lpss driver needs > > > to import the matching namespace. So this can just be done in the header > > > together with the prototypes. > > > > > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import > > > that namespace before. (However this didn't hurt because the pwm-lpss > > > module namespace isn't used; see the next commit.) > > > > I disagree on this change, I think it had been discussed already. > > Who discussed this? If I was involved, I don't remember. So if you have > a link, that would be great. Sure, https://lore.kernel.org/linux-pwm/20220908135658.64463-1-andriy.shevchenko@linux.intel.com/ you were a participant there. > > The header must not provide any module importing features as it effectively > > diminishes the point of namespace. Any (ab)user can include just a header and > > be okay with that. > > Huh? Any abuser can also just do the IMPORT_NS statement? Module > namespaces are not a safeguard against bad code? So I don't see why > making it simple for the regular users should be the wrong choice. > > Actually I think this is very elegant because this way all needs to use > these symbols (i.e. prototype and namespace) are in a single place and > users just do the #include and get all the preconditions. Recent LWN https://lwn.net/Articles/998221/ article describes in more details what I implied under abuser here. > > Besides that, you should have based this on recent changes in the NS area of > > the module symbols, i.e. module namespaces needs to be provided as string > > literals. > > I coordinated my patch set with the pwm maintainer and he is ok with it > :-) That's why I wrote "This conflicts with > ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is currently in Linus' > tree. I'll take care about that." in the cover letter. Other subsystems simply backmerged those changes. Note, at any time one may consider origin/master as an immutable (at the point of a given change). It might be better (if no urgent need) to wait for rc2 and merge it before doing other changes.
Hello Andy, On Wed, Dec 04, 2024 at 12:28:23AM +0200, Andy Shevchenko wrote: > On Tue, Dec 03, 2024 at 10:32:17PM +0100, Uwe Kleine-König wrote: > > On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote: > > > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote: > > > > Each user of the exported symbols related to the pwm-lpss driver needs > > > > to import the matching namespace. So this can just be done in the header > > > > together with the prototypes. > > > > > > > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import > > > > that namespace before. (However this didn't hurt because the pwm-lpss > > > > module namespace isn't used; see the next commit.) > > > > > > I disagree on this change, I think it had been discussed already. > > > > Who discussed this? If I was involved, I don't remember. So if you have > > a link, that would be great. > > Sure, https://lore.kernel.org/linux-pwm/20220908135658.64463-1-andriy.shevchenko@linux.intel.com/ > you were a participant there. Ah that. When I read "it had been discussed already" I expected a discussion with an agreement in the end. Let me note that you chose the more complicated way back then (and I let you) and the result is that the pinctrl driver didn't get the (up to now not yet) needed import. > > > The header must not provide any module importing features as it effectively > > > diminishes the point of namespace. Any (ab)user can include just a header and > > > be okay with that. > > > > Huh? Any abuser can also just do the IMPORT_NS statement? Module > > namespaces are not a safeguard against bad code? So I don't see why > > making it simple for the regular users should be the wrong choice. > > > > Actually I think this is very elegant because this way all needs to use > > these symbols (i.e. prototype and namespace) are in a single place and > > users just do the #include and get all the preconditions. > > Recent LWN https://lwn.net/Articles/998221/ article describes in more details > what I implied under abuser here. Ok. But I don't see how this supports your case. Independent of where the import for a given namespace happens, you can see the used namespaces in modinfo and that is the only place where suspicious imports can be noted reliably. And MODULE_foo isn't relevant here as the namespace of the pwm-lpss driver combo is used in several modules. I thought the point of namespaces is grouping of symbols and reduction of the set of globally available symbols to speed up module loading. I didn't have "duplicate IMPORT_NS statements to make it harder for bad out-of-tree code" on my radar and that shouldn't be a motivation if the price is that in-tree code faces the same constraints IMHO. And I'm pretty sure, we won't prevent a bad actor from successfully using a symbol they should not, just because they need an include *and* an import. Am I missing something? > > > Besides that, you should have based this on recent changes in the NS area of > > > the module symbols, i.e. module namespaces needs to be provided as string > > > literals. > > > > I coordinated my patch set with the pwm maintainer and he is ok with it > > :-) That's why I wrote "This conflicts with > > ceb8bf2ceaa77fe222fe8fe32cb7789c9099ddf1 that is currently in Linus' > > tree. I'll take care about that." in the cover letter. > > Other subsystems simply backmerged those changes. Note, at any time one may > consider origin/master as an immutable (at the point of a given change). > It might be better (if no urgent need) to wait for rc2 and merge it before > doing other changes. I'm well aware of my options. Thanks Uwe
On Wed, Dec 04, 2024 at 12:38:49AM +0100, Uwe Kleine-König wrote: > On Wed, Dec 04, 2024 at 12:28:23AM +0200, Andy Shevchenko wrote: > > On Tue, Dec 03, 2024 at 10:32:17PM +0100, Uwe Kleine-König wrote: > > > On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote: > > > > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote: ... > > > > The header must not provide any module importing features as it effectively > > > > diminishes the point of namespace. Any (ab)user can include just a header and > > > > be okay with that. > > > > > > Huh? Any abuser can also just do the IMPORT_NS statement? Module > > > namespaces are not a safeguard against bad code? So I don't see why > > > making it simple for the regular users should be the wrong choice. > > > > > > Actually I think this is very elegant because this way all needs to use > > > these symbols (i.e. prototype and namespace) are in a single place and > > > users just do the #include and get all the preconditions. > > > > Recent LWN https://lwn.net/Articles/998221/ article describes in more details > > what I implied under abuser here. > > Ok. But I don't see how this supports your case. Independent of where > the import for a given namespace happens, you can see the used > namespaces in modinfo and that is the only place where suspicious > imports can be noted reliably. And MODULE_foo isn't relevant here as the > namespace of the pwm-lpss driver combo is used in several modules. The initial point that the (ab)users will be visible at review stage. Yes, runtime restrictions are more difficult to enforce. > I thought the point of namespaces is grouping of symbols and reduction > of the set of globally available symbols to speed up module loading. > I didn't have "duplicate IMPORT_NS statements to make it harder for bad > out-of-tree code" on my radar and that shouldn't be a motivation if the > price is that in-tree code faces the same constraints IMHO. > > And I'm pretty sure, we won't prevent a bad actor from successfully > using a symbol they should not, just because they need an include *and* > an import. > > Am I missing something? They may not need the include actually, as they can just tell the compiler with extern keyword to look for a prototype somewhere else. In any case let's return to the main topic here. My proposal is to add the respective module namespace import into pin control driver.
Hello Linus, On Tue, Dec 03, 2024 at 09:12:36PM +0200, Andy Shevchenko wrote: > On Tue, Dec 03, 2024 at 06:16:14PM +0100, Uwe Kleine-König wrote: > > Each user of the exported symbols related to the pwm-lpss driver needs > > to import the matching namespace. So this can just be done in the header > > together with the prototypes. > > > > This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import > > that namespace before. (However this didn't hurt because the pwm-lpss > > module namespace isn't used; see the next commit.) > > I disagree on this change, I think it had been discussed already. > > The header must not provide any module importing features as it effectively > diminishes the point of namespace. Any (ab)user can include just a header and > be okay with that. Andy and I disagree about this change. If you happen to not have the patch in your inbox, see https://lore.kernel.org/linux-pwm/3a99048a52aeee356d01dbf7f2f06e6e0826ed78.1733245406.git.ukleinek@kernel.org/ . Would you please volunteer as the impartial judge here as you're the upstream maintainer of drivers/pinctrl/intel/pinctrl-intel.c? The TL;DR; is: Do you prefer a single MODULE_IMPORT_NS() in a header, or should every consumer driver explicitly have its own MODULE_IMPORT_NS() invokation? I won't repeat the arguments because I think I cannot present them without making my opinion too obvious, so please see the discussion between Andy and me in the above linked thread. Best regards and thanks, Uwe
diff --git a/drivers/pwm/pwm-lpss-pci.c b/drivers/pwm/pwm-lpss-pci.c index f7ece2809e6b..8615c44c1034 100644 --- a/drivers/pwm/pwm-lpss-pci.c +++ b/drivers/pwm/pwm-lpss-pci.c @@ -8,7 +8,6 @@ */ #include <linux/kernel.h> -#include <linux/module.h> #include <linux/pci.h> #include <linux/pm_runtime.h> @@ -70,4 +69,3 @@ module_pci_driver(pwm_lpss_driver_pci); MODULE_DESCRIPTION("PWM PCI driver for Intel LPSS"); MODULE_LICENSE("GPL v2"); -MODULE_IMPORT_NS(PWM_LPSS); diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c index 5130238a4567..3de1ab2cff54 100644 --- a/drivers/pwm/pwm-lpss-platform.c +++ b/drivers/pwm/pwm-lpss-platform.c @@ -9,7 +9,6 @@ #include <linux/kernel.h> #include <linux/mod_devicetable.h> -#include <linux/module.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/property.h> @@ -78,5 +77,4 @@ module_platform_driver(pwm_lpss_driver_platform); MODULE_DESCRIPTION("PWM platform driver for Intel LPSS"); MODULE_LICENSE("GPL v2"); -MODULE_IMPORT_NS(PWM_LPSS); MODULE_ALIAS("platform:pwm-lpss"); diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h index 752c06b47cc8..0a1025f6cd58 100644 --- a/include/linux/platform_data/x86/pwm-lpss.h +++ b/include/linux/platform_data/x86/pwm-lpss.h @@ -4,6 +4,7 @@ #ifndef __PLATFORM_DATA_X86_PWM_LPSS_H #define __PLATFORM_DATA_X86_PWM_LPSS_H +#include <linux/module.h> #include <linux/types.h> struct device; @@ -30,4 +31,10 @@ struct pwm_lpss_boardinfo { struct pwm_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base, const struct pwm_lpss_boardinfo *info); +/* + * The above function and the pwm_lpss_boardinfo variables in + * drivers/pwm/pwm-lpss.h are defined in the PWM_LPSS namespace. + */ +MODULE_IMPORT_NS(PWM_LPSS); + #endif /* __PLATFORM_DATA_X86_PWM_LPSS_H */
Each user of the exported symbols related to the pwm-lpss driver needs to import the matching namespace. So this can just be done in the header together with the prototypes. This fixes drivers/pinctrl/intel/pinctrl-intel.c which failed to import that namespace before. (However this didn't hurt because the pwm-lpss module namespace isn't used; see the next commit.) Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/pwm/pwm-lpss-pci.c | 2 -- drivers/pwm/pwm-lpss-platform.c | 2 -- include/linux/platform_data/x86/pwm-lpss.h | 7 +++++++ 3 files changed, 7 insertions(+), 4 deletions(-)