| Message ID | 20180404121347.3089169-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 87248d31d1055b56e01a62d9320b4e118bc84e0e |
| Headers | show |
| Series | netdevsim: remove incorrect __net_initdata annotations | expand |
On 4/4/18 6:12 AM, Arnd Bergmann wrote: > The __net_initdata section cannot currently be used for structures that > get cleaned up in an exitcall using unregister_pernet_operations: > > WARNING: vmlinux.o(.text+0x868c34): Section mismatch in reference from the function nsim_devlink_exit() to the (unknown reference) .init.data:(unknown) > The function nsim_devlink_exit() references > the (unknown reference) __initdata (unknown). > This is often because nsim_devlink_exit lacks a __initdata > annotation or the annotation of (unknown) is wrong. > WARNING: vmlinux.o(.text+0x868c64): Section mismatch in reference from the function nsim_devlink_init() to the (unknown reference) .init.data:(unknown) > WARNING: vmlinux.o(.text+0x8692bc): Section mismatch in reference from the function nsim_fib_exit() to the (unknown reference) .init.data:(unknown) > WARNING: vmlinux.o(.text+0x869300): Section mismatch in reference from the function nsim_fib_init() to the (unknown reference) .init.data:(unknown) > > As that warning tells us, discarding the structure after a module is > loaded would lead to a undefined behavior when that module is removed. > > It might be possible to change that annotation so it has no effect for > loadable modules, but I have not figured out exactly how to do that, and > we want this to be fixed in -rc1. > > This just removes the annotations, just like we do for all other such > modules. > > Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/netdevsim/devlink.c | 2 +- > drivers/net/netdevsim/fib.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c > index 27ae05c5fdaf..1dba47936456 100644 > --- a/drivers/net/netdevsim/devlink.c > +++ b/drivers/net/netdevsim/devlink.c > @@ -267,7 +267,7 @@ static int __net_init nsim_devlink_netns_init(struct net *net) > return 0; > } > > -static struct pernet_operations nsim_devlink_net_ops __net_initdata = { > +static struct pernet_operations nsim_devlink_net_ops = { > .init = nsim_devlink_netns_init, > .id = &nsim_devlink_id, > .size = sizeof(bool), > diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c > index 0d105bafa261..9bfe9e151e13 100644 > --- a/drivers/net/netdevsim/fib.c > +++ b/drivers/net/netdevsim/fib.c > @@ -230,7 +230,7 @@ static int __net_init nsim_fib_netns_init(struct net *net) > return 0; > } > > -static struct pernet_operations nsim_fib_net_ops __net_initdata = { > +static struct pernet_operations nsim_fib_net_ops = { > .init = nsim_fib_netns_init, > .id = &nsim_fib_net_id, > .size = sizeof(struct nsim_fib_data), > I am confused ... I do the same thing in the VRF driver and that code has been there since June 2017.
On Wed, Apr 4, 2018 at 5:33 PM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 4/4/18 6:12 AM, Arnd Bergmann wrote: >> The __net_initdata section cannot currently be used for structures that >> get cleaned up in an exitcall using unregister_pernet_operations: >> > > I am confused ... I do the same thing in the VRF driver and that code > has been there since June 2017. I don't see any exit call in that driver, the only caller of unregister_pernet_operations() there is in the init function itself, and the module cannot be removed. Arnd
From: Arnd Bergmann <arnd@arndb.de> Date: Wed, 4 Apr 2018 14:12:39 +0200 > The __net_initdata section cannot currently be used for structures that > get cleaned up in an exitcall using unregister_pernet_operations: > > WARNING: vmlinux.o(.text+0x868c34): Section mismatch in reference from the function nsim_devlink_exit() to the (unknown reference) .init.data:(unknown) > The function nsim_devlink_exit() references > the (unknown reference) __initdata (unknown). > This is often because nsim_devlink_exit lacks a __initdata > annotation or the annotation of (unknown) is wrong. > WARNING: vmlinux.o(.text+0x868c64): Section mismatch in reference from the function nsim_devlink_init() to the (unknown reference) .init.data:(unknown) > WARNING: vmlinux.o(.text+0x8692bc): Section mismatch in reference from the function nsim_fib_exit() to the (unknown reference) .init.data:(unknown) > WARNING: vmlinux.o(.text+0x869300): Section mismatch in reference from the function nsim_fib_init() to the (unknown reference) .init.data:(unknown) > > As that warning tells us, discarding the structure after a module is > loaded would lead to a undefined behavior when that module is removed. > > It might be possible to change that annotation so it has no effect for > loadable modules, but I have not figured out exactly how to do that, and > we want this to be fixed in -rc1. > > This just removes the annotations, just like we do for all other such > modules. > > Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Hmmm... __net_initdata defines to nothing if network namespaces are enabled. That's the whole point. Therefore, if NETNS is disabled, "unregister_pernet_operations" cannot happen and this is the only time when __net_initdata actually does something.
On Wed, Apr 4, 2018 at 5:52 PM, David Miller <davem@davemloft.net> wrote: > From: Arnd Bergmann <arnd@arndb.de> > Date: Wed, 4 Apr 2018 14:12:39 +0200 > >> The __net_initdata section cannot currently be used for structures that >> get cleaned up in an exitcall using unregister_pernet_operations: >> >> WARNING: vmlinux.o(.text+0x868c34): Section mismatch in reference from the function nsim_devlink_exit() to the (unknown reference) .init.data:(unknown) >> The function nsim_devlink_exit() references >> the (unknown reference) __initdata (unknown). >> This is often because nsim_devlink_exit lacks a __initdata >> annotation or the annotation of (unknown) is wrong. >> WARNING: vmlinux.o(.text+0x868c64): Section mismatch in reference from the function nsim_devlink_init() to the (unknown reference) .init.data:(unknown) >> WARNING: vmlinux.o(.text+0x8692bc): Section mismatch in reference from the function nsim_fib_exit() to the (unknown reference) .init.data:(unknown) >> WARNING: vmlinux.o(.text+0x869300): Section mismatch in reference from the function nsim_fib_init() to the (unknown reference) .init.data:(unknown) >> >> As that warning tells us, discarding the structure after a module is >> loaded would lead to a undefined behavior when that module is removed. >> >> It might be possible to change that annotation so it has no effect for >> loadable modules, but I have not figured out exactly how to do that, and >> we want this to be fixed in -rc1. >> >> This just removes the annotations, just like we do for all other such >> modules. >> >> Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Hmmm... > > __net_initdata defines to nothing if network namespaces are enabled. > > That's the whole point. > > Therefore, if NETNS is disabled, "unregister_pernet_operations" cannot > happen and this is the only time when __net_initdata actually does > something. The problem happens with CONFIG_NETNS=n: __net_initdata turns into __initdata, and nsim_fib_net_ops is referenced from a function that is not marked __init (i.e. nsim_fib_exit). The logic to turn __net_initdata into __init only makes sense for subsystems that have no way to be unregistered, i.e. code which is always built-in, or non-unloadable modules. Arnd
From: Arnd Bergmann <arnd@arndb.de> Date: Wed, 4 Apr 2018 18:03:41 +0200 > On Wed, Apr 4, 2018 at 5:52 PM, David Miller <davem@davemloft.net> wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> Date: Wed, 4 Apr 2018 14:12:39 +0200 >> >>> The __net_initdata section cannot currently be used for structures that >>> get cleaned up in an exitcall using unregister_pernet_operations: >>> >>> WARNING: vmlinux.o(.text+0x868c34): Section mismatch in reference from the function nsim_devlink_exit() to the (unknown reference) .init.data:(unknown) >>> The function nsim_devlink_exit() references >>> the (unknown reference) __initdata (unknown). >>> This is often because nsim_devlink_exit lacks a __initdata >>> annotation or the annotation of (unknown) is wrong. >>> WARNING: vmlinux.o(.text+0x868c64): Section mismatch in reference from the function nsim_devlink_init() to the (unknown reference) .init.data:(unknown) >>> WARNING: vmlinux.o(.text+0x8692bc): Section mismatch in reference from the function nsim_fib_exit() to the (unknown reference) .init.data:(unknown) >>> WARNING: vmlinux.o(.text+0x869300): Section mismatch in reference from the function nsim_fib_init() to the (unknown reference) .init.data:(unknown) >>> >>> As that warning tells us, discarding the structure after a module is >>> loaded would lead to a undefined behavior when that module is removed. >>> >>> It might be possible to change that annotation so it has no effect for >>> loadable modules, but I have not figured out exactly how to do that, and >>> we want this to be fixed in -rc1. >>> >>> This just removes the annotations, just like we do for all other such >>> modules. >>> >>> Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> Hmmm... >> >> __net_initdata defines to nothing if network namespaces are enabled. >> >> That's the whole point. >> >> Therefore, if NETNS is disabled, "unregister_pernet_operations" cannot >> happen and this is the only time when __net_initdata actually does >> something. > > The problem happens with CONFIG_NETNS=n: __net_initdata turns into > __initdata, and nsim_fib_net_ops is referenced from a function that is > not marked __init (i.e. nsim_fib_exit). > > The logic to turn __net_initdata into __init only makes sense for > subsystems that have no way to be unregistered, i.e. code which is > always built-in, or non-unloadable modules. Ok, I see it. I'll apply this for now, thanks.
diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c index 27ae05c5fdaf..1dba47936456 100644 --- a/drivers/net/netdevsim/devlink.c +++ b/drivers/net/netdevsim/devlink.c @@ -267,7 +267,7 @@ static int __net_init nsim_devlink_netns_init(struct net *net) return 0; } -static struct pernet_operations nsim_devlink_net_ops __net_initdata = { +static struct pernet_operations nsim_devlink_net_ops = { .init = nsim_devlink_netns_init, .id = &nsim_devlink_id, .size = sizeof(bool), diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c index 0d105bafa261..9bfe9e151e13 100644 --- a/drivers/net/netdevsim/fib.c +++ b/drivers/net/netdevsim/fib.c @@ -230,7 +230,7 @@ static int __net_init nsim_fib_netns_init(struct net *net) return 0; } -static struct pernet_operations nsim_fib_net_ops __net_initdata = { +static struct pernet_operations nsim_fib_net_ops = { .init = nsim_fib_netns_init, .id = &nsim_fib_net_id, .size = sizeof(struct nsim_fib_data),
The __net_initdata section cannot currently be used for structures that get cleaned up in an exitcall using unregister_pernet_operations: WARNING: vmlinux.o(.text+0x868c34): Section mismatch in reference from the function nsim_devlink_exit() to the (unknown reference) .init.data:(unknown) The function nsim_devlink_exit() references the (unknown reference) __initdata (unknown). This is often because nsim_devlink_exit lacks a __initdata annotation or the annotation of (unknown) is wrong. WARNING: vmlinux.o(.text+0x868c64): Section mismatch in reference from the function nsim_devlink_init() to the (unknown reference) .init.data:(unknown) WARNING: vmlinux.o(.text+0x8692bc): Section mismatch in reference from the function nsim_fib_exit() to the (unknown reference) .init.data:(unknown) WARNING: vmlinux.o(.text+0x869300): Section mismatch in reference from the function nsim_fib_init() to the (unknown reference) .init.data:(unknown) As that warning tells us, discarding the structure after a module is loaded would lead to a undefined behavior when that module is removed. It might be possible to change that annotation so it has no effect for loadable modules, but I have not figured out exactly how to do that, and we want this to be fixed in -rc1. This just removes the annotations, just like we do for all other such modules. Fixes: 37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/net/netdevsim/devlink.c | 2 +- drivers/net/netdevsim/fib.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.9.0