[PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric Dumazet]

When proc_doulongvec_minmax() is used with an array of longs,
and no min/max check requested (.extra1 or .extra2 being NULL), we
dereference a NULL pointer for the second element of the array.

Noticed while doing some changes in network stack for the "16TB problem"

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 kernel/sysctl.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..4fba86d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 				break;
 			if (neg)
 				continue;
-			if ((min && val < *min) || (max && val > *max))
+			if ((table->extra1 && val < *min) ||
+			    (table->extra2 && val > *max))
 				continue;
 			*i = val;
 		} else {

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote:
>When proc_doulongvec_minmax() is used with an array of longs,
>and no min/max check requested (.extra1 or .extra2 being NULL), we
>dereference a NULL pointer for the second element of the array.
>
>Noticed while doing some changes in network stack for the "16TB problem"
>
>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>---
> kernel/sysctl.c |    3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>index f88552c..4fba86d 100644
>--- a/kernel/sysctl.c
>+++ b/kernel/sysctl.c
>@@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> 				break;
> 			if (neg)
> 				continue;
>-			if ((min && val < *min) || (max && val > *max))
>+			if ((table->extra1 && val < *min) ||
>+			    (table->extra2 && val > *max))
> 				continue;


Yes? This is wrong for me, min and max are pointers to ->extra{1,2},
they are increased within the for loop, you are only checking the
the pointer to the first element of ->extra{1,2}.

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Robin Holt]

On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote:
> When proc_doulongvec_minmax() is used with an array of longs,
> and no min/max check requested (.extra1 or .extra2 being NULL), we
> dereference a NULL pointer for the second element of the array.
> 
> Noticed while doing some changes in network stack for the "16TB problem"
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  kernel/sysctl.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f88552c..4fba86d 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>  				break;
>  			if (neg)
>  				continue;
> -			if ((min && val < *min) || (max && val > *max))
> +			if ((table->extra1 && val < *min) ||
> +			    (table->extra2 && val > *max))

