All of lore.kernel.org
 help / color / mirror / Atom feed
* Issue with /proc/sys/net/ipv4/tcp_mem
@ 2015-10-12  9:38 wangyufen
  2015-10-12 15:24 ` Eric W. Biederman
  2015-10-12 16:37 ` Eric W. Biederman
  0 siblings, 2 replies; 6+ messages in thread
From: wangyufen @ 2015-10-12  9:38 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: netdev

Hi,

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.

I checked, patch f594d63199688ad568fb caused the issue.


thanks,
Wang

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Issue with /proc/sys/net/ipv4/tcp_mem
  2015-10-12  9:38 Issue with /proc/sys/net/ipv4/tcp_mem wangyufen
@ 2015-10-12 15:24 ` Eric W. Biederman
  2015-10-12 16:37 ` Eric W. Biederman
  1 sibling, 0 replies; 6+ messages in thread
From: Eric W. Biederman @ 2015-10-12 15:24 UTC (permalink / raw)
  To: wangyufen; +Cc: netdev

wangyufen <wangyufen@huawei.com> writes:

> Hi,
>
> 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.
>
> I checked, patch f594d63199688ad568fb caused the issue.

Use the cgroup interface if you want per cgroup control.

Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Issue with /proc/sys/net/ipv4/tcp_mem
  2015-10-12  9:38 Issue with /proc/sys/net/ipv4/tcp_mem wangyufen
  2015-10-12 15:24 ` Eric W. Biederman
@ 2015-10-12 16:37 ` Eric W. Biederman
  2015-10-13  1:44   ` Eric Dumazet
  1 sibling, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2015-10-12 16:37 UTC (permalink / raw)
  To: wangyufen; +Cc: netdev

wangyufen <wangyufen@huawei.com> writes:

> Hi,
>
> 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.
>
> I checked, patch f594d63199688ad568fb caused the issue.


If your problem is that you can not write a single value and instead
have to write all three values I don't know what to tell you.  I don't
see how that could have ever worked.

Certainly the commit you pointed at did not change that behavior.

Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Issue with /proc/sys/net/ipv4/tcp_mem
  2015-10-12 16:37 ` Eric W. Biederman
@ 2015-10-13  1:44   ` Eric Dumazet
  2015-10-13  5:07     ` Eric W. Biederman
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2015-10-13  1:44 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: wangyufen, netdev

On Mon, 2015-10-12 at 11:37 -0500, Eric W. Biederman wrote:
> wangyufen <wangyufen@huawei.com> writes:
> 
> > Hi,
> >
> > 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.
> >
> > I checked, patch f594d63199688ad568fb caused the issue.
> 
> 
> If your problem is that you can not write a single value and instead
> have to write all three values I don't know what to tell you.  I don't
> see how that could have ever worked.
> 
> Certainly the commit you pointed at did not change that behavior.

I would not be so sure.
Above commit added a regression for partial writes.
If a write() returns an error like EINVAL, we expect no change occurred.

Prior code was calling proc_doulongvec_minmax() using a temporary array,
and updated tcp_mem[0 .. 2] only of proc_doulongvec_minmax() returned 0

       ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
       if (ret)
               return ret;

#ifdef CONFIG_MEMCG_KMEM
	// deleted for clarity
#endif

       net->ipv4.sysctl_tcp_mem[0] = vec[0];
       net->ipv4.sysctl_tcp_mem[1] = vec[1];
       net->ipv4.sysctl_tcp_mem[2] = vec[2];

       return 0;

