[PATCH] adjust __net_exit

[PATCH] adjust __net_exit

[Jan Beulich]

__net_exit, judging by the majority of its uses, was intended to serve
as an abstraction to allow calling such annotated functions from both
__init and __exit functions. Using the (bogus and unused elsewhere)
__exit_refok to implement this is inefficient - any non-modular code
really can reside in __init (as non-modular __exit code is never used).

Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text)
in modules, and __init in the core kernel.

A few other adjustments are necessary/possible with this done - those
were likely just oversights when added originally.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 include/net/net_namespace.h    |    6 +++++-
 net/ipv4/fib_frontend.c        |    2 +-
 net/netfilter/ipvs/ip_vs_ctl.c |    8 ++++----
 3 files changed, 10 insertions(+), 6 deletions(-)

--- 3.3-rc6/include/net/net_namespace.h
+++ 3.3-rc6-net-exit/include/net/net_namespace.h
@@ -242,7 +242,11 @@ static inline struct net *read_pnet(stru
 #define __net_initdata
 #else
 #define __net_init	__init
-#define __net_exit	__exit_refok
+#ifdef MODULE
+#define __net_exit
+#else
+#define __net_exit	__init
+#endif
 #define __net_initdata	__initdata
 #endif
 
--- 3.3-rc6/net/ipv4/fib_frontend.c
+++ 3.3-rc6-net-exit/net/ipv4/fib_frontend.c
@@ -1062,7 +1062,7 @@ fail:
 	return err;
 }
 
-static void ip_fib_net_exit(struct net *net)
+static void __net_exit ip_fib_net_exit(struct net *net)
 {
 	unsigned int i;
 
--- 3.3-rc6/net/netfilter/ipvs/ip_vs_ctl.c
+++ 3.3-rc6-net-exit/net/netfilter/ipvs/ip_vs_ctl.c
@@ -3621,7 +3621,7 @@ static void ip_vs_genl_unregister(void)
  * per netns intit/exit func.
  */
 #ifdef CONFIG_SYSCTL
-int __net_init ip_vs_control_net_init_sysctl(struct net *net)
+static int __net_init ip_vs_control_net_init_sysctl(struct net *net)
 {
 	int idx;
 	struct netns_ipvs *ipvs = net_ipvs(net);
@@ -3680,7 +3680,7 @@ int __net_init ip_vs_control_net_init_sy
 	return 0;
 }
 
-void __net_init ip_vs_control_net_cleanup_sysctl(struct net *net)
+static void __net_exit ip_vs_control_net_cleanup_sysctl(struct net *net)
 {
 	struct netns_ipvs *ipvs = net_ipvs(net);
 
@@ -3691,8 +3691,8 @@ void __net_init ip_vs_control_net_cleanu
 
 #else
 
-int __net_init ip_vs_control_net_init_sysctl(struct net *net) { return 0; }
-void __net_init ip_vs_control_net_cleanup_sysctl(struct net *net) { }
+static int __net_init ip_vs_control_net_init_sysctl(struct net *net) { return 0; }
+static void __net_exit ip_vs_control_net_cleanup_sysctl(struct net *net) { }
 
 #endif
 

Re: [PATCH] adjust __net_exit

[David Miller]

From: "Jan Beulich" <JBeulich@suse.com>
Date: Thu, 08 Mar 2012 09:37:32 +0000

> __net_exit, judging by the majority of its uses, was intended to serve
> as an abstraction to allow calling such annotated functions from both
> __init and __exit functions. Using the (bogus and unused elsewhere)
> __exit_refok to implement this is inefficient - any non-modular code
> really can reside in __init (as non-modular __exit code is never used).
> 
> Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text)
> in modules, and __init in the core kernel.
> 
> A few other adjustments are necessary/possible with this done - those
> were likely just oversights when added originally.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

[ I have been waiting for more than a week for a netns developer
  to review this patch, I guess I'm too optimistic these days. :-( ]

The only reason you think __exit_refok is "bogus" is because it's
semantics got changed by Sam Ravnborg in commit
312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section
reference annotations tags: __ref, __refdata, __refconst")

Beforehand the __exit_refok was a real .exit section, so it got
completely discarded AT LINK TIME.  Now it sits together with
__init_refok which is an unremovable kernel image section, which
neither gets removed at compile time nor boot time.

So __exit_refok did exactly what you say it should do before Sam's
change.

It's just completely stupid to change the netns section defines,
and instead we should revert __exit_refok to mean what it always
meant previously.

I'm not applying this patch.

Re: [PATCH] adjust __net_exit

[Sam Ravnborg]

On Fri, Mar 16, 2012 at 10:18:13PM -0700, David Miller wrote:
> From: "Jan Beulich" <JBeulich@suse.com>
> Date: Thu, 08 Mar 2012 09:37:32 +0000
> 
> > __net_exit, judging by the majority of its uses, was intended to serve
> > as an abstraction to allow calling such annotated functions from both
> > __init and __exit functions. Using the (bogus and unused elsewhere)
> > __exit_refok to implement this is inefficient - any non-modular code
> > really can reside in __init (as non-modular __exit code is never used).
> > 
> > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text)
> > in modules, and __init in the core kernel.
> > 
> > A few other adjustments are necessary/possible with this done - those
> > were likely just oversights when added originally.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> [ I have been waiting for more than a week for a netns developer
>   to review this patch, I guess I'm too optimistic these days. :-( ]
> 
> The only reason you think __exit_refok is "bogus" is because it's
> semantics got changed by Sam Ravnborg in commit
> 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section
> reference annotations tags: __ref, __refdata, __refconst")
> 
> Beforehand the __exit_refok was a real .exit section, so it got
> completely discarded AT LINK TIME.  Now it sits together with
> __init_refok which is an unremovable kernel image section, which
> neither gets removed at compile time nor boot time.

Some misunderstanding is going on here.

The *ref* annotation is used to teach modpost that this function (or data)
may reference functions (or data) which is annotated __init*.
And the *ref* annotation never caused the annotated code to be discarded.

This was true before the above mentioned patch - and it is still true.

Before the path ("Introduce new section reference ....") the __exit_refok
annotation moved functions to the section named ".exit.text.refok"
which was explicit part of .text (TEXT_TEXT in vmlinux).

So __exit_refok does exactly what it is intended to do:
It puts the function in a section so modpost does not warn about
references to __init or __exit sections.

As Jan points out there is only a single user left - so
this would be a good time to kill it. It is even documented
in init.h that this is a backward compatibility define.

The intention with Jan's patch is to move functions annotated
__net_exit to a discardable section in the core kernel.
Then at least __exit should be used - there is no logic
using __init for exit code.

I suggest to:
1) fix the patch to use __exit
2) fix up the bogus commit message

Then it should be OK - iff the assumption hold that the functions
can be discarded in the core kernel.
I have grepped a little and saw no uses where this did not hold true.
So based on this I assume the assumption is OK.

	Sam

Re: [PATCH] adjust __net_exit

[Eric W. Biederman]

Sam Ravnborg <sam@ravnborg.org> writes:

> On Fri, Mar 16, 2012 at 10:18:13PM -0700, David Miller wrote:
>> From: "Jan Beulich" <JBeulich@suse.com>
>> Date: Thu, 08 Mar 2012 09:37:32 +0000
>> 
>> > __net_exit, judging by the majority of its uses, was intended to serve
>> > as an abstraction to allow calling such annotated functions from both
>> > __init and __exit functions. Using the (bogus and unused elsewhere)
>> > __exit_refok to implement this is inefficient - any non-modular code
>> > really can reside in __init (as non-modular __exit code is never used).
>> > 
>> > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text)
>> > in modules, and __init in the core kernel.
>> > 
>> > A few other adjustments are necessary/possible with this done - those
>> > were likely just oversights when added originally.
>> > 
>> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> [ I have been waiting for more than a week for a netns developer
>>   to review this patch, I guess I'm too optimistic these days. :-( ]
>> 
>> The only reason you think __exit_refok is "bogus" is because it's
>> semantics got changed by Sam Ravnborg in commit
>> 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section
>> reference annotations tags: __ref, __refdata, __refconst")
>> 
>> Beforehand the __exit_refok was a real .exit section, so it got
>> completely discarded AT LINK TIME.  Now it sits together with
>> __init_refok which is an unremovable kernel image section, which
>> neither gets removed at compile time nor boot time.
>
> Some misunderstanding is going on here.
>
> The *ref* annotation is used to teach modpost that this function (or data)
> may reference functions (or data) which is annotated __init*.
> And the *ref* annotation never caused the annotated code to be discarded.
>
> This was true before the above mentioned patch - and it is still true.
>
> Before the path ("Introduce new section reference ....") the __exit_refok
> annotation moved functions to the section named ".exit.text.refok"
> which was explicit part of .text (TEXT_TEXT in vmlinux).
>
> So __exit_refok does exactly what it is intended to do:
> It puts the function in a section so modpost does not warn about
> references to __init or __exit sections.
>
> As Jan points out there is only a single user left - so
> this would be a good time to kill it. It is even documented
> in init.h that this is a backward compatibility define.

Strange.

>
> The intention with Jan's patch is to move functions annotated
> __net_exit to a discardable section in the core kernel.
> Then at least __exit should be used - there is no logic
> using __init for exit code.
>
> I suggest to:
> 1) fix the patch to use __exit
> 2) fix up the bogus commit message

Those two sound reasonable to me.

The purpose of __net_exit is to mark code that can be __exit if we don't
enable network namespace support.

Looking at the original code it appears that __exit was wrong because
we had __init sections referring to the __exit code and that was
causing modpost to complain.

Now that the definition of __exit_refok has changed half a dozen times
since it was introduced I don't know what will happen.

Since this really isn't about network namespaces but what to do when
the network namespace code is disabled I'm going to bow out now.

> Then it should be OK - iff the assumption hold that the functions
> can be discarded in the core kernel.
> I have grepped a little and saw no uses where this did not hold true.
> So based on this I assume the assumption is OK.

Eric

Re: [PATCH] adjust __net_exit

[Jan Beulich]

>>> On 18.03.12 at 00:03, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Fri, Mar 16, 2012 at 10:18:13PM -0700, David Miller wrote:
>> From: "Jan Beulich" <JBeulich@suse.com>
>> Date: Thu, 08 Mar 2012 09:37:32 +0000
>> 
>> > __net_exit, judging by the majority of its uses, was intended to serve
>> > as an abstraction to allow calling such annotated functions from both
>> > __init and __exit functions. Using the (bogus and unused elsewhere)
>> > __exit_refok to implement this is inefficient - any non-modular code
>> > really can reside in __init (as non-modular __exit code is never used).
>> > 
>> > Therefore, adjust __net_exit to resolve to nothing (i.e. normal .text)
>> > in modules, and __init in the core kernel.
>> > 
>> > A few other adjustments are necessary/possible with this done - those
>> > were likely just oversights when added originally.
>> > 
>> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> [ I have been waiting for more than a week for a netns developer
>>   to review this patch, I guess I'm too optimistic these days. :-( ]
>> 
>> The only reason you think __exit_refok is "bogus" is because it's
>> semantics got changed by Sam Ravnborg in commit
>> 312b1485fb509c9bc32eda28ad29537896658cb8 ("Introduce new section
>> reference annotations tags: __ref, __refdata, __refconst")
>> 
>> Beforehand the __exit_refok was a real .exit section, so it got
>> completely discarded AT LINK TIME.  Now it sits together with
>> __init_refok which is an unremovable kernel image section, which
>> neither gets removed at compile time nor boot time.
> 
> Some misunderstanding is going on here.
> 
> The *ref* annotation is used to teach modpost that this function (or data)
> may reference functions (or data) which is annotated __init*.
> And the *ref* annotation never caused the annotated code to be discarded.
> 
> This was true before the above mentioned patch - and it is still true.
> 
> Before the path ("Introduce new section reference ....") the __exit_refok
> annotation moved functions to the section named ".exit.text.refok"
> which was explicit part of .text (TEXT_TEXT in vmlinux).
> 
> So __exit_refok does exactly what it is intended to do:
> It puts the function in a section so modpost does not warn about
> references to __init or __exit sections.
> 
> As Jan points out there is only a single user left - so
> this would be a good time to kill it. It is even documented
> in init.h that this is a backward compatibility define.
> 
> The intention with Jan's patch is to move functions annotated
> __net_exit to a discardable section in the core kernel.
> Then at least __exit should be used - there is no logic
> using __init for exit code.
> 
> I suggest to:
> 1) fix the patch to use __exit

That won't work, as the goal is to have the exit functions call from
__net_init iiuc.

> 2) fix up the bogus commit message

Bogus in what way?

> Then it should be OK - iff the assumption hold that the functions
> can be discarded in the core kernel.

Since __exit functions won't ever be called when built in, they can as
well be marked __init and thus become possible to call from __init code
(again iiuc). Perhaps this should even become a concept outside of
net/, as not marking exit code __exit just because is also gets called
on error paths from __init is not uncommon. Maybe it should even be
__exit itself to get changed that way (albeit I vaguely recall that
differences between arches of when .exit.* sections get discarded
might need further consideration), as long as no arch would get
penalized in that it discards .exit.* but not .init.* post-boot.

Jan

> I have grepped a little and saw no uses where this did not hold true.
> So based on this I assume the assumption is OK.
> 
> 	Sam

Re: [PATCH] adjust __net_exit

[Sam Ravnborg]

> >> > Using the (bogus and unused elsewhere)
> >> > __exit_refok to implement this is inefficient - any non-modular code
> >> > really can reside in __init (as non-modular __exit code is never used).

> > 2) fix up the bogus commit message
> 
> Bogus in what way?

I fail to see that __exit_refok is bogus.
The name is not optimal - which is why is is deprecated.

	Sam

Re: [PATCH] adjust __net_exit

[Jan Beulich]

>>> On 19.03.12 at 09:47, Sam Ravnborg <sam@ravnborg.org> wrote:
>> >> > Using the (bogus and unused elsewhere)
>> >> > __exit_refok to implement this is inefficient - any non-modular code
>> >> > really can reside in __init (as non-modular __exit code is never used).
> 
>> > 2) fix up the bogus commit message
>> 
>> Bogus in what way?
> 
> I fail to see that __exit_refok is bogus.

Either an exit function references only permitted code (in which case
it is legitimately using __exit), or it references non-__exit code, in
which case it shouldn't have an override at all. Permitting an exit
function to reference e.g. __init code is suspicious (at best) in all
cases I can think of (in contrast to allowing a non-init function to
reference __init code on a code path that can be proven to be taken
only during system startup).

Jan

> The name is not optimal - which is why is is deprecated.
> 
> 	Sam

Re: [PATCH] adjust __net_exit

[Eric W. Biederman]

"Jan Beulich" <JBeulich@suse.com> writes:

>>>> On 19.03.12 at 09:47, Sam Ravnborg <sam@ravnborg.org> wrote:
>>> >> > Using the (bogus and unused elsewhere)
>>> >> > __exit_refok to implement this is inefficient - any non-modular code
>>> >> > really can reside in __init (as non-modular __exit code is never used).
>> 
>>> > 2) fix up the bogus commit message
>>> 
>>> Bogus in what way?
>> 
>> I fail to see that __exit_refok is bogus.
>
> Either an exit function references only permitted code (in which case
> it is legitimately using __exit), or it references non-__exit code, in
> which case it shouldn't have an override at all. Permitting an exit
> function to reference e.g. __init code is suspicious (at best) in all
> cases I can think of (in contrast to allowing a non-init function to
> reference __init code on a code path that can be proven to be taken
> only during system startup).

My apologies I said that there was nothing network namespace specific
left to talk about and I had goofed in my analysis.  The weirdness with
references comes from how struct pernet_operations is used.

Structures of type struct pernet_operations hold holds pointers to .init
functions marked as __net_init and .exit and .exit_batch marked as
__net_exit.

The assertion is that references from static instances of struct
pernet_operations that live in __net_initdata should be ok.


Structures of type pernet_operations are passed to
register_pernet_subsys or register_pernet_device which ultimately call
register_pernet_operations.  Register pernet_operations when the network
namespace is disabled simply calls the .init function when the network
namespace code is compiled out.  The .exit and .exit_batch functions
are not referenced at all.

When the network namespace code is compiled in __net_init __net_exit
and __net_initdata all go away.

So the exit function references only permitted code.  But the exit
function is being referenced by data in an init section which requires
the override.

How should we mark functions in __net_exit that can be thrown away
when outside of a module and never even loaded when compiled into the
kernel when the network namespace is disabled?

The original definition of __exit_refok seemed to say this very clearly
to me.  This is an __exit section but references to it from __init data
are ok.

Eric