How about changing:
        for (; left && vleft--; i++, min++, max++, first=0) {
into:
        for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) {

That would make min and max correct and reduce the chances somebody in
the future overlooks the fact they are currently filled with garbage.

Robin

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric Dumazet]

Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit :
> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote:
> > When proc_doulongvec_minmax() is used with an array of longs,
> > and no min/max check requested (.extra1 or .extra2 being NULL), we
> > dereference a NULL pointer for the second element of the array.
> > 
> > Noticed while doing some changes in network stack for the "16TB problem"
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> >  kernel/sysctl.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index f88552c..4fba86d 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >  				break;
> >  			if (neg)
> >  				continue;
> > -			if ((min && val < *min) || (max && val > *max))
> > +			if ((table->extra1 && val < *min) ||
> > +			    (table->extra2 && val > *max))
> 
> How about changing:
>         for (; left && vleft--; i++, min++, max++, first=0) {
> into:
>         for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) {
> 
> That would make min and max correct and reduce the chances somebody in
> the future overlooks the fact they are currently filled with garbage.

I prefer my solution, because the check is done only in the 'write'
case, while its done also for 'read' in your solution, not counting the
for (;;) is really ugly...

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Mon, Oct 04, 2010 at 11:04:18AM +0200, Eric Dumazet wrote:
>Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit :
>> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote:
>> > When proc_doulongvec_minmax() is used with an array of longs,
>> > and no min/max check requested (.extra1 or .extra2 being NULL), we
>> > dereference a NULL pointer for the second element of the array.
>> > 
>> > Noticed while doing some changes in network stack for the "16TB problem"
>> > 
>> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> > ---
>> >  kernel/sysctl.c |    3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> > index f88552c..4fba86d 100644
>> > --- a/kernel/sysctl.c
>> > +++ b/kernel/sysctl.c
>> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>> >  				break;
>> >  			if (neg)
>> >  				continue;
>> > -			if ((min && val < *min) || (max && val > *max))
>> > +			if ((table->extra1 && val < *min) ||
>> > +			    (table->extra2 && val > *max))
>> 
>> How about changing:
>>         for (; left && vleft--; i++, min++, max++, first=0) {
>> into:
>>         for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) {
>> 
>> That would make min and max correct and reduce the chances somebody in
>> the future overlooks the fact they are currently filled with garbage.
>
>I prefer my solution, because the check is done only in the 'write'
>case, while its done also for 'read' in your solution, not counting the
>for (;;) is really ugly...
>

Sorry, I still don't get the point here, min and max
are pointers, they are already checked before dereferenced.
After your patch, min and max will be still increased, while
you are still checking ->extra{1,2} which is wrong.

I see no problem with the original code, or I must have missed something?

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric Dumazet]

Le lundi 04 octobre 2010 à 17:34 +0800, Américo Wang a écrit :
> On Mon, Oct 04, 2010 at 11:04:18AM +0200, Eric Dumazet wrote:
> >Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit :
> >> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote:
> >> > When proc_doulongvec_minmax() is used with an array of longs,
> >> > and no min/max check requested (.extra1 or .extra2 being NULL), we
> >> > dereference a NULL pointer for the second element of the array.
> >> > 
> >> > Noticed while doing some changes in network stack for the "16TB problem"
> >> > 
> >> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> > ---
> >> >  kernel/sysctl.c |    3 ++-
> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >> > index f88552c..4fba86d 100644
> >> > --- a/kernel/sysctl.c
> >> > +++ b/kernel/sysctl.c
> >> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >> >  				break;
> >> >  			if (neg)
> >> >  				continue;
> >> > -			if ((min && val < *min) || (max && val > *max))
> >> > +			if ((table->extra1 && val < *min) ||
> >> > +			    (table->extra2 && val > *max))
> >> 
> >> How about changing:
> >>         for (; left && vleft--; i++, min++, max++, first=0) {
> >> into:
> >>         for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) {
> >> 
> >> That would make min and max correct and reduce the chances somebody in
> >> the future overlooks the fact they are currently filled with garbage.
> >
> >I prefer my solution, because the check is done only in the 'write'
> >case, while its done also for 'read' in your solution, not counting the
> >for (;;) is really ugly...
> >
> 
> Sorry, I still don't get the point here, min and max
> are pointers, they are already checked before dereferenced.
> After your patch, min and max will be still increased, while
> you are still checking ->extra{1,2} which is wrong.
> 
> I see no problem with the original code, or I must have missed something?

Please re-read again. I had crashes, so original code is bugyy.

Say you call __do_proc_doulongvec_minmax() with an array of three
elements. And .extra1 = NULL, .extra2 = NULL (no range checks, this is a
valid use case)

First element, min = NULL OK. no problem so far.

Second element, min = (long *)(NULL + sizeof(long))   -> BUG 

Third element, min = (long *)(NULL + 2*sizeof(long)) -> BUG 

After my patch, min/max increases normally (they are only pointers after
all)

But they are _dereferenced_ only if they were _not_ NULL at the
beginning (extra1 not NULL for *min, extra2 not NULL for *max)

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Mon, Oct 04, 2010 at 12:10:30PM +0200, Eric Dumazet wrote:
>Le lundi 04 octobre 2010 à 17:34 +0800, Américo Wang a écrit :
>> On Mon, Oct 04, 2010 at 11:04:18AM +0200, Eric Dumazet wrote:
>> >Le lundi 04 octobre 2010 à 03:59 -0500, Robin Holt a écrit :
>> >> On Sat, Oct 02, 2010 at 03:17:49PM +0200, Eric Dumazet wrote:
>> >> > When proc_doulongvec_minmax() is used with an array of longs,
>> >> > and no min/max check requested (.extra1 or .extra2 being NULL), we
>> >> > dereference a NULL pointer for the second element of the array.
>> >> > 
>> >> > Noticed while doing some changes in network stack for the "16TB problem"
>> >> > 
>> >> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> >> > ---
>> >> >  kernel/sysctl.c |    3 ++-
>> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >> > 
>> >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> >> > index f88552c..4fba86d 100644
>> >> > --- a/kernel/sysctl.c
>> >> > +++ b/kernel/sysctl.c
>> >> > @@ -2500,7 +2500,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>> >> >  				break;
>> >> >  			if (neg)
>> >> >  				continue;
>> >> > -			if ((min && val < *min) || (max && val > *max))
>> >> > +			if ((table->extra1 && val < *min) ||
>> >> > +			    (table->extra2 && val > *max))
>> >> 
>> >> How about changing:
>> >>         for (; left && vleft--; i++, min++, max++, first=0) {
>> >> into:
>> >>         for (; left && vleft--; i++, min = min ? min + 1 : NULL, max = max ? max + 1: NULL, first=0) {
>> >> 
>> >> That would make min and max correct and reduce the chances somebody in
>> >> the future overlooks the fact they are currently filled with garbage.
>> >
>> >I prefer my solution, because the check is done only in the 'write'
>> >case, while its done also for 'read' in your solution, not counting the
>> >for (;;) is really ugly...
>> >
>> 
>> Sorry, I still don't get the point here, min and max
>> are pointers, they are already checked before dereferenced.
>> After your patch, min and max will be still increased, while
>> you are still checking ->extra{1,2} which is wrong.
>> 
>> I see no problem with the original code, or I must have missed something?
>
>Please re-read again. I had crashes, so original code is bugyy.
>
>Say you call __do_proc_doulongvec_minmax() with an array of three
>elements. And .extra1 = NULL, .extra2 = NULL (no range checks, this is a
>valid use case)
>
>First element, min = NULL OK. no problem so far.
>
>Second element, min = (long *)(NULL + sizeof(long))   -> BUG 
>
>Third element, min = (long *)(NULL + 2*sizeof(long)) -> BUG 
>
>After my patch, min/max increases normally (they are only pointers after
>all)
>
>But they are _dereferenced_ only if they were _not_ NULL at the
>beginning (extra1 not NULL for *min, extra2 not NULL for *max)
>

Hmm, I see, thanks for explanation.

Your patch does fix the problem, but seems not a good solution,
we should skip all min/max checking if ->extra(1|2) is NULL,
instead of checking it every time within the loop.

Thanks.

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric Dumazet]

Le lundi 04 octobre 2010 à 18:35 +0800, Américo Wang a écrit :

> Your patch does fix the problem, but seems not a good solution,
> we should skip all min/max checking if ->extra(1|2) is NULL,
> instead of checking it every time within the loop.

Please do submit a patch, we'll see if you come to a better solution,
with no added code size (this is slow path, I dont care for checking it
'every time winthin the loop')

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Mon, Oct 04, 2010 at 12:38:21PM +0200, Eric Dumazet wrote:
>Le lundi 04 octobre 2010 à 18:35 +0800, Américo Wang a écrit :
>
>> Your patch does fix the problem, but seems not a good solution,
>> we should skip all min/max checking if ->extra(1|2) is NULL,
>> instead of checking it every time within the loop.
>
>Please do submit a patch, we'll see if you come to a better solution,
>with no added code size (this is slow path, I dont care for checking it
>'every time winthin the loop')
>
>

I have one, but just did compile test. :)
I will test it tomorrow.

NOT-Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..345a193 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2448,86 +2448,119 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
-static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
-				     void __user *buffer,
+static int __doulongvec_minmax_read(void *data, void __user *buffer,
 				     size_t *lenp, loff_t *ppos,
 				     unsigned long convmul,
 				     unsigned long convdiv)
 {
-	unsigned long *i, *min, *max;
-	int vleft, first = 1, err = 0;
-	unsigned long page = 0;
-	size_t left;
-	char *kbuf;
+	unsigned long *i = (unsigned long *) data;
+	int err = 0;
+	bool first = true;
+	size_t left = *lenp;
 
-	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
-		*lenp = 0;
-		return 0;
+	for (; left; i++, first=false) {
+		unsigned long val;
+
+		val = convdiv * (*i) / convmul;
+		if (!first)
+			err = proc_put_char(&buffer, &left, '\t');
+		err = proc_put_long(&buffer, &left, val, false);
+		if (err)
+			break;
 	}
 
-	i = (unsigned long *) data;
-	min = (unsigned long *) table->extra1;
-	max = (unsigned long *) table->extra2;
-	vleft = table->maxlen / sizeof(unsigned long);
-	left = *lenp;
+	if (!first && left && !err)
+		err = proc_put_char(&buffer, &left, '\n');
 
-	if (write) {
-		if (left > PAGE_SIZE - 1)
-			left = PAGE_SIZE - 1;
-		page = __get_free_page(GFP_TEMPORARY);
-		kbuf = (char *) page;
-		if (!kbuf)
-			return -ENOMEM;
-		if (copy_from_user(kbuf, buffer, left)) {
-			err = -EFAULT;
-			goto free;
-		}
-		kbuf[left] = 0;
+	*lenp -= left;
+	*ppos += *lenp;
+	return err;
+}
+
+static int __doulongvec_minmax_write(void *data, void __user *buffer,
+				     size_t *lenp, loff_t *ppos, int vleft,
+				     unsigned long min, unsigned long max)
+{
+	char *kbuf;
+	size_t left = *lenp;
+	unsigned long page = 0;
+	unsigned long *i = (unsigned long *) data;
+	int err = 0;
+	bool first = true;
+
+	if (left > PAGE_SIZE - 1)
+		left = PAGE_SIZE - 1;
+	page = __get_free_page(GFP_TEMPORARY);
+	kbuf = (char *) page;
+	if (!kbuf)
+		return -ENOMEM;
+	if (copy_from_user(kbuf, buffer, left)) {
+		err = -EFAULT;
+		goto free;
 	}
+	kbuf[left] = 0;
 
-	for (; left && vleft--; i++, min++, max++, first=0) {
+	for (; left && vleft--; i++, min++, max++, first=false) {
 		unsigned long val;
+		bool neg;
 
-		if (write) {
-			bool neg;
-
-			left -= proc_skip_spaces(&kbuf);
+		left -= proc_skip_spaces(&kbuf);
 
-			err = proc_get_long(&kbuf, &left, &val, &neg,
-					     proc_wspace_sep,
-					     sizeof(proc_wspace_sep), NULL);
-			if (err)
-				break;
-			if (neg)
-				continue;
-			if ((min && val < *min) || (max && val > *max))
-				continue;
-			*i = val;
-		} else {
-			val = convdiv * (*i) / convmul;
-			if (!first)
-				err = proc_put_char(&buffer, &left, '\t');
-			err = proc_put_long(&buffer, &left, val, false);
-			if (err)
-				break;
-		}
+		err = proc_get_long(&kbuf, &left, &val, &neg,
+				     proc_wspace_sep,
+				     sizeof(proc_wspace_sep), NULL);
+		if (err)
+			break;
+		if (neg)
+			continue;
+		if (val < min || val > max)
+			continue;
+		*i = val;
 	}
 
-	if (!write && !first && left && !err)
-		err = proc_put_char(&buffer, &left, '\n');
-	if (write && !err)
+	if (!err)
 		left -= proc_skip_spaces(&kbuf);
 free:
-	if (write) {
-		free_page(page);
-		if (first)
-			return err ? : -EINVAL;
-	}
+	free_page(page);
+	if (first)
+		return err ? : -EINVAL;
+
 	*lenp -= left;
 	*ppos += *lenp;
 	return err;
 }
 
+static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
+				     void __user *buffer,
+				     size_t *lenp, loff_t *ppos,
+				     unsigned long convmul,
+				     unsigned long convdiv)
+{
+	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
+		*lenp = 0;
+		return 0;
+	}
+
+	if (write) {
+		unsigned long min, max;
+		int vleft;
+
+		vleft = table->maxlen / sizeof(unsigned long);
+		if (table->extra1)
+			min = *(unsigned long *) table->extra1;
+		else
+			min = 0;
+		if (table->extra2)
+			max = *(unsigned long *) table->extra2;
+		else
+			max = ULONG_MAX;
+		return __doulongvec_minmax_write(data, buffer, lenp,
+						  ppos, vleft, min, max);
+	} else
+		return __doulongvec_minmax_read(data, buffer, lenp,
+						 ppos, convmul, convdiv);
+}
+
 static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,
 				     void __user *buffer,
 				     size_t *lenp, loff_t *ppos,

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Tue, Oct 05, 2010 at 09:01:17PM +0800, Américo Wang wrote:
>On Mon, Oct 04, 2010 at 12:38:21PM +0200, Eric Dumazet wrote:
>>Le lundi 04 octobre 2010 à 18:35 +0800, Américo Wang a écrit :
>>
>>> Your patch does fix the problem, but seems not a good solution,
>>> we should skip all min/max checking if ->extra(1|2) is NULL,
>>> instead of checking it every time within the loop.
>>
>>Please do submit a patch, we'll see if you come to a better solution,
>>with no added code size (this is slow path, I dont care for checking it
>>'every time winthin the loop')
>>
>>
>
>I have one, but just did compile test. :)
>I will test it tomorrow.
>

Here is the final one.

--------------->

Eric D. noticed that we may trigger an OOPS if we leave ->extra{1,2}
to NULL when we use proc_doulongvec_minmax().

Actually, we don't need to store min/max values in a vector,
because all the elements in the vector should share the same min/max
value, like what proc_dointvec_minmax() does.

This also shrinks the binary size a little bit.

   text    data     bss     dec     hex filename
  12419    8744    4016   25179    625b kernel/sysctl.o.BEFORE
  12395    8744    4024   25163    624b kernel/sysctl.o.AFTER

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Eric W. <ebiederm@xmission.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..ba5e511 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2448,86 +2448,119 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
-static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
-				     void __user *buffer,
+static int __doulongvec_minmax_read(void *data, void __user *buffer,
 				     size_t *lenp, loff_t *ppos,
 				     unsigned long convmul,
 				     unsigned long convdiv)
 {
-	unsigned long *i, *min, *max;
-	int vleft, first = 1, err = 0;
-	unsigned long page = 0;
-	size_t left;
-	char *kbuf;
+	unsigned long *i = (unsigned long *) data;
+	int err = 0;
+	bool first = true;
+	size_t left = *lenp;
 
-	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
-		*lenp = 0;
-		return 0;
+	for (; left; i++, first=false) {
+		unsigned long val;
+
+		val = convdiv * (*i) / convmul;
+		if (!first)
+			err = proc_put_char(&buffer, &left, '\t');
+		err = proc_put_long(&buffer, &left, val, false);
+		if (err)
+			break;
 	}
 
-	i = (unsigned long *) data;
-	min = (unsigned long *) table->extra1;
-	max = (unsigned long *) table->extra2;
-	vleft = table->maxlen / sizeof(unsigned long);
-	left = *lenp;
+	if (!first && left && !err)
+		err = proc_put_char(&buffer, &left, '\n');
 
-	if (write) {
-		if (left > PAGE_SIZE - 1)
-			left = PAGE_SIZE - 1;
-		page = __get_free_page(GFP_TEMPORARY);
-		kbuf = (char *) page;
-		if (!kbuf)
-			return -ENOMEM;
-		if (copy_from_user(kbuf, buffer, left)) {
-			err = -EFAULT;
-			goto free;
-		}
-		kbuf[left] = 0;
+	*lenp -= left;
+	*ppos += *lenp;
+	return err;
+}
+
+static int __doulongvec_minmax_write(void *data, void __user *buffer,
+				     size_t *lenp, loff_t *ppos, int vleft,
+				     unsigned long min, unsigned long max)
+{
+	char *kbuf;
+	size_t left = *lenp;
+	unsigned long page = 0;
+	unsigned long *i = (unsigned long *) data;
+	int err = 0;
+	bool first = true;
+
+	if (left > PAGE_SIZE - 1)
+		left = PAGE_SIZE - 1;
+	page = __get_free_page(GFP_TEMPORARY);
+	kbuf = (char *) page;
+	if (!kbuf)
+		return -ENOMEM;
+	if (copy_from_user(kbuf, buffer, left)) {
+		err = -EFAULT;
+		goto free;
 	}
+	kbuf[left] = 0;
 
-	for (; left && vleft--; i++, min++, max++, first=0) {
+	for (; left && vleft--; i++, first=false) {
 		unsigned long val;
+		bool neg;
 
-		if (write) {
-			bool neg;
-
-			left -= proc_skip_spaces(&kbuf);
+		left -= proc_skip_spaces(&kbuf);
 
-			err = proc_get_long(&kbuf, &left, &val, &neg,
-					     proc_wspace_sep,
-					     sizeof(proc_wspace_sep), NULL);
-			if (err)
-				break;
-			if (neg)
-				continue;
-			if ((min && val < *min) || (max && val > *max))
-				continue;
-			*i = val;
-		} else {
-			val = convdiv * (*i) / convmul;
-			if (!first)
-				err = proc_put_char(&buffer, &left, '\t');
-			err = proc_put_long(&buffer, &left, val, false);
-			if (err)
-				break;
-		}
+		err = proc_get_long(&kbuf, &left, &val, &neg,
+				     proc_wspace_sep,
+				     sizeof(proc_wspace_sep), NULL);
+		if (err)
+			break;
+		if (neg)
+			continue;
+		if (val < min || val > max)
+			continue;
+		*i = val;
 	}
 
-	if (!write && !first && left && !err)
-		err = proc_put_char(&buffer, &left, '\n');
-	if (write && !err)
+	if (!err)
 		left -= proc_skip_spaces(&kbuf);
 free:
-	if (write) {
-		free_page(page);
-		if (first)
-			return err ? : -EINVAL;
-	}
+	free_page(page);
+	if (first)
+		return err ? : -EINVAL;
+
 	*lenp -= left;
 	*ppos += *lenp;
 	return err;
 }
 
+static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
+				     void __user *buffer,
+				     size_t *lenp, loff_t *ppos,
+				     unsigned long convmul,
+				     unsigned long convdiv)
+{
+	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
+		*lenp = 0;
+		return 0;
+	}
+
+	if (write) {
+		unsigned long min, max;
+		int vleft;
+
+		vleft = table->maxlen / sizeof(unsigned long);
+		if (table->extra1)
+			min = *(unsigned long *) table->extra1;
+		else
+			min = 0;
+		if (table->extra2)
+			max = *(unsigned long *) table->extra2;
+		else
+			max = ULONG_MAX;
+		return __doulongvec_minmax_write(data, buffer, lenp,
+						  ppos, vleft, min, max);
+	} else
+		return __doulongvec_minmax_read(data, buffer, lenp,
+						 ppos, convmul, convdiv);
+}
+
 static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,
 				     void __user *buffer,
 				     size_t *lenp, loff_t *ppos,

[PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

>>
>
>Here is the final one.

Oops, that one is not correct. Hopefully this one
is correct.

--------------->

Eric D. noticed that we may trigger an OOPS if we leave ->extra{1,2}
to NULL when we use proc_doulongvec_minmax().

Actually, we don't need to store min/max values in a vector,
because all the elements in the vector should share the same min/max
value, like what proc_dointvec_minmax() does.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Eric W. <ebiederm@xmission.com>
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..fad9208 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2448,86 +2448,119 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
-static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
-				     void __user *buffer,
-				     size_t *lenp, loff_t *ppos,
+static int __doulongvec_minmax_read(void *data, void __user *buffer,
+				     size_t *lenp, loff_t *ppos, int vleft,
 				     unsigned long convmul,
 				     unsigned long convdiv)
 {
-	unsigned long *i, *min, *max;
-	int vleft, first = 1, err = 0;
-	unsigned long page = 0;
-	size_t left;
-	char *kbuf;
+	unsigned long *i = data;
+	int err = 0;
+	bool first = true;
+	size_t left = *lenp;
 
-	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
-		*lenp = 0;
-		return 0;
+	for (; left && vleft--; i++, first=false) {
+		unsigned long val;
+
+		val = convdiv * (*i) / convmul;
+		if (!first)
+			err = proc_put_char(&buffer, &left, '\t');
+		err = proc_put_long(&buffer, &left, val, false);
+		if (err)
+			break;
 	}
 
-	i = (unsigned long *) data;
-	min = (unsigned long *) table->extra1;
-	max = (unsigned long *) table->extra2;
-	vleft = table->maxlen / sizeof(unsigned long);
-	left = *lenp;
+	if (!first && left && !err)
+		err = proc_put_char(&buffer, &left, '\n');
 
-	if (write) {
-		if (left > PAGE_SIZE - 1)
-			left = PAGE_SIZE - 1;
-		page = __get_free_page(GFP_TEMPORARY);
-		kbuf = (char *) page;
-		if (!kbuf)
-			return -ENOMEM;
-		if (copy_from_user(kbuf, buffer, left)) {
-			err = -EFAULT;
-			goto free;
-		}
-		kbuf[left] = 0;
+	*lenp -= left;
+	*ppos += *lenp;
+	return err;
+}
+
+static int __doulongvec_minmax_write(void *data, void __user *buffer,
+				     size_t *lenp, loff_t *ppos, int vleft,
+				     unsigned long min, unsigned long max)
+{
+	char *kbuf;
+	size_t left = *lenp;
+	unsigned long page = 0;
+	unsigned long *i = (unsigned long *) data;
+	int err = 0;
+	bool first = true;
+
+	if (left > PAGE_SIZE - 1)
+		left = PAGE_SIZE - 1;
+	page = __get_free_page(GFP_TEMPORARY);
+	kbuf = (char *) page;
+	if (!kbuf)
+		return -ENOMEM;
+	if (copy_from_user(kbuf, buffer, left)) {
+		err = -EFAULT;
+		goto free;
 	}
+	kbuf[left] = 0;
 
-	for (; left && vleft--; i++, min++, max++, first=0) {
+	for (; left && vleft--; i++, first=false) {
 		unsigned long val;
+		bool neg;
 
-		if (write) {
-			bool neg;
-
-			left -= proc_skip_spaces(&kbuf);
+		left -= proc_skip_spaces(&kbuf);
 
-			err = proc_get_long(&kbuf, &left, &val, &neg,
-					     proc_wspace_sep,
-					     sizeof(proc_wspace_sep), NULL);
-			if (err)
-				break;
-			if (neg)
-				continue;
-			if ((min && val < *min) || (max && val > *max))
-				continue;
-			*i = val;
-		} else {
-			val = convdiv * (*i) / convmul;
-			if (!first)
-				err = proc_put_char(&buffer, &left, '\t');
-			err = proc_put_long(&buffer, &left, val, false);
-			if (err)
-				break;
-		}
+		err = proc_get_long(&kbuf, &left, &val, &neg,
+				     proc_wspace_sep,
+				     sizeof(proc_wspace_sep), NULL);
+		if (err)
+			break;
+		if (neg)
+			continue;
+		if (val < min || val > max)
+			continue;
+		*i = val;
 	}
 
-	if (!write && !first && left && !err)
-		err = proc_put_char(&buffer, &left, '\n');
-	if (write && !err)
+	if (!err)
 		left -= proc_skip_spaces(&kbuf);
 free:
-	if (write) {
-		free_page(page);
-		if (first)
-			return err ? : -EINVAL;
-	}
+	free_page(page);
+	if (first)
+		return err ? : -EINVAL;
+
 	*lenp -= left;
 	*ppos += *lenp;
 	return err;
 }
 
+static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
+				     void __user *buffer,
+				     size_t *lenp, loff_t *ppos,
+				     unsigned long convmul,
+				     unsigned long convdiv)
+{
+	int vleft;
+	if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
+		*lenp = 0;
+		return 0;
+	}
+
+	vleft = table->maxlen / sizeof(unsigned long);
+	if (write) {
+		unsigned long min, max;
+
+		if (table->extra1)
+			min = *(unsigned long *) table->extra1;
+		else
+			min = 0;
+		if (table->extra2)
+			max = *(unsigned long *) table->extra2;
+		else
+			max = ULONG_MAX;
+		return __doulongvec_minmax_write(data, buffer, lenp,
+						  ppos, vleft, min, max);
+	} else
+		return __doulongvec_minmax_read(data, buffer, lenp,
+						 ppos, vleft, convmul, convdiv);
+}
+
 static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,
 				     void __user *buffer,
 				     size_t *lenp, loff_t *ppos,

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric Dumazet]

Le jeudi 07 octobre 2010 à 17:25 +0800, Américo Wang a écrit :
> >>
> >
> >Here is the final one.
> 
> Oops, that one is not correct. Hopefully this one
> is correct.
> 
> --------------->
> 
> Eric D. noticed that we may trigger an OOPS if we leave ->extra{1,2}
> to NULL when we use proc_doulongvec_minmax().
> 
> Actually, we don't need to store min/max values in a vector,
> because all the elements in the vector should share the same min/max
> value, like what proc_dointvec_minmax() does.
> 

If we assert same min/max limits are to be applied to all elements,
a much simpler fix than yours would be :

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..8e45451 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 		kbuf[left] = 0;
 	}
 
