* Re: [patch 0/5] qeth bug fixes for 2.6.34 next rc
From: David Miller @ 2010-03-10 15:32 UTC (permalink / raw)
To: frank.blaschka; +Cc: netdev, linux-s390
In-Reply-To: <20100309063652.706791000@de.ibm.com>
From: frank.blaschka@de.ibm.com
Date: Tue, 09 Mar 2010 07:36:52 +0100
> here are some qeth bug fixes for 2.6.34 next rc
All applied, thanks.
^ permalink raw reply
* [PATCH net-next-2.6] bridge: ensure to unlock in error path in br_multicast_query().
From: YOSHIFUJI Hideaki @ 2010-03-10 15:07 UTC (permalink / raw)
To: davem, netdev; +Cc: yoshfuji
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
net/bridge/br_multicast.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2559fb5..2326a34 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -823,6 +823,7 @@ static int br_multicast_query(struct net_bridge *br,
unsigned long max_delay;
unsigned long now = jiffies;
__be32 group;
+ int err = 0;
spin_lock(&br->multicast_lock);
if (!netif_running(br->dev) ||
@@ -841,12 +842,14 @@ static int br_multicast_query(struct net_bridge *br,
group = 0;
}
} else {
- if (!pskb_may_pull(skb, sizeof(struct igmpv3_query)))
- return -EINVAL;
+ if (!pskb_may_pull(skb, sizeof(struct igmpv3_query))) {
+ err = -EINVAL;
+ goto out;
+ }
ih3 = igmpv3_query_hdr(skb);
if (ih3->nsrcs)
- return 0;
+ goto out;
max_delay = ih3->code ? 1 :
IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE);
@@ -876,7 +879,7 @@ static int br_multicast_query(struct net_bridge *br,
out:
spin_unlock(&br->multicast_lock);
- return 0;
+ return err;
}
static void br_multicast_leave_group(struct net_bridge *br,
--
1.5.6.5
^ permalink raw reply related
* Re: bug report: netpoll: allow execution of multiple rx_hooks per interface
From: David Miller @ 2010-03-10 14:52 UTC (permalink / raw)
To: danborkmann; +Cc: error27, netdev
In-Reply-To: <4B977EDB.5010202@gmail.com>
From: Daniel Borkmann <danborkmann@googlemail.com>
Date: Wed, 10 Mar 2010 12:13:31 +0100
> Sorry for the late reply, I'm currently on holiday and have hardly
> access to the Internet. I'll fix the problem as soon as possible.
>
> You'll hear from me.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-10 14:07 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Eric Dumazet, Arnd Bergmann, David S. Miller, netdev,
Stephen Hemminger
In-Reply-To: <20100310131317.GA6267@linux.vnet.ibm.com>
On Wed, Mar 10, 2010 at 05:13:18AM -0800, Paul E. McKenney wrote:
>
> If CONFIG_PROVE_RCU is set, rcu_dereference() checks for rcu_read_lock()
> and rcu_dereference_bh() checks for either rcu_read_lock_bh() or BH
> being disabled. Yes, this is a bit restrictive, but there are a few too
> many to check by hand these days.
Fair enough. We should get those fixed then. In fact I reckon
most of them should be using the BH variant so we might be able
to kill a few rcu_read_lock's which would be a real gain.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-10 13:57 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <201003101430.06736.arnd@arndb.de>
On Wed, Mar 10, 2010 at 02:30:06PM +0100, Arnd Bergmann wrote:
> On Wednesday 10 March 2010, Paul E. McKenney wrote:
> > > When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> > > turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> > > build with sparse. Would that change be
> > > a) correct from RCU perspective,
> > > b) desirable for code inspection, and
> > > c) lockdep-clean?
> >
> > I have a patch queued up that will make rcu_dereference_bh() handle this
> > correctly -- current -tip and mainline would complain. Please see below
> > for a sneak preview.
> >
> > Thoughts?
>
> Ok, so that would mean we can convert it all to rcu_dereference_bh().
> I guess an alternative to this would be to also change the rcu_read_lock()
> inside local_bh_disable() sections to rcu_read_lock_bh(), which is not
> necessary but also not harmful, right?
It does impose additional overhead, which the networking guys are eager
to avoid, given that network links speeds are continuing to increase. ;-)
So moving to rcu_dereference_bh() seems better to me.
(Please note that rcu_dereference_bh() imposes checking overhead only
in the presence of both CONFIG_PROVE_LOCKING and CONFIG_PROVE_RCU.)
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 13:39 UTC (permalink / raw)
To: Herbert Xu
Cc: Eric Dumazet, paulmck, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310104907.GA23752@gondor.apana.org.au>
On Wednesday 10 March 2010, Herbert Xu wrote:
> >
> > Its really rcu_dereference_bh() that could/should be used:
> > I see no problem changing
> >
> >
> > local_bh_disable();
> > ...
> > rcu_read_lock();
> > rcu_dereference(rt_hash_table[h].chain);
> > rcu_read_unlock();
> > ...
> > local_bh_enable();
>
> Why don't we just ignore the bh part for rcu_dereference?
>
> After all it's call_rcu_bh and the other primitives that we really
> care about. For rcu_dereference bh should make no difference
> whatsoever.
To add some background on what I'm doing, I'm currently adding
new address space modifier __rcu, __rcu_bh, __rcu_sched and __srcu
to the sparse annotations along the same lines that our __iomem,
__user and __percpu annotations work [1].
In order to check all cases, I want to ensure that you can not
use any of those pointers outside of rcu_dereference* and
rcu_assign_pointer, as well as making sure that you cannot pass
a pointer without these annotations in there, so we can catch
code that uses rcu_dereference without rcu_assign_pointer or
call_rcu.
Consequently, rcu_dereference also checks that the pointer is actually
__rcu, and passing an __rcu_bh pointer in would be considered as
wrong as passing a regular pointer by sparse.
With the work that Paul has done on lockdep, rcu_dereference_bh
now also checks that bottom halves are really disabled, which is
a very useful thing to check if you want to prove that the
call_rcu is really serialized with the use of the data.
Arnd
[1] http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=shortlog;h=refs/heads/rcu-annotate
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 13:27 UTC (permalink / raw)
To: Herbert Xu
Cc: Eric Dumazet, paulmck, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310104907.GA23752@gondor.apana.org.au>
On Wednesday 10 March 2010, Herbert Xu wrote:
> >
> > Its really rcu_dereference_bh() that could/should be used:
> > I see no problem changing
> >
> >
> > local_bh_disable();
> > ...
> > rcu_read_lock();
> > rcu_dereference(rt_hash_table[h].chain);
> > rcu_read_unlock();
> > ...
> > local_bh_enable();
>
> Why don't we just ignore the bh part for rcu_dereference?
>
> After all it's call_rcu_bh and the other primitives that we really
> care about. For rcu_dereference bh should make no difference
> whatsoever.
To add some background on what I'm doing, I'm currently adding
new address space modifier __rcu, __rcu_bh, __rcu_sched and __srcu
to the sparse annotations along the same lines that our __iomem,
__user and __percpu annotations work [1].
In order to check all cases, I want to ensure that you can not
use any of those pointers outside of rcu_dereference* and
rcu_assign_pointer, as well as making sure that you cannot pass
a pointer without these annotations in there, so we can catch
code that uses rcu_dereference without rcu_assign_pointer or
call_rcu.
Consequently, rcu_dereference also checks that the pointer is actually
__rcu, and passing an __rcu_bh pointer in would be considered as
wrong as passing a regular pointer by sparse.
With the work that Paul has done on lockdep, rcu_dereference_bh
now also checks that bottom halves are really disabled, which is
a very useful thing to check if you want to prove that the
call_rcu is really serialized with the use of the data.
Arnd
[1] http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=shortlog;h=refs/heads/rcu-annotate
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 13:30 UTC (permalink / raw)
To: paulmck; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310131946.GB6267@linux.vnet.ibm.com>
On Wednesday 10 March 2010, Paul E. McKenney wrote:
> > When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> > turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> > build with sparse. Would that change be
> > a) correct from RCU perspective,
> > b) desirable for code inspection, and
> > c) lockdep-clean?
>
> I have a patch queued up that will make rcu_dereference_bh() handle this
> correctly -- current -tip and mainline would complain. Please see below
> for a sneak preview.
>
> Thoughts?
Ok, so that would mean we can convert it all to rcu_dereference_bh().
I guess an alternative to this would be to also change the rcu_read_lock()
inside local_bh_disable() sections to rcu_read_lock_bh(), which is not
necessary but also not harmful, right?
Arnd
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-10 13:19 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <201003101041.32518.arnd@arndb.de>
On Wed, Mar 10, 2010 at 10:41:32AM +0100, Arnd Bergmann wrote:
> On Wednesday 10 March 2010 03:14:10 Paul E. McKenney wrote:
> > On Tue, Mar 09, 2010 at 10:12:59PM +0100, Arnd Bergmann wrote:
> >
> > > I've just tried annotating net/ipv4/route.c like this and did not get
> > > very far, because the same pointers are used for rcu and rcu_bh.
> > > Could you check if this is a false positive or an actual finding?
> >
> > Hmmm... I am only seeing a call_rcu_bh() here, so unless I am missing
> > something, this is a real problem in TREE_PREEMPT_RCU kernels. The
> > call_rcu_bh() only interacts with the rcu_read_lock_bh() readers, not
> > the rcu_read_lock() readers.
> >
> > One approach is to run freed blocks through both types of grace periods,
> > I suppose.
>
> Well, if I introduce different __rcu and __rcu_bh address space annotations,
> sparse would still not like that, because then you can only pass the annotated
> pointers into either rcu_dereference or rcu_dereference_bh.
>
> What the code seems to be doing here is in some places
>
> local_bh_disable();
> ...
> rcu_read_lock();
> rcu_dereference(rt_hash_table[h].chain);
> rcu_read_unlock();
> ...
> local_bh_enable();
>
> and in others
>
> rcu_read_lock_bh();
> rcu_dereference_bh(rt_hash_table[h].chain);
> rcu_read_unlock_bh();
Hmmm... This is actually legal.
> When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> build with sparse. Would that change be
> a) correct from RCU perspective,
> b) desirable for code inspection, and
> c) lockdep-clean?
I have a patch queued up that will make rcu_dereference_bh() handle this
correctly -- current -tip and mainline would complain. Please see below
for a sneak preview.
Thoughts?
Thanx, Paul
rcu: make rcu_read_lock_bh_held() allow for disabled BH
Disabling BH can stand in for rcu_read_lock_bh(), and this patch updates
rcu_read_lock_bh_held() to allow for this. In order to avoid
include-file hell, this function is moved out of line to kernel/rcupdate.c.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
include/linux/rcupdate.h | 19 ++++---------------
kernel/rcupdate.c | 22 ++++++++++++++++++++++
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 75921b8..c393acc 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -119,22 +119,11 @@ static inline int rcu_read_lock_held(void)
return lock_is_held(&rcu_lock_map);
}
-/**
- * rcu_read_lock_bh_held - might we be in RCU-bh read-side critical section?
- *
- * If CONFIG_PROVE_LOCKING is selected and enabled, returns nonzero iff in
- * an RCU-bh read-side critical section. In absence of CONFIG_PROVE_LOCKING,
- * this assumes we are in an RCU-bh read-side critical section unless it can
- * prove otherwise.
- *
- * Check rcu_scheduler_active to prevent false positives during boot.
+/*
+ * rcu_read_lock_bh_held() is defined out of line to avoid #include-file
+ * hell.
*/
-static inline int rcu_read_lock_bh_held(void)
-{
- if (!debug_lockdep_rcu_enabled())
- return 1;
- return lock_is_held(&rcu_bh_lock_map);
-}
+extern int rcu_read_lock_bh_held(void);
/**
* rcu_read_lock_sched_held - might we be in RCU-sched read-side critical section?
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index f1125c1..913eccb 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -45,6 +45,7 @@
#include <linux/mutex.h>
#include <linux/module.h>
#include <linux/kernel_stat.h>
+#include <linux/hardirq.h>
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key rcu_lock_key;
@@ -66,6 +67,27 @@ EXPORT_SYMBOL_GPL(rcu_sched_lock_map);
int rcu_scheduler_active __read_mostly;
EXPORT_SYMBOL_GPL(rcu_scheduler_active);
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
+/**
+ * rcu_read_lock_bh_held - might we be in RCU-bh read-side critical section?
+ *
+ * Check for bottom half being disabled, which covers both the
+ * CONFIG_PROVE_RCU and not cases. Note that if someone uses
+ * rcu_read_lock_bh(), but then later enables BH, lockdep (if enabled)
+ * will show the situation.
+ *
+ * Check debug_lockdep_rcu_enabled() to prevent false positives during boot.
+ */
+int rcu_read_lock_bh_held(void)
+{
+ if (!debug_lockdep_rcu_enabled())
+ return 1;
+ return in_softirq();
+}
+
+#endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
+
/*
* This function is invoked towards the end of the scheduler's initialization
* process. Before this is called, the idle task might contain
^ permalink raw reply related
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-10 13:13 UTC (permalink / raw)
To: Herbert Xu
Cc: Eric Dumazet, Arnd Bergmann, David S. Miller, netdev,
Stephen Hemminger
In-Reply-To: <20100310104907.GA23752@gondor.apana.org.au>
On Wed, Mar 10, 2010 at 06:49:07PM +0800, Herbert Xu wrote:
> On Wed, Mar 10, 2010 at 11:39:43AM +0100, Eric Dumazet wrote:
> >
> > Its really rcu_dereference_bh() that could/should be used:
> > I see no problem changing
> >
> >
> > local_bh_disable();
> > ...
> > rcu_read_lock();
> > rcu_dereference(rt_hash_table[h].chain);
> > rcu_read_unlock();
> > ...
> > local_bh_enable();
>
> Why don't we just ignore the bh part for rcu_dereference?
>
> After all it's call_rcu_bh and the other primitives that we really
> care about. For rcu_dereference bh should make no difference
> whatsoever.
If CONFIG_PROVE_RCU is set, rcu_dereference() checks for rcu_read_lock()
and rcu_dereference_bh() checks for either rcu_read_lock_bh() or BH
being disabled. Yes, this is a bit restrictive, but there are a few too
many to check by hand these days.
Thanx, Paul
^ permalink raw reply
* Re: [net-next PATCH v6 0/3] net: reserve ports for applications using fixed port numbers
From: Octavian Purdila @ 2010-03-10 12:42 UTC (permalink / raw)
To: Cong Wang
Cc: Eric W. Biederman, David Miller, netdev, linux-kernel, nhorman,
eric.dumazet
In-Reply-To: <4B976503.4050702@redhat.com>
On Wednesday 10 March 2010 11:23:15 you wrote:
> Eric W. Biederman wrote:
> > I would add the restriction that the values in the list of ranges
> > always must be increasing, and in general restrict the set of accepted
> > values as much as possible. If we don't accept it now we don't have
> > to worry about some userspace application relying on some unitended
> > side effect a few years into the future.
>
> I don't think this is good.
>
> Suppose that when I just want to add one port into the list and keep the
> original ones, I want to do this:
>
> orig=$(cat ip_local_reserved_ports)
> new_list="$orig, $new_one"
> echo "$new_list" > ip_local_reserved_ports
>
> If we add this restriction, the above could be failed if the new port
> is lower than the original ones. This will be not convenient.
>
> > I think it is a serious bug that you clear the destination bitmap
> > in the middle of parsing it. That will either open or close all
> > ports in the middle of parsing, and I can't see how that would
> > ever be a good thing.
>
> Agreed.
>
> By the way, Octavian, any new updates?
>
Sorry, didn't got time to work on this lately, but I will submit a new version
I hope end of this week to address Eric's comments.
^ permalink raw reply
* Re: can SFQ and perturb break paquet ordering ?
From: Patrick McHardy @ 2010-03-10 12:30 UTC (permalink / raw)
To: Julien Vehent; +Cc: netdev
In-Reply-To: <ca4f1a12b3ba9c81f3276c091063613f@localhost>
Julien Vehent wrote:
> Hi netdev folks,
>
> I was digging into the sch_sfq code and I was wondering the following (I
> assume it's nothing new, I'm just trying to get it right):
>
> Let's consider one UDP connection that has two packets. Packet#1 goes to
> bucket#1, then perturb modifies the hashing algorithms and thus Packet#2
> goes to bucket#800.
>
> If the round robin was positionned after bucket#1 but before bucket#800
> when perturb occured, then Packet#2 will be sent before Packet#1. And since
> UDP can't reordered, it will be processed before on the received end.
>
> Am I getting this right or is there something in the code that I missed ?
That's correct.
^ permalink raw reply
* can SFQ and perturb break paquet ordering ?
From: Julien Vehent @ 2010-03-10 12:22 UTC (permalink / raw)
To: netdev
Hi netdev folks,
I was digging into the sch_sfq code and I was wondering the following (I
assume it's nothing new, I'm just trying to get it right):
Let's consider one UDP connection that has two packets. Packet#1 goes to
bucket#1, then perturb modifies the hashing algorithms and thus Packet#2
goes to bucket#800.
If the round robin was positionned after bucket#1 but before bucket#800
when perturb occured, then Packet#2 will be sent before Packet#1. And since
UDP can't reordered, it will be processed before on the received end.
Am I getting this right or is there something in the code that I missed ?
Thanks for your help.
Julien Vehent
^ permalink raw reply
* Re: [Uclinux-dist-devel] [PATCH] can: bfin_can: switch to common Blackfin can header
From: Mike Frysinger @ 2010-03-10 12:17 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Urs Thuermann, netdev, socketcan-core, Oliver Hartkopp,
uclinux-dist-devel, David S. Miller
In-Reply-To: <4B974D38.6000501@grandegger.com>
On Wed, Mar 10, 2010 at 02:41, Wolfgang Grandegger wrote:
> Mike Frysinger wrote:
>> The MMR bits are being moved to this header, so include it.
>>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>
>> ---
>> note: this can be merged for 2.6.34 or 2.6.35
>
> Is it required to make the driver working under 2.6.34?
not for 2.6.34, but it will be for 2.6.35
-mike
^ permalink raw reply
* Re: bug report: netpoll: allow execution of multiple rx_hooks per interface
From: Daniel Borkmann @ 2010-03-10 11:13 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Daniel Borkmann, netdev, David S. Miller
In-Reply-To: <20100306110659.GE4958@bicker>
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
Hi Dan,
Dan Carpenter wrote:
> Smatch complains about 508e14b4a4f: netpoll: allow execution of multiple
> rx_hooks per interface
>
> net/core/netpoll.c +840 netpoll_setup(124) error: potential null dereference 'npinfo'. (kmalloc returns null)
> 839 if (!ndev->npinfo) {
> 840 spin_lock_irqsave(&npinfo->rx_lock, flags);
>
> npinfo can be null here if the kmalloc() failed.
>
> 841 list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
> 842 npe->dev = NULL;
> 843 }
> 844 spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> 845
> 846 kfree(npinfo);
> 847 }
Sorry for the late reply, I'm currently on holiday and have hardly
access to the Internet. I'll fix the problem as soon as possible.
You'll hear from me.
Thanks,
Daniel
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-10 10:49 UTC (permalink / raw)
To: Eric Dumazet
Cc: Arnd Bergmann, paulmck, David S. Miller, netdev,
Stephen Hemminger
In-Reply-To: <1268217583.2880.2.camel@edumazet-laptop>
On Wed, Mar 10, 2010 at 11:39:43AM +0100, Eric Dumazet wrote:
>
> Its really rcu_dereference_bh() that could/should be used:
> I see no problem changing
>
>
> local_bh_disable();
> ...
> rcu_read_lock();
> rcu_dereference(rt_hash_table[h].chain);
> rcu_read_unlock();
> ...
> local_bh_enable();
Why don't we just ignore the bh part for rcu_dereference?
After all it's call_rcu_bh and the other primitives that we really
care about. For rcu_dereference bh should make no difference
whatsoever.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Eric Dumazet @ 2010-03-10 10:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: paulmck, Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <201003101041.32518.arnd@arndb.de>
Le mercredi 10 mars 2010 à 10:41 +0100, Arnd Bergmann a écrit :
> On Wednesday 10 March 2010 03:14:10 Paul E. McKenney wrote:
> > On Tue, Mar 09, 2010 at 10:12:59PM +0100, Arnd Bergmann wrote:
> >
> > > I've just tried annotating net/ipv4/route.c like this and did not get
> > > very far, because the same pointers are used for rcu and rcu_bh.
> > > Could you check if this is a false positive or an actual finding?
> >
> > Hmmm... I am only seeing a call_rcu_bh() here, so unless I am missing
> > something, this is a real problem in TREE_PREEMPT_RCU kernels. The
> > call_rcu_bh() only interacts with the rcu_read_lock_bh() readers, not
> > the rcu_read_lock() readers.
> >
> > One approach is to run freed blocks through both types of grace periods,
> > I suppose.
>
> Well, if I introduce different __rcu and __rcu_bh address space annotations,
> sparse would still not like that, because then you can only pass the annotated
> pointers into either rcu_dereference or rcu_dereference_bh.
>
> What the code seems to be doing here is in some places
>
> local_bh_disable();
> ...
> rcu_read_lock();
> rcu_dereference(rt_hash_table[h].chain);
> rcu_read_unlock();
> ...
> local_bh_enable();
>
> and in others
>
> rcu_read_lock_bh();
> rcu_dereference_bh(rt_hash_table[h].chain);
> rcu_read_unlock_bh();
>
> When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
> turn first rcu_dereference into rcu_dereference_bh in order to have a clean
> build with sparse. Would that change be
> a) correct from RCU perspective,
> b) desirable for code inspection, and
> c) lockdep-clean?
>
Its really rcu_dereference_bh() that could/should be used:
I see no problem changing
local_bh_disable();
...
rcu_read_lock();
rcu_dereference(rt_hash_table[h].chain);
rcu_read_unlock();
...
local_bh_enable();
to
local_bh_disable();
...
rcu_read_lock();
rcu_dereference_bh(rt_hash_table[h].chain);
rcu_read_unlock();
...
local_bh_enable();
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Arnd Bergmann @ 2010-03-10 9:41 UTC (permalink / raw)
To: paulmck; +Cc: Herbert Xu, David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100310021410.GD6203@linux.vnet.ibm.com>
On Wednesday 10 March 2010 03:14:10 Paul E. McKenney wrote:
> On Tue, Mar 09, 2010 at 10:12:59PM +0100, Arnd Bergmann wrote:
>
> > I've just tried annotating net/ipv4/route.c like this and did not get
> > very far, because the same pointers are used for rcu and rcu_bh.
> > Could you check if this is a false positive or an actual finding?
>
> Hmmm... I am only seeing a call_rcu_bh() here, so unless I am missing
> something, this is a real problem in TREE_PREEMPT_RCU kernels. The
> call_rcu_bh() only interacts with the rcu_read_lock_bh() readers, not
> the rcu_read_lock() readers.
>
> One approach is to run freed blocks through both types of grace periods,
> I suppose.
Well, if I introduce different __rcu and __rcu_bh address space annotations,
sparse would still not like that, because then you can only pass the annotated
pointers into either rcu_dereference or rcu_dereference_bh.
What the code seems to be doing here is in some places
local_bh_disable();
...
rcu_read_lock();
rcu_dereference(rt_hash_table[h].chain);
rcu_read_unlock();
...
local_bh_enable();
and in others
rcu_read_lock_bh();
rcu_dereference_bh(rt_hash_table[h].chain);
rcu_read_unlock_bh();
When rt_hash_table[h].chain gets the __rcu_bh annotation, we'd have to
turn first rcu_dereference into rcu_dereference_bh in order to have a clean
build with sparse. Would that change be
a) correct from RCU perspective,
b) desirable for code inspection, and
c) lockdep-clean?
Arnd
^ permalink raw reply
* Re: [net-next PATCH v6 0/3] net: reserve ports for applications using fixed port numbers
From: Cong Wang @ 2010-03-10 9:23 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Miller, opurdila, netdev, linux-kernel, nhorman,
eric.dumazet
In-Reply-To: <m1vddbdhmo.fsf@fess.ebiederm.org>
Eric W. Biederman wrote:
>
> I would add the restriction that the values in the list of ranges
> always must be increasing, and in general restrict the set of accepted
> values as much as possible. If we don't accept it now we don't have
> to worry about some userspace application relying on some unitended
> side effect a few years into the future.
I don't think this is good.
Suppose that when I just want to add one port into the list and keep the
original ones, I want to do this:
orig=$(cat ip_local_reserved_ports)
new_list="$orig, $new_one"
echo "$new_list" > ip_local_reserved_ports
If we add this restriction, the above could be failed if the new port
is lower than the original ones. This will be not convenient.
>
>
> I think it is a serious bug that you clear the destination bitmap
> in the middle of parsing it. That will either open or close all
> ports in the middle of parsing, and I can't see how that would
> ever be a good thing.
>
Agreed.
By the way, Octavian, any new updates?
^ permalink raw reply
* Re: [net-next-2.6 PATCH 0/3] bonding: refuse to change bond type if it's used V2
From: Jiri Pirko @ 2010-03-10 8:37 UTC (permalink / raw)
To: netdev; +Cc: fubar, bonding-devel, davem, shemminger
In-Reply-To: <20100309141724.GB2783@psychotron.lab.eng.brq.redhat.com>
Reposting.
Tue, Mar 09, 2010 at 03:17:24PM CET, jpirko@redhat.com wrote:
>Reworked version of patch/patchset. This time I use netdevice notifier as
>Stephen suggested. Changed the name of the original ones (NETDEV_BONDING_*TYPE)
>to be more general and reused them. Now the change is refused by brigde only.
>I will follow up with vlan soon.
>
>Jirka
^ permalink raw reply
* Re: [PATCH] can: bfin_can: switch to common Blackfin can header
From: Wolfgang Grandegger @ 2010-03-10 7:41 UTC (permalink / raw)
To: Mike Frysinger
Cc: Urs Thuermann, netdev-u79uwXL29TY76Z2rM5mHXA,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w, Oliver Hartkopp,
uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b,
David S. Miller
In-Reply-To: <1268150589-27123-1-git-send-email-vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Mike Frysinger wrote:
> The MMR bits are being moved to this header, so include it.
>
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> ---
> note: this can be merged for 2.6.34 or 2.6.35
Is it required to make the driver working under 2.6.34?
Wolfgang.
^ permalink raw reply
* [PATCH] net: remove rcu locking from fib_rules_event()
From: Eric Dumazet @ 2010-03-10 6:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Paul E. McKenney
We hold RTNL at this point and dont use RCU variants of list traversals,
we dont need rcu_read_lock()/rcu_read_unlock()
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/core/fib_rules.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index 9a24377..2ff3489 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -108,7 +108,7 @@ fib_rules_register(struct fib_rules_ops *tmpl, struct net *net)
struct fib_rules_ops *ops;
int err;
- ops = kmemdup(tmpl, sizeof (*ops), GFP_KERNEL);
+ ops = kmemdup(tmpl, sizeof(*ops), GFP_KERNEL);
if (ops == NULL)
return ERR_PTR(-ENOMEM);
@@ -123,7 +123,6 @@ fib_rules_register(struct fib_rules_ops *tmpl, struct net *net)
return ops;
}
-
EXPORT_SYMBOL_GPL(fib_rules_register);
void fib_rules_cleanup_ops(struct fib_rules_ops *ops)
@@ -157,7 +156,6 @@ void fib_rules_unregister(struct fib_rules_ops *ops)
call_rcu(&ops->rcu, fib_rules_put_rcu);
}
-
EXPORT_SYMBOL_GPL(fib_rules_unregister);
static int fib_rule_match(struct fib_rule *rule, struct fib_rules_ops *ops,
@@ -220,7 +218,6 @@ out:
return err;
}
-
EXPORT_SYMBOL_GPL(fib_rules_lookup);
static int validate_rulemsg(struct fib_rule_hdr *frh, struct nlattr **tb,
@@ -613,7 +610,7 @@ static int fib_nl_dumprule(struct sk_buff *skb, struct netlink_callback *cb)
break;
cb->args[1] = 0;
- skip:
+skip:
idx++;
}
rcu_read_unlock();
@@ -685,7 +682,6 @@ static int fib_rules_event(struct notifier_block *this, unsigned long event,
struct fib_rules_ops *ops;
ASSERT_RTNL();
- rcu_read_lock();
switch (event) {
case NETDEV_REGISTER:
@@ -699,8 +695,6 @@ static int fib_rules_event(struct notifier_block *this, unsigned long event,
break;
}
- rcu_read_unlock();
-
return NOTIFY_DONE;
}
^ permalink raw reply related
* [PATCH] net: Annotates neigh_invalidate()
From: Eric Dumazet @ 2010-03-10 5:40 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Annotates neigh_invalidate() with __releases() and __acquires() for
sparse sake.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index d102f6d..6cee643 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -771,6 +771,8 @@ static __inline__ int neigh_max_probes(struct neighbour *n)
}
static void neigh_invalidate(struct neighbour *neigh)
+ __releases(neigh->lock)
+ __acquires(neigh->lock)
{
struct sk_buff *skb;
^ permalink raw reply related
* [PATCH] net: Fix dev_mc_add()
From: Eric Dumazet @ 2010-03-10 5:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jiri Pirko
Commit 6e17d45a (net: add addr len check to dev_mc_add)
added a bug in dev_mc_add(), since it can now exit with a lock
imbalance.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Jiri Pirko <jpirko@redhat.com>
---
diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index fd91569..3dc295b 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -97,8 +97,9 @@ int dev_mc_add(struct net_device *dev, void *addr, int alen, int glbl)
netif_addr_lock_bh(dev);
if (alen != dev->addr_len)
- return -EINVAL;
- err = __dev_addr_add(&dev->mc_list, &dev->mc_count, addr, alen, glbl);
+ err = -EINVAL;
+ else
+ err = __dev_addr_add(&dev->mc_list, &dev->mc_count, addr, alen, glbl);
if (!err)
__dev_set_rx_mode(dev);
netif_addr_unlock_bh(dev);
^ permalink raw reply related
* Re: [PATCH net-2.6] ipv6 ip6_tunnel: eliminate unused recursion field from ip6_tnl{}.
From: Eric Dumazet @ 2010-03-10 4:48 UTC (permalink / raw)
To: YOSHIFUJI Hideaki; +Cc: davem, netdev
In-Reply-To: <201003100247.o2A2lqaX001898@94.43.138.210.xn.2iij.net>
Le mercredi 10 mars 2010 à 11:47 +0900, YOSHIFUJI Hideaki a écrit :
> Commit a43912ab19... ("tunnel: eliminate recursion field") eliminated
> use of recursion field from tunnel structures, but its definition
> still exists in ip6_tnl{}.
>
> Let's remove that unused field.
>
> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox