Netdev List
 help / color / mirror / Atom feed
* Re: [34/44] Limit sysctl_tcp_mem and sysctl_udp_mem initializers to prevent integer overflows.
From: Linus Torvalds @ 2010-12-08  1:22 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel, stable, David S. Miller, stable-review, akpm, alan,
	Robin Holt, Willy Tarreau, netdev, linux-sctp, Alexey Kuznetsov,
	Pekka Savola (ipv6), James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, Vlad Yasevich, Sridhar Samudrala
In-Reply-To: <20101208000642.975564500@clark.site>

On Tue, Dec 7, 2010 at 4:04 PM, Greg KH <gregkh@suse.de> wrote:
>
> From: Robin Holt <holt@sgi.com>
>
> [ Problem was fixed differently upstream. -DaveM ]

Gaah. I'd really like to see more of a description for things like
this. A commit ID for the alternate fix, or at least a few words about
the different fix or reason why upstream doesn't need the stable
commit.

                  Linus

^ permalink raw reply

* CWND/rcache/multiq slides...
From: David Miller @ 2010-12-08  1:17 UTC (permalink / raw)
  To: netdev


For those that don't follow me on twitter (http://twitter.com/davemloft)
I posted my slides from my presentation today at IBM Watson Research Labs:

http://vger.kernel.org/~davem/davem_ibm2010.pdf

Enjoy.

^ permalink raw reply

* [268/289] [stable] [PATCH 2.6.36 stable] vlan: Avoid hwaccel vlan packets when vid not used.
From: Greg KH @ 2010-12-08  1:00 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: stable-review, torvalds, akpm, alan, netdev, David Miller,
	Jesse Gross
In-Reply-To: <20101208005821.GA2922@kroah.com>

2.6.36-stable review patch.  If anyone has any objections, please let us know.

------------------

From: Jesse Gross <jesse@nicira.com>

[This patch applies only to 2.6.36 stable.  The problem was introduced
in that release and is already fixed by larger changes to the vlan
code in 2.6.37.]

Normally hardware accelerated vlan packets are quickly dropped if
there is no corresponding vlan device configured.  The one exception
is promiscuous mode, where we allow all of these packets through so
they can be picked up by tcpdump.  However, this behavior causes a
crash if we actually try to receive these packets.  This fixes that
crash by ignoring packets with vids not corresponding to a configured
device in the vlan hwaccel routines and then dropping them before they
get to consumers in the network stack.


Reported-by: Ben Greear <greearb@candelatech.com>
Tested-by: Nikola Ciprich <extmaillist@linuxbox.cz>
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: David Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 net/8021q/vlan_core.c |    3 +++
 net/core/dev.c        |   13 +++++++++++++
 2 files changed, 16 insertions(+)

--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -43,6 +43,9 @@ int vlan_hwaccel_do_receive(struct sk_bu
 	struct net_device *dev = skb->dev;
 	struct vlan_rx_stats     *rx_stats;
 
+	if (unlikely(!is_vlan_dev(dev)))
+		return 0;
+
 	skb->dev = vlan_dev_info(dev)->real_dev;
 	netif_nit_deliver(skb);
 
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2891,6 +2891,19 @@ static int __netif_receive_skb(struct sk
 ncls:
 #endif
 
+	/* If we got this far with a hardware accelerated VLAN tag, it means
+	 * that we were put in promiscuous mode but nobody is interested in
+	 * this vid.  Drop the packet now to prevent it from getting propagated
+	 * to other parts of the stack that won't know how to deal with packets
+	 * tagged in this manner.
+	 */
+	if (unlikely(vlan_tx_tag_present(skb))) {
+		if (pt_prev)
+			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
+		kfree_skb(skb);
+		goto out;
+	}
+
 	/* Handle special case of bridge or macvlan */
 	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (rx_handler) {



^ permalink raw reply

* Re: [PATCH] use total_highpages when calculating lowmem-only allocation sizes (dccp)
From: David Miller @ 2010-12-08  0:49 UTC (permalink / raw)
  To: JBeulich; +Cc: akpm, netdev
In-Reply-To: <4CFE54ED02000078000266AD@vpn.id2.novell.com>

From: "Jan Beulich" <JBeulich@novell.com>
Date: Tue, 07 Dec 2010 14:38:21 +0000

>>>> On 06.12.10 at 18:36, David Miller <davem@davemloft.net> wrote:
>> From: "Jan Beulich" <JBeulich@novell.com>
>> Date: Mon, 06 Dec 2010 16:42:46 +0000
>> 
>>> For those (large) table allocations that come only from lowmem, the
>>> total amount of memory shouldn't really matter.
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> 
>> Instead of continually tweaking the bits in these code paths,
>> we should be converting them over to using a central routine
>> such as alloc_large_system_hash() where the logic is consolidated
>> _AND_ the code knows to use vmalloc() and NUMA aware allocations
>> when warranted.
> 
> Hmm, not sure. For one, alloc_large_system_hash() is (and imo
> ought to remain) an __init function. Second, looking at the one
> non-modular case (netlink_proto_init()), I don't think using this
> function would be the intention here.

I think the one and only reason DCCP and friends do not use this
interface is exactly because it's not available to modules.

So much (partially incorrect, as you found) code would be removed
if this common routine were exported and utilized.

^ permalink raw reply

* [097/127] Limit sysctl_tcp_mem and sysctl_udp_mem initializers to prevent integer overflows.
From: Greg KH @ 2010-12-08  0:44 UTC (permalink / raw)
  To: linux-kernel, stable, David S. Miller
  Cc: stable-review, torvalds, akpm, alan, Robin Holt, Willy Tarreau,
	netdev, linux-sctp, Alexey Kuznetsov, Pekka Savola (ipv6),
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Vlad Yasevich,
	Sridhar Samudrala
In-Reply-To: <20101208004456.GA23578@kroah.com>

2.6.32-stable review patch.  If anyone has any objections, please let us know.

------------------


From: Robin Holt <holt@sgi.com>

[ Upstream fixed this in a different way. -DaveM ]

On a 16TB x86_64 machine, sysctl_tcp_mem[2], sysctl_udp_mem[2], and
sysctl_sctp_mem[2] can integer overflow.  Set limit such that they are
maximized without overflowing.

Signed-off-by: Robin Holt <holt@sgi.com>
To: "David S. Miller" <davem@davemloft.net>
Cc: Willy Tarreau <w@1wt.eu>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-sctp@vger.kernel.org
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: "Pekka Savola (ipv6)" <pekkas@netcore.fi>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Vlad Yasevich <vladislav.yasevich@hp.com>
Cc: Sridhar Samudrala <sri@us.ibm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 net/ipv4/tcp.c      |    4 +++-
 net/ipv4/udp.c      |    4 +++-
 net/sctp/protocol.c |    4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2940,12 +2940,14 @@ void __init tcp_init(void)
 
 	/* Set the pressure threshold to be a fraction of global memory that
 	 * is up to 1/2 at 256 MB, decreasing toward zero with the amount of
-	 * memory, with a floor of 128 pages.
+	 * memory, with a floor of 128 pages, and a ceiling that prevents an
+	 * integer overflow.
 	 */
 	nr_pages = totalram_pages - totalhigh_pages;
 	limit = min(nr_pages, 1UL<<(28-PAGE_SHIFT)) >> (20-PAGE_SHIFT);
 	limit = (limit * (nr_pages >> (20-PAGE_SHIFT))) >> (PAGE_SHIFT-11);
 	limit = max(limit, 128UL);
+	limit = min(limit, INT_MAX * 4UL / 3 / 2);
 	sysctl_tcp_mem[0] = limit / 4 * 3;
 	sysctl_tcp_mem[1] = limit;
 	sysctl_tcp_mem[2] = sysctl_tcp_mem[0] * 2;
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1832,12 +1832,14 @@ void __init udp_init(void)
 	udp_table_init(&udp_table);
 	/* Set the pressure threshold up by the same strategy of TCP. It is a
 	 * fraction of global memory that is up to 1/2 at 256 MB, decreasing
-	 * toward zero with the amount of memory, with a floor of 128 pages.
+	 * toward zero with the amount of memory, with a floor of 128 pages,
+	 * and a ceiling that prevents an integer overflow.
 	 */
 	nr_pages = totalram_pages - totalhigh_pages;
 	limit = min(nr_pages, 1UL<<(28-PAGE_SHIFT)) >> (20-PAGE_SHIFT);
 	limit = (limit * (nr_pages >> (20-PAGE_SHIFT))) >> (PAGE_SHIFT-11);
 	limit = max(limit, 128UL);
+	limit = min(limit, INT_MAX * 4UL / 3 / 2);
 	sysctl_udp_mem[0] = limit / 4 * 3;
 	sysctl_udp_mem[1] = limit;
 	sysctl_udp_mem[2] = sysctl_udp_mem[0] * 2;
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1157,7 +1157,8 @@ SCTP_STATIC __init int sctp_init(void)
 
 	/* Set the pressure threshold to be a fraction of global memory that
 	 * is up to 1/2 at 256 MB, decreasing toward zero with the amount of
-	 * memory, with a floor of 128 pages.
+	 * memory, with a floor of 128 pages, and a ceiling that prevents an
+	 * integer overflow.
 	 * Note this initalizes the data in sctpv6_prot too
 	 * Unabashedly stolen from tcp_init
 	 */
@@ -1165,6 +1166,7 @@ SCTP_STATIC __init int sctp_init(void)
 	limit = min(nr_pages, 1UL<<(28-PAGE_SHIFT)) >> (20-PAGE_SHIFT);
 	limit = (limit * (nr_pages >> (20-PAGE_SHIFT))) >> (PAGE_SHIFT-11);
 	limit = max(limit, 128UL);
+	limit = min(limit, INT_MAX * 4UL / 3 / 2);
 	sysctl_sctp_mem[0] = limit / 4 * 3;
 	sysctl_sctp_mem[1] = limit;
 	sysctl_sctp_mem[2] = sysctl_sctp_mem[0] * 2;

^ permalink raw reply

* [34/44] Limit sysctl_tcp_mem and sysctl_udp_mem initializers to prevent integer overflows.
From: Greg KH @ 2010-12-08  0:04 UTC (permalink / raw)
  To: linux-kernel, stable, David S. Miller
  Cc: stable-review, torvalds, akpm, alan, Robin Holt, Willy Tarreau,
	netdev, linux-sctp, Alexey Kuznetsov, Pekka Savola (ipv6),
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Vlad Yasevich,
	Sridhar Samudrala
In-Reply-To: <20101208003205.GA4286@kroah.com>

2.6.27-stable review patch.  If anyone has any objections, please let us know.

------------------

From: Robin Holt <holt@sgi.com>

[ Problem was fixed differently upstream. -DaveM ]

On a 16TB x86_64 machine, sysctl_tcp_mem[2], sysctl_udp_mem[2], and
sysctl_sctp_mem[2] can integer overflow.  Set limit such that they are
maximized without overflowing.

Signed-off-by: Robin Holt <holt@sgi.com>
To: "David S. Miller" <davem@davemloft.net>
Cc: Willy Tarreau <w@1wt.eu>
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-sctp@vger.kernel.org
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: "Pekka Savola (ipv6)" <pekkas@netcore.fi>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Vlad Yasevich <vladislav.yasevich@hp.com>
Cc: Sridhar Samudrala <sri@us.ibm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 net/ipv4/tcp.c      |    4 +++-
 net/ipv4/udp.c      |    4 +++-
 net/sctp/protocol.c |    4 +++-
 3 files changed, 9 insertions(+), 3 deletions(-)

--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2754,12 +2754,14 @@ void __init tcp_init(void)
 
 	/* Set the pressure threshold to be a fraction of global memory that
 	 * is up to 1/2 at 256 MB, decreasing toward zero with the amount of
-	 * memory, with a floor of 128 pages.
+	 * memory, with a floor of 128 pages, and a ceiling that prevents an
+	 * integer overflow.
 	 */
 	nr_pages = totalram_pages - totalhigh_pages;
 	limit = min(nr_pages, 1UL<<(28-PAGE_SHIFT)) >> (20-PAGE_SHIFT);
 	limit = (limit * (nr_pages >> (20-PAGE_SHIFT))) >> (PAGE_SHIFT-11);
 	limit = max(limit, 128UL);
+	limit = min(limit, INT_MAX * 4UL / 3 / 2);
 	sysctl_tcp_mem[0] = limit / 4 * 3;
 	sysctl_tcp_mem[1] = limit;
 	sysctl_tcp_mem[2] = sysctl_tcp_mem[0] * 2;
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1722,11 +1722,13 @@ void __init udp_init(void)
 
 	/* Set the pressure threshold up by the same strategy of TCP. It is a
 	 * fraction of global memory that is up to 1/2 at 256 MB, decreasing
-	 * toward zero with the amount of memory, with a floor of 128 pages.
+	 * toward zero with the amount of memory, with a floor of 128 pages,
+	 * and a ceiling that prevents an integer overflow.
 	 */
 	limit = min(nr_all_pages, 1UL<<(28-PAGE_SHIFT)) >> (20-PAGE_SHIFT);
 	limit = (limit * (nr_all_pages >> (20-PAGE_SHIFT))) >> (PAGE_SHIFT-11);
 	limit = max(limit, 128UL);
+	limit = min(limit, INT_MAX * 4UL / 3 / 2);
 	sysctl_udp_mem[0] = limit / 4 * 3;
 	sysctl_udp_mem[1] = limit;
 	sysctl_udp_mem[2] = sysctl_udp_mem[0] * 2;
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1179,7 +1179,8 @@ SCTP_STATIC __init int sctp_init(void)
 
 	/* Set the pressure threshold to be a fraction of global memory that
 	 * is up to 1/2 at 256 MB, decreasing toward zero with the amount of
-	 * memory, with a floor of 128 pages.
+	 * memory, with a floor of 128 pages, and a ceiling that prevents an
+	 * integer overflow.
 	 * Note this initalizes the data in sctpv6_prot too
 	 * Unabashedly stolen from tcp_init
 	 */
@@ -1187,6 +1188,7 @@ SCTP_STATIC __init int sctp_init(void)
 	limit = min(nr_pages, 1UL<<(28-PAGE_SHIFT)) >> (20-PAGE_SHIFT);
 	limit = (limit * (nr_pages >> (20-PAGE_SHIFT))) >> (PAGE_SHIFT-11);
 	limit = max(limit, 128UL);
+	limit = min(limit, INT_MAX * 4UL / 3 / 2);
 	sysctl_sctp_mem[0] = limit / 4 * 3;
 	sysctl_sctp_mem[1] = limit;
 	sysctl_sctp_mem[2] = sysctl_sctp_mem[0] * 2;

^ permalink raw reply

* Re: [PATCH] X.25: decrement netdev reference counts on unload
From: Andrew Hendry @ 2010-12-07 22:48 UTC (permalink / raw)
  To: Apollon Oikonomopoulos; +Cc: netdev
In-Reply-To: <20101207194330.GA22608@noc.grnet.gr>

Thanks for the patch.
Can reproduce it on a vm here, I'll verify it when have access to some
X.25 networks tomorrow.

2010/12/8 Apollon Oikonomopoulos <apollon@noc.grnet.gr>:
> Hello,
>
> We came across this bug when we accidentally loaded and unloaded the x25
> kernel module on a host with KVM virtual machines. All subsequent
> attempts to stop a KVM instance resulted in the KVM process entering D
> state. Reproducing this behaviour can be done in the following way:
>
> 1. modprobe x25
> 2. tunctl && ifconfig tap0 up
> 3. rmmod x25
> 4. tunctl -d tap0 (hangs)
>
> A patch follows - we don't use X.25 ourselves so I'm not sure it won't
> panic someone's system.
>
> Regards,
> Apollon
>
> PS: I am not subscribed to the list, so please copy any replies to my
> address. Thanks!
> ----
> x25 does not decrement the network device reference counts on module unload.
> Thus unregistering any pre-existing interface after unloading the x25 module
> hangs and results in
>
>  unregister_netdevice: waiting for tap0 to become free. Usage count = 1
>
> This patch decrements the reference counts of all interfaces in x25_link_free,
> the way it is already done in x25_link_device_down for NETDEV_DOWN events.
>
> Signed-off-by: Apollon Oikonomopoulos <apollon@noc.grnet.gr>
> ---
>  net/x25/x25_link.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
> index 73e7b95..b25c646 100644
> --- a/net/x25/x25_link.c
> +++ b/net/x25/x25_link.c
> @@ -394,6 +394,7 @@ void __exit x25_link_free(void)
>        list_for_each_safe(entry, tmp, &x25_neigh_list) {
>                nb = list_entry(entry, struct x25_neigh, node);
>                __x25_remove_neigh(nb);
> +               dev_put(nb->dev);
>        }
>        write_unlock_bh(&x25_neigh_list_lock);
>  }
> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply

* [PATCH] tcp: protect sysctl_tcp_cookie_size reads
From: Eric Dumazet @ 2010-12-07 22:20 UTC (permalink / raw)
  To: David Miller
  Cc: Martin Steigerwald, netdev, Ben Hutchings, William Allen Simpson
In-Reply-To: <1291757288.5324.18.camel@edumazet-laptop>

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

* Re: [patch] econet: unlock on -EPERM path
From: Jarek Poplawski @ 2010-12-07 22:09 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: netdev, Phil Blundell, Eric Dumazet, David S. Miller,
	kernel-janitors
In-Reply-To: <4CFEAEC2.7080506@gmail.com>

Jarek Poplawski wrote:
> Dan Carpenter wrote:
>> We need to do a mutex_unlock() and a put_dev() before returning.
> 
> Good catch! Btw, isn't this put_dev() below, in case SIOCGIFADDR,
> superfluous?

Hmm... put_dev() a.k.a. dev_put() ;-)

Jarek P.

^ permalink raw reply

* Re: [PATCH] tcp: avoid a possible divide by zero
From: Eric Dumazet @ 2010-12-07 22:03 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, Martin Steigerwald, netdev
In-Reply-To: <1291757563.21627.15.camel@bwh-desktop>

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

* Re: [patch] econet: unlock on -EPERM path
From: Jarek Poplawski @ 2010-12-07 22:01 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: netdev, Phil Blundell, Eric Dumazet, David S. Miller,
	kernel-janitors
In-Reply-To: <20101207110138.GM10623@bicker>

Dan Carpenter wrote:
> We need to do a mutex_unlock() and a put_dev() before returning.

Good catch! Btw, isn't this put_dev() below, in case SIOCGIFADDR,
superfluous?

Jarek P.

^ permalink raw reply

* Re: pull request: sfc-2.6 2010-12-07
From: Ben Hutchings @ 2010-12-07 21:45 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, sf-linux-drivers
In-Reply-To: <1291758047.21627.19.camel@bwh-desktop>

On Tue, 2010-12-07 at 21:40 +0000, Ben Hutchings wrote:
> The following changes since commit 46bcf14f44d8f31ecfdc8b6708ec15a3b33316d9:
> 
>   filter: fix sk_filter rcu handling (2010-12-06 09:29:43 -0800)
> 
> are available in the git repository at:
>   git://git.kernel.org/linux/kernel/git/bwh/sfc-2.6.git sfc-2.6.37

That should of course be:

  git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-2.6.git sfc-2.6.37

Ben.

> (or will be shortly).
> 
> These fix bugs introduced in 2.6.37-rc1.  Please pull.
> 
> Ben.
> 
> Ben Hutchings (2):
>       sfc: Fix crash in legacy onterrupt handler during ring reallocation
>       sfc: Fix NAPI list corruption during ring reallocation
> 
>  drivers/net/sfc/efx.c        |   43 ++++++++++++++++++++++++++++-------------
>  drivers/net/sfc/net_driver.h |    2 +
>  drivers/net/sfc/nic.c        |    6 +++++
>  3 files changed, 37 insertions(+), 14 deletions(-)
> 

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

* [PATCH net-2.6 2/2] sfc: Fix NAPI list corruption during ring reallocation
From: Ben Hutchings @ 2010-12-07 21:42 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1291758047.21627.19.camel@bwh-desktop>

Call netif_napi_{add,del}() on the NAPI contexts in the new and
old channels, respectively.

Since efx_init_napi() cannot fail, make its return type void.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/efx.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index d06cb74..fb83cdd 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -197,7 +197,9 @@ MODULE_PARM_DESC(debug, "Bitmapped debugging message enable value");
 
 static void efx_remove_channels(struct efx_nic *efx);
 static void efx_remove_port(struct efx_nic *efx);
+static void efx_init_napi(struct efx_nic *efx);
 static void efx_fini_napi(struct efx_nic *efx);
+static void efx_fini_napi_channel(struct efx_channel *channel);
 static void efx_fini_struct(struct efx_nic *efx);
 static void efx_start_all(struct efx_nic *efx);
 static void efx_stop_all(struct efx_nic *efx);
@@ -430,6 +432,7 @@ efx_alloc_channel(struct efx_nic *efx, int i, struct efx_channel *old_channel)
 
 		*channel = *old_channel;
 
+		channel->napi_dev = NULL;
 		memset(&channel->eventq, 0, sizeof(channel->eventq));
 
 		rx_queue = &channel->rx_queue;
@@ -740,9 +743,13 @@ efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
 	if (rc)
 		goto rollback;
 
+	efx_init_napi(efx);
+
 	/* Destroy old channels */
-	for (i = 0; i < efx->n_channels; i++)
+	for (i = 0; i < efx->n_channels; i++) {
+		efx_fini_napi_channel(other_channel[i]);
 		efx_remove_channel(other_channel[i]);
+	}
 out:
 	/* Free unused channel structures */
 	for (i = 0; i < efx->n_channels; i++)
@@ -1601,7 +1608,7 @@ static int efx_ioctl(struct net_device *net_dev, struct ifreq *ifr, int cmd)
  *
  **************************************************************************/
 
-static int efx_init_napi(struct efx_nic *efx)
+static void efx_init_napi(struct efx_nic *efx)
 {
 	struct efx_channel *channel;
 
@@ -1610,18 +1617,21 @@ static int efx_init_napi(struct efx_nic *efx)
 		netif_napi_add(channel->napi_dev, &channel->napi_str,
 			       efx_poll, napi_weight);
 	}
-	return 0;
+}
+
+static void efx_fini_napi_channel(struct efx_channel *channel)
+{
+	if (channel->napi_dev)
+		netif_napi_del(&channel->napi_str);
+	channel->napi_dev = NULL;
 }
 
 static void efx_fini_napi(struct efx_nic *efx)
 {
 	struct efx_channel *channel;
 
-	efx_for_each_channel(channel, efx) {
-		if (channel->napi_dev)
-			netif_napi_del(&channel->napi_str);
-		channel->napi_dev = NULL;
-	}
+	efx_for_each_channel(channel, efx)
+		efx_fini_napi_channel(channel);
 }
 
 /**************************************************************************
@@ -2343,9 +2353,7 @@ static int efx_pci_probe_main(struct efx_nic *efx)
 	if (rc)
 		goto fail1;
 
-	rc = efx_init_napi(efx);
-	if (rc)
-		goto fail2;
+	efx_init_napi(efx);
 
 	rc = efx->type->init(efx);
 	if (rc) {
@@ -2376,7 +2384,6 @@ static int efx_pci_probe_main(struct efx_nic *efx)
 	efx->type->fini(efx);
  fail3:
 	efx_fini_napi(efx);
- fail2:
 	efx_remove_all(efx);
  fail1:
 	return rc;
-- 
1.7.3.2


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

* [PATCH net-2.6 1/2] sfc: Fix crash in legacy onterrupt handler during ring reallocation
From: Ben Hutchings @ 2010-12-07 21:42 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1291758047.21627.19.camel@bwh-desktop>

If we are using a legacy interrupt, our IRQ may be shared and our
interrupt handler may be called even though interrupts are disabled on
the NIC. When we change ring sizes, we reallocate the event queue and
the interrupt handler may use an invalid pointer when called for
another device's interrupt.

Maintain a legacy_irq_enabled flag and test that at the top of the
interrupt handler.  Note that this problem results from the need to
work around broken INT_ISR0 reads, and does not affect the legacy
interrupt handler for Falcon A1.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/efx.c        |   12 ++++++++++--
 drivers/net/sfc/net_driver.h |    2 ++
 drivers/net/sfc/nic.c        |    6 ++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 05df20e..d06cb74 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -335,8 +335,10 @@ void efx_process_channel_now(struct efx_channel *channel)
 
 	/* Disable interrupts and wait for ISRs to complete */
 	efx_nic_disable_interrupts(efx);
