netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: bugs/regressions: report in LKML or in bugzilla?
       [not found] <201012071639.58884.Martin@lichtvoll.de>
@ 2010-12-07 16:12 ` Eric Dumazet
  2010-12-07 21:02   ` Ben Hutchings
  2010-12-07 21:11   ` Martin Steigerwald
  0 siblings, 2 replies; 12+ messages in thread
From: Eric Dumazet @ 2010-12-07 16:12 UTC (permalink / raw)
  To: Martin Steigerwald; +Cc: linux-kernel, netdev

Le mardi 07 décembre 2010 à 16:39 +0100, Martin Steigerwald a écrit :

> A participant of a linux performance training I hold found a bug with 
> window scaling which did not receive any reply as well:
> 
> Bug 20312 -  System freeze with multiples of 32 in 
> /proc/sys/net/ipv4/tcp_adv_win_scale
> https://bugzilla.kernel.org/show_bug.cgi?id=20312
> 

User bug ?

Documentation/networking/ip-sysctl.txt

tcp_adv_win_scale - INTEGER
	Count buffering overhead as bytes/2^tcp_adv_win_scale 
	(if tcp_adv_win_scale > 0) or bytes-bytes/2^(-tcp_adv_win_scale),
	if it is <= 0.
	Default: 2

Given we use 32bit numbers, using values outside of [-31 ... 31] makes litle sense.

We could add sysctl range limit, but user should not mess with 
/proc/sys/net/ipv4/parameters unless he knows what he is doing ?

Almost all /proc/sys/net/ipv4/parameters dont have range limits and unexpected
results with insane values feeded.

An other way to freeze a machine being root is :

halt

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bugs/regressions: report in LKML or in bugzilla?
  2010-12-07 16:12 ` bugs/regressions: report in LKML or in bugzilla? Eric Dumazet
@ 2010-12-07 21:02   ` Ben Hutchings
  2010-12-07 21:28     ` [PATCH] tcp: avoid a possible divide by zero Eric Dumazet
  2010-12-07 21:28     ` bugs/regressions: report in LKML or in bugzilla? Martin Steigerwald
  2010-12-07 21:11   ` Martin Steigerwald
  1 sibling, 2 replies; 12+ messages in thread
From: Ben Hutchings @ 2010-12-07 21:02 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Martin Steigerwald, linux-kernel, netdev

On Tue, 2010-12-07 at 17:12 +0100, Eric Dumazet wrote:
> Le mardi 07 décembre 2010 à 16:39 +0100, Martin Steigerwald a écrit :
> 
> > A participant of a linux performance training I hold found a bug with 
> > window scaling which did not receive any reply as well:
> > 
> > Bug 20312 -  System freeze with multiples of 32 in 
> > /proc/sys/net/ipv4/tcp_adv_win_scale
> > https://bugzilla.kernel.org/show_bug.cgi?id=20312
> > 
> 
> User bug ?
> 
> Documentation/networking/ip-sysctl.txt
> 
> tcp_adv_win_scale - INTEGER
> 	Count buffering overhead as bytes/2^tcp_adv_win_scale 
> 	(if tcp_adv_win_scale > 0) or bytes-bytes/2^(-tcp_adv_win_scale),
> 	if it is <= 0.
> 	Default: 2
> 
> Given we use 32bit numbers, using values outside of [-31 ... 31] makes litle sense.
> 
> We could add sysctl range limit, but user should not mess with 
> /proc/sys/net/ipv4/parameters unless he knows what he is doing ?
[...]

For mere humans, the range is not quite os obvious.  Which is why this
has been fixed in net-2.6 (as noted on that bug report now).

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bugs/regressions: report in LKML or in bugzilla?
  2010-12-07 16:12 ` bugs/regressions: report in LKML or in bugzilla? Eric Dumazet
  2010-12-07 21:02   ` Ben Hutchings
