[PATCH] netdevsim: remove incorrect __net_initdata annotations

[PATCH] netdevsim: remove incorrect __net_initdata annotations

[Arnd Bergmann]

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),
-- 
2.9.0

Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

[David Ahern]

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.

Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

[Arnd Bergmann]

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

Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

[David Miller]

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.

Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

[Arnd Bergmann]

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

Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

[David Miller]

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.