From: ebiederm@xmission•com (Eric W. Biederman)
To: Eric Dumazet <eric.dumazet@gmail•com>
Cc: "Américo Wang" <xiyou.wangcong@gmail•com>,
"Robin Holt" <holt@sgi•com>,
"Andrew Morton" <akpm@linux-foundation•org>,
linux-kernel <linux-kernel@vger•kernel.org>,
"Willy Tarreau" <w@1wt•eu>,
"David S. Miller" <davem@davemloft•net>,
netdev@vger•kernel.org, "James Morris" <jmorris@namei•org>,
"Pekka Savola (ipv6)" <pekkas@netcore•fi>,
"Patrick McHardy" <kaber@trash•net>,
"Alexey Kuznetsov" <kuznet@ms2•inr.ac.ru>
Subject: Re: [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax()
Date: Thu, 07 Oct 2010 09:37:32 -0700 [thread overview]
Message-ID: <m1iq1e3qnn.fsf@fess.ebiederm.org> (raw)
In-Reply-To: <1286445081.2912.15.camel@edumazet-laptop> (Eric Dumazet's message of "Thu, 07 Oct 2010 11:51:21 +0200")
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
next prev parent reply other threads:[~2010-10-07 16:37 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-02 13:17 [PATCH] sysctl: fix min/max handling in __do_proc_doulongvec_minmax() Eric Dumazet
2010-10-04 3:09 ` Américo Wang
2010-10-04 8:59 ` Robin Holt
2010-10-04 9:04 ` Eric Dumazet
2010-10-04 9:34 ` Américo Wang
2010-10-04 10:10 ` Eric Dumazet
2010-10-04 10:35 ` Américo Wang
2010-10-04 10:38 ` Eric Dumazet
2010-10-05 13:01 ` Américo Wang
2010-10-07 7:18 ` Américo Wang
2010-10-07 9:25 ` Américo Wang
2010-10-07 9:51 ` Eric Dumazet
2010-10-07 16:37 ` Eric W. Biederman [this message]
2010-10-07 16:59 ` Eric Dumazet
2010-10-07 19:18 ` Andrew Morton
2010-10-07 19:38 ` Eric W. Biederman
2010-10-08 16:22 ` Américo Wang
2010-10-08 16:13 ` Américo Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m1iq1e3qnn.fsf@fess.ebiederm.org \
--to=ebiederm@xmission$(echo .)com \
--cc=akpm@linux-foundation$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=holt@sgi$(echo .)com \
--cc=jmorris@namei$(echo .)org \
--cc=kaber@trash$(echo .)net \
--cc=kuznet@ms2$(echo .)inr.ac.ru \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pekkas@netcore$(echo .)fi \
--cc=w@1wt$(echo .)eu \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox