[PATCH net] net:sysctl fix the confusing corner of tcp_mem

[PATCH net] net:sysctl fix the confusing corner of tcp_mem

[Wang Yufen]

From: Yufen Wang <wangyufen@huawei.com>

I tried on linux-4.1:
    linux:~# cat /proc/sys/net/ipv4/tcp_mem 
    8388608	12582912	16777216
    linux:~# echo 1234 >/proc/sys/net/ipv4/tcp_mem 
    -bash: echo: write error: Invalid argument
    linux:~# cat /proc/sys/net/ipv4/tcp_mem 
    1234	12582912	16777216

the echo operation got error, but value already written to tcp_mem.
If a write() returns an error like EINVAL, we expect no change occurred.
This patch fix the confusing corner and makes __do_proc_doulongvec_minmax 
works the same as __do_proc_dointvec

Signed-off-by: Yufen Wang <wangyufen@huawei.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/sysctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index c3eee4c..e3ee4be 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2318,6 +2318,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 			bool neg;
 
 			left -= proc_skip_spaces(&kbuf);
+                        if (!left)
+                                break;
 
 			err = proc_get_long(&kbuf, &left, &val, &neg,
 					     proc_wspace_sep,
-- 
2.5.0

Re: [PATCH net] net:sysctl fix the confusing corner of tcp_mem

[Sergei Shtylyov]

Hello.

On 12/9/2015 5:01 AM, Wang Yufen wrote:

> From: Yufen Wang <wangyufen@huawei.com>
>
> I tried on linux-4.1:
>      linux:~# cat /proc/sys/net/ipv4/tcp_mem
>      8388608	12582912	16777216
>      linux:~# echo 1234 >/proc/sys/net/ipv4/tcp_mem
>      -bash: echo: write error: Invalid argument
>      linux:~# cat /proc/sys/net/ipv4/tcp_mem
>      1234	12582912	16777216
>
> the echo operation got error, but value already written to tcp_mem.
> If a write() returns an error like EINVAL, we expect no change occurred.
> This patch fix the confusing corner and makes __do_proc_doulongvec_minmax
> works the same as __do_proc_dointvec
>
> Signed-off-by: Yufen Wang <wangyufen@huawei.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>   kernel/sysctl.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index c3eee4c..e3ee4be 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2318,6 +2318,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>   			bool neg;
>
>   			left -= proc_skip_spaces(&kbuf);
> +                        if (!left)
> +                                break;

     Please use tabs for indentation, not spaces.

[...]

MBR, Sergei

Re: [PATCH net] net:sysctl fix the confusing corner of tcp_mem

[Eric W. Biederman]

Wang Yufen <wangyufen@huawei.com> writes:

> From: Yufen Wang <wangyufen@huawei.com>
>
> I tried on linux-4.1:
>     linux:~# cat /proc/sys/net/ipv4/tcp_mem 
>     8388608	12582912	16777216
>     linux:~# echo 1234 >/proc/sys/net/ipv4/tcp_mem 
>     -bash: echo: write error: Invalid argument
>     linux:~# cat /proc/sys/net/ipv4/tcp_mem 
>     1234	12582912	16777216
>
> the echo operation got error, but value already written to tcp_mem.
> If a write() returns an error like EINVAL, we expect no change occurred.
> This patch fix the confusing corner and makes __do_proc_doulongvec_minmax 
> works the same as __do_proc_dointvec

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Except for possibly breaking your muscle memory this does not explain
why this is a problem.

Further you are changing a whole lot more than tcp_mem, without a word
of justification in your description.

I do not think changing every integer use of sysctl for some unknown
reason.  Is justified in this case.

Eric



> Signed-off-by: Yufen Wang <wangyufen@huawei.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  kernel/sysctl.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index c3eee4c..e3ee4be 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2318,6 +2318,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>  			bool neg;
>  
>  			left -= proc_skip_spaces(&kbuf);
> +                        if (!left)
> +                                break;
>  
>  			err = proc_get_long(&kbuf, &left, &val, &neg,
>  					     proc_wspace_sep,

Re: [PATCH net] net:sysctl fix the confusing corner of tcp_mem

[Hanjun Guo]

Hi Eric,

On 2015/12/10 0:38, Eric W. Biederman wrote:
> Wang Yufen <wangyufen@huawei.com> writes:
>
>> From: Yufen Wang <wangyufen@huawei.com>
>>
>> I tried on linux-4.1:
>>     linux:~# cat /proc/sys/net/ipv4/tcp_mem 
>>     8388608	12582912	16777216
>>     linux:~# echo 1234 >/proc/sys/net/ipv4/tcp_mem 
>>     -bash: echo: write error: Invalid argument
>>     linux:~# cat /proc/sys/net/ipv4/tcp_mem 
>>     1234	12582912	16777216
>>
>> the echo operation got error, but value already written to tcp_mem.
>> If a write() returns an error like EINVAL, we expect no change occurred.
>> This patch fix the confusing corner and makes __do_proc_doulongvec_minmax 
>> works the same as __do_proc_dointvec
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Except for possibly breaking your muscle memory this does not explain
> why this is a problem.

I think it's really confusing that we got write error but actually the
value echoed is written properly, at least we need to fix it and make
the result match the behavior, right?

>
> Further you are changing a whole lot more than tcp_mem, without a word
> of justification in your description.

I agree, we can update the change log.

Thanks
Hanjun