Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
This change is fine for tcp_rmem and udp_rmem_min, since SOCK_MIN_RCVBUF
is equal to equal to TCP_SKB_MIN_TRUESIZE. But it breaks tcp_wmem and
udp_wmem_min for previously valid values because SOCK_MIN_SNDBUF is
(2 * TCP_SKB_MIN_TRUESIZE), which ends up being greater than 4KB.
Thus, 4096 is no longer accepted as a valid value, despite still being
the default for udp_wmem_min, and for 'min' in tcp_wmem. A huge number
of sysctl configurations at FB use 4096 as 'min', so this change breaks
all of them.
This patch changes the sysctls to simply enforce that the value written
is greater than or equal to the default value of SK_MEM_QUANTUM.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Signed-off-by: Calvin Owens <redacted>
---
net/ipv4/sysctl_net_ipv4.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
@@ -41,8 +41,7 @@ static int tcp_syn_retries_min = 1;staticinttcp_syn_retries_max=MAX_TCP_SYNCNT;staticintip_ping_group_range_min[]={0,0};staticintip_ping_group_range_max[]={GID_T_MAX,GID_T_MAX};-staticintmin_sndbuf=SOCK_MIN_SNDBUF;-staticintmin_rcvbuf=SOCK_MIN_RCVBUF;+staticintmin_buf=SK_MEM_QUANTUM;/* Update system visible IP port range */staticvoidset_local_port_range(structnet*net,intrange[2])
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
This change is fine for tcp_rmem and udp_rmem_min, since SOCK_MIN_RCVBUF
is equal to equal to TCP_SKB_MIN_TRUESIZE. But it breaks tcp_wmem and
udp_wmem_min for previously valid values because SOCK_MIN_SNDBUF is
(2 * TCP_SKB_MIN_TRUESIZE), which ends up being greater than 4KB.
Thus, 4096 is no longer accepted as a valid value, despite still being
the default for udp_wmem_min, and for 'min' in tcp_wmem. A huge number
of sysctl configurations at FB use 4096 as 'min', so this change breaks
all of them.
This patch changes the sysctls to simply enforce that the value written
is greater than or equal to the default value of SK_MEM_QUANTUM.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Signed-off-by: Calvin Owens <redacted>
I think increasing the default makes more sense.
If we don't allow applications to set 4K, the kernel shouldn't start
with that value either.
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
This change is fine for tcp_rmem and udp_rmem_min, since SOCK_MIN_RCVBUF
is equal to equal to TCP_SKB_MIN_TRUESIZE. But it breaks tcp_wmem and
udp_wmem_min for previously valid values because SOCK_MIN_SNDBUF is
(2 * TCP_SKB_MIN_TRUESIZE), which ends up being greater than 4KB.
Thus, 4096 is no longer accepted as a valid value, despite still being
the default for udp_wmem_min, and for 'min' in tcp_wmem. A huge number
of sysctl configurations at FB use 4096 as 'min', so this change breaks
all of them.
This patch changes the sysctls to simply enforce that the value written
is greater than or equal to the default value of SK_MEM_QUANTUM.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Signed-off-by: Calvin Owens <redacted>
I think increasing the default makes more sense.
If we don't allow applications to set 4K, the kernel shouldn't start
with that value either.
I'm really questioning the limitation itself: why enforce a minimum of
SOCK_MIN_SNDBUF here? Why not SK_MEM_QUANTUM?
Commit 8133534c760d4083 referred to b1cb59cf2efe7971, which choose to
use the SOCK_MIN constants as the lower limits to avoid nasty bugs. But
AFAICS, a limit of SOCK_MIN_SNDBUF isn't necessary to do that: the
BUG_ON cited in the commit message for b1cb59cf2efe7971 seems to have
happened because unix_stream_sendmsg() expects a minimum of a full page
(ie SK_MEM_QUANTUM) and the math broke, not because it had less than
SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play
with, so my argument is that enforcing a minimum of SK_MEM_QUANTUM
avoids the sort of bugs commit 8133534c760d4083 was trying to avoid, and
it does so without breaking anybody's sysctl configurations. What do you
think?
Thanks very much,
Calvin
I'm really questioning the limitation itself: why enforce a minimum of
SOCK_MIN_SNDBUF here? Why not SK_MEM_QUANTUM?
Commit 8133534c760d4083 referred to b1cb59cf2efe7971, which choose to
use the SOCK_MIN constants as the lower limits to avoid nasty bugs. But
AFAICS, a limit of SOCK_MIN_SNDBUF isn't necessary to do that: the
BUG_ON cited in the commit message for b1cb59cf2efe7971 seems to have
happened because unix_stream_sendmsg() expects a minimum of a full page
(ie SK_MEM_QUANTUM) and the math broke, not because it had less than
SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play
with, so my argument is that enforcing a minimum of SK_MEM_QUANTUM
avoids the sort of bugs commit 8133534c760d4083 was trying to avoid, and
it does so without breaking anybody's sysctl configurations. What do you
think?
The author of said commit argues that too small values lead to really
bad performance, but I guess he should have adjusted the default if he
cared about it so much.
Ok, can you respin your patch with some added details in the commit
message like what you said above?
Thanks.
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 (or SK_MEM_QUANTUM) to no longer be accepted as
a valid value for 'min' in tcp_wmem and udp_wmem_min. 4096 has been the
default for both of those sysctls for a long time, and unfortunately
seems to be an extremely popular setting. This change breaks a large
number of sysctl configurations at FB.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check
SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN
constants as the lower limits to avoid nasty bugs. But AFAICS, a limit
of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the
commit message seems to have happened because unix_stream_sendmsg()
expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke,
not because it had less than SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play
with, so I think enforcing a minimum of SK_MEM_QUANTUM avoids the sort
of bugs 8133534c was trying to avoid, and it does so without breaking
anybody's sysctl configurations.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Signed-off-by: Calvin Owens <redacted>
---
net/ipv4/sysctl_net_ipv4.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
@@ -41,8 +41,7 @@ static int tcp_syn_retries_min = 1;staticinttcp_syn_retries_max=MAX_TCP_SYNCNT;staticintip_ping_group_range_min[]={0,0};staticintip_ping_group_range_max[]={GID_T_MAX,GID_T_MAX};-staticintmin_sndbuf=SOCK_MIN_SNDBUF;-staticintmin_rcvbuf=SOCK_MIN_RCVBUF;+staticintmin_buf=SK_MEM_QUANTUM;/* Update system visible IP port range */staticvoidset_local_port_range(structnet*net,intrange[2])
From: Eric Dumazet <hidden> Date: 2015-08-12 14:21:08
On Tue, 2015-08-11 at 21:54 -0700, Calvin Owens wrote:
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 (or SK_MEM_QUANTUM) to no longer be accepted as
a valid value for 'min' in tcp_wmem and udp_wmem_min. 4096 has been the
default for both of those sysctls for a long time, and unfortunately
seems to be an extremely popular setting. This change breaks a large
number of sysctl configurations at FB.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check
SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN
constants as the lower limits to avoid nasty bugs. But AFAICS, a limit
of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the
commit message seems to have happened because unix_stream_sendmsg()
expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke,
not because it had less than SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play
with, so I think enforcing a minimum of SK_MEM_QUANTUM avoids the sort
of bugs 8133534c was trying to avoid, and it does so without breaking
anybody's sysctl configurations.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Signed-off-by: Calvin Owens <redacted>
---
#define SK_MEM_QUANTUM ((int)PAGE_SIZE)
Some arches have PAGE_SIZE = 65536
So your patch might break scripts as well for them.
We should revert 8133534c760d4083.
From: Sorin Dumitru <hidden> Date: 2015-08-12 17:01:00
On Wed, Aug 12, 2015 at 5:21 PM, Eric Dumazet [off-list ref] wrote:
On Tue, 2015-08-11 at 21:54 -0700, Calvin Owens wrote:
quoted
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 (or SK_MEM_QUANTUM) to no longer be accepted as
a valid value for 'min' in tcp_wmem and udp_wmem_min. 4096 has been the
default for both of those sysctls for a long time, and unfortunately
seems to be an extremely popular setting. This change breaks a large
number of sysctl configurations at FB.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check
SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN
constants as the lower limits to avoid nasty bugs. But AFAICS, a limit
of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the
commit message seems to have happened because unix_stream_sendmsg()
expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke,
not because it had less than SOCK_MIN_SNDBUF allocated.
Nothing seems to assume that it has at least SOCK_MIN_SNDBUF to play
with, so I think enforcing a minimum of SK_MEM_QUANTUM avoids the sort
of bugs 8133534c was trying to avoid, and it does so without breaking
anybody's sysctl configurations.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Signed-off-by: Calvin Owens <redacted>
---
#define SK_MEM_QUANTUM ((int)PAGE_SIZE)
Some arches have PAGE_SIZE = 65536
So your patch might break scripts as well for them.
We should revert 8133534c760d4083.
Would clamping the values to a min value, like setsockopt(SO_SNDBUF)
does, be an option?
I still find it odd that SO_SNDBUF limits you, while the /proc
interface doesn't. If you think it's
too much, I'm ok with reverting it since it affects scripts.
On those arches where PAGE_SIZE == 64K(or > 16K) it looks like we have
tcp_wmem[1]
smaller than tcp_wmem[0]. Shouldn't we do something about this?
From: Eric Dumazet <hidden> Date: 2015-08-12 17:46:29
On Wed, 2015-08-12 at 20:00 +0300, Sorin Dumitru wrote:
Would clamping the values to a min value, like setsockopt(SO_SNDBUF)
does, be an option?
I still find it odd that SO_SNDBUF limits you, while the /proc
interface doesn't. If you think it's
too much, I'm ok with reverting it since it affects scripts.
On those arches where PAGE_SIZE == 64K(or > 16K) it looks like we have
tcp_wmem[1]
smaller than tcp_wmem[0]. Shouldn't we do something about this?
As long as we do not crash if/when root user changes /proc/sys/net
settings, we are good.
I would not care if performance is bad if root does something really
stupid. root user is supposed to not mess things just for fun.
On Wednesday 08/12 at 10:46 -0700, Eric Dumazet wrote:
On Wed, 2015-08-12 at 20:00 +0300, Sorin Dumitru wrote:
quoted
Would clamping the values to a min value, like setsockopt(SO_SNDBUF)
does, be an option?
I still find it odd that SO_SNDBUF limits you, while the /proc
interface doesn't. If you think it's
too much, I'm ok with reverting it since it affects scripts.
On those arches where PAGE_SIZE == 64K(or > 16K) it looks like we have
tcp_wmem[1]
smaller than tcp_wmem[0]. Shouldn't we do something about this?
As long as we do not crash if/when root user changes /proc/sys/net
settings, we are good.
Using "1 1 1" for tcp_{r,w}mem seems not to explode, so this sounds good
to me. I'll send a new patch reverting the original.
Thanks,
Calvin
I would not care if performance is bad if root does something really
stupid. root user is supposed to not mess things just for fun.
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 to no longer be accepted as a valid value for
'min' in tcp_wmem and udp_wmem_min. 4096 has been the default for both
of those sysctls for a long time, and unfortunately seems to be an
extremely popular setting. This change breaks a large number of sysctl
configurations at Facebook.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check
SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN
constants as the lower limits to avoid nasty bugs. But AFAICS, a limit
of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the
commit message seems to have happened because unix_stream_sendmsg()
expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke,
not because it had less than SOCK_MIN_SNDBUF allocated.
This particular issue doesn't seem to affect TCP however: using a
setting of "1 1 1" for tcp_{r,w}mem works, although it's obviously
suboptimal. SK_MEM_QUANTUM would be a nice minimum, but it's 64K on
some archs, so there would still be breakage.
Since a value of one doesn't seem to cause any problems, we can drop the
minimum 8133534c added to fix this.
This reverts commit 8133534c760d4083f79d2cde42c636ccc0b2792e.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Cc: Eric Dumazet <redacted>
Cc: Sorin Dumitru <redacted>
Signed-off-by: Calvin Owens <redacted>
---
net/ipv4/sysctl_net_ipv4.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
@@ -41,8 +41,6 @@ static int tcp_syn_retries_min = 1;staticinttcp_syn_retries_max=MAX_TCP_SYNCNT;staticintip_ping_group_range_min[]={0,0};staticintip_ping_group_range_max[]={GID_T_MAX,GID_T_MAX};-staticintmin_sndbuf=SOCK_MIN_SNDBUF;-staticintmin_rcvbuf=SOCK_MIN_RCVBUF;/* Update system visible IP port range */staticvoidset_local_port_range(structnet*net,intrange[2])
From: Eric Dumazet <hidden> Date: 2015-08-13 22:56:39
On Thu, 2015-08-13 at 14:21 -0700, Calvin Owens wrote:
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
...
This reverts commit 8133534c760d4083f79d2cde42c636ccc0b2792e.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Cc: Eric Dumazet <redacted>
Cc: Sorin Dumitru <redacted>
Signed-off-by: Calvin Owens <redacted>
Commit 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to
SOCK_{RCV,SND}BUF_MIN") modified four sysctls to enforce that the values
written to them are not less than SOCK_MIN_{RCV,SND}BUF.
That change causes 4096 to no longer be accepted as a valid value for
'min' in tcp_wmem and udp_wmem_min. 4096 has been the default for both
of those sysctls for a long time, and unfortunately seems to be an
extremely popular setting. This change breaks a large number of sysctl
configurations at Facebook.
That commit referred to b1cb59cf2efe7971 ("net: sysctl_net_core: check
SNDBUF and RCVBUF for min length"), which choose to use the SOCK_MIN
constants as the lower limits to avoid nasty bugs. But AFAICS, a limit
of SOCK_MIN_SNDBUF isn't necessary to do that: the BUG_ON cited in the
commit message seems to have happened because unix_stream_sendmsg()
expects a minimum of a full page (ie SK_MEM_QUANTUM) and the math broke,
not because it had less than SOCK_MIN_SNDBUF allocated.
This particular issue doesn't seem to affect TCP however: using a
setting of "1 1 1" for tcp_{r,w}mem works, although it's obviously
suboptimal. SK_MEM_QUANTUM would be a nice minimum, but it's 64K on
some archs, so there would still be breakage.
Since a value of one doesn't seem to cause any problems, we can drop the
minimum 8133534c added to fix this.
This reverts commit 8133534c760d4083f79d2cde42c636ccc0b2792e.
Fixes: 8133534c760d4083 ("net: limit tcp/udp rmem/wmem to SOCK_MIN...")
Cc: Eric Dumazet <redacted>
Cc: Sorin Dumitru <redacted>
Signed-off-by: Calvin Owens <redacted>