-	for (; left && vleft--; i++, min++, max++, first=0) {
+	for (; left && vleft--; i++, first=0) {
 		unsigned long val;
 
 		if (write) {


Please dont send huge patches like this to 'fix' a bug,
especially on slow path.

First we fix the bug, _then_ we can try to make code more 
efficient or more pretty or shorter.

So the _real_ question is :

Should the min/max limits should be a single pair,
shared by all elements, or a vector of limits.

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric W. Biederman]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> Le jeudi 07 octobre 2010 à 17:25 +0800, Américo Wang a écrit :
>> >>
>> >
>> >Here is the final one.
>> 
>> Oops, that one is not correct. Hopefully this one
>> is correct.
>> 
>> --------------->
>> 
>> Eric D. noticed that we may trigger an OOPS if we leave ->extra{1,2}
>> to NULL when we use proc_doulongvec_minmax().
>> 
>> Actually, we don't need to store min/max values in a vector,
>> because all the elements in the vector should share the same min/max
>> value, like what proc_dointvec_minmax() does.
>> 
>
> If we assert same min/max limits are to be applied to all elements,
> a much simpler fix than yours would be :
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f88552c..8e45451 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>  		kbuf[left] = 0;
>  	}
>  
> -	for (; left && vleft--; i++, min++, max++, first=0) {
> +	for (; left && vleft--; i++, first=0) {
>  		unsigned long val;
>  
>  		if (write) {
>
>
> Please dont send huge patches like this to 'fix' a bug,
> especially on slow path.
>
> First we fix the bug, _then_ we can try to make code more 
> efficient or more pretty or shorter.
>
> So the _real_ question is :
>
> Should the min/max limits should be a single pair,
> shared by all elements, or a vector of limits.

The difference between long handling and int handling is a
usability issue.  I don't expect we will be exporting new
vectors via sysctl, so the conversion of a handful of vectors
from int to long is where this is most likely to be used.

I skimmed through all of what I presume are the current users
aka linux-2.6.36-rcX and there don't appear to be any users
of proc_dounlongvec_minmax that use it's vector properties there.

Which doubly tells me that incrementing the min and max pointers
is not what we want to do.

Eric

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric Dumazet]

Le jeudi 07 octobre 2010 à 09:37 -0700, Eric W. Biederman a écrit :

> The difference between long handling and int handling is a
> usability issue.  I don't expect we will be exporting new
> vectors via sysctl, so the conversion of a handful of vectors
> from int to long is where this is most likely to be used.
> 
> I skimmed through all of what I presume are the current users
> aka linux-2.6.36-rcX and there don't appear to be any users
> of proc_dounlongvec_minmax that use it's vector properties there.
> 
> Which doubly tells me that incrementing the min and max pointers
> is not what we want to do.
> 

Thats fine by me, thanks Eric.

Andrew, please remove previous patch from your tree and replace it by
following one :

[PATCH v2] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

When proc_doulongvec_minmax() is used with an array of longs,
and no min/max check requested (.extra1 or .extra2 being NULL), we
dereference a NULL pointer for the second element of the array.

Noticed while doing some changes in network stack for the "16TB problem"

Fix is to not change min & max pointers in
__do_proc_doulongvec_minmax(), so that all elements of the vector share
an unique min/max limit, like proc_dointvec_minmax().

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 kernel/sysctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..8e45451 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 		kbuf[left] = 0;
 	}
 
-	for (; left && vleft--; i++, min++, max++, first=0) {
+	for (; left && vleft--; i++, first=0) {
 		unsigned long val;
 
 		if (write) {

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Andrew Morton]

On Thu, 07 Oct 2010 18:59:03 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le jeudi 07 octobre 2010 __ 09:37 -0700, Eric W. Biederman a __crit :
> 
> > The difference between long handling and int handling is a
> > usability issue.  I don't expect we will be exporting new
> > vectors via sysctl, so the conversion of a handful of vectors
> > from int to long is where this is most likely to be used.
> > 
> > I skimmed through all of what I presume are the current users
> > aka linux-2.6.36-rcX and there don't appear to be any users
> > of proc_dounlongvec_minmax that use it's vector properties there.
> > 
> > Which doubly tells me that incrementing the min and max pointers
> > is not what we want to do.
> > 
> 
> Thats fine by me, thanks Eric.
> 
> Andrew, please remove previous patch from your tree and replace it by
> following one :
> 
> [PATCH v2] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()
> 
> When proc_doulongvec_minmax() is used with an array of longs,
> and no min/max check requested (.extra1 or .extra2 being NULL), we
> dereference a NULL pointer for the second element of the array.
> 
> Noticed while doing some changes in network stack for the "16TB problem"
> 
> Fix is to not change min & max pointers in
> __do_proc_doulongvec_minmax(), so that all elements of the vector share
> an unique min/max limit, like proc_dointvec_minmax().
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  kernel/sysctl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f88552c..8e45451 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>  		kbuf[left] = 0;
>  	}
>  
> -	for (; left && vleft--; i++, min++, max++, first=0) {
> +	for (; left && vleft--; i++, first=0) {
>  		unsigned long val;
>  
>  		if (write) {

Did we check to see whether any present callers are passing in pointers
to arrays of min/max values?

I wonder if there's any documentation for this interface which just
became wrong.

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Eric W. Biederman]

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

> On Thu, 07 Oct 2010 18:59:03 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:

>> Thats fine by me, thanks Eric.
>> 
>> Andrew, please remove previous patch from your tree and replace it by
>> following one :
>> 
>> [PATCH v2] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()
>> 
>> When proc_doulongvec_minmax() is used with an array of longs,
>> and no min/max check requested (.extra1 or .extra2 being NULL), we
>> dereference a NULL pointer for the second element of the array.
>> 
>> Noticed while doing some changes in network stack for the "16TB problem"
>> 
>> Fix is to not change min & max pointers in
>> __do_proc_doulongvec_minmax(), so that all elements of the vector share
>> an unique min/max limit, like proc_dointvec_minmax().
>> 
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>>  kernel/sysctl.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index f88552c..8e45451 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>>  		kbuf[left] = 0;
>>  	}
>>  
>> -	for (; left && vleft--; i++, min++, max++, first=0) {
>> +	for (; left && vleft--; i++, first=0) {
>>  		unsigned long val;
>>  
>>  		if (write) {
>
> Did we check to see whether any present callers are passing in pointers
> to arrays of min/max values?

In 2.6.36 there are not any callers that pass in a vector of anything, I
don't know about linux-next.  It looks to me like incrementing min and
max was simply a bug.

> I wonder if there's any documentation for this interface which just
> became wrong.

Or it just became right.  Clearly no one has been expecting min
and max to be vectors.

Eric

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Thu, Oct 07, 2010 at 12:38:22PM -0700, Eric W. Biederman wrote:
>Andrew Morton <akpm@linux-foundation.org> writes:
>
>> On Thu, 07 Oct 2010 18:59:03 +0200
>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>>> Thats fine by me, thanks Eric.
>>> 
>>> Andrew, please remove previous patch from your tree and replace it by
>>> following one :
>>> 
>>> [PATCH v2] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()
>>> 
>>> When proc_doulongvec_minmax() is used with an array of longs,
>>> and no min/max check requested (.extra1 or .extra2 being NULL), we
>>> dereference a NULL pointer for the second element of the array.
>>> 
>>> Noticed while doing some changes in network stack for the "16TB problem"
>>> 
>>> Fix is to not change min & max pointers in
>>> __do_proc_doulongvec_minmax(), so that all elements of the vector share
>>> an unique min/max limit, like proc_dointvec_minmax().
>>> 
>>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>>> ---
>>>  kernel/sysctl.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>> index f88552c..8e45451 100644
>>> --- a/kernel/sysctl.c
>>> +++ b/kernel/sysctl.c
>>> @@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>>>  		kbuf[left] = 0;
>>>  	}
>>>  
>>> -	for (; left && vleft--; i++, min++, max++, first=0) {
>>> +	for (; left && vleft--; i++, first=0) {
>>>  		unsigned long val;
>>>  
>>>  		if (write) {
>>
>> Did we check to see whether any present callers are passing in pointers
>> to arrays of min/max values?
>
>In 2.6.36 there are not any callers that pass in a vector of anything, I
>don't know about linux-next.  It looks to me like incrementing min and
>max was simply a bug.
>

Agreed, I checked them too.

>> I wonder if there's any documentation for this interface which just
>> became wrong.
>
>Or it just became right.  Clearly no one has been expecting min
>and max to be vectors.
>

I think we need to document this before we rewrite the code.

-- 
Live like a child, think like the god.
 

Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

[Américo Wang]

On Thu, Oct 07, 2010 at 11:51:21AM +0200, Eric Dumazet wrote:
>Le jeudi 07 octobre 2010 à 17:25 +0800, Américo Wang a écrit :
>> >>
>> >
>> >Here is the final one.
>> 
>> Oops, that one is not correct. Hopefully this one
>> is correct.
>> 
>> --------------->
>> 
>> Eric D. noticed that we may trigger an OOPS if we leave ->extra{1,2}
>> to NULL when we use proc_doulongvec_minmax().
>> 
>> Actually, we don't need to store min/max values in a vector,
>> because all the elements in the vector should share the same min/max
>> value, like what proc_dointvec_minmax() does.
>> 
>
>If we assert same min/max limits are to be applied to all elements,
>a much simpler fix than yours would be :
>
>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>index f88552c..8e45451 100644
>--- a/kernel/sysctl.c
>+++ b/kernel/sysctl.c
>@@ -2485,7 +2485,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> 		kbuf[left] = 0;
> 	}
> 
>-	for (; left && vleft--; i++, min++, max++, first=0) {
>+	for (; left && vleft--; i++, first=0) {
> 		unsigned long val;
> 
> 		if (write) {
>
>
>Please dont send huge patches like this to 'fix' a bug,
>especially on slow path.

Well, my patch makes that horrible code a little better. :)

>
>First we fix the bug, _then_ we can try to make code more 
>efficient or more pretty or shorter.
>
>So the _real_ question is :
>
>Should the min/max limits should be a single pair,
>shared by all elements, or a vector of limits.
>

Yes, actually I talked with Eric W. about this before
sending the patch.

I also checked the users of proc_doulongvec_minmax(),
none of them are using more than one limit, so it is
safe to remove that.


-- 
Live like a child, think like the god.