parameter of module_init() and module_exit() must not be a macro

parameter of module_init() and module_exit() must not be a macro

[Warlich, Christof]

I'd just like to get some feedback on the following issue and if the patch that I'm suggesting would be appropriate to be considered for upstream submission:

While writing a driver template, I just came across an issue with the module_init() and module_exit() macros: They don't work properly when the parameter being passed to them is a macro itself. Here is a minimal example that shows the issue:

$ cat test.c
#include <linux/module.h>
#define DRIVER_INIT test_init
static int __init DRIVER_INIT(void)
{
	return 0;
}
//module_init(test_init); // This works, ...
module_init(DRIVER_INIT); // ... but this doesn't.

Building the module gives the following errors:

$ make -C /lib/modules/$(uname -r)/build MODULES=test M=$(pwd) modules
make: Entering directory `/usr/src/linux-headers-3.13.0-65-generic'
  CC [M]  /home/adwach13/tmp/module/tst.o
In file included from /usr/src/linux-headers-3.13.0-65-generic/arch/x86/include/asm/ptrace.h:63:0,
                 from /usr/src/linux-headers-3.13.0-65-generic/arch/x86/include/asm/alternative.h:8,
                 from /usr/src/linux-headers-3.13.0-65-generic/arch/x86/include/asm/bitops.h:16,
                 from include/linux/bitops.h:36,
                 from include/linux/kernel.h:10,
                 from include/linux/cache.h:4,
                 from include/linux/time.h:4,
                 from include/linux/stat.h:18,
                 from include/linux/module.h:10,
                 from /home/adwach13/tmp/module/tst.c:1:
include/linux/init.h:298:6: error: 'init_module' aliased to undefined symbol 'DRIVER_INIT'
  int init_module(void) __attribute__((alias(#initfn)));
      ^
/home/adwach13/tmp/module/tst.c:8:1: note: in expansion of macro 'module_init'
 module_init(DRIVER_INIT);
 ^
make[1]: *** [/home/adwach13/tmp/module/tst.o] Error 1
make: *** [_module_/home/adwach13/tmp/module] Error 2
make: Leaving directory `/usr/src/linux-headers-3.13.0-65-generic'

The reason for this behavior is because macro stringification is not done properly in the module_init() and module_exit() macro definitions in include/linux/init.h. But a fix would be easy:

$ diff -Nau /usr/src/linux-headers-3.13.0-65-generic/include/linux/init.h.orig  /usr/src/linux-headers-3.13.0-65-generic/include/linux/init.h
--- /usr/src/linux-headers-3.13.0-65-generic/include/linux/init.h.orig  2015-10-15 11:40:36.300458826 +0200
+++ /usr/src/linux-headers-3.13.0-65-generic/include/linux/init.h       2015-10-15 11:42:05.370985262 +0200
@@ -291,17 +291,20 @@
 
 #define security_initcall(fn)          module_init(fn)
 
+#define _STRINGIFY(x) #x
+#define STRINGIFY(x) _STRINGIFY(x)
+
 /* Each module must use one module_init(). */
 #define module_init(initfn)                                    \
        static inline initcall_t __inittest(void)               \
        { return initfn; }                                      \