We could argue it is a bug in proc_doulongvec_minmax().
This helper probably should allocate a temp buffer,
as we have the same issue with udp_mem[].

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Issue with /proc/sys/net/ipv4/tcp_mem
  2015-10-13  1:44   ` Eric Dumazet
@ 2015-10-13  5:07     ` Eric W. Biederman
  2015-11-28  8:13       ` wangyufen
  0 siblings, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2015-10-13  5:07 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: wangyufen, netdev

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

> On Mon, 2015-10-12 at 11:37 -0500, Eric W. Biederman wrote:
>> wangyufen <wangyufen@huawei.com> writes:
>> 
>> > Hi,
>> >
>> > 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.
>> >
>> > I checked, patch f594d63199688ad568fb caused the issue.
>> 
>> 
>> If your problem is that you can not write a single value and instead
>> have to write all three values I don't know what to tell you.  I don't
>> see how that could have ever worked.
>> 
>> Certainly the commit you pointed at did not change that behavior.
>
> I would not be so sure.
> Above commit added a regression for partial writes.
> If a write() returns an error like EINVAL, we expect no change occurred.
>
> Prior code was calling proc_doulongvec_minmax() using a temporary array,
> and updated tcp_mem[0 .. 2] only of proc_doulongvec_minmax() returned 0
>
>        ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
>        if (ret)
>                return ret;
> #ifdef CONFIG_MEMCG_KMEM
> 	// deleted for clarity
> #endif
>
>        net->ipv4.sysctl_tcp_mem[0] = vec[0];
>        net->ipv4.sysctl_tcp_mem[1] = vec[1];
>        net->ipv4.sysctl_tcp_mem[2] = vec[2];
>
>        return 0;
>
> We could argue it is a bug in proc_doulongvec_minmax().
> This helper probably should allocate a temp buffer,
> as we have the same issue with udp_mem[].

Point.  We do store the value on partial writes when before we did not.

That is weird.  Clearly someone noticed.  I agree this is a confusing
corner case in proc_doulongvec_minmax that it may be worth addressing.

Does this cause a regression in a real application?   I definitely would
like to know what in the world a real application is doing that causes
it to break with this difference in behavior before doing anything,
because I am dense enough not to see how an application could
meaningfully care about this difference in behavior.

Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Issue with /proc/sys/net/ipv4/tcp_mem
  2015-10-13  5:07     ` Eric W. Biederman
@ 2015-11-28  8:13       ` wangyufen
  0 siblings, 0 replies; 6+ messages in thread
From: wangyufen @ 2015-11-28  8:13 UTC (permalink / raw)
  To: Eric W. Biederman, Eric Dumazet
  Cc: netdev, Hanjun Guo, Dingtianhong, Dianfang Zhang, Xinwei Hu

On 2015/10/13 13:07, Eric W. Biederman wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
>> On Mon, 2015-10-12 at 11:37 -0500, Eric W. Biederman wrote:
>>> wangyufen <wangyufen@huawei.com> writes:
>>>
>>>> Hi,
>>>>
>>>> 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.
>>>>
>>>> I checked, patch f594d63199688ad568fb caused the issue.
>>>
>>>
>>> If your problem is that you can not write a single value and instead
>>> have to write all three values I don't know what to tell you.  I don't
>>> see how that could have ever worked.
>>>
>>> Certainly the commit you pointed at did not change that behavior.
>>
>> I would not be so sure.
>> Above commit added a regression for partial writes.
>> If a write() returns an error like EINVAL, we expect no change occurred.
>>
>> Prior code was calling proc_doulongvec_minmax() using a temporary array,
>> and updated tcp_mem[0 .. 2] only of proc_doulongvec_minmax() returned 0
>>
>>        ret = proc_doulongvec_minmax(&tmp, write, buffer, lenp, ppos);
>>        if (ret)
>>                return ret;
>> #ifdef CONFIG_MEMCG_KMEM
>> 	// deleted for clarity
>> #endif
>>
>>        net->ipv4.sysctl_tcp_mem[0] = vec[0];
>>        net->ipv4.sysctl_tcp_mem[1] = vec[1];
>>        net->ipv4.sysctl_tcp_mem[2] = vec[2];
>>
>>        return 0;
>>
>> We could argue it is a bug in proc_doulongvec_minmax().
>> This helper probably should allocate a temp buffer,
>> as we have the same issue with udp_mem[].
> 
> Point.  We do store the value on partial writes when before we did not.
> 
> That is weird.  Clearly someone noticed.  I agree this is a confusing
> corner case in proc_doulongvec_minmax that it may be worth addressing.
> 

I think maybe we can fix the confusing corner with that patch:

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

~         

The patch makes __do_proc_doulongvec_minmax works the same as __do_proc_dointvec, 
but I'm not sure this change will not break something.

thanks,
Wang 

> Does this cause a regression in a real application?   I definitely would
> like to know what in the world a real application is doing that causes
> it to break with this difference in behavior before doing anything,
> because I am dense enough not to see how an application could
> meaningfully care about this difference in behavior.
> 
> Eric
> 
> 
> 

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-28  8:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-12  9:38 Issue with /proc/sys/net/ipv4/tcp_mem wangyufen
2015-10-12 15:24 ` Eric W. Biederman
2015-10-12 16:37 ` Eric W. Biederman
2015-10-13  1:44   ` Eric Dumazet
2015-10-13  5:07     ` Eric W. Biederman
2015-11-28  8:13       ` wangyufen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.