@ 2010-12-07 21:11   ` Martin Steigerwald
  1 sibling, 0 replies; 12+ messages in thread
From: Martin Steigerwald @ 2010-12-07 21:11 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: linux-kernel, netdev

[-- Attachment #1: Type: Text/Plain, Size: 2059 bytes --]

Am Dienstag 07 Dezember 2010 schrieb Eric Dumazet:
> Le mardi 07 décembre 2010 à 16:39 +0100, Martin Steigerwald a écrit :
> > A participant of a linux performance training I hold found a bug with
> > window scaling which did not receive any reply as well:
> > 
> > Bug 20312 -  System freeze with multiples of 32 in
> > /proc/sys/net/ipv4/tcp_adv_win_scale
> > https://bugzilla.kernel.org/show_bug.cgi?id=20312
> 
> User bug ?

Sure, but whats the point?

> Documentation/networking/ip-sysctl.txt
> 
> tcp_adv_win_scale - INTEGER
> 	Count buffering overhead as bytes/2^tcp_adv_win_scale
> 	(if tcp_adv_win_scale > 0) or bytes-bytes/2^(-tcp_adv_win_scale),
> 	if it is <= 0.
> 	Default: 2
> 
> Given we use 32bit numbers, using values outside of [-31 ... 31] makes
> litle sense.

Granted, it does not make sense. The user tried that on an exercise to 
make TCP/IP networking in Linux as slow as possible (to understand why its 
fast at all).

Still: Here isn't documented that the kernel freezes when writing a wrong 
value in there.
 
> We could add sysctl range limit, but user should not mess with
> /proc/sys/net/ipv4/parameters unless he knows what he is doing ?
> 
> Almost all /proc/sys/net/ipv4/parameters dont have range limits and
> unexpected results with insane values feeded.

Well I disagree. Its a user interface, even tough a root user interface, 
that is even writable without writing a program. And as far as I 
understand even at least some system calls do some basic sanity checking 
on arguments.

If it doesn't cost too much overhead, arguments in there should receive at 
least some basic sanity checking.

> An other way to freeze a machine being root is :
> 
> halt

It won't freeze the machine. It does a clean halt which reduces the chance 
to reduce valuable data in yet unwritten pages.

And here it is documented that this will halt the machine.

Ciao,
-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] tcp: avoid a possible divide by zero
  2010-12-07 21:02   ` Ben Hutchings
@ 2010-12-07 21:28     ` Eric Dumazet
  2010-12-07 21:32       ` Ben Hutchings
  2010-12-07 22:20       ` [PATCH] tcp: protect sysctl_tcp_cookie_size reads Eric Dumazet
  2010-12-07 21:28     ` bugs/regressions: report in LKML or in bugzilla? Martin Steigerwald
  1 sibling, 2 replies; 12+ messages in thread
From: Eric Dumazet @ 2010-12-07 21:28 UTC (permalink / raw)
  To: Ben Hutchings, David Miller; +Cc: Martin Steigerwald, netdev

Le mardi 07 décembre 2010 à 21:02 +0000, Ben Hutchings a écrit :
> On Tue, 2010-12-07 at 17:12 +0100, Eric Dumazet wrote:
> > Le mardi 07 décembre 2010 à 16:39 +0100, Martin Steigerwald a écrit :
> > 
> > > A participant of a linux performance training I hold found a bug with 
> > > window scaling which did not receive any reply as well:
> > > 
> > > Bug 20312 -  System freeze with multiples of 32 in 
> > > /proc/sys/net/ipv4/tcp_adv_win_scale
> > > https://bugzilla.kernel.org/show_bug.cgi?id=20312
> > > 
> > 
> > User bug ?
> > 
> > Documentation/networking/ip-sysctl.txt
> > 
> > tcp_adv_win_scale - INTEGER
> > 	Count buffering overhead as bytes/2^tcp_adv_win_scale 
> > 	(if tcp_adv_win_scale > 0) or bytes-bytes/2^(-tcp_adv_win_scale),
> > 	if it is <= 0.
> > 	Default: 2
> > 
> > Given we use 32bit numbers, using values outside of [-31 ... 31] makes litle sense.
> > 
> > We could add sysctl range limit, but user should not mess with 
> > /proc/sys/net/ipv4/parameters unless he knows what he is doing ?
> [...]
> 
> For mere humans, the range is not quite os obvious.  Which is why this
> has been fixed in net-2.6 (as noted on that bug report now).
> 

Thanks

Great, I feel we are going to fix all sysctls, one by one then :(

lkml removed from Cc


[PATCH] tcp: avoid a possible divide by zero

sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in
tcp_tso_should_defer(). Make sure we dont allow a divide by zero by
reading sysctl_tcp_tso_win_divisor once.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv4/tcp_output.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 05b1ecf..0281223 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
 	struct tcp_sock *tp = tcp_sk(sk);
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 	u32 send_win, cong_win, limit, in_flight;
+	int win_divisor;
 
 	if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN)
 		goto send_now;
