include/linux/module.h: mark init/cleanup_module aliases as __init/exit

PATCH In review Miguel Ojeda 2 participants 4 messages Feb 6, 2019 – Feb 8, 2019 Module support

Share & tools

Miguel Ojeda

author

2019-02-06 17:56 UTC #0 lore raw

PATCH v2 1/1 include/linux/module.h: mark init/cleanup_module aliases as __init/exit

The upcoming GCC 9 release extends the -Wmissing-attributes warnings (enabled by -Wall) to C and aliases: it warns when particular function attributes are missing in the aliases but not in their target.

In particular, it triggers for all the init/cleanup_module aliases in the kernel (defined by the module_init/exit macros), ending up being very noisy.

These aliases point to the __init/__exit functions of a module, which are defined as __cold (among other attributes). However, the aliases themselves do not have the __cold attribute.

Since the compiler behaves differently when compiling a __cold function as well as when compiling paths leading to calls to __cold functions, the warning is trying to point out the possibly-forgotten attribute in the alias.

In order to keep the warning enabled, we choose to silence the warning by marking the aliases as __init/__exit.

Note that the warning would go away marking either the extern declaration, the definition, or both. However, we only mark the definition of the alias, since we do not want callers (which only see the declaration) to be compiled as if the function was __cold (and therefore the paths leading to those calls would be assumed to be unlikely).

Link https://lore.kernel.org/lkml/[email protected]/ open here

Tested-by Laura Abbott <[email protected]>

Signed-off-by Miguel Ojeda <[email protected]>

---
The new version I pushed for -next.

 include/linux/module.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/linux/module.h +2 -2

index 8fa38d3e7538..1b5e370f1bc0 100644
@@ -129,13 +129,13 @@ extern void cleanup_module(void);
 #define module_init(initfn)					\
 	static inline initcall_t __maybe_unused __inittest(void)		\
 	{ return initfn; }					\
