sysctl: fix min/max handling in __do_proc_doulongvec_minmax()

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(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.