@@ -1544,13 +1545,14 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
 	if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len))
 		goto send_now;
 
-	if (sysctl_tcp_tso_win_divisor) {
+	win_divisor = sysctl_tcp_tso_win_divisor;
+	if (win_divisor) {
 		u32 chunk = min(tp->snd_wnd, tp->snd_cwnd * tp->mss_cache);
 
 		/* If at least some fraction of a window is available,
 		 * just use it.
 		 */
-		chunk /= sysctl_tcp_tso_win_divisor;
+		chunk /= win_divisor;
 		if (limit >= chunk)
 			goto send_now;
 	} else {



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: bugs/regressions: report in LKML or in bugzilla?
  2010-12-07 21:02   ` Ben Hutchings
  2010-12-07 21:28     ` [PATCH] tcp: avoid a possible divide by zero Eric Dumazet
@ 2010-12-07 21:28     ` Martin Steigerwald
  1 sibling, 0 replies; 12+ messages in thread
From: Martin Steigerwald @ 2010-12-07 21:28 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Eric Dumazet, linux-kernel, netdev

[-- Attachment #1: Type: Text/Plain, Size: 1359 bytes --]

Am Dienstag 07 Dezember 2010 schrieb Ben Hutchings:
> On Tue, 2010-12-07 at 17:12 +0100, Eric Dumazet wrote:
> > Le mardi 07 décembre 2010 à 16:39 +0100, Martin Steigerwald a écrit :
> > > A participant of a linux performance training I hold found a bug
> > > with window scaling which did not receive any reply as well:
> > > 
> > > Bug 20312 -  System freeze with multiples of 32 in
> > > /proc/sys/net/ipv4/tcp_adv_win_scale
> > > https://bugzilla.kernel.org/show_bug.cgi?id=20312
> > 
> > User bug ?
> > 
> > Documentation/networking/ip-sysctl.txt
> > 
> > tcp_adv_win_scale - INTEGER
> > 
> > 	Count buffering overhead as bytes/2^tcp_adv_win_scale
> > 	(if tcp_adv_win_scale > 0) or bytes-bytes/2^(-tcp_adv_win_scale),
> > 	if it is <= 0.
> > 	Default: 2
> > 
> > Given we use 32bit numbers, using values outside of [-31 ... 31]
> > makes litle sense.
> > 
> > We could add sysctl range limit, but user should not mess with
> > /proc/sys/net/ipv4/parameters unless he knows what he is doing ?
> 
> [...]
> 
> For mere humans, the range is not quite os obvious.  Which is why this
> has been fixed in net-2.6 (as noted on that bug report now).
> 
> Ben.

I verified the fix on 2.6.37-rc5.

Thanks,
-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] tcp: avoid a possible divide by zero
  2010-12-07 21:28     ` [PATCH] tcp: avoid a possible divide by zero Eric Dumazet
@ 2010-12-07 21:32       ` Ben Hutchings
  2010-12-07 22:03         ` Eric Dumazet
  2010-12-07 22:20       ` [PATCH] tcp: protect sysctl_tcp_cookie_size reads Eric Dumazet
  1 sibling, 1 reply; 12+ messages in thread
From: Ben Hutchings @ 2010-12-07 21:32 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, Martin Steigerwald, netdev

On Tue, 2010-12-07 at 22:28 +0100, Eric Dumazet wrote:
[...]
> Thanks
> 
> Great, I feel we are going to fix all sysctls, one by one then :(
> 
> lkml removed from Cc
> 
> 
> [PATCH] tcp: avoid a possible divide by zero
> 
> sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in
> tcp_tso_should_defer(). Make sure we dont allow a divide by zero by
> reading sysctl_tcp_tso_win_divisor once.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/ipv4/tcp_output.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 05b1ecf..0281223 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	const struct inet_connection_sock *icsk = inet_csk(sk);
>  	u32 send_win, cong_win, limit, in_flight;
> +	int win_divisor;
>  
>  	if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN)
>  		goto send_now;
> @@ -1544,13 +1545,14 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
>  	if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len))
>  		goto send_now;
>  
> -	if (sysctl_tcp_tso_win_divisor) {
> +	win_divisor = sysctl_tcp_tso_win_divisor;

You need to use ACCESS_ONCE(sysctl_tcp_tso_win_divisor).  Otherwise the
compiler may eliminate the local variable and read the global twice.

Ben.

> +	if (win_divisor) {
>  		u32 chunk = min(tp->snd_wnd, tp->snd_cwnd * tp->mss_cache);
>  
>  		/* If at least some fraction of a window is available,
>  		 * just use it.
>  		 */
> -		chunk /= sysctl_tcp_tso_win_divisor;
> +		chunk /= win_divisor;
>  		if (limit >= chunk)
>  			goto send_now;
>  	} else {
> 
> 

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] tcp: avoid a possible divide by zero
  2010-12-07 21:32       ` Ben Hutchings
@ 2010-12-07 22:03         ` Eric Dumazet
  2010-12-08  8:23           ` Martin Steigerwald
  2010-12-08 20:35           ` David Miller
  0 siblings, 2 replies; 12+ messages in thread
From: Eric Dumazet @ 2010-12-07 22:03 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, Martin Steigerwald, netdev

Le mardi 07 décembre 2010 à 21:32 +0000, Ben Hutchings a écrit :
> On Tue, 2010-12-07 at 22:28 +0100, Eric Dumazet wrote:
> [...]
> > Thanks
> > 
> > Great, I feel we are going to fix all sysctls, one by one then :(
> > 
> > lkml removed from Cc
> > 
> > 
> > [PATCH] tcp: avoid a possible divide by zero
> > 
> > sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in
> > tcp_tso_should_defer(). Make sure we dont allow a divide by zero by
> > reading sysctl_tcp_tso_win_divisor once.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> >  net/ipv4/tcp_output.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 05b1ecf..0281223 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
> >  	struct tcp_sock *tp = tcp_sk(sk);
> >  	const struct inet_connection_sock *icsk = inet_csk(sk);
> >  	u32 send_win, cong_win, limit, in_flight;
> > +	int win_divisor;
> >  
> >  	if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN)
> >  		goto send_now;
> > @@ -1544,13 +1545,14 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
> >  	if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len))
> >  		goto send_now;
> >  
> > -	if (sysctl_tcp_tso_win_divisor) {
> > +	win_divisor = sysctl_tcp_tso_win_divisor;
> 
> You need to use ACCESS_ONCE(sysctl_tcp_tso_win_divisor).  Otherwise the
> compiler may eliminate the local variable and read the global twice.

Yes, I knew that, of course :)

I wonder how many bugs like that we have in sysctls

Thanks

[PATCH v2] tcp: avoid a possible divide by zero

sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in
tcp_tso_should_defer(). Make sure we dont allow a divide by zero by
reading sysctl_tcp_tso_win_divisor exactly once.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
v2: Use ACCESS_ONCE() as Ben suggested

 net/ipv4/tcp_output.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 05b1ecf..0464d70 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
 	struct tcp_sock *tp = tcp_sk(sk);
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 	u32 send_win, cong_win, limit, in_flight;
+	int win_divisor;
 
 	if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN)
 		goto send_now;
@@ -1544,13 +1545,14 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
 	if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len))
 		goto send_now;
 
-	if (sysctl_tcp_tso_win_divisor) {
+	win_divisor = ACCESS_ONCE(sysctl_tcp_tso_win_divisor);
+	if (win_divisor) {
 		u32 chunk = min(tp->snd_wnd, tp->snd_cwnd * tp->mss_cache);
 
 		/* If at least some fraction of a window is available,
 		 * just use it.
 		 */
-		chunk /= sysctl_tcp_tso_win_divisor;
+		chunk /= win_divisor;
 		if (limit >= chunk)
 			goto send_now;
 	} else {



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] tcp: protect sysctl_tcp_cookie_size reads
  2010-12-07 21:28     ` [PATCH] tcp: avoid a possible divide by zero Eric Dumazet
  2010-12-07 21:32       ` Ben Hutchings
@ 2010-12-07 22:20       ` Eric Dumazet
  2010-12-08 20:35         ` David Miller
  1 sibling, 1 reply; 12+ messages in thread
From: Eric Dumazet @ 2010-12-07 22:20 UTC (permalink / raw)
  To: David Miller
  Cc: Martin Steigerwald, netdev, Ben Hutchings, William Allen Simpson

Make sure sysctl_tcp_cookie_size is read once in
tcp_cookie_size_check(), or we might return an illegal value to caller
if sysctl_tcp_cookie_size is changed by another cpu.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: William Allen Simpson <william.allen.simpson@gmail.com>
---
 net/ipv4/tcp_output.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 05b1ecf..8cecb30 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -386,27 +386,30 @@ struct tcp_out_options {
  */
 static u8 tcp_cookie_size_check(u8 desired)
 {
-	if (desired > 0) {
+	int cookie_size;
+
+	if (desired > 0)
 		/* previously specified */
 		return desired;
-	}
-	if (sysctl_tcp_cookie_size <= 0) {
+
+	cookie_size = ACCESS_ONCE(sysctl_tcp_cookie_size);
+	if (cookie_size <= 0)
 		/* no default specified */
 		return 0;
-	}
-	if (sysctl_tcp_cookie_size <= TCP_COOKIE_MIN) {
+
+	if (cookie_size <= TCP_COOKIE_MIN)
 		/* value too small, specify minimum */
 		return TCP_COOKIE_MIN;
-	}
-	if (sysctl_tcp_cookie_size >= TCP_COOKIE_MAX) {
+
+	if (cookie_size >= TCP_COOKIE_MAX)
 		/* value too large, specify maximum */
 		return TCP_COOKIE_MAX;
-	}
-	if (0x1 & sysctl_tcp_cookie_size) {
+
+	if (cookie_size & 1)
 		/* 8-bit multiple, illegal, fix it */
-		return (u8)(sysctl_tcp_cookie_size + 0x1);
-	}
-	return (u8)sysctl_tcp_cookie_size;
+		cookie_size++;
+
+	return (u8)cookie_size;
 }
 
 /* Write previously computed TCP options to the packet.



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] tcp: avoid a possible divide by zero
  2010-12-07 22:03         ` Eric Dumazet
@ 2010-12-08  8:23           ` Martin Steigerwald
  2010-12-08  8:33             ` Eric Dumazet
  2010-12-08 20:35           ` David Miller
  1 sibling, 1 reply; 12+ messages in thread
From: Martin Steigerwald @ 2010-12-08  8:23 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Ben Hutchings, David Miller, netdev

[-- Attachment #1: Type: Text/Plain, Size: 3084 bytes --]

Am Dienstag 07 Dezember 2010 schrieb Eric Dumazet:
> Le mardi 07 décembre 2010 à 21:32 +0000, Ben Hutchings a écrit :
> > On Tue, 2010-12-07 at 22:28 +0100, Eric Dumazet wrote:
> > [...]
> > 
> > > Thanks
> > > 
> > > Great, I feel we are going to fix all sysctls, one by one then :(

Are there so many sysctls which are likely to freeze the kernel when fed 
with wrong value? Once could argue for sysctls where invalid values don't 
cause any serious harm, its not so important to fix it. I probably could 
have next weeks training members a go at poking creative values in other 
controls as well to see what happens.

> > > lkml removed from Cc
> > > 
> > > 
> > > [PATCH] tcp: avoid a possible divide by zero
> > > 
> > > sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs
> > > in tcp_tso_should_defer(). Make sure we dont allow a divide by
> > > zero by reading sysctl_tcp_tso_win_divisor once.
> > > 
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > ---
[...]
> > > +	win_divisor = sysctl_tcp_tso_win_divisor;
> > 
> > You need to use ACCESS_ONCE(sysctl_tcp_tso_win_divisor).  Otherwise
> > the compiler may eliminate the local variable and read the global
> > twice.
> 
> Yes, I knew that, of course :)
> 
> I wonder how many bugs like that we have in sysctls
> 
> Thanks
> 
> [PATCH v2] tcp: avoid a possible divide by zero
> 
> sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in
> tcp_tso_should_defer(). Make sure we dont allow a divide by zero by
> reading sysctl_tcp_tso_win_divisor exactly once.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> v2: Use ACCESS_ONCE() as Ben suggested
> 
>  net/ipv4/tcp_output.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 05b1ecf..0464d70 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk,
> struct sk_buff *skb) struct tcp_sock *tp = tcp_sk(sk);
>  	const struct inet_connection_sock *icsk = inet_csk(sk);
>  	u32 send_win, cong_win, limit, in_flight;
> +	int win_divisor;
[...]
> -	if (sysctl_tcp_tso_win_divisor) {
> +	win_divisor = ACCESS_ONCE(sysctl_tcp_tso_win_divisor);
> +	if (win_divisor) {
>  		u32 chunk = min(tp->snd_wnd, tp->snd_cwnd * tp->mss_cache);
> 
>  		/* If at least some fraction of a window is available,
>  		 * just use it.
>  		 */
> -		chunk /= sysctl_tcp_tso_win_divisor;
> +		chunk /= win_divisor;
>  		if (limit >= chunk)
>  			goto send_now;
>  	} else {

So this patch helps other cases as well? Or is it, as I think just a 
different approach, to fix the issue my training member brought up, by its 
cause instead of or additional to limiting its range?

Want to check whether I basically understood the patch. Do you want me to 
test it? 

Thanks,
-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] tcp: avoid a possible divide by zero
  2010-12-08  8:23           ` Martin Steigerwald
@ 2010-12-08  8:33             ` Eric Dumazet
  0 siblings, 0 replies; 12+ messages in thread
From: Eric Dumazet @ 2010-12-08  8:33 UTC (permalink / raw)
  To: Martin Steigerwald; +Cc: Ben Hutchings, David Miller, netdev

Le mercredi 08 décembre 2010 à 09:23 +0100, Martin Steigerwald a écrit :

> Are there so many sysctls which are likely to freeze the kernel when fed 
> with wrong value? Once could argue for sysctls where invalid values don't 
> cause any serious harm, its not so important to fix it. I probably could 
> have next weeks training members a go at poking creative values in other 
> controls as well to see what happens.
> 

We have many sysctls that can lead to non working machine.

Any kind of limits actually. Just set them to 0 (or maybe a negative
number :( )

0 socket, 0 file descriptor, 0 memory, 0 speed limit, 0 lengthes ...



> So this patch helps other cases as well? Or is it, as I think just a 
> different approach, to fix the issue my training member brought up, by its 
> cause instead of or additional to limiting its range?
> 
> Want to check whether I basically understood the patch. Do you want me to 
> test it? 

It has nothing to do with the issue you raised, and is a completely
different subject. I got it while spending 5 minutes yesterday night
grep-ing some sysctls in network tree. 0 value is one of expected value
for this sysctl, but the test was not safe.




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] tcp: avoid a possible divide by zero
  2010-12-07 22:03         ` Eric Dumazet
  2010-12-08  8:23           ` Martin Steigerwald
@ 2010-12-08 20:35           ` David Miller
  1 sibling, 0 replies; 12+ messages in thread
From: David Miller @ 2010-12-08 20:35 UTC (permalink / raw)
  To: eric.dumazet; +Cc: bhutchings, Martin, netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 07 Dec 2010 23:03:55 +0100

> [PATCH v2] tcp: avoid a possible divide by zero
> 
> sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in
> tcp_tso_should_defer(). Make sure we dont allow a divide by zero by
> reading sysctl_tcp_tso_win_divisor exactly once.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] tcp: protect sysctl_tcp_cookie_size reads
  2010-12-07 22:20       ` [PATCH] tcp: protect sysctl_tcp_cookie_size reads Eric Dumazet
@ 2010-12-08 20:35         ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2010-12-08 20:35 UTC (permalink / raw)
  To: eric.dumazet; +Cc: Martin, netdev, bhutchings, william.allen.simpson

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 07 Dec 2010 23:20:47 +0100

> Make sure sysctl_tcp_cookie_size is read once in
> tcp_cookie_size_check(), or we might return an illegal value to caller
> if sysctl_tcp_cookie_size is changed by another cpu.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-12-08 20:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <201012071639.58884.Martin@lichtvoll.de>
2010-12-07 16:12 ` bugs/regressions: report in LKML or in bugzilla? Eric Dumazet
2010-12-07 21:02   ` Ben Hutchings
2010-12-07 21:28     ` [PATCH] tcp: avoid a possible divide by zero Eric Dumazet
2010-12-07 21:32       ` Ben Hutchings
2010-12-07 22:03         ` Eric Dumazet
2010-12-08  8:23           ` Martin Steigerwald
2010-12-08  8:33             ` Eric Dumazet
2010-12-08 20:35           ` David Miller
2010-12-07 22:20       ` [PATCH] tcp: protect sysctl_tcp_cookie_size reads Eric Dumazet
2010-12-08 20:35         ` David Miller
2010-12-07 21:28     ` bugs/regressions: report in LKML or in bugzilla? Martin Steigerwald
2010-12-07 21:11   ` Martin Steigerwald

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).