-       int init_module(void) __attribute__((alias(#initfn)));
+       int init_module(void) __attribute__((alias(STRINGIFY(initfn))));
 
 /* This is only required if you want to be unloadable. */
 #define module_exit(exitfn)                                    \
        static inline exitcall_t __exittest(void)               \
        { return exitfn; }                                      \
-       void cleanup_module(void) __attribute__((alias(#exitfn)));
+       void cleanup_module(void) __attribute__((alias(STRINGIFY(exitfn))));
 
 #define __setup_param(str, unique_id, fn)      /* nothing */
 #define __setup(str, func)                     /* nothing */

What do you think: Is this patch worth to be suggested? Or is the current behavior intended?

parameter of module_init() and module_exit() must not be a macro

[Greg KH]

On Thu, Oct 15, 2015 at 09:48:53AM +0000, Warlich, Christof wrote:
> I'd just like to get some feedback on the following issue and if the patch that I'm suggesting would be appropriate to be considered for upstream submission:
> 
> While writing a driver template, I just came across an issue with the module_init() and module_exit() macros: They don't work properly when the parameter being passed to them is a macro itself. Here is a minimal example that shows the issue:
> 
> $ cat test.c
> #include <linux/module.h>
> #define DRIVER_INIT test_init
> static int __init DRIVER_INIT(void)
> {
> 	return 0;
> }
> //module_init(test_init); // This works, ...
> module_init(DRIVER_INIT); // ... but this doesn't.

I'll ask, why would you ever want to pass a macro to module_init()?

We don't like functions to be macros in the kernel, do you have a
real-world need for this somewhere?  If so, can you show the code?

thanks,

greg k-h

AW: parameter of module_init() and module_exit() must not be a macro

[Warlich, Christof]

> I'll ask, why would you ever want to pass a macro to module_init()?
> 
> We don't like functions to be macros in the kernel, do you have a
> real-world need for this somewhere?  If so, can you show the code?

Yes, certainly: As I said in my first post, I'd like to provide a driver template for a certain type of driver.
To reduce the number of changes when using that template to a minimum, that template looks similar to
something like this:

#define DRIVER_NAME driver_template // Change to your driver's name.

#define _CONCAT(x, y) x##y
#define CONCAT(x, y) _CONCAT(x, y)
#define _STRINGIFY(x) #x
#define STRINGIFY(x) _STRINGIFY(x)

static int CONCAT(DRIVER_NAME, _open)(struct inode *inode, struct file *fp)
{
	...
}

static int CONCAT(DRIVER_NAME, _release)(struct inode *inode, struct file *fp)
{
	...
}

static long CONCAT(DRIVER_NAME, _ioctl)(struct file *fp, unsigned int cmd, unsigned long value)
{
	...
}

static struct file_operations fops = {
	.owner = THIS_MODULE,
	.open = CONCAT(DRIVER_NAME, _open),
	.release = CONCAT(DRIVER_NAME, _release),
	.unlocked_ioctl = CONCAT(DRIVER_NAME, _ioctl),
};

static int __init CONCAT(DRIVER_NAME, _init)(void)
{
	...
}

static void __exit CONCAT(DRIVER_NAME, _exit)(void)
{
	...
}

module_init(CONCAT(DRIVER_NAME, _init));
module_exit(CONCAT(DRIVER_NAME, _exit));

This way, someone using that template to start writing a new driver just needs to change the definition of DRIVER_NAME (besides adding device specific functionality).
Oh, and I'm well aware that all those functions are static, so that no name conflicts would occur if all drivers derived from that template would simply all use the same names,
but doing so may make debugging harder, I'd think.

parameter of module_init() and module_exit() must not be a macro

[Greg KH]

On Thu, Oct 15, 2015 at 02:26:41PM +0000, Warlich, Christof wrote:
> 
> > I'll ask, why would you ever want to pass a macro to module_init()?
> > 
> > We don't like functions to be macros in the kernel, do you have a
> > real-world need for this somewhere?  If so, can you show the code?
> 
> Yes, certainly: As I said in my first post, I'd like to provide a driver template for a certain type of driver.
> To reduce the number of changes when using that template to a minimum, that template looks similar to
> something like this:
> 
> #define DRIVER_NAME driver_template // Change to your driver's name.
> 
> #define _CONCAT(x, y) x##y
> #define CONCAT(x, y) _CONCAT(x, y)
> #define _STRINGIFY(x) #x
> #define STRINGIFY(x) _STRINGIFY(x)
> 
> static int CONCAT(DRIVER_NAME, _open)(struct inode *inode, struct file *fp)
> {
> 	...
> }
> 
> static int CONCAT(DRIVER_NAME, _release)(struct inode *inode, struct file *fp)
> {
> 	...
> }
> 
> static long CONCAT(DRIVER_NAME, _ioctl)(struct file *fp, unsigned int cmd, unsigned long value)
> {
> 	...
> }
> 
> static struct file_operations fops = {
> 	.owner = THIS_MODULE,
> 	.open = CONCAT(DRIVER_NAME, _open),
> 	.release = CONCAT(DRIVER_NAME, _release),
> 	.unlocked_ioctl = CONCAT(DRIVER_NAME, _ioctl),
> };
> 
> static int __init CONCAT(DRIVER_NAME, _init)(void)
> {
> 	...
> }
> 
> static void __exit CONCAT(DRIVER_NAME, _exit)(void)
> {
> 	...
> }
> 
> module_init(CONCAT(DRIVER_NAME, _init));
> module_exit(CONCAT(DRIVER_NAME, _exit));
> 
> This way, someone using that template to start writing a new driver
> just needs to change the definition of DRIVER_NAME (besides adding
> device specific functionality).
> Oh, and I'm well aware that all those functions are static, so that no
> name conflicts would occur if all drivers derived from that template
> would simply all use the same names,
> but doing so may make debugging harder, I'd think.

Ick, no, don't do that, you will make life much harder for everyone
involved in the end.  Just write out the code "for real", it's trivial
to do so, and you aren't really saving anyone anytime as nothing like
this would ever be acceptable to the upstream kernel developers.

Really, you aren't saving any time/energy here, the "template" code is
the easiest part to write, it's what is in those functions that really
matters.

thanks,

greg k-h

AW: parameter of module_init() and module_exit() must not be a macro

[Warlich, Christof]

> Ick, no, don't do that, you will make life much harder for everyone
> involved in the end.  Just write out the code "for real", it's trivial
> to do so, and you aren't really saving anyone anytime as nothing like
> this would ever be acceptable to the upstream kernel developers.
> 
> Really, you aren't saving any time/energy here, the "template" code is
> the easiest part to write, it's what is in those functions that really
> matters.

Ok, agreed, the CONCAT() construct looks a bit strange at first sight, although
things could further be simplified, e.g.:

#define _CONCAT(x, y) x##y
#define CONCAT(x, y) _CONCAT(x, y)
#define _STRINGIFY(x) #x
#define STRINGIFY(x) _STRINGIFY(x)
#define DRIVER_NAMESPACE(x) CONCAT(CONCAT(DRIVER_NAME, _), x)

...

static void __exit DRIVER_NAMESPACE(exit)(void)
{
	...
}

module_init(DRIVER_NAMESPACE(init));
module_exit(DRIVER_NAMESPACE(exit));

But anyhow, as you are most probably right that this would not be accepted
upstream, there is not much point in arguing further whether my example is a
good idea.

I'd still like to make a point if the current implementation of the module_init()
and module_exit() macros is _correct_ though. With the following code snippet:

#define FOO BAR
module_init(FOO);

the current implementation of the module_init() macro would expand to

	static inline initcall_t __inittest(void)		\
	{ return BAR; }					\
	int init_module(void) __attribute__((alias("FOO")));

i.e. we see both FOO and BAR in the expanded code, which is rather unexpected.
The patch that I've suggested would fix that, as the macro would then expand to:

	static inline initcall_t __inittest(void)		\
	{ return BAR; }					\
	int init_module(void) __attribute__((alias("BAR")));

Thus, even when not being able to give an acceptable real world example, my
proposed patch would still enforce the principle of "least surprise". Just consider
this similar to coding standardization patches: It would just improve overall
code quality.

So my initial question remains: How are the chances to get my (code quality
improvement) patch upstream?

Oh, and by the way: Many thanks for taking your precious time answering :-).

parameter of module_init() and module_exit() must not be a macro

[Greg KH]

On Fri, Oct 16, 2015 at 06:40:40AM +0000, Warlich, Christof wrote:
> > Ick, no, don't do that, you will make life much harder for everyone
> > involved in the end.  Just write out the code "for real", it's trivial
> > to do so, and you aren't really saving anyone anytime as nothing like
> > this would ever be acceptable to the upstream kernel developers.
> > 
> > Really, you aren't saving any time/energy here, the "template" code is
> > the easiest part to write, it's what is in those functions that really
> > matters.
> 
> Ok, agreed, the CONCAT() construct looks a bit strange at first sight, although
> things could further be simplified, e.g.:
> 
> #define _CONCAT(x, y) x##y
> #define CONCAT(x, y) _CONCAT(x, y)
> #define _STRINGIFY(x) #x
> #define STRINGIFY(x) _STRINGIFY(x)
> #define DRIVER_NAMESPACE(x) CONCAT(CONCAT(DRIVER_NAME, _), x)
> 
> ...
> 
> static void __exit DRIVER_NAMESPACE(exit)(void)
> {
> 	...
> }
> 
> module_init(DRIVER_NAMESPACE(init));
> module_exit(DRIVER_NAMESPACE(exit));
> 
> But anyhow, as you are most probably right that this would not be accepted
> upstream, there is not much point in arguing further whether my example is a
> good idea.
> 
> I'd still like to make a point if the current implementation of the module_init()
> and module_exit() macros is _correct_ though. With the following code snippet:
> 
> #define FOO BAR
> module_init(FOO);
> 
> the current implementation of the module_init() macro would expand to
> 
> 	static inline initcall_t __inittest(void)		\
> 	{ return BAR; }					\
> 	int init_module(void) __attribute__((alias("FOO")));
> 
> i.e. we see both FOO and BAR in the expanded code, which is rather unexpected.
> The patch that I've suggested would fix that, as the macro would then expand to:
> 
> 	static inline initcall_t __inittest(void)		\
> 	{ return BAR; }					\
> 	int init_module(void) __attribute__((alias("BAR")));
> 
> Thus, even when not being able to give an acceptable real world example, my
> proposed patch would still enforce the principle of "least surprise". Just consider
> this similar to coding standardization patches: It would just improve overall
> code quality.
> 
> So my initial question remains: How are the chances to get my (code quality
> improvement) patch upstream?

As it doesn't fix an in-kernel issues, I doubt it's worth the effort,
but I'm not the maintainer of this portion of the kernel, so I do not
know if it will be accepted or not, sorry.

good luck!

greg k-h