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

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

[Miguel Ojeda]

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/20190123173707.GA16603@gmail.com/
Suggested-by: Martin Sebor <msebor@gcc.gnu.org>
Tested-by: Laura Abbott <labbott@redhat.com>
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
The new version I pushed for -next.

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

diff --git a/include/linux/module.h b/include/linux/module.h
index 8fa38d3e7538..1b5e370f1bc0 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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
 
-- 
2.17.1

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

[Miguel Ojeda]

On Wed, Feb 6, 2019 at 6:56 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 8fa38d3e7538..1b5e370f1bc0 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -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)));

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

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

[Jessica Yu]

+++ Miguel Ojeda [06/02/19 22:19 +0100]:
>On Wed, Feb 6, 2019 at 6:56 PM Miguel Ojeda
><miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 8fa38d3e7538..1b5e370f1bc0 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -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)));
>
>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.

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

>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).

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

Thanks Miguel!

Jessica

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

[Miguel Ojeda]

On Thu, Feb 7, 2019 at 11:54 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> 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