-	if (efx->legacy_irq)
+	if (efx->legacy_irq) {
 		synchronize_irq(efx->legacy_irq);
+		efx->legacy_irq_enabled = false;
+	}
 	if (channel->irq)
 		synchronize_irq(channel->irq);
 
@@ -351,6 +353,8 @@ void efx_process_channel_now(struct efx_channel *channel)
 	efx_channel_processed(channel);
 
 	napi_enable(&channel->napi_str);
+	if (efx->legacy_irq)
+		efx->legacy_irq_enabled = true;
 	efx_nic_enable_interrupts(efx);
 }
 
@@ -1400,6 +1404,8 @@ static void efx_start_all(struct efx_nic *efx)
 		efx_start_channel(channel);
 	}
 
+	if (efx->legacy_irq)
+		efx->legacy_irq_enabled = true;
 	efx_nic_enable_interrupts(efx);
 
 	/* Switch to event based MCDI completions after enabling interrupts.
@@ -1460,8 +1466,10 @@ static void efx_stop_all(struct efx_nic *efx)
 
 	/* Disable interrupts and wait for ISR to complete */
 	efx_nic_disable_interrupts(efx);
-	if (efx->legacy_irq)
+	if (efx->legacy_irq) {
 		synchronize_irq(efx->legacy_irq);
+		efx->legacy_irq_enabled = false;
+	}
 	efx_for_each_channel(channel, efx) {
 		if (channel->irq)
 			synchronize_irq(channel->irq);
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index 0a7e26d..b137c88 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -621,6 +621,7 @@ struct efx_filter_state;
  * @pci_dev: The PCI device
  * @type: Controller type attributes
  * @legacy_irq: IRQ number
+ * @legacy_irq_enabled: Are IRQs enabled on NIC (INT_EN_KER register)?
  * @workqueue: Workqueue for port reconfigures and the HW monitor.
  *	Work items do not hold and must not acquire RTNL.
  * @workqueue_name: Name of workqueue
@@ -709,6 +710,7 @@ struct efx_nic {
 	struct pci_dev *pci_dev;
 	const struct efx_nic_type *type;
 	int legacy_irq;
+	bool legacy_irq_enabled;
 	struct workqueue_struct *workqueue;
 	char workqueue_name[16];
 	struct work_struct reset_work;
diff --git a/drivers/net/sfc/nic.c b/drivers/net/sfc/nic.c
index 41c36b9..67cb0c9 100644
--- a/drivers/net/sfc/nic.c
+++ b/drivers/net/sfc/nic.c
@@ -1418,6 +1418,12 @@ static irqreturn_t efx_legacy_interrupt(int irq, void *dev_id)
 	u32 queues;
 	int syserr;
 
+	/* Could this be ours?  If interrupts are disabled then the
+	 * channel state may not be valid.
+	 */
+	if (!efx->legacy_irq_enabled)
+		return result;
+
 	/* Read the ISR which also ACKs the interrupts */
 	efx_readd(efx, &reg, FR_BZ_INT_ISR0);
 	queues = EFX_EXTRACT_DWORD(reg, 0, 31);
-- 
1.7.3.2



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

* pull request: sfc-2.6 2010-12-07
From: Ben Hutchings @ 2010-12-07 21:40 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, sf-linux-drivers

The following changes since commit 46bcf14f44d8f31ecfdc8b6708ec15a3b33316d9:

  filter: fix sk_filter rcu handling (2010-12-06 09:29:43 -0800)

are available in the git repository at:
  git://git.kernel.org/linux/kernel/git/bwh/sfc-2.6.git sfc-2.6.37
(or will be shortly).

These fix bugs introduced in 2.6.37-rc1.  Please pull.

Ben.

Ben Hutchings (2):
      sfc: Fix crash in legacy onterrupt handler during ring reallocation
      sfc: Fix NAPI list corruption during ring reallocation

 drivers/net/sfc/efx.c        |   43 ++++++++++++++++++++++++++++-------------
 drivers/net/sfc/net_driver.h |    2 +
 drivers/net/sfc/nic.c        |    6 +++++
 3 files changed, 37 insertions(+), 14 deletions(-)

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

* Re: [PATCH 1/4] ath: Add and use ath_printk and ath_<level>
From: John W. Linville @ 2010-12-07 21:38 UTC (permalink / raw)
  To: Joe Perches
  Cc: Luis R. Rodriguez, linux-kernel, Peter Stuge, Felix Fietkau,
	linux-wireless, netdev
In-Reply-To: <241e75c84fdd26bfae1c7b8806fe64fbac5de627.1291333544.git.joe@perches.com>

On Thu, Dec 02, 2010 at 07:12:35PM -0800, Joe Perches wrote:
> Add ath_printk and ath_<level> similar to
> dev_printk and dev_<level> from device.h
> 
> This allows a more gradual rename of ath_print
> to to ath_dbg or perhaps ath_debug.
> 
> This basically removes debug.h leaving
> only an #define ath_printk ath_dbg
> there and moving all the ATH_DBG_<foo>
> enums to ath.h
> 
> I do not think there's much purpose for struct
> ath_common * being passed to the ath_printk
> functions, but perhaps there might be.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

> +#define ATH_DBG_WARN(foo, arg...) WARN(foo, arg)
> +
> +#else
> +
> +static inline  __attribute__ ((format (printf, 3, 4))) int
> +ath_dbg(struct ath_common *common, enum ATH_DEBUG dbg_mask,
> +	const char *fmt, ...)
> +{
> +	return 0;
> +}
> +#define ATH_DBG_WARN(foo, arg) do {} while (0)

Missing the "..." after "arg" -- I fixed it up...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

^ permalink raw reply

* [PATCH] tcp: avoid a possible divide by zero
From: Eric Dumazet @ 2010-12-07 21:28 UTC (permalink / raw)
  To: Ben Hutchings, David Miller; +Cc: Martin Steigerwald, netdev
In-Reply-To: <1291755776.21627.13.camel@bwh-desktop>

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

* Re: [PATCH] tcp: avoid a possible divide by zero
From: Ben Hutchings @ 2010-12-07 21:32 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, Martin Steigerwald, netdev
In-Reply-To: <1291757288.5324.18.camel@edumazet-laptop>

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

* Re: bugs/regressions: report in LKML or in bugzilla?
From: Martin Steigerwald @ 2010-12-07 21:28 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Eric Dumazet, linux-kernel, netdev
In-Reply-To: <1291755776.21627.13.camel@bwh-desktop>

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

* Re: bugs/regressions: report in LKML or in bugzilla?
From: Martin Steigerwald @ 2010-12-07 21:11 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: linux-kernel, netdev
In-Reply-To: <1291738321.2695.338.camel@edumazet-laptop>

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

* Re: [stable] [PATCH 2.6.36 stable] vlan: Avoid hwaccel vlan packets when vid not used.
From: Greg KH @ 2010-12-07 21:03 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Jesse Gross, stable, David Miller
In-Reply-To: <1291755026.5324.3.camel@edumazet-laptop>

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Tue, Dec 07, 2010 at 09:50:26PM +0100, Eric Dumazet wrote:
> Le mardi 07 décembre 2010 à 11:50 -0800, Greg KH a écrit :
> > On Mon, Nov 08, 2010 at 01:23:01PM -0800, Jesse Gross wrote:
> > > Normally hardware accelerated vlan packets are quickly dropped if
> > > there is no corresponding vlan device configured.  The one exception
> > > is promiscuous mode, where we allow all of these packets through so
> > > they can be picked up by tcpdump.  However, this behavior causes a
> > > crash if we actually try to receive these packets.  This fixes that
> > > crash by ignoring packets with vids not corresponding to a configured
> > > device in the vlan hwaccel routines and then dropping them before they
> > > get to consumers in the network stack.
> > > 
> > > This patch applies only to 2.6.36 stable.  The problem was introduced
> > > in that release and is already fixed by larger changes to the vlan
> > > code in 2.6.37.
> > 
> > Applied, thanks.
> > 
> 
> Oh well, which version ?
> 
> A new version of the patch was submitted 6 days ago .
> 
> http://patchwork.ozlabs.org/patch/73791/

I applied the one below.

If that is incorrect, please send me the correct one.

thanks,

greg k-h

[-- Attachment #2: vlan-avoid-hwaccel-vlan-packets-when-vid-not-used.patch --]
[-- Type: text/x-patch, Size: 2482 bytes --]

>From jesse@nicira.com  Tue Dec  7 11:49:39 2010
From: Jesse Gross <jesse@nicira.com>
Date: Mon,  8 Nov 2010 13:23:01 -0800
Subject: [stable] [PATCH 2.6.36 stable] vlan: Avoid hwaccel vlan packets when vid not used.
To: stable@kernel.org
Cc: netdev@vger.kernel.org, David Miller <davem@davemloft.net>
Message-ID: <1289251381-6671-1-git-send-email-jesse@nicira.com>

From: Jesse Gross <jesse@nicira.com>

[This patch applies only to 2.6.36 stable.  The problem was introduced
in that release and is already fixed by larger changes to the vlan
code in 2.6.37.]

Normally hardware accelerated vlan packets are quickly dropped if
there is no corresponding vlan device configured.  The one exception
is promiscuous mode, where we allow all of these packets through so
they can be picked up by tcpdump.  However, this behavior causes a
crash if we actually try to receive these packets.  This fixes that
crash by ignoring packets with vids not corresponding to a configured
device in the vlan hwaccel routines and then dropping them before they
get to consumers in the network stack.


Reported-by: Ben Greear <greearb@candelatech.com>
Tested-by: Nikola Ciprich <extmaillist@linuxbox.cz>
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: David Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 net/8021q/vlan_core.c |    3 +++
 net/core/dev.c        |   13 +++++++++++++
 2 files changed, 16 insertions(+)

--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -43,6 +43,9 @@ int vlan_hwaccel_do_receive(struct sk_bu
 	struct net_device *dev = skb->dev;
 	struct vlan_rx_stats     *rx_stats;
 
+	if (unlikely(!is_vlan_dev(dev)))
+		return 0;
+
 	skb->dev = vlan_dev_info(dev)->real_dev;
 	netif_nit_deliver(skb);
 
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2891,6 +2891,19 @@ static int __netif_receive_skb(struct sk
 ncls:
 #endif
 
+	/* If we got this far with a hardware accelerated VLAN tag, it means
+	 * that we were put in promiscuous mode but nobody is interested in
+	 * this vid.  Drop the packet now to prevent it from getting propagated
+	 * to other parts of the stack that won't know how to deal with packets
+	 * tagged in this manner.
+	 */
+	if (unlikely(vlan_tx_tag_present(skb))) {
+		if (pt_prev)
+			ret = pt_prev->func(skb, skb->dev, pt_prev, orig_dev);
+		kfree_skb(skb);
+		goto out;
+	}
+
 	/* Handle special case of bridge or macvlan */
 	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (rx_handler) {

^ permalink raw reply

* Re: bugs/regressions: report in LKML or in bugzilla?
From: Ben Hutchings @ 2010-12-07 21:02 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Martin Steigerwald, linux-kernel, netdev
In-Reply-To: <1291738321.2695.338.camel@edumazet-laptop>

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

* Re: WARNING at local_bh_enable while tcp_retransmit
From: Jarek Poplawski @ 2010-12-07 20:56 UTC (permalink / raw)
  To: Ilya Loginov; +Cc: netdev, davem
In-Reply-To: <20101207182308.664e171d.isloginov@gmail.com>

Ilya Loginov wrote:
> 	Hi, I am working on some network drivers.
...
> 	I watched bonding, but I could not realize why it didn't get
> same warning. It use very similar scheme of work.
> 
> 	Do you have any ideas?

Hi, isn't any of your drivers disabling irqs on this path?
(If no, what is the kernel version?)

Jarek P.

^ permalink raw reply

* Re: [stable] [PATCH 2.6.36 stable] vlan: Avoid hwaccel vlan packets when vid not used.
From: Eric Dumazet @ 2010-12-07 20:50 UTC (permalink / raw)
  To: Greg KH; +Cc: Jesse Gross, stable, netdev, David Miller
In-Reply-To: <20101207195019.GP13189@kroah.com>

Le mardi 07 décembre 2010 à 11:50 -0800, Greg KH a écrit :
> On Mon, Nov 08, 2010 at 01:23:01PM -0800, Jesse Gross wrote:
> > Normally hardware accelerated vlan packets are quickly dropped if
> > there is no corresponding vlan device configured.  The one exception
> > is promiscuous mode, where we allow all of these packets through so
> > they can be picked up by tcpdump.  However, this behavior causes a
> > crash if we actually try to receive these packets.  This fixes that
> > crash by ignoring packets with vids not corresponding to a configured
> > device in the vlan hwaccel routines and then dropping them before they
> > get to consumers in the network stack.
> > 
> > This patch applies only to 2.6.36 stable.  The problem was introduced
> > in that release and is already fixed by larger changes to the vlan
> > code in 2.6.37.
> 
> Applied, thanks.
> 

Oh well, which version ?

A new version of the patch was submitted 6 days ago .

http://patchwork.ozlabs.org/patch/73791/

Thanks





^ permalink raw reply

* Re: [stable] [PATCH 2.6.36 stable] vlan: Avoid hwaccel vlan packets when vid not used.
From: Greg KH @ 2010-12-07 19:50 UTC (permalink / raw)
  To: Jesse Gross; +Cc: stable, netdev, David Miller
In-Reply-To: <1289251381-6671-1-git-send-email-jesse@nicira.com>

On Mon, Nov 08, 2010 at 01:23:01PM -0800, Jesse Gross wrote:
> Normally hardware accelerated vlan packets are quickly dropped if
> there is no corresponding vlan device configured.  The one exception
> is promiscuous mode, where we allow all of these packets through so
> they can be picked up by tcpdump.  However, this behavior causes a
> crash if we actually try to receive these packets.  This fixes that
> crash by ignoring packets with vids not corresponding to a configured
> device in the vlan hwaccel routines and then dropping them before they
> get to consumers in the network stack.
> 
> This patch applies only to 2.6.36 stable.  The problem was introduced
> in that release and is already fixed by larger changes to the vlan
> code in 2.6.37.

Applied, thanks.

greg k-h

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox