[PATCH] Move sysctl check into debugging section and don't make it default y

[PATCH] Move sysctl check into debugging section and don't make it default y

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

I noticed that sysctl_check.o was the largest object file in
a allnoconfig build in kernel/*.

  36243       0       0   36243    8d93 kernel/sysctl_check.o

This is because it was default y and && EMBEDDED. But I don't
really see a need for a non kernel developer to have their
sysctls checked all the time.

So move the Kconfig into the kernel debugging section and
also drop the default y and the EMBEDDED check.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 init/Kconfig      |   11 -----------
 lib/Kconfig.debug |    8 ++++++++
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index b678803..c11da38 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -558,17 +558,6 @@ config SYSCTL_SYSCALL
 
 	  If unsure say Y here.
 
-config SYSCTL_SYSCALL_CHECK
-	bool "Sysctl checks" if EMBEDDED
-	depends on SYSCTL_SYSCALL
-	default y
-	---help---
-	  sys_sysctl uses binary paths that have been found challenging
-	  to properly maintain and use. This enables checks that help
-	  you to keep things correct.
-
-	  If unsure say Y here.
-
 config KALLSYMS
 	 bool "Load all symbols for debugging/ksymoops" if EMBEDDED
 	 default y
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 800ac84..8b5a7d3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -693,6 +693,14 @@ config LATENCYTOP
 	  Enable this option if you want to use the LatencyTOP tool
 	  to find out which userspace is blocking on what kernel operations.
 
+config SYSCTL_SYSCALL_CHECK
+	bool "Sysctl checks"
+	depends on SYSCTL_SYSCALL
+	---help---
+	  sys_sysctl uses binary paths that have been found challenging
+	  to properly maintain and use. This enables checks that help
+	  you to keep things correct.
+
 source kernel/trace/Kconfig
 
 config PROVIDE_OHCI1394_DMA_INIT
-- 
1.5.6

Re: [PATCH] Move sysctl check into debugging section and don't make it default y

[Eric W. Biederman]

"Andi Kleen" <andi@firstfloor.org> writes:

> From: Andi Kleen <ak@linux.intel.com>
>
> I noticed that sysctl_check.o was the largest object file in
> a allnoconfig build in kernel/*.
>
>   36243       0       0   36243    8d93 kernel/sysctl_check.o
>
> This is because it was default y and && EMBEDDED. But I don't
> really see a need for a non kernel developer to have their
> sysctls checked all the time.

What is a feature change like this doing coming in after the
merge window?

Why doesn't an allnoconfig disable sysctl all together?

> So move the Kconfig into the kernel debugging section and
> also drop the default y and the EMBEDDED check.

Which is an idiotic thing to do.

These are the only checks we have against someone doing something
nasty in the sysctl hierarchy.   We have proven that we don't
have the discipline to do the right thing with code in the
core kernel.  I expect out of tree code will be much worse.

Eric

Re: [PATCH] Move sysctl check into debugging section and don't make it default y

[Andi Kleen]

> What is a feature change like this doing coming in after the
> merge window?

I considered it a "anti bloat bugfix". Adding 30k of 
object code to allno was a bit too much. 

> Why doesn't an allnoconfig disable sysctl all together?

Because it depends on EMBEDDED and EMBEDDED is not y. Yes it's not
intuitive, on the other hand the end result is reasonable.

> These are the only checks we have against someone doing something
> nasty in the sysctl hierarchy.   We have proven that we don't
> have the discipline to do the right thing with code in the
> core kernel.  I expect out of tree code will be much worse.

My assumption is that they will be run at least once during
a release cycle by someone and then the messages will appear
and be reported. We do the same thing with a lot of other
debug options (lockdep, slab debug, sleep debug etc.,). There's no 
need for this one to be special.

Also I'm not sure the check is all that useful anyways. We
should just not accept any new binary numbered sysctl, and
that's nearly the case anyways.

-Andi

Re: [PATCH] Move sysctl check into debugging section and don't make it default y

[Eric W. Biederman]

Andi Kleen <andi@firstfloor.org> writes:

>> What is a feature change like this doing coming in after the
>> merge window?
>
> I considered it a "anti bloat bugfix". Adding 30k of 
> object code to allno was a bit too much. 
>
>> Why doesn't an allnoconfig disable sysctl all together?
>
> Because it depends on EMBEDDED and EMBEDDED is not y. Yes it's not
> intuitive, on the other hand the end result is reasonable.

That makes sense in a silly sort of way.  Making
allnoconfig not a particularly good minimal size check.

>> These are the only checks we have against someone doing something
>> nasty in the sysctl hierarchy.   We have proven that we don't
>> have the discipline to do the right thing with code in the
>> core kernel.  I expect out of tree code will be much worse.
>
> My assumption is that they will be run at least once during
> a release cycle by someone and then the messages will appear
> and be reported. We do the same thing with a lot of other
> debug options (lockdep, slab debug, sleep debug etc.,). There's no 
> need for this one to be special.

But it really isn't a debug option.

> Also I'm not sure the check is all that useful anyways. We
> should just not accept any new binary numbered sysctl, and
> that's nearly the case anyways.

This code is the mechanism by which we do not accept any
new binary numbered sysctl into the kernel.

Andrew used to get them just often enough that I would get a message
ever couple of months.  What and why is our policy with respect to new
binary sysctls?

Since this code has yet to ship in any enterprise kernel to my knowledge
I expect there are going to be another raft load of kernel bugs discovered
in out of tree code when it does.  We have a decade or more of near
total neglect to make up for.

As for what the code does.  There is one big expensive (space wise)
check in there that ensures we don't add new sysctl binary names.
Beyond that the checks that sysctl_check performs are actual sanity
checks with the only expensive one being to ensure we don't register
the same name twice.  Real code hits those checks, and frequently not
in development, but in some weird production scenario.  And the code
only runs when we register a sysctl so it is cheap.

Which is the big difference between this code and debugging checks,
even when enabled it barely ever runs.

Now if you would like to fix the size issue.  The thing to do is to
add a type field or a conversion function onto those tables.  Which is
enough to implement all of our binary sysctls by looking up the ascii
equivalents and calling the proc handling functions.  Then those tables
would be much more then dead weight.

Eric

Re: [PATCH] Move sysctl check into debugging section and don't make it default y

[Eric W. Biederman]

Andi Kleen <andi@firstfloor.org> writes:

>> What is a feature change like this doing coming in after the
>> merge window?
>
> I considered it a "anti bloat bugfix". Adding 30k of 
> object code to allno was a bit too much. 

30k???  Which platform are you testing on ia64????

On x86_64 it is 8k text and 8k data.

Eric

Re: [PATCH] Move sysctl check into debugging section and don't make it default y

[Andi Kleen]

On Thu, Aug 21, 2008 at 11:15:37AM -0700, Eric W. Biederman wrote:
> Andi Kleen <andi@firstfloor.org> writes:
> 
> >> What is a feature change like this doing coming in after the
> >> merge window?
> >
> > I considered it a "anti bloat bugfix". Adding 30k of 
> > object code to allno was a bit too much. 
> 
> 30k???  Which platform are you testing on ia64????
> 
> On x86_64 it is 8k text and 8k data.

x86-64 with 4.1. See the size output in the original commit.

  text    data     bss     dec     hex filename
  36243       0       0   36243    8d93 kernel/sysctl_check.o

36k actually.

-Andi

Re: [PATCH] Move sysctl check into debugging section and don't make it default y

[Eric W. Biederman]

Andi Kleen <andi@firstfloor.org> writes:

> On Thu, Aug 21, 2008 at 11:15:37AM -0700, Eric W. Biederman wrote:
>> Andi Kleen <andi@firstfloor.org> writes:
>> 
>> >> What is a feature change like this doing coming in after the
>> >> merge window?
>> >
>> > I considered it a "anti bloat bugfix". Adding 30k of 
>> > object code to allno was a bit too much. 
>> 
>> 30k???  Which platform are you testing on ia64????
>> 
>> On x86_64 it is 8k text and 8k data.
>
> x86-64 with 4.1. See the size output in the original commit.
>
>   text    data     bss     dec     hex filename
>   36243       0       0   36243    8d93 kernel/sysctl_check.o
>
> 36k actually.

With gcc-4.1.1 on x86_64 I see:

size kernel/sysctl.o  
   text    data     bss     dec     hex filename
   9133    8948     208   18289    4771 kernel/sysctl.o

And looking at the readelf output confirms that size isn't missing something
important.  That is extremely weird that you are seeing something so much different.
It does appear that I have CONFIG_CC_OPTIMIZE_FOR_SIZE=y but I am surprised that
even that would make such a difference.  Has gcc decided just to way over-optimize
that code?

Eric