Issue with /proc/sys/net/ipv4/tcp_mem

DISCUSSION wangyufen 3 participants 6 messages Oct 12, 2015 – Nov 28, 2015

Subsystems (3)

Networking Network drivers Proc sysctl

lore

Share & tools

lore

wangyufen

author

2015-10-12 09:40 UTC #0 lore raw

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

Eric W. Biederman

2015-10-12 15:32 UTC #1 lore raw

Quoted 10 lines · wangyufen

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

Quoted 3 lines

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

Eric W. Biederman

2015-10-12 16:44 UTC #2 lore raw

In reply to wangyufen

Quoted 10 lines · wangyufen

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

Quoted 3 lines

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

Eric Dumazet

2015-10-13 01:44 UTC #3 lore raw

Quoted 14 lines · Eric W. Biederman

Quoted 12 lines · wangyufen
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.

Quoted 8 lines

Quoted 1 line
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[].

Eric W. Biederman

2015-10-13 05:15 UTC #4 lore raw

Quoted 44 lines · Eric Dumazet

Quoted 14 lines · Eric W. Biederman
Quoted 12 lines · wangyufen
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.
Quoted 8 lines
Quoted 1 line
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;

Quoted 3 lines

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

wangyufen

author

2015-11-28 08:13 UTC #5 lore raw

Quoted 46 lines · Eric W. Biederman

Quoted 44 lines · Eric Dumazet
Quoted 14 lines · Eric W. Biederman
Quoted 12 lines · wangyufen
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.
Quoted 8 lines
Quoted 1 line
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;

Quoted 9 lines

Quoted 3 lines
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(+)

kernel/sysctl.c +2 -0

index c3eee4c..e3ee4be 100644
@@ -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,

Signature · 9 lines

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

Quoted 10 lines

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