2018-04-04 12:15:27

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] netdevsim: remove incorrect __net_initdata annotations

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 <[email protected]>
---
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



2018-04-04 15:36:05

by David Ahern

[permalink] [raw]
Subject: Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

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 <[email protected]>
> ---
> 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.


2018-04-04 15:43:36

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

On Wed, Apr 4, 2018 at 5:33 PM, David Ahern <[email protected]> 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

2018-04-04 15:53:36

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

From: Arnd Bergmann <[email protected]>
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 <[email protected]>

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.

2018-04-04 16:05:36

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

On Wed, Apr 4, 2018 at 5:52 PM, David Miller <[email protected]> wrote:
> From: Arnd Bergmann <[email protected]>
> 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 <[email protected]>
>
> 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

2018-04-04 16:58:10

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] netdevsim: remove incorrect __net_initdata annotations

From: Arnd Bergmann <[email protected]>
Date: Wed, 4 Apr 2018 18:03:41 +0200

> On Wed, Apr 4, 2018 at 5:52 PM, David Miller <[email protected]> wrote:
>> From: Arnd Bergmann <[email protected]>
>> 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 <[email protected]>
>>
>> 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.