[PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[Linas Vepstas]

This is a patch (& bug report) for a crash in sysctl_set_parent() 
in 2.6.22-git2. 

Problem: 2.6.22-git2 crashes with a stack trace 
[c000000001d0fb00] c000000000067b4c .sysctl_set_parent+0x48/0x7c
[c000000001d0fb90] c000000000069b40 .register_sysctl_table+0x7c/0xf4
[c000000001d0fc30] c00000000065e710 .devinet_init+0x88/0xb0
[c000000001d0fcc0] c00000000065db74 .ip_rt_init+0x17c/0x32c
[c000000001d0fd70] c00000000065deec .ip_init+0x10/0x34
[c000000001d0fdf0] c00000000065e898 .inet_init+0x160/0x3dc
[c000000001d0fea0] c000000000630bc4 .kernel_init+0x204/0x3c8

A bit of poking around makes it clear what the problem is:
In sysctl_set_parent(), the for loop 

   for (; table->ctl_name || table->procname; table++) {

walks off the end of the table, and into garbage.  Basically,
this for-loop iterator expects all table arrays to be 
"null terminated".  However, net/ipv4/devinet.c statically 
declares an array that is not null-terminated.  The patch 
below fixes that; it works for me.  Its somewhat conservative;
if one wishes to assume that the compiler will always zero out
the empty parts of the structure, then this pach can be shrunk 
to one line: +	ctl_table		devinet_root_dir[3];

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
I tried to audit some of the code to see where else there 
might be similar badly-formed static declarations.  This is hard,
as there's a lot of code. Most seems fine.


 net/core/neighbour.c |    4 ++++
 net/ipv4/devinet.c   |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Index: linux-2.6.22-git2/net/ipv4/devinet.c
===================================================================
--- linux-2.6.22-git2.orig/net/ipv4/devinet.c	2007-07-13 14:23:21.000000000 -0500
+++ linux-2.6.22-git2/net/ipv4/devinet.c	2007-07-13 14:24:15.000000000 -0500
@@ -1424,7 +1424,7 @@ static struct devinet_sysctl_table {
 	ctl_table		devinet_dev[2];
 	ctl_table		devinet_conf_dir[2];
 	ctl_table		devinet_proto_dir[2];
-	ctl_table		devinet_root_dir[2];
+	ctl_table		devinet_root_dir[3];
 } devinet_sysctl = {
 	.devinet_vars = {
 		DEVINET_SYSCTL_COMPLEX_ENTRY(FORWARDING, "forwarding",
@@ -1493,8 +1493,13 @@ static struct devinet_sysctl_table {
 			.data		= &ipv4_devconf.loop,
 			.maxlen		= sizeof(int),
 			.mode		= 0644,
+			.child	= 0x0,
 			.proc_handler	= &proc_dointvec,
 		},
+		{
+			.ctl_name	= 0,
+			.procname	= 0,
+		},
 	},
 };
 

Re: [PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[David Miller]

From: linas@austin.ibm.com (Linas Vepstas)
Date: Fri, 13 Jul 2007 15:05:15 -0500

> 
> This is a patch (& bug report) for a crash in sysctl_set_parent() 
> in 2.6.22-git2. 
> 
> Problem: 2.6.22-git2 crashes with a stack trace 
> [c000000001d0fb00] c000000000067b4c .sysctl_set_parent+0x48/0x7c
> [c000000001d0fb90] c000000000069b40 .register_sysctl_table+0x7c/0xf4
> [c000000001d0fc30] c00000000065e710 .devinet_init+0x88/0xb0
> [c000000001d0fcc0] c00000000065db74 .ip_rt_init+0x17c/0x32c
> [c000000001d0fd70] c00000000065deec .ip_init+0x10/0x34
> [c000000001d0fdf0] c00000000065e898 .inet_init+0x160/0x3dc
> [c000000001d0fea0] c000000000630bc4 .kernel_init+0x204/0x3c8
> 
> A bit of poking around makes it clear what the problem is:
> In sysctl_set_parent(), the for loop 
> 
>    for (; table->ctl_name || table->procname; table++) {
> 
> walks off the end of the table, and into garbage.  Basically,
> this for-loop iterator expects all table arrays to be 
> "null terminated".  However, net/ipv4/devinet.c statically 
> declares an array that is not null-terminated.  The patch 
> below fixes that; it works for me.  Its somewhat conservative;
> if one wishes to assume that the compiler will always zero out
> the empty parts of the structure, then this pach can be shrunk 
> to one line: +	ctl_table		devinet_root_dir[3];
> 
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

Thanks for tracking this down, I'll apply your patch.

Re: [PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[Satyam Sharma]

Hi,

I'm totally confuzed by this patch ...

On 7/14/07, Linas Vepstas <linas@austin.ibm.com> wrote:
>
> This is a patch (& bug report) for a crash in sysctl_set_parent()
> in 2.6.22-git2.

Are you sure you saw this crash on 22-git2 (Linus' tree) ???

> Problem: 2.6.22-git2 crashes with a stack trace
> [c000000001d0fb00] c000000000067b4c .sysctl_set_parent+0x48/0x7c
> [c000000001d0fb90] c000000000069b40 .register_sysctl_table+0x7c/0xf4
> [c000000001d0fc30] c00000000065e710 .devinet_init+0x88/0xb0
> [c000000001d0fcc0] c00000000065db74 .ip_rt_init+0x17c/0x32c
> [c000000001d0fd70] c00000000065deec .ip_init+0x10/0x34
> [c000000001d0fdf0] c00000000065e898 .inet_init+0x160/0x3dc
> [c000000001d0fea0] c000000000630bc4 .kernel_init+0x204/0x3c8

Please post the full dmesg output, if possible.

> A bit of poking around makes it clear what the problem is:
> In sysctl_set_parent(), the for loop
>
>    for (; table->ctl_name || table->procname; table++) {

Yup, but note that the "table" there is a struct ctl_table * ...

> walks off the end of the table, and into garbage.  Basically,
> this for-loop iterator expects all table arrays to be
> "null terminated".  However, net/ipv4/devinet.c statically
> declares an array that is not null-terminated.

Whereas the "not null-terminated" array you're talking about
in devinet.c is a struct devinet_sysctl_table, whose *members*
are ctl_table structs.

Also note that all those members have already been declared
as arrays: struct ctl_table xxx[2]; *precisely* because the
second element (which is left un-initialized, hence will get
zeroed-out being static) is what will protect us from running
out into garbage in sysctl_set_parent().

> The patch
> below fixes that; it works for me.

Now I'm even more confuzed ...

> Its somewhat conservative;
> if one wishes to assume that the compiler will always zero out
> the empty parts of the structure,

You can always safely assume that.

Off-topic, but note that unintialized static variables being automagically
initialized to zero / NULL is guaranteed by C (the language) itself, so
nothing left in the hands of compiler implementations here. [6.7.8:10]

> then this pach can be shrunk
> to one line: +  ctl_table               devinet_root_dir[3];

But that's pointless. It was already [2] for the reason I mentioned.

> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> [...]
>  net/core/neighbour.c |    4 ++++
>  net/ipv4/devinet.c   |    7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)

Weirder. diffstat does not match the patch ...

> Index: linux-2.6.22-git2/net/ipv4/devinet.c
> ===================================================================
> --- linux-2.6.22-git2.orig/net/ipv4/devinet.c   2007-07-13 14:23:21.000000000 -0500
> +++ linux-2.6.22-git2/net/ipv4/devinet.c        2007-07-13 14:24:15.000000000 -0500
> @@ -1424,7 +1424,7 @@ static struct devinet_sysctl_table {
>         ctl_table               devinet_dev[2];
>         ctl_table               devinet_conf_dir[2];
>         ctl_table               devinet_proto_dir[2];
> -       ctl_table               devinet_root_dir[2];
> +       ctl_table               devinet_root_dir[3];
>  } devinet_sysctl = {
>         .devinet_vars = {
>                 DEVINET_SYSCTL_COMPLEX_ENTRY(FORWARDING, "forwarding",
> @@ -1493,8 +1493,13 @@ static struct devinet_sysctl_table {

And Linus' latest -git doesn't even have this stuff at line 1493.

>                         .data           = &ipv4_devconf.loop,
>                         .maxlen         = sizeof(int),
>                         .mode           = 0644,
> +                       .child  = 0x0,
>                         .proc_handler   = &proc_dointvec,
>                 },
> +               {
> +                       .ctl_name       = 0,
> +                       .procname       = 0,
> +               },
>         },
>  };

Well, as I said, I'm totally confuzed ...

Satyam

Re: [PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[Eric W. Biederman]

linas@austin.ibm.com (Linas Vepstas) writes:

> This is a patch (& bug report) for a crash in sysctl_set_parent() 
> in 2.6.22-git2. 
>
> Problem: 2.6.22-git2 crashes with a stack trace 
> [c000000001d0fb00] c000000000067b4c .sysctl_set_parent+0x48/0x7c
> [c000000001d0fb90] c000000000069b40 .register_sysctl_table+0x7c/0xf4
> [c000000001d0fc30] c00000000065e710 .devinet_init+0x88/0xb0
> [c000000001d0fcc0] c00000000065db74 .ip_rt_init+0x17c/0x32c
> [c000000001d0fd70] c00000000065deec .ip_init+0x10/0x34
> [c000000001d0fdf0] c00000000065e898 .inet_init+0x160/0x3dc
> [c000000001d0fea0] c000000000630bc4 .kernel_init+0x204/0x3c8
>
> A bit of poking around makes it clear what the problem is:
> In sysctl_set_parent(), the for loop 
>
>    for (; table->ctl_name || table->procname; table++) {
>
> walks off the end of the table, and into garbage.  Basically,
> this for-loop iterator expects all table arrays to be 
> "null terminated".  However, net/ipv4/devinet.c statically 
> declares an array that is not null-terminated.  The patch 
> below fixes that; it works for me.  Its somewhat conservative;
> if one wishes to assume that the compiler will always zero out
> the empty parts of the structure, then this pach can be shrunk 
> to one line: +	ctl_table		devinet_root_dir[3];
>
> Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
>
> ----
> I tried to audit some of the code to see where else there 
> might be similar badly-formed static declarations.  This is hard,
> as there's a lot of code. Most seems fine.

Right.  I believe I performed a similar audit not to long ago
when everything was converted to C99 initializers and everything
was fine then.


>  net/core/neighbour.c |    4 ++++
>  net/ipv4/devinet.c   |    7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.22-git2/net/ipv4/devinet.c
> ===================================================================
> --- linux-2.6.22-git2.orig/net/ipv4/devinet.c 2007-07-13 14:23:21.000000000
> -0500
> +++ linux-2.6.22-git2/net/ipv4/devinet.c 2007-07-13 14:24:15.000000000 -0500
> @@ -1424,7 +1424,7 @@ static struct devinet_sysctl_table {
>  	ctl_table		devinet_dev[2];
>  	ctl_table		devinet_conf_dir[2];
>  	ctl_table		devinet_proto_dir[2];
> -	ctl_table		devinet_root_dir[2];
> +	ctl_table		devinet_root_dir[3];
>  } devinet_sysctl = {
>  	.devinet_vars = {
>  		DEVINET_SYSCTL_COMPLEX_ENTRY(FORWARDING, "forwarding",
> @@ -1493,8 +1493,13 @@ static struct devinet_sysctl_table {
>  			.data		= &ipv4_devconf.loop,
>  			.maxlen		= sizeof(int),
>  			.mode		= 0644,
> +			.child	= 0x0,
>  			.proc_handler	= &proc_dointvec,
>  		},
Where did this entry above in devinet_sysctl come from?
> +		{
> +			.ctl_name	= 0,
> +			.procname	= 0,
> +		},
I probably would have just done:
+		{},
>  	},
>  };
>  

What added the additional entry to devinet_root_dir?  I don't see that
in Linus' tree?

The result may be fine but if it isn't named in a per network device
manner we are adding duplicate entries to the root /proc/sys directory
which is wrong.

Actually come to think of it I am concerned that someone added a
settable entry into /proc/sys/ it should at least be in /proc/sys/net/
where it won't conflict with other uses of that directory.  Especially
as things like network devices have user controlled names.

Eric

Re: [PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[Linas Vepstas]

On Fri, Jul 13, 2007 at 07:06:56PM -0600, Eric W. Biederman wrote:
> >  			.data		= &ipv4_devconf.loop,
> >  			.maxlen		= sizeof(int),
> >  			.mode		= 0644,
> > +			.child	= 0x0,
> >  			.proc_handler	= &proc_dointvec,
> >  		},
> Where did this entry above in devinet_sysctl come from?

My bad.
I habitually apply the "send-to-self" patch, since some of the 
network testing that I do is easiest if I load up the all of the 
adapters in the same box. (If you're not familiar with this patch ... 
its great, and I wish it was integratedd into mainline. It allows
one to drive network traffic through the physical devices, even
if they are in the same box.  Without it, the network stack is
too clever, and won't allow you to do this.)

> > +		{
> > +			.ctl_name	= 0,
> > +			.procname	= 0,
> > +		},
> I probably would have just done:
> +		{},

Yes, in retrospect, this would have been the simplest solution.

> What added the additional entry to devinet_root_dir?  I don't see that
> in Linus' tree?
> 
> The result may be fine but if it isn't named in a per network device
> manner we are adding duplicate entries to the root /proc/sys directory
> which is wrong.
> 
> Actually come to think of it I am concerned that someone added a
> settable entry into /proc/sys/ it should at least be in /proc/sys/net/
> where it won't conflict with other uses of that directory.  Especially
> as things like network devices have user controlled names.

Sigh. Silly me. Haste makes waste.

--linas

Re: [PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[Linas Vepstas]

On Fri, Jul 13, 2007 at 03:47:02PM -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Fri, 13 Jul 2007 15:05:15 -0500
> 
> > 
> > This is a patch (& bug report) for a crash in sysctl_set_parent() 
> > in 2.6.22-git2. 
> > 
> > Problem: 2.6.22-git2 crashes with a stack trace 
> > 
> > Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> 
> Thanks for tracking this down, I'll apply your patch.

NAK. As I just explained in another email, this bug
was introduced by the "send-to-self" patch I habitually
apply -- so habitually, that I forgot I was not working 
with a "clean" tree. So it goes ... 

--linas

Re: [PATCH] crash in 2.6.22-git2 sysctl_set_parent()

[David Miller]

From: linas@austin.ibm.com (Linas Vepstas)
Date: Mon, 16 Jul 2007 12:25:35 -0500

> On Fri, Jul 13, 2007 at 03:47:02PM -0700, David Miller wrote:
> > From: linas@austin.ibm.com (Linas Vepstas)
> > Date: Fri, 13 Jul 2007 15:05:15 -0500
> > 
> > > 
> > > This is a patch (& bug report) for a crash in sysctl_set_parent() 
> > > in 2.6.22-git2. 
> > > 
> > > Problem: 2.6.22-git2 crashes with a stack trace 
> > > 
> > > Signed-off-by: Linas Vepstas <linas@austin.ibm.com>
> > 
> > Thanks for tracking this down, I'll apply your patch.
> 
> NAK. As I just explained in another email, this bug
> was introduced by the "send-to-self" patch I habitually
> apply -- so habitually, that I forgot I was not working 
> with a "clean" tree. So it goes ... 

I saw all of this, thanks.