[PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[Holger Schurig]

Disable sysctl_check.c for embedded targets. This saves about about 11 kB
in .text and another 11 kB in .data on a PXA255 embedded platform.

Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

--- linux.orig/init/Kconfig
+++ linux/init/Kconfig
@@ -475,6 +475,17 @@
 
 	  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
--- linux.orig/kernel/Makefile
+++ linux/kernel/Makefile
@@ -11,7 +11,7 @@
 	    hrtimer.o rwsem.o nsproxy.o srcu.o \
 	    utsname.o notifier.o ksysfs.o pm_qos_params.o
 
-obj-$(CONFIG_SYSCTL) += sysctl_check.o
+obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
 obj-y += time/
 obj-$(CONFIG_DEBUG_MUTEXES) += mutex-debug.o
--- linux.orig/kernel/sysctl.c
+++ linux/kernel/sysctl.c
@@ -1604,9 +1604,13 @@
 
 static __init int sysctl_init(void)
 {
-	int err;
 	sysctl_set_parent(NULL, root_table);
-	err = sysctl_check_table(current->nsproxy, root_table);
+#ifdef CONFIG_SYSCTL_SYSCALL_CHECK
+	{
+		int err;
+		err = sysctl_check_table(current->nsproxy, root_table);
+	}
+#endif
 	return 0;
 }
 
@@ -1733,10 +1737,12 @@
 	header->unregistering = NULL;
 	header->root = root;
 	sysctl_set_parent(NULL, header->ctl_table);
+#ifdef CONFIG_SYSCTL_SYSCALL_CHECK
 	if (sysctl_check_table(namespaces, header->ctl_table)) {
 		kfree(header);
 		return NULL;
 	}
+#endif
 	spin_lock(&sysctl_lock);
 	header_list = lookup_header_list(root, namespaces);
 	list_add_tail(&header->ctl_entry, header_list);

Re: [PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[Andrew Morton]

On Thu, 7 Feb 2008 14:38:58 +0100 Holger Schurig <hs4233@mail.mn-solutions.de> wrote:

> Disable sysctl_check.c for embedded targets. This saves about about 11 kB
> in .text and another 11 kB in .data on a PXA255 embedded platform.
> 

Nice improvement.  But iirc sysctl_check was overtly a temporary thing.
Eric, was that the intention?

> 
> --- linux.orig/init/Kconfig
> +++ linux/init/Kconfig
> @@ -475,6 +475,17 @@
>  
>  	  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.
> +

Re: [PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Thu, 7 Feb 2008 14:38:58 +0100 Holger Schurig <hs4233@mail.mn-solutions.de>
> wrote:
>
>> Disable sysctl_check.c for embedded targets. This saves about about 11 kB
>> in .text and another 11 kB in .data on a PXA255 embedded platform.
>> 
>
> Nice improvement.  But iirc sysctl_check was overtly a temporary thing.
> Eric, was that the intention?

Well so far sysctl_check has been a remarkably effective little piece of code
in catching a great many long over looked bugs.

I do agree that the static tables are big.  My current inclination is to modify
sys_sysctl so that it does a look up in the binary tables to find the ascii
names and then sys_sysctl can lookup the information in the ascii tables.

If we do that we can completely remove ctl_name form the external sysctl data
structures, which should save us quite a bit of space and make it absolutely
impossible to add a new binary name.  And with the current ability to compile
out sys_sysctl the embedded folks would get their space savings.

I believe the only tricky bit is there are a few places in the network code
where we need to translate from ifindex to interface name.  Otherwise
the mapping is fixed.

No that isn't quite right.  Getting the binary to ascii translation for the
values is also a bit tricky.

As for the rest of the checks I don't know if they are that big.  If they
are then an option to compile them out on embedded platforms where you
know what you are doing makes sense.  At the same time sysctl has been so
badly abused in the past, and so very many bugs have been over looked
that I am extremely reluctant to disable simple sanity checks at
registration time.

If we can remove the need for sysctl users to implement the binary
interface many of those checks go completely away as the reason for their
existence would be gone.

I have seen to many absolutely horrible things in the usage of the sysctl
tables to be happy with an option that removes the sanity checks at this
point, although the patch likely makes sense from a code size perspective.

Let's see if we can find a bit of time to make those big tables completely
specific to sys_sysctl and kill ctl_name in the kernel.  Long term that is
a whole lot more maintainable, and smaller for everyone who can disable
sys_sysctl.

Eric

Re: [PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[Michael Opdenacker]

On 02/08/2008 11:36 AM, Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
>
>   
>> On Thu, 7 Feb 2008 14:38:58 +0100 Holger Schurig <hs4233@mail.mn-solutions.de>
>> wrote:
>>
>>     
>>> Disable sysctl_check.c for embedded targets. This saves about about 11 kB
>>> in .text and another 11 kB in .data on a PXA255 embedded platform.
>>>
>>>       
>> Nice improvement.  But iirc sysctl_check was overtly a temporary thing.
>> Eric, was that the intention?
>>     
>
> Well so far sysctl_check has been a remarkably effective little piece of code
> in catching a great many long over looked bugs.
>
> I do agree that the static tables are big.  My current inclination is to modify
> sys_sysctl so that it does a look up in the binary tables to find the ascii
> names and then sys_sysctl can lookup the information in the ascii tables.
>
> If we do that we can completely remove ctl_name form the external sysctl data
> structures, which should save us quite a bit of space and make it absolutely
> impossible to add a new binary name.  And with the current ability to compile
> out sys_sysctl the embedded folks would get their space savings.
>
> I believe the only tricky bit is there are a few places in the network code
> where we need to translate from ifindex to interface name.  Otherwise
> the mapping is fixed.
>
> No that isn't quite right.  Getting the binary to ascii translation for the
> values is also a bit tricky.
>
> As for the rest of the checks I don't know if they are that big.  If they
> are then an option to compile them out on embedded platforms where you
> know what you are doing makes sense.  At the same time sysctl has been so
> badly abused in the past, and so very many bugs have been over looked
> that I am extremely reluctant to disable simple sanity checks at
> registration time.
>
> If we can remove the need for sysctl users to implement the binary
> interface many of those checks go completely away as the reason for their
> existence would be gone.
>
> I have seen to many absolutely horrible things in the usage of the sysctl
> tables to be happy with an option that removes the sanity checks at this
> point, although the patch likely makes sense from a code size perspective.
>
> Let's see if we can find a bit of time to make those big tables completely
> specific to sys_sysctl and kill ctl_name in the kernel.  Long term that is
> a whole lot more maintainable, and smaller for everyone who can disable
> sys_sysctl.
>
> Eric
>   
Holger, this is a nice improvement allowing to keep /proc/sys but
reducing its size. Thanks! On x86, they save 9K in the text section and
12 K in data.

For those embedded system developers who are unfamiliar with this topic,
you can already completely remove the /proc/sys interface if you don't
need it. Then you save 19K of text and 16 k of data).

Copying the Linux-Tiny mailing list to draw the attraction of more
people to this discussion.

Michael.

-- 
Michael Opdenacker, Free Electrons
Free Embedded Linux Training Materials
on http://free-electrons.com/training
(More than 1500 pages!)

Re: [PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[Andrew Morton]

On Fri, 08 Feb 2008 03:36:41 -0700
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Thu, 7 Feb 2008 14:38:58 +0100 Holger Schurig <hs4233@mail.mn-solutions.de>
> > wrote:
> >
> >> Disable sysctl_check.c for embedded targets. This saves about about 11 kB
> >> in .text and another 11 kB in .data on a PXA255 embedded platform.
> >> 
> >
> > Nice improvement.  But iirc sysctl_check was overtly a temporary thing.
> > Eric, was that the intention?
> 
> Well so far sysctl_check has been a remarkably effective little piece of code
> in catching a great many long over looked bugs.
> 
> I do agree that the static tables are big.  My current inclination is to modify
> sys_sysctl so that it does a look up in the binary tables to find the ascii
> names and then sys_sysctl can lookup the information in the ascii tables.
> 
> If we do that we can completely remove ctl_name form the external sysctl data
> structures, which should save us quite a bit of space and make it absolutely
> impossible to add a new binary name.  And with the current ability to compile
> out sys_sysctl the embedded folks would get their space savings.
> 
> I believe the only tricky bit is there are a few places in the network code
> where we need to translate from ifindex to interface name.  Otherwise
> the mapping is fixed.
> 
> No that isn't quite right.  Getting the binary to ascii translation for the
> values is also a bit tricky.
> 
> As for the rest of the checks I don't know if they are that big.  If they
> are then an option to compile them out on embedded platforms where you
> know what you are doing makes sense.  At the same time sysctl has been so
> badly abused in the past, and so very many bugs have been over looked
> that I am extremely reluctant to disable simple sanity checks at
> registration time.
> 
> If we can remove the need for sysctl users to implement the binary
> interface many of those checks go completely away as the reason for their
> existence would be gone.
> 
> I have seen to many absolutely horrible things in the usage of the sysctl
> tables to be happy with an option that removes the sanity checks at this
> point, although the patch likely makes sense from a code size perspective.
> 
> Let's see if we can find a bit of time to make those big tables completely
> specific to sys_sysctl and kill ctl_name in the kernel.  Long term that is
> a whole lot more maintainable, and smaller for everyone who can disable
> sys_sysctl.

mm...  I'm inclined to merge the patch.  It's a decent saving, and it
requires CONFIG_EMBEDDED which most people don't appear to set.

Re: [PATCH] sysctl: allow embedded targets to disable sysctl_check.c

[Michael Opdenacker]

On 02/08/2008 10:58 PM, Andrew Morton wrote:
> mm...  I'm inclined to merge the patch.  It's a decent saving, and it
> requires CONFIG_EMBEDDED which most people don't appear to set.
>   
That would be great! It does no harm to most people and doesn't make
things more complex to manage. And of course, it doesn't prevent from
improving sysctl support code in the long run.

Thanks!

:-)

Michael.

-- 
Michael Opdenacker, Free Electrons
Free Embedded Linux Training Materials
on http://free-electrons.com/training
(More than 1500 pages!)