-	int init_module(void) __attribute__((alias(#initfn)));
+	int init_module(void) __init __attribute__((alias(#initfn)));
 
 /* This is only required if you want to be unloadable. */
 #define module_exit(exitfn)					\
 	static inline exitcall_t __maybe_unused __exittest(void)		\
 	{ return exitfn; }					\
-	void cleanup_module(void) __attribute__((alias(#exitfn)));
+	void cleanup_module(void) __exit __attribute__((alias(#exitfn)));
 
 #endif

Signature · 1 line

2.17.1

Miguel Ojeda

author

2019-02-06 21:19 UTC #1 lore raw

Quoted 8 lines · Miguel Ojeda

include/linux/module.h +0 -0

index 8fa38d3e7538..1b5e370f1bc0 100644
@@ -129,13 +129,13 @@ extern void cleanup_module(void);
 #define module_init(initfn)                                    \
        static inline initcall_t __maybe_unused __inittest(void)                \

include/linux/module.h extern void cleanup_module(void); · line 138

Quoted 10 lines

include/linux/module.h +2 -2

        { return initfn; }                                      \
-       int init_module(void) __attribute__((alias(#initfn)));
+       int init_module(void) __init __attribute__((alias(#initfn)));
 
 /* This is only required if you want to be unloadable. */
 #define module_exit(exitfn)                                    \
        static inline exitcall_t __maybe_unused __exittest(void)                \
        { return exitfn; }                                      \
-       void cleanup_module(void) __attribute__((alias(#exitfn)));
+       void cleanup_module(void) __exit __attribute__((alias(#exitfn)));

It turns out that there are some modules without __init/__exit marked functions, which GCC complains about, since now the alias is in a different section than the target:

* In some cases, this is due to a missing __init/__exit marking
(e.g. drivers/connector/connector.c). These should be fixed in any case.
* In other cases, the cleanup function is not marked as such because
it is called from another place in the TU, like the init function (e.g. arch/x86/kvm/vmx.c). We would need to create an actual cleanup function (marked as __exit) that simply calls the current exit static function.

So we have a few alternatives:

1) Going only with __cold.
2) Using the new __copy attribute (because then they are copied only
in the cases they are actually used).
3) Fix and go for __init/__exit. While this requires some tweaking
as explained above, it would be good if we can achieve it since then we are enforcing proper __init/__exit markings for all modules (whereas __copy wouldn't spot).

I think it is worth achieving 3), but that will take a bit more of time. In that case, I suggest we push 1) or 2) for the moment (so that the warning is fixed) and then work on fixing all instances. As soon as that is done, we can push 3).

What do you think?

Cheers,
Miguel

Jessica Yu

2019-02-07 10:54 UTC #2 lore raw

+++ Miguel Ojeda [06/02/19 22:19 +0100]:

Quoted 25 lines

Quoted 8 lines · Miguel Ojeda

include/linux/module.h +0 -0

index 8fa38d3e7538..1b5e370f1bc0 100644
@@ -129,13 +129,13 @@ extern void cleanup_module(void);
 #define module_init(initfn)                                    \
        static inline initcall_t __maybe_unused __inittest(void)                \

include/linux/module.h extern void cleanup_module(void); · line 138

Quoted 10 lines

include/linux/module.h +2 -2

        { return initfn; }                                      \
-       int init_module(void) __attribute__((alias(#initfn)));
+       int init_module(void) __init __attribute__((alias(#initfn)));
 
 /* This is only required if you want to be unloadable. */
 #define module_exit(exitfn)                                    \
        static inline exitcall_t __maybe_unused __exittest(void)                \
        { return exitfn; }                                      \
-       void cleanup_module(void) __attribute__((alias(#exitfn)));
+       void cleanup_module(void) __exit __attribute__((alias(#exitfn)));

It turns out that there are some modules without __init/__exit marked functions, which GCC complains about, since now the alias is in a different section than the target:

Quoted 8 lines

* In some cases, this is due to a missing __init/__exit marking
(e.g. drivers/connector/connector.c). These should be fixed in any case.
* In other cases, the cleanup function is not marked as such because
it is called from another place in the TU, like the init function (e.g. arch/x86/kvm/vmx.c). We would need to create an actual cleanup function (marked as __exit) that simply calls the current exit static function.

Grr, I guess it was not so simple after all. :)

Quoted 10 lines

So we have a few alternatives:
1) Going only with __cold.
2) Using the new __copy attribute (because then they are copied only
in the cases they are actually used).
3) Fix and go for __init/__exit. While this requires some tweaking
as explained above, it would be good if we can achieve it since then we are enforcing proper __init/__exit markings for all modules (whereas __copy wouldn't spot).

Quoted 4 lines

I think it is worth achieving 3), but that will take a bit more of time. In that case, I suggest we push 1) or 2) for the moment (so that the warning is fixed) and then work on fixing all instances. As soon as that is done, we can push 3).

My order of preference would be 2, 1, striving for 3 eventually.

Thanks Miguel!

Jessica

Miguel Ojeda

author

2019-02-08 23:32 UTC #3 lore raw

Quoted 2 lines · Jessica Yu

My order of preference would be 2, 1, striving for 3 eventually.

For reference, for 3), we would need to tweak the following ~130 modules [1]. Quite a lot of them are the simple kind. Also, if we end up with 3), we could potentially remove a few modpost checks (e.g. ANY_INIT_TO_ANY_EXIT).

I am submitting the patch for 2). It requires introducing the new attribute, so let's see if people like it.

Cheers,
Miguel

[1]

arch/x86/kvm/vmx/vmx.c crypto/async_tx/raid6test.c
crypto/ecdh.c
drivers/block/drbd/drbd_main.c drivers/bluetooth/btrsi.c drivers/char/ipmi/ipmi_si_intf.c drivers/char/ipmi/ipmi_ssif.c
drivers/char/lp.c
drivers/char/mwave/mwavedd.c drivers/connector/connector.c drivers/cpufreq/powernow-k8.c drivers/crypto/ccp/ccp-crypto-main.c drivers/dma/ti/edma.c
drivers/dma/ti/omap-dma.c drivers/firmware/efi/efivars.c drivers/firmware/google/memconsole-coreboot.c drivers/fmc/fmc-chardev.c
drivers/fmc/fmc-core.c
drivers/fmc/fmc-fakedev.c drivers/fmc/fmc-trivial.c drivers/fmc/fmc-write-eeprom.c drivers/fsi/fsi-core.c
drivers/fsi/fsi-occ.c
drivers/fsi/fsi-sbefifo.c
drivers/fsi/fsi-scom.c
drivers/gpu/drm/drm_drv.c
drivers/hv/hv_util.c drivers/hwtracing/stm/console.c drivers/hwtracing/stm/dummy_stm.c drivers/hwtracing/stm/heartbeat.c drivers/hwtracing/stm/p_basic.c drivers/hwtracing/stm/p_sys-t.c drivers/infiniband/sw/rdmavt/vt.c drivers/infiniband/ulp/opa_vnic/opa_vnic_vema.c drivers/isdn/mISDN/core.c drivers/isdn/mISDN/l1oip_core.c drivers/lightnvm/pblk-init.c drivers/mcb/mcb-core.c
drivers/md/bcache/super.c drivers/md/dm-integrity.c
drivers/md/dm-thin.c
drivers/md/md-cluster.c
drivers/md/md-faulty.c
drivers/md/md-linear.c
drivers/md/raid0.c
drivers/md/raid1.c
drivers/md/raid10.c
drivers/md/raid5.c drivers/media/common/b2c2/flexcop.c drivers/media/pci/cx25821/cx25821-alsa.c drivers/media/pci/ivtv/ivtvfb.c drivers/media/pci/saa7134/saa7134-alsa.c drivers/media/platform/davinci/vpif.c drivers/media/platform/davinci/vpss.c drivers/media/platform/exynos4-is/fimc-is.c drivers/media/platform/omap/omap_vout.c drivers/message/fusion/mptctl.c drivers/mtd/devices/block2mtd.c drivers/mtd/maps/sbc_gxx.c drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c drivers/net/ethernet/microchip/encx24j600.c drivers/net/fddi/defxx.c drivers/net/ipvlan/ipvtap.c drivers/net/macvtap.c
drivers/net/phy/sfp.c
drivers/net/tun.c drivers/net/vmxnet3/vmxnet3_drv.c drivers/net/wireless/marvell/mwifiex/main.c drivers/net/wireless/rsi/rsi_91x_main.c drivers/net/wireless/rsi/rsi_91x_sdio.c drivers/nfc/nfcmrvl/uart.c drivers/pcmcia/i82092.c drivers/platform/x86/classmate-laptop.c drivers/platform/x86/thinkpad_acpi.c drivers/rpmsg/rpmsg_char.c drivers/scsi/aic7xxx/aic7xxx_osm.c drivers/scsi/arcmsr/arcmsr_hba.c drivers/soc/fsl/dpio/dpio-driver.c drivers/staging/gasket/apex_driver.c drivers/staging/greybus/fw-core.c drivers/staging/greybus/loopback.c drivers/staging/greybus/raw.c drivers/staging/greybus/uart.c drivers/staging/unisys/visorhba/visorhba_main.c drivers/staging/unisys/visornic/visornic_main.c drivers/tty/rocket.c
drivers/tty/serial/kgdboc.c drivers/tty/serial/owl-uart.c drivers/usb/gadget/function/f_tcm.c drivers/usb/gadget/function/u_serial.c drivers/vhost/net.c
drivers/vhost/scsi.c drivers/video/fbdev/aty/aty128fb.c drivers/video/fbdev/geode/gx1fb_core.c drivers/video/fbdev/gxt4500.c drivers/video/fbdev/matrox/matroxfb_crtc2.c drivers/video/fbdev/mb862xx/mb862xxfbdrv.c drivers/video/fbdev/nvidia/nvidia.c drivers/video/fbdev/riva/fbdev.c drivers/video/fbdev/sstfb.c drivers/video/fbdev/uvesafb.c drivers/virtio/virtio.c drivers/watchdog/cpu5wdt.c kernel/backtracetest.c kernel/locking/locktorture.c kernel/rcu/rcuperf.c
kernel/rcu/rcutorture.c
lib/interval_tree_test.c lib/memory-notifier-error-inject.c lib/netdev-notifier-error-inject.c lib/of-reconfig-notifier-error-inject.c lib/pm-notifier-error-inject.c
lib/raid6/algos.c
lib/test_ida.c
lib/test_xarray.c
mm/hwpoison-inject.c
net/9p/trans_xen.c
net/core/drop_monitor.c
net/l2tp/l2tp_netlink.c net/netfilter/xt_NETMAP.c net/openvswitch/datapath.c
net/rds/af_rds.c
net/rds/rdma_transport.c
net/rds/tcp.c
samples/connector/cn_test.c samples/livepatch/livepatch-callbacks-busymod.c samples/livepatch/livepatch-callbacks-demo.c samples/livepatch/livepatch-callbacks-mod.c samples/livepatch/livepatch-sample.c samples/livepatch/livepatch-shadow-fix1.c samples/livepatch/livepatch-shadow-fix2.c samples/livepatch/livepatch-shadow-mod.c