* Re: [PATCH] MAINTAINER: Correct CAN Maintainer responsibilities and paths
From: Wolfgang Grandegger @ 2010-03-06 17:48 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: SocketCAN Core Mailing List, Linux Netdev List, Urs Thuermann,
David Miller, Oliver Hartkopp
In-Reply-To: <4B929F96.1030402-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
Oliver Hartkopp wrote:
> Update the CAN Maintainer responsibilities and add source paths.
> Additional the SocketCAN core ML is not subscribers-only anymore.
>
> Signed-off-by: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
>
> ---
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 34f52a1..9ee1f78 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1372,20 +1372,28 @@ F: arch/x86/include/asm/calgary.h
> F: arch/x86/include/asm/tce.h
>
> CAN NETWORK LAYER
> -M: Urs Thuermann <urs.thuermann-l29pVbxQd1IUtdQbppsyvg@public.gmane.org>
> +M: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
> M: Oliver Hartkopp <oliver.hartkopp-l29pVbxQd1IUtdQbppsyvg@public.gmane.org>
> -L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org (subscribers-only)
> +M: Urs Thuermann <urs.thuermann-l29pVbxQd1IUtdQbppsyvg@public.gmane.org>
> +L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> W: http://developer.berlios.de/projects/socketcan/
> S: Maintained
> -F: drivers/net/can/
> -F: include/linux/can/
> +F: net/can/
> F: include/linux/can.h
> +F: include/linux/can/core.h
> +F: include/linux/can/bcm.h
> +F: include/linux/can/raw.h
>
> CAN NETWORK DRIVERS
> M: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
> -L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org (subscribers-only)
> +L: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> W: http://developer.berlios.de/projects/socketcan/
> S: Maintained
> +F: drivers/net/can/
> +F: include/linux/can/dev.h
> +F: include/linux/can/error.h
> +F: include/linux/can/netlink.h
> +F: include/linux/can/platform/
>
> CELL BROADBAND ENGINE ARCHITECTURE
> M: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>
Looks good ==>
Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
^ permalink raw reply
* [PATCH] MAINTAINER: Correct CAN Maintainer responsibilities and paths
From: Oliver Hartkopp @ 2010-03-06 18:31 UTC (permalink / raw)
To: Wolfgang Grandegger, David Miller
Cc: Oliver Hartkopp, Urs Thuermann, Linux Netdev List,
SocketCAN Core Mailing List
Update the CAN Maintainer responsibilities and add source paths.
Additional the SocketCAN core ML is not subscribers-only anymore.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
diff --git a/MAINTAINERS b/MAINTAINERS
index 34f52a1..9ee1f78 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1372,20 +1372,28 @@ F: arch/x86/include/asm/calgary.h
F: arch/x86/include/asm/tce.h
CAN NETWORK LAYER
-M: Urs Thuermann <urs.thuermann@volkswagen.de>
+M: Oliver Hartkopp <socketcan@hartkopp.net>
M: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
-L: socketcan-core@lists.berlios.de (subscribers-only)
+M: Urs Thuermann <urs.thuermann@volkswagen.de>
+L: socketcan-core@lists.berlios.de
W: http://developer.berlios.de/projects/socketcan/
S: Maintained
-F: drivers/net/can/
-F: include/linux/can/
+F: net/can/
F: include/linux/can.h
+F: include/linux/can/core.h
+F: include/linux/can/bcm.h
+F: include/linux/can/raw.h
CAN NETWORK DRIVERS
M: Wolfgang Grandegger <wg@grandegger.com>
-L: socketcan-core@lists.berlios.de (subscribers-only)
+L: socketcan-core@lists.berlios.de
W: http://developer.berlios.de/projects/socketcan/
S: Maintained
+F: drivers/net/can/
+F: include/linux/can/dev.h
+F: include/linux/can/error.h
+F: include/linux/can/netlink.h
+F: include/linux/can/platform/
CELL BROADBAND ENGINE ARCHITECTURE
M: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply related
* Re: [PATCH v1 3/3] Let host NIC driver to DMA to guest user space.
From: Stephen Hemminger @ 2010-03-06 17:18 UTC (permalink / raw)
To: xiaohui.xin; +Cc: netdev, kvm, linux-kernel, mingo, mst, jdike, Zhao Yu
In-Reply-To: <1267868318-19268-4-git-send-email-xiaohui.xin@intel.com>
On Sat, 6 Mar 2010 17:38:38 +0800
xiaohui.xin@intel.com wrote:
> From: Xin Xiaohui <xiaohui.xin@intel.com>
>
> The patch let host NIC driver to receive user space skb,
> then the driver has chance to directly DMA to guest user
> space buffers thru single ethX interface.
>
> Signed-off-by: Xin Xiaohui <xiaohui.xin@intel.com>
> Signed-off-by: Zhao Yu <yzhao81@gmail.com>
> Sigend-off-by: Jeff Dike <jdike@c2.user-mode-linux.org>
> ---
> include/linux/netdevice.h | 76 ++++++++++++++++++++++++++++++++++++++++++-
> include/linux/skbuff.h | 30 +++++++++++++++--
> net/core/dev.c | 32 ++++++++++++++++++
> net/core/skbuff.c | 79 +++++++++++++++++++++++++++++++++++++++++----
> 4 files changed, 205 insertions(+), 12 deletions(-)
>
There are too many ifdef's in this implementation.
I would prefer to see a few functions (with stub for the non-ifdef case),
like the network namespace code.
^ permalink raw reply
* [PATCH] fix PHY polling system blocking
From: Stefani Seibold @ 2010-03-06 16:50 UTC (permalink / raw)
To: linux-kernel, netdev
This patch fix the PHY poller, which can block the whole system. On a
Freescale PPC 834x this result in a delay of 450 us due the slow
communication with the PHY chip.
For PHY chips without interrupts, the status of the ethernet will be
polled every 2 sec. The poll function will read some register of the MII
PHY. The time between the sending the MII_READ_COMMAND and receiving the
result is more the 100 us on a PPC 834x.
The patch modifies the poller a lit bit. Only a link status state change
will result in a successive detection of the connection type. The poll
cycle on the other hand will be increased to every seconds.
Together this patch will prevent a blocking of nearly 400 us every two
seconds of the whole system on a PPC 834x.
The patch is against kernel 2.6.33. Please merge it.
Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
phy.c | 5 ++---
phy_device.c | 12 +++++++++---
2 files changed, 11 insertions(+), 6 deletions(-)
diff -u -N -r -p linux-2.6.33.orig/drivers/net/phy/phy.c linux-2.6.33/drivers/net//phy/phy.c
--- linux-2.6.33.orig/drivers/net/phy/phy.c 2010-02-24 19:52:17.000000000 +0100
+++ linux-2.6.33/drivers/net//phy/phy.c 2010-02-28 22:53:14.725464101 +0100
@@ -871,9 +871,8 @@ void phy_state_machine(struct work_struc
case PHY_RUNNING:
/* Only register a CHANGE if we are
* polling */
- if (PHY_POLL == phydev->irq)
- phydev->state = PHY_CHANGELINK;
- break;
+ if (PHY_POLL != phydev->irq)
+ break;
case PHY_CHANGELINK:
err = phy_read_status(phydev);
diff -u -N -r -p linux-2.6.33.orig/drivers/net/phy/phy_device.c linux-2.6.33/drivers/net//phy/phy_device.c
--- linux-2.6.33.orig/drivers/net/phy/phy_device.c 2010-02-24 19:52:17.000000000 +0100
+++ linux-2.6.33/drivers/net//phy/phy_device.c 2010-02-28 22:53:14.726464145 +0100
@@ -161,7 +161,7 @@ struct phy_device* phy_device_create(str
dev->speed = 0;
dev->duplex = -1;
dev->pause = dev->asym_pause = 0;
- dev->link = 1;
+ dev->link = 0;
dev->interface = PHY_INTERFACE_MODE_GMII;
dev->autoneg = AUTONEG_ENABLE;
@@ -694,10 +694,16 @@ int genphy_update_link(struct phy_device
if (status < 0)
return status;
- if ((status & BMSR_LSTATUS) == 0)
+ if ((status & BMSR_LSTATUS) == 0) {
+ if (phydev->link==0)
+ return 1;
phydev->link = 0;
- else
+ }
+ else {
+ if (phydev->link==1)
+ return 1;
phydev->link = 1;
+ }
return 0;
}
^ permalink raw reply
* Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)
From: Dan Smith @ 2010-03-06 17:09 UTC (permalink / raw)
To: Oren Laadan
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
containers-qjLDD68F18O7TbgM5vRIOg, den-GEFAQzZX7r8dnm+yROfE0A,
David Miller, benjamin.thery-6ktuUTfB/bM
In-Reply-To: <4B91D234.2020003-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
OL> Ditto for the existing ipv4 (socket) c/r code ... ?
Yes. I've got a couple of header format changes lined up for IPv6
stuff, so I can add a patch to do this in that set, if it's okay.
--
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org
^ permalink raw reply
* Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)
From: Dan Smith @ 2010-03-06 17:08 UTC (permalink / raw)
To: Oren Laadan
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
containers-qjLDD68F18O7TbgM5vRIOg, den-GEFAQzZX7r8dnm+yROfE0A,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, benjamin.thery-6ktuUTfB/bM
In-Reply-To: <4B91D1A3.9030404-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
OL> What about leak detection ?
OL> Aren't we missing {netns,netdev}_users()?
This is something I need to give more thought to, but it's not as easy
as it sounds. Network devices aren't released at the last put() like
a lot of other things, and my initial attempts to reconcile the
refcount after a checkpoint operation have not been successful.
However, I'm not sure that it's as important here, because AFAIK, a
network device can only exist in one network namespace at a time. If
we're checkpointing a netdev, it's because we are checkpointing the
namespace that it lives in. Making sure the netns isn't leaked out of
the process tree would be much easier and just as effective, no?
>> +config CHECKPOINT_NETNS
>> + bool
>> + default y if NET && NET_NS && CHECKPOINT
>> +
OL> Did you mean this to be visible (settable) by the user ?
No, it was specifically supposed to enable itself when those other
items are enabled, but not be a user adjustable toggle. I had a
discussion with Serge about it and we came to this as a solution,
although I don't remember what the problem we started with was. I'll
dig through my IRC logs to see if I can figure it out.
>> + retry:
>> + if (++pages > 4) {
>> + addrs = -E2BIG;
>> + goto out;
>> + }
OL> Why 4 ?
It's just a sanity limit.
OL> Do we really need this special case ? I'd be happy with a ckpt_err()
OL> for any error - and the actual error number would be useful to tell
OL> which case it was.
Unless I'm missing something, you asked for this specifically:
https://lists.linux-foundation.org/pipermail/containers/2010-February/022844.html
OL> Isn't this check redundant ? I expect it to fail promptly in
OL> checkpoint_netdev() above.
No, as I've said a couple of times previously, this isn't the only way
we can arrive at a netdev for checkpointing. This case is the one
where we're marching through the netns and find a netdev that is not
supported. The other is where we arrive at a device as a peer of
another device, so the other check may come into play at times where
this one doesn't and vice versa.
OL> This may be a bit simpler if you move the first deferqueue_add()
OL> forward to just before the other one. Or better: change dq_netdev
OL> to have two pointers, dev and peer (if any is null, the cleanup
OL> function will skip).
The reason it is this messy is because of the way network devices are
deallocated. Since they don't destroy themselves on the final put(),
we have to explicitly call unregister_netdev() on them when we know
they're no longer used (else we block). Once we've added them to the
deferqueue, we can no longer destroy them here because a reference is
held and the deferqueue will run afterwards.
The ordering of this is a result of me injecting failures at each step
and working it out until I got it to not block on unregistering either
of the devices in all of the error paths. That's not to say it's the
best way, but there is a reason it's ordered the way it is.
--
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org
^ permalink raw reply
* Re: [patch] bluetooth: debugfs changes use too much stack
From: Dan Carpenter @ 2010-03-06 15:30 UTC (permalink / raw)
To: Jan Ceuleers
Cc: Marcel Holtmann, David S. Miller, Dave Young, Greg Kroah-Hartman,
Roger Quadros, Bing Zhao, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4B92695F.60601-bdq14YP6qtRg9hUCZPvPmw@public.gmane.org>
On Sat, Mar 06, 2010 at 03:40:31PM +0100, Jan Ceuleers wrote:
> Dan Carpenter wrote:
>
> Error handling?
>
> > The original code would break with a 4K stack.
> >
> > Signed-off-by: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > This was compile tested only. Sorry about that.
> >
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 1a79a6c..835758f 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -417,9 +417,11 @@ static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
> > struct hci_dev *hdev = file->private_data;
> > struct inquiry_cache *cache = &hdev->inq_cache;
> > struct inquiry_entry *e;
> > - char buf[4096];
> > + char *buf;
> > int n = 0;
> > + ssize_t ret;
> >
> > + buf = kmalloc(4096, GFP_KERNEL);
>
> Could this kmalloc not fail?
Grr... I'm really sorry about that.
I will send an updated patch tomorrow.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-06 15:19 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306065655.GA14326@gondor.apana.org.au>
On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
> On Fri, Mar 05, 2010 at 09:06:56PM -0800, Paul E. McKenney wrote:
> >
> > Agreed, but the callbacks registered by the call_rcu_bh() might run
> > at any time, possibly quite some time after the synchronize_rcu_bh()
> > completes. For example, the last call_rcu_bh() might register on
> > one CPU, and the synchronize_rcu_bh() on another CPU. Then there
> > is no guarantee that the call_rcu_bh()'s callback will execute before
> > the synchronize_rcu_bh() returns.
> >
> > In contrast, rcu_barrier_bh() is guaranteed not to return until all
> > pending RCU-bh callbacks have executed.
>
> You're absolutely right. I'll send a patch to fix this.
>
> Incidentally, does rcu_barrier imply rcu_barrier_bh? What about
> synchronize_rcu and synchronize_rcu_bh? The reason I'm asking is
> that we use a mixture of rcu_read_lock_bh and rcu_read_lock all
> over the place but only ever use rcu_barrier and synchronize_rcu.
Hmmm... rcu_barrier() definitely does -not- imply rcu_barrier_bh(),
because there are separate sets of callbacks whose execution can
be throttled separately. So, while you would expect RCU-bh grace
periods to complete more quickly, if there was a large number of
RCU-bh callbacks on a given CPU but very few RCU callbacks, it might
well take longer for the RCU-bh callbacks to be invoked.
With TREE_PREEMPT_RCU, if there were no RCU readers but one long-running
RCU-bh reader, then synchronize_rcu_bh() could return before
synchronize_rcu() does.
The simple approach would be to do something like:
synchronize_rcu();
synchronize_rcu_bh();
on the one hand, and:
rcu_barrier();
rcu_barrier_bh();
on the other. However, this is not so good for update-side latency.
Perhaps we need a primitive that waits for both RCU and RCU-bh in
parallel? This is pretty easy for synchronize_rcu() and
synchronize_rcu_bh(), and probably not too hard for rcu_barrier()
and rcu_barrier_bh().
Hmmm... Do we have the same issue with call_rcu() and call_rcu_bh()?
Thanx, Paul
> > > I understand. However, AFAICS whatever it is that we are destroying
> > > is taken off the reader's visible data structure before call_rcu_bh.
> > > Do you have a particular case in mind where this is not the case?
> >
> > I might simply have missed the operation that removed reader
> > visibility, looking again...
> >
> > Ah, I see it. The "br->mdb = NULL" in br_multicast_stop() makes
> > it impossible for the readers to get to any of the data. Right?
>
> Yes. The read-side will see it and get nothing, while all write-side
> paths will see that netif_running is false and exit.
>
> > > > The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
> > > > rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
> > > > scheme correctly.
> > >
> > > Any function that modifies the data structure is done under the
> > > multicast_lock, including br_multicast_del_pg.
> >
> > But spin_lock() does not take the place of rcu_read_lock_bh().
> > And so, in theory, the RCU-bh grace period could complete between
> > the time that br_multicast_del_pg() does its call_rcu_bh() and the
> > "*pp = p->next;" at the top of the next loop iteration. If so,
> > then br_multicast_free_pg()'s kfree() will possibly have clobbered
> > "p->next". Low probability, yes, but a long-running interrupt
> > could do the trick.
> >
> > Or is there something I am missing that is preventing an RCU-bh
> > grace period from completing near the bottom of br_multicast_del_pg()'s
> > "for" loop?
>
> Well all the locks are taken with BH disabled, this should prevent
> this problem, no?
>
> > > The read-side is the data path (non-IGMP multicast packets). The
> > > sole entry point is br_mdb_get().
> >
> > Hmmm... So the caller is responsible for rcu_read_lock_bh()?
>
> Yes, all data paths through the bridge operate with BH disabled.
>
> > Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
> > in __br_mdb_ip_get(), then? Or is something else going on here?
>
> Indeed it should, I'll fix this up too.
>
> Thanks for reviewing Paul!
> --
> 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-06 15:00 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306065655.GA14326@gondor.apana.org.au>
On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
> On Fri, Mar 05, 2010 at 09:06:56PM -0800, Paul E. McKenney wrote:
> >
> > Agreed, but the callbacks registered by the call_rcu_bh() might run
> > at any time, possibly quite some time after the synchronize_rcu_bh()
> > completes. For example, the last call_rcu_bh() might register on
> > one CPU, and the synchronize_rcu_bh() on another CPU. Then there
> > is no guarantee that the call_rcu_bh()'s callback will execute before
> > the synchronize_rcu_bh() returns.
> >
> > In contrast, rcu_barrier_bh() is guaranteed not to return until all
> > pending RCU-bh callbacks have executed.
>
> You're absolutely right. I'll send a patch to fix this.
>
> Incidentally, does rcu_barrier imply rcu_barrier_bh? What about
> synchronize_rcu and synchronize_rcu_bh? The reason I'm asking is
> that we use a mixture of rcu_read_lock_bh and rcu_read_lock all
> over the place but only ever use rcu_barrier and synchronize_rcu.
>
> > > I understand. However, AFAICS whatever it is that we are destroying
> > > is taken off the reader's visible data structure before call_rcu_bh.
> > > Do you have a particular case in mind where this is not the case?
> >
> > I might simply have missed the operation that removed reader
> > visibility, looking again...
> >
> > Ah, I see it. The "br->mdb = NULL" in br_multicast_stop() makes
> > it impossible for the readers to get to any of the data. Right?
>
> Yes. The read-side will see it and get nothing, while all write-side
> paths will see that netif_running is false and exit.
>
> > > > The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
> > > > rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
> > > > scheme correctly.
> > >
> > > Any function that modifies the data structure is done under the
> > > multicast_lock, including br_multicast_del_pg.
> >
> > But spin_lock() does not take the place of rcu_read_lock_bh().
> > And so, in theory, the RCU-bh grace period could complete between
> > the time that br_multicast_del_pg() does its call_rcu_bh() and the
> > "*pp = p->next;" at the top of the next loop iteration. If so,
> > then br_multicast_free_pg()'s kfree() will possibly have clobbered
> > "p->next". Low probability, yes, but a long-running interrupt
> > could do the trick.
> >
> > Or is there something I am missing that is preventing an RCU-bh
> > grace period from completing near the bottom of br_multicast_del_pg()'s
> > "for" loop?
>
> Well all the locks are taken with BH disabled, this should prevent
> this problem, no?
Those locks are indeed taken with BH disabled, you are right!
And I need to fix my RCU lockdep rcu_dereference_bh() checks to
look for disabled BH as well as rcu_read_lock_bh(), for that matter.
Thanx, Paul
> > > The read-side is the data path (non-IGMP multicast packets). The
> > > sole entry point is br_mdb_get().
> >
> > Hmmm... So the caller is responsible for rcu_read_lock_bh()?
>
> Yes, all data paths through the bridge operate with BH disabled.
>
> > Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
> > in __br_mdb_ip_get(), then? Or is something else going on here?
>
> Indeed it should, I'll fix this up too.
>
> Thanks for reviewing Paul!
> --
> 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: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Daniel Lezcano @ 2010-03-06 14:47 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Pavel Emelyanov, Sukadev Bhattiprolu, Serge Hallyn,
Linux Netdev List, containers, Netfilter Development Mailinglist,
Ben Greear
In-Reply-To: <m1vdda1pmx.fsf@fess.ebiederm.org>
Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@parallels.com> writes:
>
>
>>> 2 parallel enters? I meant you have pid 0 in the entered pid namespace.
>>> You have pid 0 because your pid simply does not map.
>>>
>> Oh, I see.
>>
>>
>>> There is nothing that makes to parallel enters impossible in that.
>>> Even today we have one thread per cpu that has task->pid == &init_struct_pid
>>> which is pid 0.
>>>
>> How about the forked processes then? Who will be their parent?
>>
>
> The normal rules of parentage apply. So the child will see simply
> see it's parent as ppid == 0. If that child daemonizes it will become
> a child of the pid namespaces init.
>
> This is a lot like something that gets started from call_usermodehelper. It's
> parent process is not a descendant of init either.
>
>
> The implementation of the join is to simply change current->nsproxy->pid_ns.
> Then to use it you simply fork to get a child in the target pid namespace.
>
If the normal rules of parentage apply, that means pid 0 has to wait
it's child.
If we are in the scenario of pid 0, it's child pid 1234 and we kill the
pid 1 of the pid namespace, I suppose pid 1234 will be killed too.
The pid 0 will stay in the pid namespace and will able to fork again a
new pid 1.
I think Serge already reported that...
That sounds good :)
>>> For the case of unshare where we are designed to be used with PAM I don't
>>> think my proposed semantics work. For a join needed an extra fork before
>>> you are really in the pid namespace should be minor.
>>>
>> Hm... One more proposal - can we adopt the planned new fork_with_pids system
>> call to fork the process right into a new pid namespace?
>>
>
> In a lot of ways I like this idea of sys_hijack/sys_cloneat, and I
> don't think anything I am doing fundamentally undermines it. The use
> case of doing things in fork is that there is automatic inheritance of
> everything. All of the namespaces and all of the control groups, and
> possibly also the parent process.
And also the rootfs for executing the command inside the container (eg.
shutdown), the uid/gid (if there is a user namespace), the mount points, ...
But I suppose we can do the same with setns for all the namespaces and
chrooting within the container rootfs.
What I see is a problem with the tty. For example, we cloneat the init
process of the container which is usually /sbin/init but this one has
its tty mapped to /dev/console, so the output of the exec'ed command
will go to the console.
> It does have the high cost that the
> process we are copying from must be stopped because there are no locks
> that let us take everything. I haven't looked at the recent proposals
> to see if anyone has solved that problem cleanly.
>
Right.
> If we can do a sys_hijack/sys_cloneat style of join, that means we can
> afford a fork. At which point the my proposed pid namespace semantics
> should be fine.
>
> aka:
> setns(NSTYPE_PID);
> pid = fork();
> if (pid == 0) {
> getpid() == 2; /* Or whatever the first free pid is joined pid namespace */
> getppid() == 0;
> } else {
> pid == 6400; /* Or whatever the first free pid is in the original pid namespace */
> waitpid(pid);
> }
>
>
>>> That doesn't handle the case of cached struct pids. A good example is
>>> waitpid, where it waits for a specific struct pid. Which means that
>>> allocating a new struct pid and changing task->pid will cause
>>> waitpid(pid) to wait forever...
>>>
>> OK. Good example. Thanks.
>>
>>
>>> To change struct pid would require the refcount on struct pid to show
>>> no references from anywhere except the task_struct.
>>>
>> I think this is OK to return -EBUSY for this. And fix the waitpid
>> respectively not to block this common case. All the others I think
>> can be stayed as is.
>>
>
> That would probably work. setsid() and setpgrp() have similar sorts
> of restrictions. That is both more challenging and more limiting than
> the semantics that come out of my unshare(CLONE_NEWPID) patch. So I
> would prefer to keep this sort of thing as a last resort.
>
>
>>> At the cost of a little memory we can solve that problem for unshare
>>> if we have a an extra upid in struct pid, how we verify there is space
>>> in struct pid I'm not certain.
>>>
>>> I do think that at least until someone calls exec the namespace pids are
>>> reported to the process itself should not change. That is kill and
>>>
>> Wait a second - in that case the wait will be blocked too! No?
>>
>
> If all we do is populate an unused struct upid in struct pid there
> isn't a chance of a problem.
>
>
>>> waitpid etc. Which suggests an implementation the opposite of what
>>> I proposed. With ns_of_pid(task_pid(current)) being used as the
>>> pid namespace of children, and current->nsproxy->pid_ns not changing
>>> in the case of unshare.
>>>
>>> Shrug.
>>>
>>> Or perhaps this is a case where we use we can implement join with
>>> an extra process but we can't implement unshare, because the effect
>>> cannot be immediate.
>>>
>> Well, I'm talking only about the join now.
>>
>
> Overall it sounds like the semantics I have proposed with
> unshare(CLONE_NEWPID) are workable, and simple to implement. The
> extra fork is a bit surprising but it certainly does not
> look like a show stopper for implementing a pid namespace join.
>
I agree, it's some kind of "ghost" process.
IMO, with a bit of userspace code it would be possible to enter or exec
a command inside a container with nsfd, setns.
+1 to test your patchset Eric :)
Just a mindless suggestion, the "nsopen" / "nsattach" syscall names
should be more clear no ?
Jumping back, one question about the nsfd and the poll for waiting the
end of the namespace.
If we have an openened file descriptor on a specific namespace, we grab
a reference on this one, so the namespace won't be destroyed until we
close the fd which is used to poll the end of the namespace, no ? Did I
miss something ?
Thanks
-- Daniel
^ permalink raw reply
* Re: [patch] bluetooth: debugfs changes use too much stack
From: Jan Ceuleers @ 2010-03-06 14:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: Marcel Holtmann, David S. Miller, Dave Young, Greg Kroah-Hartman,
Roger Quadros, Bing Zhao, linux-bluetooth, netdev,
kernel-janitors
In-Reply-To: <20100306111552.GK4958@bicker>
Dan Carpenter wrote:
Error handling?
> The original code would break with a 4K stack.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> This was compile tested only. Sorry about that.
>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 1a79a6c..835758f 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -417,9 +417,11 @@ static ssize_t inquiry_cache_read(struct file *file, char __user *userbuf,
> struct hci_dev *hdev = file->private_data;
> struct inquiry_cache *cache = &hdev->inq_cache;
> struct inquiry_entry *e;
> - char buf[4096];
> + char *buf;
> int n = 0;
> + ssize_t ret;
>
> + buf = kmalloc(4096, GFP_KERNEL);
Could this kmalloc not fail?
^ permalink raw reply
* bug report: 9p: 9P2010.L handshake: Add mount option
From: Dan Carpenter @ 2010-03-06 12:22 UTC (permalink / raw)
To: sripathik; +Cc: netdev
+static unsigned char get_protocol_version(const substring_t *name)
+{
+ unsigned char version = -EINVAL;
Assigning a negative value to an unsigned char is problematic. (If it's
an unsigned int or long that's different).
I have included a test program to illustrate.
regards,
dan carpenter
#include <stdio.h>
unsigned char n22(void) { return -22; }
int main()
{
int ret = n22();
if (ret == -22)
printf("true\n");
else
printf("false\n");
}
^ permalink raw reply
* [patch] bridge: cleanup: remove unneed check
From: Dan Carpenter @ 2010-03-06 11:14 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, kernel-janitors, bridge, David S. Miller
We dereference "port" on the lines immediately before and immediately
after the test so port should hopefully never be null here.
Signed-off-by: Dan Carpenter <error27@gmail.com>
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2559fb5..8b25887 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -627,8 +627,8 @@ static void br_multicast_port_query_expired(unsigned long data)
struct net_bridge *br = port->br;
spin_lock(&br->multicast_lock);
- if (port && (port->state == BR_STATE_DISABLED ||
- port->state == BR_STATE_BLOCKING))
+ if (port->state == BR_STATE_DISABLED ||
+ port->state == BR_STATE_BLOCKING)
goto out;
if (port->multicast_startup_queries_sent <
^ permalink raw reply related
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-06 7:07 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306065655.GA14326@gondor.apana.org.au>
On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
> On Fri, Mar 05, 2010 at 09:06:56PM -0800, Paul E. McKenney wrote:
>
> > Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
> > in __br_mdb_ip_get(), then? Or is something else going on here?
>
> Indeed it should, I'll fix this up too.
bridge: Use RCU list primitive in __br_mdb_ip_get
As Paul McKenney correctly pointed out, __br_mdb_ip_get needs
to use the RCU list walking primitive in order to work correctly
on platforms where data-dependency ordering is not guaranteed.
Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2559fb5..ba30f41 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -38,7 +38,7 @@ static struct net_bridge_mdb_entry *__br_mdb_ip_get(
struct net_bridge_mdb_entry *mp;
struct hlist_node *p;
- hlist_for_each_entry(mp, p, &mdb->mhash[hash], hlist[mdb->ver]) {
+ hlist_for_each_entry_rcu(mp, p, &mdb->mhash[hash], hlist[mdb->ver]) {
if (dst == mp->addr)
return mp;
}
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 related
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-06 7:03 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306065655.GA14326@gondor.apana.org.au>
On Sat, Mar 06, 2010 at 02:56:55PM +0800, Herbert Xu wrote:
>
> > In contrast, rcu_barrier_bh() is guaranteed not to return until all
> > pending RCU-bh callbacks have executed.
>
> You're absolutely right. I'll send a patch to fix this.
bridge: Fix RCU race in br_multicast_stop
Thanks to Paul McKenny for pointing out that it is incorrect to use
synchronize_rcu_bh to ensure that pending callbacks have completed.
Instead we should use rcu_barrier_bh.
Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2559fb5..48f3b00 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1135,7 +1135,7 @@ void br_multicast_stop(struct net_bridge *br)
if (mdb->old) {
spin_unlock_bh(&br->multicast_lock);
- synchronize_rcu_bh();
+ rcu_barrier_bh();
spin_lock_bh(&br->multicast_lock);
WARN_ON(mdb->old);
}
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 related
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-06 6:56 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306050656.GA6812@linux.vnet.ibm.com>
On Fri, Mar 05, 2010 at 09:06:56PM -0800, Paul E. McKenney wrote:
>
> Agreed, but the callbacks registered by the call_rcu_bh() might run
> at any time, possibly quite some time after the synchronize_rcu_bh()
> completes. For example, the last call_rcu_bh() might register on
> one CPU, and the synchronize_rcu_bh() on another CPU. Then there
> is no guarantee that the call_rcu_bh()'s callback will execute before
> the synchronize_rcu_bh() returns.
>
> In contrast, rcu_barrier_bh() is guaranteed not to return until all
> pending RCU-bh callbacks have executed.
You're absolutely right. I'll send a patch to fix this.
Incidentally, does rcu_barrier imply rcu_barrier_bh? What about
synchronize_rcu and synchronize_rcu_bh? The reason I'm asking is
that we use a mixture of rcu_read_lock_bh and rcu_read_lock all
over the place but only ever use rcu_barrier and synchronize_rcu.
> > I understand. However, AFAICS whatever it is that we are destroying
> > is taken off the reader's visible data structure before call_rcu_bh.
> > Do you have a particular case in mind where this is not the case?
>
> I might simply have missed the operation that removed reader
> visibility, looking again...
>
> Ah, I see it. The "br->mdb = NULL" in br_multicast_stop() makes
> it impossible for the readers to get to any of the data. Right?
Yes. The read-side will see it and get nothing, while all write-side
paths will see that netif_running is false and exit.
> > > The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
> > > rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
> > > scheme correctly.
> >
> > Any function that modifies the data structure is done under the
> > multicast_lock, including br_multicast_del_pg.
>
> But spin_lock() does not take the place of rcu_read_lock_bh().
> And so, in theory, the RCU-bh grace period could complete between
> the time that br_multicast_del_pg() does its call_rcu_bh() and the
> "*pp = p->next;" at the top of the next loop iteration. If so,
> then br_multicast_free_pg()'s kfree() will possibly have clobbered
> "p->next". Low probability, yes, but a long-running interrupt
> could do the trick.
>
> Or is there something I am missing that is preventing an RCU-bh
> grace period from completing near the bottom of br_multicast_del_pg()'s
> "for" loop?
Well all the locks are taken with BH disabled, this should prevent
this problem, no?
> > The read-side is the data path (non-IGMP multicast packets). The
> > sole entry point is br_mdb_get().
>
> Hmmm... So the caller is responsible for rcu_read_lock_bh()?
Yes, all data paths through the bridge operate with BH disabled.
> Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
> in __br_mdb_ip_get(), then? Or is something else going on here?
Indeed it should, I'll fix this up too.
Thanks for reviewing Paul!
--
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-06 5:06 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100306011718.GA12812@gondor.apana.org.au>
On Sat, Mar 06, 2010 at 09:17:18AM +0800, Herbert Xu wrote:
> On Fri, Mar 05, 2010 at 03:43:27PM -0800, Paul E. McKenney wrote:
> >
> > Cool!!! You use a pair of list_head structures, so that a given
> > element can be in both the old and the new hash table simultaneously.
> > Of course, an RCU grace period must elapse between consecutive resizings.
> > Which appears to be addressed.
>
> Thanks :) I will try to extend this to other existing hash tables
> where the number of updates can be limited like it is here.
>
> > The teardown needs an rcu_barrier_bh() rather than the current
> > synchronize_rcu_bh(), please see below.
>
> All the call_rcu_bh's are done under multicast_lock. The first
> check taken after taking the multicast_lock is whether we've
> started the tear-down. So where it currently calls synchronize()
> it should already be the case that no call_rcu_bh's are still
> running.
Agreed, but the callbacks registered by the call_rcu_bh() might run
at any time, possibly quite some time after the synchronize_rcu_bh()
completes. For example, the last call_rcu_bh() might register on
one CPU, and the synchronize_rcu_bh() on another CPU. Then there
is no guarantee that the call_rcu_bh()'s callback will execute before
the synchronize_rcu_bh() returns.
In contrast, rcu_barrier_bh() is guaranteed not to return until all
pending RCU-bh callbacks have executed.
> > Also, I don't see how the teardown code is preventing new readers from
> > finding the data structures before they are being passed to call_rcu_bh().
> > You can't safely start the RCU grace period until -after- all new readers
> > have been excluded. (But I could easily be missing something here.)
>
> I understand. However, AFAICS whatever it is that we are destroying
> is taken off the reader's visible data structure before call_rcu_bh.
> Do you have a particular case in mind where this is not the case?
I might simply have missed the operation that removed reader
visibility, looking again...
Ah, I see it. The "br->mdb = NULL" in br_multicast_stop() makes
it impossible for the readers to get to any of the data. Right?
If so, my confusion, you are right, this one is OK.
> > The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
> > rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
> > scheme correctly.
>
> Any function that modifies the data structure is done under the
> multicast_lock, including br_multicast_del_pg.
But spin_lock() does not take the place of rcu_read_lock_bh().
And so, in theory, the RCU-bh grace period could complete between
the time that br_multicast_del_pg() does its call_rcu_bh() and the
"*pp = p->next;" at the top of the next loop iteration. If so,
then br_multicast_free_pg()'s kfree() will possibly have clobbered
"p->next". Low probability, yes, but a long-running interrupt
could do the trick.
Or is there something I am missing that is preventing an RCU-bh
grace period from completing near the bottom of br_multicast_del_pg()'s
"for" loop?
> > Hmmm... Where is the read-side code? Wherever it is, it cannot safely
> > dereference the ->old pointer.
>
> Right, the old pointer is merely there to limit rehashings to one
> per window. So it isn't used by the read-side.
Good!
> The read-side is the data path (non-IGMP multicast packets). The
> sole entry point is br_mdb_get().
Hmmm... So the caller is responsible for rcu_read_lock_bh()?
Shouldn't the br_mdb_get() code path be using hlist_for_each_entry_rcu()
in __br_mdb_ip_get(), then? Or is something else going on here?
Thanx, Paul
> 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 2/6] C/R: Basic support for network namespaces and devices (v5)
From: Oren Laadan @ 2010-03-06 3:55 UTC (permalink / raw)
To: Dan Smith; +Cc: David Miller, containers, den, netdev, ebiederm, benjamin.thery
In-Reply-To: <87iq9kkptz.fsf@caffeine.danplanet.com>
Ditto for the existing ipv4 (socket) c/r code ... ?
Dan Smith wrote:
> DM> To be safe you should probably use __be32 and store the IP
> DM> addresses in network byte order.
>
> Okay, yeah, good call.
>
> DM> Acked-by: David S. Miller <davem@davemloft.net>
>
> Thanks Dave!
>
^ permalink raw reply
* Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)
From: Oren Laadan @ 2010-03-06 3:53 UTC (permalink / raw)
To: Dan Smith; +Cc: containers, den, netdev, davem, ebiederm, benjamin.thery
In-Reply-To: <1267130595-23637-3-git-send-email-danms@us.ibm.com>
Hi Dan,
I finally got to look at it - see comments inline:
Dan Smith wrote:
> When checkpointing a task tree with network namespaces, we hook into
> do_checkpoint_ns() along with the others. Any devices in a given namespace
> are checkpointed (including their peer, in the case of veth) sequentially.
> Each network device stores a list of protocol addresses, as well as other
> information, such as hardware address.
>
> This patch supports veth pairs, as well as the loopback adapter. The
> loopback support is there to make sure that any additional addresses and
> state (such as up/down) is copied to the loopback adapter that we are
> given in the new network namespace.
>
> On restart, we instantiate new network namespaces and veth pairs as
> necessary. Any device we encounter that isn't in a network namespace
> that was checkpointed as part of a task is left in the namespace of the
> restarting process. This will be the case for a veth half that exists
> in the init netns to provide network access to a container.
I think that this documentation (and other pieces from q&a on your
patches, e.g. your reply to Serge on 2/22) deserve an honorable
mention either in the source files, or in Documentation/checkpoint/.
As it turns out, the network related c/r logic - sockets, netns and
netdev - all of these are nontrivial and we need to explain them for
reviewers, future coders and ... ourselves :)
[...]
>
> Signed-off-by: Dan Smith <danms@us.ibm.com>
> Cc: netdev@vger.kernel.org
> ---
> checkpoint/checkpoint.c | 6 +-
> checkpoint/objhash.c | 48 +++
> include/linux/checkpoint.h | 23 ++
> include/linux/checkpoint_hdr.h | 58 +++
> include/linux/checkpoint_types.h | 1 +
> kernel/nsproxy.c | 20 +-
> net/Kconfig | 4 +
> net/Makefile | 1 +
> net/checkpoint_dev.c | 815 ++++++++++++++++++++++++++++++++++++++
> 9 files changed, 970 insertions(+), 6 deletions(-)
> create mode 100644 net/checkpoint_dev.c
>
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index b3c1c4f..466f594 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -184,6 +184,7 @@ static int checkpoint_container(struct ckpt_ctx *ctx)
> h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_CONTAINER);
> if (!h)
> return -ENOMEM;
> +
[nit] noise ?
> ret = ckpt_write_obj(ctx, &h->h);
> ckpt_hdr_put(ctx, h);
>
> @@ -284,11 +285,6 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
> _ckpt_err(ctx, -EPERM, "%(T)Nested mnt_ns unsupported\n");
> ret = -EPERM;
> }
> - /* no support for >1 private netns */
> - if (nsproxy->net_ns != ctx->root_nsproxy->net_ns) {
> - _ckpt_err(ctx, -EPERM, "%(T)Nested net_ns unsupported\n");
> - ret = -EPERM;
> - }
> /* no support for >1 private pidns */
> if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
> _ckpt_err(ctx, -EPERM, "%(T)Nested pid_ns unsupported\n");
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index fbc58ea..16f2c43 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -348,6 +348,36 @@ static void lsm_string_drop(void *ptr, int lastref)
> kref_put(&s->kref, lsm_string_free);
> }
>
> +static int netns_grab(void *ptr)
> +{
> + struct net *net = ptr;
> +
> + get_net(net);
> + return 0;
> +}
> +
> +static void netns_drop(void *ptr, int lastref)
> +{
> + struct net *net = ptr;
> +
> + put_net(net);
> +}
> +
> +static int netdev_grab(void *ptr)
> +{
> + struct net_device *dev = ptr;
> +
> + dev_hold(dev);
> + return 0;
> +}
> +
> +static void netdev_drop(void *ptr, int lastref)
> +{
> + struct net_device *dev = ptr;
> +
> + dev_put(dev);
> +}
> +
> /* security context strings */
> static int checkpoint_lsm_string(struct ckpt_ctx *ctx, void *ptr);
> static struct ckpt_lsm_string *restore_lsm_string(struct ckpt_ctx *ctx);
> @@ -550,6 +580,24 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
> .checkpoint = checkpoint_lsm_string,
> .restore = restore_lsm_string_wrap,
> },
> + /* Network Namespace Object */
> + {
> + .obj_name = "NET_NS",
> + .obj_type = CKPT_OBJ_NET_NS,
> + .ref_grab = netns_grab,
> + .ref_drop = netns_drop,
> + .checkpoint = checkpoint_netns,
> + .restore = restore_netns,
> + },
> + /* Network Device Object */
> + {
> + .obj_name = "NET_DEV",
> + .obj_type = CKPT_OBJ_NETDEV,
> + .ref_grab = netdev_grab,
> + .ref_drop = netdev_drop,
> + .checkpoint = checkpoint_netdev,
> + .restore = restore_netdev,
> + },
> };
What about leak detection ?
Aren't we missing {netns,netdev}_users()?
>
>
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index 7101d6f..a25bac1 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -35,6 +35,7 @@
> #include <linux/checkpoint_types.h>
> #include <linux/checkpoint_hdr.h>
> #include <linux/err.h>
> +#include <linux/inetdevice.h>
> #include <net/sock.h>
>
> /* sycall helpers */
> @@ -119,6 +120,28 @@ extern int ckpt_sock_getnames(struct ckpt_ctx *ctx,
> extern struct sk_buff *sock_restore_skb(struct ckpt_ctx *ctx, struct sock *sk);
> extern void sock_listening_list_free(struct list_head *head);
>
> +#ifdef CONFIG_CHECKPOINT_NETNS
> +extern int checkpoint_netns(struct ckpt_ctx *ctx, void *ptr);
> +extern void *restore_netns(struct ckpt_ctx *ctx);
> +extern int checkpoint_netdev(struct ckpt_ctx *ctx, void *ptr);
> +extern void *restore_netdev(struct ckpt_ctx *ctx);
> +
> +extern int ckpt_netdev_in_init_netns(struct ckpt_ctx *ctx,
> + struct net_device *dev);
> +extern int ckpt_netdev_inet_addrs(struct in_device *indev,
> + struct ckpt_netdev_addr *list[]);
> +extern int ckpt_netdev_hwaddr(struct net_device *dev,
> + struct ckpt_hdr_netdev *h);
> +extern struct ckpt_hdr_netdev *ckpt_netdev_base(struct ckpt_ctx *ctx,
> + struct net_device *dev,
> + struct ckpt_netdev_addr *addrs[]);
> +#else
> +# define checkpoint_netns NULL
> +# define restore_netns NULL
> +# define checkpoint_netdev NULL
> +# define restore_netdev NULL
> +#endif
> +
> /* ckpt kflags */
> #define ckpt_set_ctx_kflag(__ctx, __kflag) \
> set_bit(__kflag##_BIT, &(__ctx)->kflags)
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 41412d1..c065739 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -181,6 +181,12 @@ enum {
> #define CKPT_HDR_SOCKET_UNIX CKPT_HDR_SOCKET_UNIX
> CKPT_HDR_SOCKET_INET,
> #define CKPT_HDR_SOCKET_INET CKPT_HDR_SOCKET_INET
> + CKPT_HDR_NET_NS,
> +#define CKPT_HDR_NET_NS CKPT_HDR_NET_NS
> + CKPT_HDR_NETDEV,
> +#define CKPT_HDR_NETDEV CKPT_HDR_NETDEV
> + CKPT_HDR_NETDEV_ADDR,
> +#define CKPT_HDR_NETDEV_ADDR CKPT_HDR_NETDEV_ADDR
>
> CKPT_HDR_TAIL = 9001,
> #define CKPT_HDR_TAIL CKPT_HDR_TAIL
> @@ -253,6 +259,10 @@ enum obj_type {
> #define CKPT_OBJ_SECURITY_PTR CKPT_OBJ_SECURITY_PTR
> CKPT_OBJ_SECURITY,
> #define CKPT_OBJ_SECURITY CKPT_OBJ_SECURITY
> + CKPT_OBJ_NET_NS,
> +#define CKPT_OBJ_NET_NS CKPT_OBJ_NET_NS
> + CKPT_OBJ_NETDEV,
> +#define CKPT_OBJ_NETDEV CKPT_OBJ_NETDEV
> CKPT_OBJ_MAX
> #define CKPT_OBJ_MAX CKPT_OBJ_MAX
> };
> @@ -313,6 +323,7 @@ struct ckpt_hdr_tail {
> /* container configuration section header */
> struct ckpt_hdr_container {
> struct ckpt_hdr h;
> + __s32 init_netns_ref;
> /*
> * the header is followed by the string:
> * char lsm_name[SECURITY_NAME_MAX + 1]
> @@ -434,6 +445,7 @@ struct ckpt_hdr_ns {
> struct ckpt_hdr h;
> __s32 uts_objref;
> __s32 ipc_objref;
> + __s32 net_objref;
> } __attribute__((aligned(8)));
>
> /* cannot include <linux/tty.h> from userspace, so define: */
> @@ -758,6 +770,52 @@ struct ckpt_hdr_file_socket {
> __s32 sock_objref;
> } __attribute__((aligned(8)));
>
> +struct ckpt_hdr_netns {
> + struct ckpt_hdr h;
> + __s32 this_ref;
> +} __attribute__((aligned(8)));
> +
> +enum ckpt_netdev_types {
> + CKPT_NETDEV_LO,
> + CKPT_NETDEV_VETH,
> + CKPT_NETDEV_SIT,
> + CKPT_NETDEV_MACVLAN,
> +};
> +
> +struct ckpt_hdr_netdev {
> + struct ckpt_hdr h;
> + __s32 netns_ref;
> + union {
> + struct {
> + __s32 this_ref;
> + __s32 peer_ref;
> + } veth;
> + struct {
> + __u32 mode;
> + } macvlan;
> + };
> + __u32 inet_addrs;
> + __u16 type;
> + __u16 flags;
> + __u8 hwaddr[6];
> +} __attribute__((aligned(8)));
> +
> +enum ckpt_netdev_addr_types {
> + CKPT_NETDEV_ADDR_IPV4,
> +};
> +
> +struct ckpt_netdev_addr {
> + __u16 type;
Bad alignments ...
> + union {
> + struct {
> + __u32 inet4_local;
> + __u32 inet4_address;
> + __u32 inet4_mask;
> + __u32 inet4_broadcast;
> + };
> + } __attribute__((aligned(8)));
> +} __attribute__((aligned(8)));
> +
> struct ckpt_hdr_eventpoll_items {
> struct ckpt_hdr h;
> __s32 epfile_objref;
> diff --git a/include/linux/checkpoint_types.h b/include/linux/checkpoint_types.h
> index 5d5e00d..efc9357 100644
> --- a/include/linux/checkpoint_types.h
> +++ b/include/linux/checkpoint_types.h
> @@ -82,6 +82,7 @@ struct ckpt_ctx {
> wait_queue_head_t ghostq; /* waitqueue for ghost tasks */
> struct cred *realcred, *ecred; /* tmp storage for cred at restart */
> struct list_head listen_sockets;/* listening parent sockets */
> + int init_netns_ref; /* Objref of root net namespace */
>
> struct ckpt_stats stats; /* statistics */
>
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index 0da0d83..b0e67ff 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -248,6 +248,11 @@ int ckpt_collect_ns(struct ckpt_ctx *ctx, struct task_struct *t)
> ret = ckpt_obj_collect(ctx, nsproxy->uts_ns, CKPT_OBJ_UTS_NS);
> if (ret < 0)
> goto out;
I'm unsure why you need a separate configuration option for this ?
If you just want a shortcut to
"#if defined(CHECKPOINT) && defined(NETNS)"
then that's ok, but then in Kconfig (below) it should be tunable by
the user.
> +#ifdef CONFIG_CHECKPOINT_NETNS
> + ret = ckpt_obj_collect(ctx, nsproxy->net_ns, CKPT_OBJ_NET_NS);
> + if (ret < 0)
> + goto out;
> +#endif
> ret = ckpt_obj_collect(ctx, nsproxy->ipc_ns, CKPT_OBJ_IPC_NS);
> if (ret < 0)
> goto out;
> @@ -288,6 +293,12 @@ static int do_checkpoint_ns(struct ckpt_ctx *ctx, struct nsproxy *nsproxy)
> if (ret < 0)
> goto out;
> h->ipc_objref = ret;
> +#ifdef CONFIG_CHECKPOINT_NETNS
> + ret = checkpoint_obj(ctx, nsproxy->net_ns, CKPT_OBJ_NET_NS);
> + if (ret < 0)
> + goto out;
> + h->net_objref = ret;
> +#endif
>
> /* FIXME: for now, only marked visited to pacify leaks */
> ret = ckpt_obj_visit(ctx, nsproxy->mnt_ns, CKPT_OBJ_MNT_NS);
> @@ -328,6 +339,14 @@ static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx)
> ret = PTR_ERR(uts_ns);
> goto out;
> }
> + if (h->net_objref == 0)
> + net_ns = current->nsproxy->net_ns;
> + else
> + net_ns = ckpt_obj_fetch(ctx, h->net_objref, CKPT_OBJ_NET_NS);
> + if (IS_ERR(net_ns)) {
> + ret = PTR_ERR(net_ns);
> + goto out;
> + }
>
> if (h->ipc_objref == 0)
> ipc_ns = ctx->root_nsproxy->ipc_ns;
> @@ -339,7 +358,6 @@ static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx)
> }
>
> mnt_ns = ctx->root_nsproxy->mnt_ns;
> - net_ns = ctx->root_nsproxy->net_ns;
>
> if (uts_ns == current->nsproxy->uts_ns &&
> ipc_ns == current->nsproxy->ipc_ns &&
> diff --git a/net/Kconfig b/net/Kconfig
> index 041c35e..64dd3cd 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -276,4 +276,8 @@ source "net/wimax/Kconfig"
> source "net/rfkill/Kconfig"
> source "net/9p/Kconfig"
>
> +config CHECKPOINT_NETNS
> + bool
> + default y if NET && NET_NS && CHECKPOINT
> +
Did you mean this to be visible (settable) by the user ?
> endif # if NET
> diff --git a/net/Makefile b/net/Makefile
> index 74b038f..570ee98 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -67,3 +67,4 @@ endif
> obj-$(CONFIG_WIMAX) += wimax/
>
> obj-$(CONFIG_CHECKPOINT) += checkpoint.o
> +obj-$(CONFIG_CHECKPOINT_NETNS) += checkpoint_dev.o
> diff --git a/net/checkpoint_dev.c b/net/checkpoint_dev.c
> new file mode 100644
> index 0000000..5479419
> --- /dev/null
> +++ b/net/checkpoint_dev.c
> @@ -0,0 +1,815 @@
> +/*
> + * Copyright 2010 IBM Corporation
> + *
> + * Author(s): Dan Smith <danms@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2 of the
> + * License.
> + */
> +
> +#include <linux/sched.h>
> +#include <linux/if.h>
> +#include <linux/if_arp.h>
> +#include <linux/inetdevice.h>
> +#include <linux/veth.h>
> +#include <linux/checkpoint.h>
> +#include <linux/checkpoint_hdr.h>
> +#include <linux/deferqueue.h>
> +
> +#include <net/net_namespace.h>
> +#include <net/sch_generic.h>
> +
> +struct dq_netdev {
> + struct net_device *dev;
> + struct ckpt_ctx *ctx;
> +};
> +
> +struct veth_newlink {
> + char *peer;
> +};
> +
> +struct mvl_newlink {
> + char this[IFNAMSIZ+1];
> + char base[IFNAMSIZ+1];
> + int mode;
> + __u8 *hwaddr;
> +};
> +
> +typedef int (*new_link_fn)(struct sk_buff *, void *);
> +
> +static int __kern_devinet_ioctl(struct net *net, unsigned int cmd, void *arg)
> +{
> + mm_segment_t fs;
> + int ret;
> +
> + fs = get_fs();
> + set_fs(KERNEL_DS);
> + ret = devinet_ioctl(net, cmd, arg);
> + set_fs(fs);
> +
> + return ret;
> +}
> +
> +static int __kern_dev_ioctl(struct net *net, unsigned int cmd, void *arg)
> +{
> + mm_segment_t fs;
> + int ret;
> +
> + fs = get_fs();
> + set_fs(KERNEL_DS);
> + ret = dev_ioctl(net, cmd, arg);
> + set_fs(fs);
> +
> + return ret;
> +}
> +
> +static struct socket *rtnl_open(void)
> +{
> + struct socket *sock;
> + int ret;
> +
> + ret = sock_create(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE, &sock);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + return sock;
> +}
> +
> +static int rtnl_close(struct socket *rtnl)
> +{
> + if (rtnl)
> + return kernel_sock_shutdown(rtnl, SHUT_RDWR);
> + else
> + return 0;
> +}
> +
> +static struct nlmsghdr *rtnl_get_response(struct socket *rtnl,
> + struct sk_buff **skb)
> +{
> + int ret;
> + long timeo = MAX_SCHEDULE_TIMEOUT;
> + struct nlmsghdr *nlh;
> +
> + ret = sk_wait_data(rtnl->sk, &timeo);
> + if (!ret)
> + return ERR_PTR(-EPIPE);
> +
> + *skb = skb_dequeue(&rtnl->sk->sk_receive_queue);
> + if (!*skb)
> + return ERR_PTR(-EPIPE);
> +
> + ret = -EINVAL;
> + nlh = nlmsg_hdr(*skb);
> + if (!nlh)
> + goto err;
> +
> + if (nlh->nlmsg_type == NLMSG_ERROR) {
> + struct nlmsgerr *errmsg = nlmsg_data(nlh);
> + ret = errmsg->error;
> + goto err;
> + }
> +
> + return nlh;
> + err:
> + kfree_skb(*skb);
> + *skb = NULL;
> +
> + return ERR_PTR(ret);
> +}
> +
> +int ckpt_netdev_in_init_netns(struct ckpt_ctx *ctx, struct net_device *dev)
> +{
> + return dev->nd_net == current->nsproxy->net_ns;
> +}
This has raised questions before - probably worth a comment that explain
what's the rationale here.
> +
> +int ckpt_netdev_hwaddr(struct net_device *dev, struct ckpt_hdr_netdev *h)
> +{
> + struct net *net = dev->nd_net;
> + struct ifreq req;
> + int ret;
> +
> + memcpy(req.ifr_name, dev->name, IFNAMSIZ);
> + ret = __kern_dev_ioctl(net, SIOCGIFFLAGS, &req);
> + h->flags = req.ifr_flags;
> + if (ret < 0)
> + return ret;
[nit] is it intentional that you assign h->flags before testing ret ?
> +
> + ret = __kern_dev_ioctl(net, SIOCGIFHWADDR, &req);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(h->hwaddr, req.ifr_hwaddr.sa_data, sizeof(h->hwaddr));
> +
> + return 0;
> +}
> +
> +int ckpt_netdev_inet_addrs(struct in_device *indev,
> + struct ckpt_netdev_addr *_abuf[])
> +{
> + struct ckpt_netdev_addr *abuf = NULL;
> + struct in_ifaddr *addr = indev->ifa_list;
> + int pages = 0;
> + int addrs = 0;
> + int max;
> +
> + retry:
> + if (++pages > 4) {
> + addrs = -E2BIG;
> + goto out;
> + }
Why 4 ?
This makes me wonder if it's worth to allocate a temp buf over the
checkpoint ctx (e.g. ctx->tmpbuf) to be used for scratch ?
> +
> + *_abuf = krealloc(abuf, PAGE_SIZE * pages, GFP_KERNEL);
> + if (*_abuf == NULL) {
> + addrs = -ENOMEM;
> + goto out;
> + }
> + abuf = *_abuf;
> +
> + read_lock(&dev_base_lock);
> +
> + max = (pages * PAGE_SIZE) / sizeof(*abuf);
> + while (addr) {
> + abuf[addrs].type = CKPT_NETDEV_ADDR_IPV4; /* Only IPv4 now */
> + abuf[addrs].inet4_local = addr->ifa_local;
> + abuf[addrs].inet4_address = addr->ifa_address;
> + abuf[addrs].inet4_mask = addr->ifa_mask;
> + abuf[addrs].inet4_broadcast = addr->ifa_broadcast;
> +
> + addr = addr->ifa_next;
> + if (++addrs >= max) {
> + read_unlock(&dev_base_lock);
> + goto retry;
> + }
> + }
> +
> + read_unlock(&dev_base_lock);
> + out:
> + if (addrs < 0) {
> + kfree(abuf);
> + *_abuf = NULL;
> + }
> +
> + return addrs;
> +}
> +
> +struct ckpt_hdr_netdev *ckpt_netdev_base(struct ckpt_ctx *ctx,
> + struct net_device *dev,
> + struct ckpt_netdev_addr *addrs[])
> +{
> + struct ckpt_hdr_netdev *h;
> + int ret;
> +
> + h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_NETDEV);
> + if (!h)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = ckpt_netdev_hwaddr(dev, h);
> + if (ret < 0)
> + goto out;
> +
> + *addrs = NULL;
> + ret = h->inet_addrs = ckpt_netdev_inet_addrs(dev->ip_ptr, addrs);
> + if (ret < 0) {
> + if (ret == -E2BIG)
> + ckpt_err(ctx, ret,
> + "Too many inet addresses on interface %s\n",
> + dev->name);
Do we really need this special case ? I'd be happy with a ckpt_err()
for any error - and the actual error number would be useful to tell
which case it was.
> + goto out;
> + }
> +
> + if (ckpt_netdev_in_init_netns(ctx, dev))
> + ret = h->netns_ref = 0;
> + else
> + ret = h->netns_ref = checkpoint_obj(ctx, dev->nd_net,
> + CKPT_OBJ_NET_NS);
> + out:
> + if (ret < 0) {
> + ckpt_hdr_put(ctx, h);
> + h = ERR_PTR(ret);
> + if (*addrs)
> + kfree(*addrs);
> + }
> +
> + return h;
> +}
> +
> +int checkpoint_netdev(struct ckpt_ctx *ctx, void *ptr)
> +{
> + struct net_device *dev = (struct net_device *)ptr;
> +
> + if (!dev->netdev_ops->ndo_checkpoint) {
> + ckpt_err(ctx, -ENOSYS,
> + "Device %s does not support checkpoint\n", dev->name);
> + return -ENOSYS;
> + }
> +
> + ckpt_debug("checkpointing netdev %s\n", dev->name);
> +
> + return dev->netdev_ops->ndo_checkpoint(ctx, dev);
> +}
> +
> +int checkpoint_netns(struct ckpt_ctx *ctx, void *ptr)
> +{
> + struct net *net = ptr;
> + struct net_device *dev;
> + struct ckpt_hdr_netns *h;
> + int ret;
> +
> + h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_NET_NS);
> + if (!h)
> + return -ENOMEM;
> +
> + h->this_ref = ckpt_obj_lookup(ctx, net, CKPT_OBJ_NET_NS);
> + BUG_ON(h->this_ref == 0);
How about s/==/<=/ ?
> +
> + ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) h);
> + if (ret < 0)
> + goto out;
> +
> + for_each_netdev(net, dev) {
> + if (!dev->netdev_ops->ndo_checkpoint) {
Isn't this check redundant ? I expect it to fail promptly in
checkpoint_netdev() above.
> + ret = -ENOSYS;
> + ckpt_err(ctx, ret,
> + "Device %s does not support checkpoint\n",
> + dev->name);
> + break;
> + }
> +
> + ret = checkpoint_obj(ctx, dev, CKPT_OBJ_NETDEV);
> + if (ret < 0)
> + break;
> + }
> + out:
> + ckpt_hdr_put(ctx, h);
> +
> + return ret;
> +}
> +
> +static int restore_in_addrs(struct ckpt_ctx *ctx,
> + __u32 naddrs,
> + struct net *net,
> + struct net_device *dev)
> +{
> + __u32 i;
> + int ret = 0;
> + int len = naddrs * sizeof(struct ckpt_netdev_addr);
> + struct ckpt_netdev_addr *addrs = NULL;
> +
> + addrs = kmalloc(len, GFP_KERNEL);
> + if (!addrs)
> + return -ENOMEM;
> +
> + ret = _ckpt_read_buffer(ctx, addrs, len);
> + if (ret < 0)
> + goto out;
Could you use ckpt_read_payload() instead of the above ?
> +
> + for (i = 0; i < naddrs; i++) {
> + struct ckpt_netdev_addr *addr = &addrs[i];
> + struct ifreq req;
> + struct sockaddr_in *inaddr;
> +
> + if (addr->type != CKPT_NETDEV_ADDR_IPV4) {
> + ret = -EINVAL;
> + ckpt_err(ctx, ret, "Unsupported netdev addr type %i\n",
> + addr->type);
> + break;
> + }
> +
> + ckpt_debug("restoring %s: %x/%x/%x\n", dev->name,
> + addr->inet4_address,
> + addr->inet4_mask,
> + addr->inet4_broadcast);
> +
> + memcpy(req.ifr_name, dev->name, IFNAMSIZ);
> +
> + inaddr = (struct sockaddr_in *)&req.ifr_addr;
> + inaddr->sin_addr.s_addr = addr->inet4_address;
> + inaddr->sin_family = AF_INET;
> + ret = __kern_devinet_ioctl(net, SIOCSIFADDR, &req);
> + if (ret < 0) {
> + ckpt_err(ctx, ret, "Failed to set address\n");
> + break;
> + }
> +
> + inaddr = (struct sockaddr_in *)&req.ifr_addr;
> + inaddr->sin_addr.s_addr = addr->inet4_mask;
> + inaddr->sin_family = AF_INET;
> + ret = __kern_devinet_ioctl(net, SIOCSIFNETMASK, &req);
> + if (ret < 0) {
> + ckpt_err(ctx, ret, "Failed to set netmask\n");
> + break;
> + }
> +
> + inaddr = (struct sockaddr_in *)&req.ifr_addr;
> + inaddr->sin_addr.s_addr = addr->inet4_broadcast;
> + inaddr->sin_family = AF_INET;
> + ret = __kern_devinet_ioctl(net, SIOCSIFBRDADDR, &req);
> + if (ret < 0) {
> + ckpt_err(ctx, ret, "Failed to set broadcast\n");
> + break;
> + }
> + }
> +
> + out:
> + kfree(addrs);
> +
> + return ret;
> +}
> +
> +static int veth_new_link_msg(struct sk_buff *skb, void *data)
> +{
> + struct nlattr *linkinfo;
> + struct nlattr *linkdata;
> + struct ifinfomsg ifm;
> + int ret = -ENOMEM;
> + struct veth_newlink *d = data;
> +
> + linkinfo = nla_nest_start(skb, IFLA_LINKINFO);
> + if (!linkinfo)
> + goto out;
> +
> + ret = nla_put_string(skb, IFLA_INFO_KIND, "veth");
> + if (ret)
> + goto out;
> +
> + linkdata = nla_nest_start(skb, IFLA_INFO_DATA);
> + if (!linkdata) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = nla_put(skb, VETH_INFO_PEER, sizeof(ifm), &ifm);
> + if (!ret)
> + ret = nla_put_string(skb, IFLA_IFNAME, d->peer);
> +
> + nla_nest_end(skb, linkdata);
> + out:
> + nla_nest_end(skb, linkinfo);
> +
> + return ret;
> +}
> +
> +static int mvl_new_link_msg(struct sk_buff *skb, void *data)
> +{
> + struct mvl_newlink *d = data;
> + struct nlattr *linkinfo;
> + struct nlattr *linkdata;
> + struct net_device *lowerdev;
> + int ret;
> +
> + lowerdev = dev_get_by_name(current->nsproxy->net_ns, d->base);
> + if (!lowerdev)
> + return -ENOENT;
> +
> + ret = nla_put(skb, IFLA_ADDRESS, ETH_ALEN, d->hwaddr);
> + if (ret)
> + goto out_put;
> +
> + ret = nla_put_u32(skb, IFLA_LINK, lowerdev->ifindex);
> + if (ret)
> + goto out_put;
> +
> + linkinfo = nla_nest_start(skb, IFLA_LINKINFO);
> + if (!linkinfo) {
> + ret = -ENOMEM;
> + goto out;
s/out/out_put/ ??
> + }
> +
> + ret = nla_put_string(skb, IFLA_INFO_KIND, "macvlan");
> + if (ret)
> + goto out;
> +
> + linkdata = nla_nest_start(skb, IFLA_INFO_DATA);
> + if (!linkdata) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = nla_put_u32(skb, IFLA_MACVLAN_MODE, d->mode);
> + nla_nest_end(skb, linkdata);
> + out:
> + nla_nest_end(skb, linkinfo);
> + out_put:
> + dev_put(lowerdev);
> +
> + return ret;
> +}
> +
> +static struct sk_buff *new_link_msg(new_link_fn fn, void *data, char *name)
> +{
> + int ret = -ENOMEM;
> + int flags = NLM_F_REQUEST | NLM_F_CREATE | NLM_F_ACK;
> + struct nlmsghdr *nlh;
> + struct sk_buff *skb;
> + struct ifinfomsg *ifm;
> +
> + skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!skb)
> + goto out;
> +
> + nlh = nlmsg_put(skb, 0, 0, RTM_NEWLINK, sizeof(*ifm), flags);
> + if (!nlh)
> + goto out;
> +
> + ifm = nlmsg_data(nlh);
> + memset(ifm, 0, sizeof(*ifm));
> +
> + ret = nla_put_string(skb, IFLA_IFNAME, name);
> + if (ret)
> + goto out;
> +
> + ret = fn(skb, data);
> +
> + nlmsg_end(skb, nlh);
> +
> + out:
> + if (ret < 0) {
> + kfree_skb(skb);
> + skb = ERR_PTR(ret);
> + }
> +
> + return skb;
> +}
> +
> +static struct net_device *rtnl_newlink(new_link_fn fn, void *data, char *name)
> +{
> + int ret = -ENOMEM;
> + struct socket *rtnl = NULL;
> + struct sk_buff *skb = NULL;
> + struct nlmsghdr *nlh;
> + struct msghdr msg;
> + struct kvec kvec;
> +
> + skb = new_link_msg(fn, data, name);
> + if (IS_ERR(skb)) {
> + ret = PTR_ERR(skb);
> + ckpt_debug("failed to create new link message: %i\n", ret);
> + skb = NULL;
> + goto out;
Is this right - you didn't open the rtnl yet ?
Perhaps only ckpt_debug() and then return PTR_ERR(skb).
> + }
> +
> + memset(&msg, 0, sizeof(msg));
> + kvec.iov_len = skb->len;
> + kvec.iov_base = skb->head;
> +
> + rtnl = rtnl_open();
> + if (IS_ERR(rtnl)) {
> + ret = PTR_ERR(rtnl);
> + ckpt_debug("Unable to open rtnetlink socket: %i\n", ret);
> + goto out_noclose;
> + }
> +
> + ret = kernel_sendmsg(rtnl, &msg, &kvec, 1, kvec.iov_len);
> + if (ret < 0)
> + goto out;
> + else if (ret != skb->len) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + /* Free the send skb to make room for the receive skb */
> + kfree_skb(skb);
> +
> + nlh = rtnl_get_response(rtnl, &skb);
> + if (IS_ERR(nlh)) {
If this happens, would rtnl_get_response() place a NULL in skb ?
If not, then the kfree_skb() below will free the previous skb again.
> + ret = PTR_ERR(nlh);
> + ckpt_debug("RTNETLINK said: %i\n", ret);
> + }
> + out:
> + rtnl_close(rtnl);
> + out_noclose:
> + kfree_skb(skb);
> +
> + if (ret < 0)
> + return ERR_PTR(ret);
> + else
> + return dev_get_by_name(current->nsproxy->net_ns, name);
> +}
> +
> +static int netdev_noop(void *data)
> +{
> + return 0;
> +}
> +
> +static int netdev_cleanup(void *data)
> +{
> + struct dq_netdev *dq = data;
> +
> + dev_put(dq->dev);
> +
> + if (dq->ctx->errno) {
> + ckpt_debug("Unregistering netdev %s\n", dq->dev->name);
> + unregister_netdev(dq->dev);
> + }
> +
> + return 0;
> +}
> +
> +static struct net_device *restore_veth(struct ckpt_ctx *ctx,
> + struct ckpt_hdr_netdev *h,
> + struct net *net)
> +{
> + int ret;
> + char this_name[IFNAMSIZ];
> + char peer_name[IFNAMSIZ];
> + struct net_device *dev;
> + struct net_device *peer;
> + int didreg = 0;
> + struct ifreq req;
> + struct dq_netdev dq;
> +
> + dq.ctx = ctx;
> +
> + ret = _ckpt_read_buffer(ctx, this_name, IFNAMSIZ);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + ret = _ckpt_read_buffer(ctx, peer_name, IFNAMSIZ);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + ckpt_debug("restored veth netdev %s:%s\n", this_name, peer_name);
> +
> + peer = ckpt_obj_try_fetch(ctx, h->veth.peer_ref, CKPT_OBJ_NETDEV);
> + if (IS_ERR(peer)) {
> + struct veth_newlink veth = {
> + .peer = peer_name,
> + };
> +
> + /* We're first: allocate the veth pair */
> + didreg = 1;
> +
> + dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
> + if (IS_ERR(dev))
> + return dev;
> +
> + peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
> + if (!peer) {
> + ret = -EINVAL;
> + goto err_dev;
> + }
> +
> + dq.dev = peer;
> + ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
> + netdev_noop, netdev_cleanup);
> + if (ret)
> + goto err_peer;
> +
> + ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
> + CKPT_OBJ_NETDEV);
> + if (ret < 0)
> + /* Can't recall peer dq, so let it cleanup peer */
> + goto err_dev;
This may be a bit simpler if you move the first deferqueue_add()
forward to just before the other one. Or better: change dq_netdev
to have two pointers, dev and peer (if any is null, the cleanup
function will skip).
> + dev_put(peer);
> +
> + dq.dev = dev;
> + ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
> + netdev_noop, netdev_cleanup);
> + if (ret)
> + /* Can't recall peer dq, so let it cleanup peer */
> + goto err_dev;
> +
> + } else {
> + /* We're second: get our dev from the hash */
> + dev = ckpt_obj_fetch(ctx, h->veth.this_ref, CKPT_OBJ_NETDEV);
> + if (IS_ERR(dev))
> + return dev;
> + }
> +
> + /* Move to our new netns */
> + rtnl_lock();
> + ret = dev_change_net_namespace(dev, net, dev->name);
> + rtnl_unlock();
> + if (ret < 0)
> + goto out;
> +
> + /* Restore MAC address */
> + memcpy(req.ifr_name, dev->name, IFNAMSIZ);
> + memcpy(req.ifr_hwaddr.sa_data, h->hwaddr, sizeof(h->hwaddr));
> + req.ifr_hwaddr.sa_family = ARPHRD_ETHER;
> + ret = __kern_dev_ioctl(net, SIOCSIFHWADDR, &req);
> + out:
> + if (ret)
> + dev = ERR_PTR(ret);
> +
> + return dev;
> +
> + err_peer:
> + dev_put(peer);
> + unregister_netdev(peer);
> + err_dev:
> + dev_put(dev);
> + unregister_netdev(dev);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static struct net_device *restore_lo(struct ckpt_ctx *ctx,
> + struct ckpt_hdr_netdev *h,
> + struct net *net)
> +{
> + struct net_device *dev;
> + char name[IFNAMSIZ+1];
> + int ret;
> +
> + dev = dev_get_by_name(net, "lo");
> + if (!dev)
> + return ERR_PTR(-EINVAL);
> +
> + ret = _ckpt_read_buffer(ctx, name, IFNAMSIZ);
> + if (ret < 0)
> + goto err;
> +
> + if (strncmp(dev->name, name, IFNAMSIZ) != 0) {
> + ret = dev_change_name(dev, name);
> + if (ret < 0)
> + goto err;
> + }
> +
> + return dev;
> + err:
> + dev_put(dev);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static struct net_device *restore_sit(struct ckpt_ctx *ctx,
> + struct ckpt_hdr_netdev *h,
> + struct net *net)
> +{
> + /* Don't actually do anything for SIT devices yet */
> + return dev_get_by_name(net, "sit0");
> +}
> +
> +static struct net_device *restore_macvlan(struct ckpt_ctx *ctx,
> + struct ckpt_hdr_netdev *h,
> + struct net *net)
> +{
> + struct net_device *dev;
> + struct mvl_newlink mvl = {
> + .mode = h->macvlan.mode,
> + .hwaddr = h->hwaddr,
> + };
> + int ret;
> +
> + ret = _ckpt_read_buffer(ctx, mvl.this, IFNAMSIZ);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + ret = _ckpt_read_buffer(ctx, mvl.base, IFNAMSIZ);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + dev = rtnl_newlink(mvl_new_link_msg, &mvl, mvl.this);
> + if (IS_ERR(dev)) {
> + ckpt_err(ctx, PTR_ERR(dev),
> + "Failed to create macvlan device %s:%s",
> + mvl.this, mvl.base);
> + goto out;
> + }
> +
> + rtnl_lock();
> + ret = dev_change_net_namespace(dev, net, dev->name);
> + rtnl_unlock();
> +
> + if (ret) {
> + ckpt_err(ctx, ret, "Failed to change netns of %s:%s\n",
> + mvl.this, mvl.base);
> + dev_put(dev);
> + unregister_netdev(dev);
> + dev = ERR_PTR(ret);
> + }
> + out:
> + return dev;
> +}
> +
> +void *restore_netdev(struct ckpt_ctx *ctx)
> +{
> + struct ckpt_hdr_netdev *h;
> + struct net_device *dev = NULL;
> + struct ifreq req;
> + struct net *net;
> + int ret;
> +
> + h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_NETDEV);
> + if (IS_ERR(h)) {
> + ckpt_err(ctx, PTR_ERR(h), "failed to read netdev\n");
This ckpt_err() is redundant. ckpt_read_obj_type() already calls
ckpt_err() if it fails.
> + return h;
> + }
> +
How about some comment to explain this following snippet ?
> + if (h->netns_ref != 0) {
> + net = ckpt_obj_try_fetch(ctx, h->netns_ref, CKPT_OBJ_NET_NS);
> + if (IS_ERR(net)) {
> + ckpt_debug("failed to get net for %i\n", h->netns_ref);
> + ret = PTR_ERR(net);
> + net = current->nsproxy->net_ns;
^^^^^^^^^^^^^^^^^
Why this ?
> + goto out;
> + }
> + } else
> + net = current->nsproxy->net_ns;
> +
> + if (h->type == CKPT_NETDEV_VETH)
> + dev = restore_veth(ctx, h, net);
> + else if (h->type == CKPT_NETDEV_LO)
> + dev = restore_lo(ctx, h, net);
> + else if (h->type == CKPT_NETDEV_SIT)
> + dev = restore_sit(ctx, h, net);
> + else if (h->type == CKPT_NETDEV_MACVLAN)
> + dev = restore_macvlan(ctx, h, net);
> + else
> + dev = ERR_PTR(-EINVAL);
This is ugly. How about a dispatch table intead ? It will also
allow in the future for kernel modules to register their restore
functions.
> +
> + if (IS_ERR(dev)) {
> + ret = PTR_ERR(dev);
> + ckpt_err(ctx, ret, "Netdev type %i not supported\n", h->type);
> + goto out;
> + }
> +
> + /* Restore flags (which will likely bring the interface up) */
> + memcpy(req.ifr_name, dev->name, IFNAMSIZ);
> + req.ifr_flags = h->flags;
> + ret = __kern_dev_ioctl(net, SIOCSIFFLAGS, &req);
> + if (ret < 0)
> + goto out;
> +
> + if (h->inet_addrs > 0)
> + ret = restore_in_addrs(ctx, h->inet_addrs, net, dev);
> + out:
> + if (ret) {
> + ckpt_err(ctx, ret, "Failed to restore netdevice\n");
> + if ((h->type == CKPT_NETDEV_VETH) && !IS_ERR(dev)) {
> + dev_put(dev);
> + }
> + dev = ERR_PTR(ret);
> + } else
> + ckpt_debug("restored netdev %s\n", dev->name);
> +
> + ckpt_hdr_put(ctx, h);
> +
> + return dev;
> +}
> +
> +void *restore_netns(struct ckpt_ctx *ctx)
> +{
> + struct ckpt_hdr_netns *h;
> + struct net *net;
> +
> + h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_NET_NS);
> + if (IS_ERR(h)) {
> + ckpt_err(ctx, PTR_ERR(h), "failed to read netns\n");
Here, too, no need for ckpt_err().
> + return h;
> + }
> +
Here, too, a comment to explain the snippet will be helpful.
> + if (h->this_ref != 0) {
> + net = copy_net_ns(CLONE_NEWNET, current->nsproxy->net_ns);
> + if (IS_ERR(net))
> + goto out;
[nit] this test isn't necessary.
> + } else
> + net = current->nsproxy->net_ns;
> + out:
> + ckpt_hdr_put(ctx, h);
> +
> + return net;
> +}
Oren.
^ permalink raw reply
* Re: KS8695: possible NAPI issue
From: Figo.zhang @ 2010-03-06 2:40 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Yegor Yefremov, Dick Hollenbeck, netdev, zealcook
In-Reply-To: <20100305082218.4f734857@nehalam>
>
> One of the requirements for NAPI to work is that the IRQ is level triggered.
> Otherwise there can be a race between packet arrival and the end of
> NAPI processing:
>
> static int ks8695_poll(struct napi_struct *napi, int budget)
> {
> struct ks8695_priv *ksp = container_of(napi, struct ks8695_priv, napi);
> unsigned long work_done;
>
> unsigned long isr = readl(KS8695_IRQ_VA + KS8695_INTEN);
> unsigned long mask_bit = 1 << ks8695_get_rx_enable_bit(ksp);
>
> work_done = ks8695_rx(ksp, budget);
>
> if (work_done < budget) {
> unsigned long flags;
> spin_lock_irqsave(&ksp->rx_lock, flags);
> >>> packet arrives >>
> /*enable rx interrupt*/
> writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);
> >>> or here >>
> __napi_complete(napi);
> spin_unlock_irqrestore(&ksp->rx_lock, flags);
>
yes, it is right, it maybe a race condition. so you can motify
ks8695_poll() to try, like this:
__napi_complete(napi);
/*enable rx interrupt*/
writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);
Best,
Figo.zhang
^ permalink raw reply
* [PATCH] fix a race in ks8695_poll
From: Figo.zhang @ 2010-03-06 2:36 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Stephen Hemminger, Yegor Yefremov
fix a race at the end of NAPI processing in ks8695_poll() function.
Signed-off-by:Figo.zhang <figo1802@gmail.com>
---
drivers/net/arm/ks8695net.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
index be256b3..ecd5819 100644
--- a/drivers/net/arm/ks8695net.c
+++ b/drivers/net/arm/ks8695net.c
@@ -576,9 +576,9 @@ static int ks8695_poll(struct napi_struct *napi, int budget)
if (work_done < budget) {
unsigned long flags;
spin_lock_irqsave(&ksp->rx_lock, flags);
+ __napi_complete(napi);
/*enable rx interrupt*/
writel(isr | mask_bit, KS8695_IRQ_VA + KS8695_INTEN);
- __napi_complete(napi);
spin_unlock_irqrestore(&ksp->rx_lock, flags);
}
return work_done;
^ permalink raw reply related
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Herbert Xu @ 2010-03-06 1:17 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <20100305234327.GJ6764@linux.vnet.ibm.com>
On Fri, Mar 05, 2010 at 03:43:27PM -0800, Paul E. McKenney wrote:
>
> Cool!!! You use a pair of list_head structures, so that a given
> element can be in both the old and the new hash table simultaneously.
> Of course, an RCU grace period must elapse between consecutive resizings.
> Which appears to be addressed.
Thanks :) I will try to extend this to other existing hash tables
where the number of updates can be limited like it is here.
> The teardown needs an rcu_barrier_bh() rather than the current
> synchronize_rcu_bh(), please see below.
All the call_rcu_bh's are done under multicast_lock. The first
check taken after taking the multicast_lock is whether we've
started the tear-down. So where it currently calls synchronize()
it should already be the case that no call_rcu_bh's are still
running.
> Also, I don't see how the teardown code is preventing new readers from
> finding the data structures before they are being passed to call_rcu_bh().
> You can't safely start the RCU grace period until -after- all new readers
> have been excluded. (But I could easily be missing something here.)
I understand. However, AFAICS whatever it is that we are destroying
is taken off the reader's visible data structure before call_rcu_bh.
Do you have a particular case in mind where this is not the case?
> The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
> rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
> scheme correctly.
Any function that modifies the data structure is done under the
multicast_lock, including br_multicast_del_pg.
> Hmmm... Where is the read-side code? Wherever it is, it cannot safely
> dereference the ->old pointer.
Right, the old pointer is merely there to limit rehashings to one
per window. So it isn't used by the read-side.
The read-side is the data path (non-IGMP multicast packets). The
sole entry point is br_mdb_get().
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] net: Fix race condition on receive path.
From: Stephen Hemminger @ 2010-03-05 23:48 UTC (permalink / raw)
To: Daniel Walker
Cc: Harford, Jim, David S. Miller, netdev@vger.kernel.org,
Smith, Alan
In-Reply-To: <1267832667.30393.16.camel@c-dwalke-linux.qualcomm.com>
On Fri, 05 Mar 2010 15:44:27 -0800
Daniel Walker <dwalker@codeaurora.org> wrote:
> On Fri, 2010-03-05 at 15:41 -0800, Harford, Jim wrote:
> > It appears that this patch is no longer necessary. It was made against 2.6.29, but I see that more recent kernel versions don't have the problem code. For a more detailed explanation, see below. All code references are in routine process_backlog(), file net/core/dev.c.
> >
> > In kernel version 2.6.27.45, __napi_complete() is invoked BEFORE interrupts are re-enabled. Thus, the receive queue status is cleaned up before another interrupt (due to a receive packet) can occur. This is good design.
> >
> > In kernel version 2.6.29, git commit ID 303c6a025 inverts this ordering. Routine napi_complete() is invoked AFTER interrupts are re-enabled. We observed interrupts taken after interrupts were re-enabled, but before napi_complete cleaned up the receive queue. This would then shut down the processing of subsequent received packets.
> >
> > In kernel versions 2.6.30.10 and later, the sequence of operations is identical to 2.6.27.45, so there is no problem.
> >
> > Jim Harford
> > Qualcomm Innovation Center
>
> Ok, I guess we can ignore this one then.
>
> Daniel
>
It was fixed by:
commit 8f1ead2d1a626ed0c85b3d2c2046a49081d5933f
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu Mar 26 00:59:10 2009 -0700
GRO: Disable GRO on legacy netif_rx path
When I fixed the GRO crash in the legacy receive path I used
napi_complete to replace __napi_complete. Unfortunately they're
not the same when NETPOLL is enabled, which may result in us
not calling __napi_complete at all.
What's more, we really do need to keep the __napi_complete call
within the IRQ-off section since in theory an IRQ can occur in
between and fill up the backlog to the maximum, causing us to
lock up.
Since we can't seem to find a fix that works properly right now,
this patch reverts all the GRO support from the netif_rx path.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* RE: [PATCH] net: Fix race condition on receive path.
From: Daniel Walker @ 2010-03-05 23:44 UTC (permalink / raw)
To: Harford, Jim
Cc: Stephen Hemminger, David S. Miller, netdev@vger.kernel.org,
Smith, Alan
In-Reply-To: <E7C319BA4F818B4BA98B1D6626E5321603261A1F03@NASANEXMB10.na.qualcomm.com>
On Fri, 2010-03-05 at 15:41 -0800, Harford, Jim wrote:
> It appears that this patch is no longer necessary. It was made against 2.6.29, but I see that more recent kernel versions don't have the problem code. For a more detailed explanation, see below. All code references are in routine process_backlog(), file net/core/dev.c.
>
> In kernel version 2.6.27.45, __napi_complete() is invoked BEFORE interrupts are re-enabled. Thus, the receive queue status is cleaned up before another interrupt (due to a receive packet) can occur. This is good design.
>
> In kernel version 2.6.29, git commit ID 303c6a025 inverts this ordering. Routine napi_complete() is invoked AFTER interrupts are re-enabled. We observed interrupts taken after interrupts were re-enabled, but before napi_complete cleaned up the receive queue. This would then shut down the processing of subsequent received packets.
>
> In kernel versions 2.6.30.10 and later, the sequence of operations is identical to 2.6.27.45, so there is no problem.
>
> Jim Harford
> Qualcomm Innovation Center
Ok, I guess we can ignore this one then.
Daniel
^ permalink raw reply
* Re: [PATCH 6/13] bridge: Add core IGMP snooping support
From: Paul E. McKenney @ 2010-03-05 23:43 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, netdev, Stephen Hemminger
In-Reply-To: <E1NlbuT-00021C-0b@gondolin.me.apana.org.au>
On Sun, Feb 28, 2010 at 01:41:45PM +0800, Herbert Xu wrote:
> bridge: Add core IGMP snooping support
>
> This patch adds the core functionality of IGMP snooping support
> without actually hooking it up. So this patch should be a no-op
> as far as the bridge's external behaviour is concerned.
>
> All the new code and data is controlled by the Kconfig option
> BRIDGE_IGMP_SNOOPING. A run-time toggle is also available.
>
> The multicast switching is done using an hash table that is
> lockless on the read-side through RCU. On the write-side the
> new multicast_lock is used for all operations. The hash table
> supports dynamic growth/rehashing.
Cool!!! You use a pair of list_head structures, so that a given
element can be in both the old and the new hash table simultaneously.
Of course, an RCU grace period must elapse between consecutive resizings.
Which appears to be addressed.
The teardown needs an rcu_barrier_bh() rather than the current
synchronize_rcu_bh(), please see below.
Also, I don't see how the teardown code is preventing new readers from
finding the data structures before they are being passed to call_rcu_bh().
You can't safely start the RCU grace period until -after- all new readers
have been excluded. (But I could easily be missing something here.)
The br_multicast_del_pg() looks to need rcu_read_lock_bh() and
rcu_read_unlock_bh() around its loop, if I understand the pointer-walking
scheme correctly.
It looks like all updates are protected by an appropriate lock, although
I don't claim to fully understand how the data structures are linked
together. The key requirement is of course that you normally don't
get to protect insertion and deletion of a given data structure using a
lock located within that same data structure. It looks to me that this
requirement is satisfied -- all the lists that you manipulate seem to
hang off of the net_bridge, which contains the lock protecting all of
those lists.
Hmmm... Where is the read-side code? Wherever it is, it cannot safely
dereference the ->old pointer.
> The hash table will be rehashed if any chain length exceeds a
> preset limit. If rehashing does not reduce the maximum chain
> length then snooping will be disabled.
>
> These features may be added in future (in no particular order):
>
> * IGMPv3 source support
> * Non-querier router detection
> * IPv6
But the bugs all look fixable to me (assuming that they are in fact
bugs). Very cool to see an RCU-protected resizeable hash table!!! ;-)
Thanx, Paul
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>
> net/bridge/Kconfig | 12
> net/bridge/Makefile | 2
> net/bridge/br_multicast.c | 1135 ++++++++++++++++++++++++++++++++++++++++++++++
> net/bridge/br_private.h | 139 +++++
> 4 files changed, 1288 insertions(+)
>
> diff --git a/net/bridge/Kconfig b/net/bridge/Kconfig
> index e143ca6..78dd549 100644
> --- a/net/bridge/Kconfig
> +++ b/net/bridge/Kconfig
> @@ -31,3 +31,15 @@ config BRIDGE
> will be called bridge.
>
> If unsure, say N.
> +
> +config BRIDGE_IGMP_SNOOPING
> + bool "IGMP snooping"
> + default y
> + ---help---
> + If you say Y here, then the Ethernet bridge will be able selectively
> + forward multicast traffic based on IGMP traffic received from each
> + port.
> +
> + Say N to exclude this support and reduce the binary size.
> +
> + If unsure, say Y.
> diff --git a/net/bridge/Makefile b/net/bridge/Makefile
> index f444c12..d0359ea 100644
> --- a/net/bridge/Makefile
> +++ b/net/bridge/Makefile
> @@ -12,4 +12,6 @@ bridge-$(CONFIG_SYSFS) += br_sysfs_if.o br_sysfs_br.o
>
> bridge-$(CONFIG_BRIDGE_NETFILTER) += br_netfilter.o
>
> +bridge-$(CONFIG_BRIDGE_IGMP_SNOOPING) += br_multicast.o
> +
> obj-$(CONFIG_BRIDGE_NF_EBTABLES) += netfilter/
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> new file mode 100644
> index 0000000..746b5a6
> --- /dev/null
> +++ b/net/bridge/br_multicast.c
> @@ -0,0 +1,1135 @@
> +/*
> + * Bridge multicast support.
> + *
> + * Copyright (c) 2010 Herbert Xu <herbert@gondor.apana.org.au>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/if_ether.h>
> +#include <linux/igmp.h>
> +#include <linux/jhash.h>
> +#include <linux/kernel.h>
> +#include <linux/netdevice.h>
> +#include <linux/netfilter_bridge.h>
> +#include <linux/random.h>
> +#include <linux/rculist.h>
> +#include <linux/skbuff.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <net/ip.h>
> +
> +#include "br_private.h"
> +
> +static inline int br_ip_hash(struct net_bridge_mdb_htable *mdb, __be32 ip)
> +{
> + return jhash_1word(mdb->secret, (u32)ip) & (mdb->max - 1);
> +}
> +
> +static struct net_bridge_mdb_entry *__br_mdb_ip_get(
> + struct net_bridge_mdb_htable *mdb, __be32 dst, int hash)
> +{
> + struct net_bridge_mdb_entry *mp;
> + struct hlist_node *p;
> +
> + hlist_for_each_entry(mp, p, &mdb->mhash[hash], hlist[mdb->ver]) {
> + if (dst == mp->addr)
> + return mp;
> + }
> +
> + return NULL;
> +}
> +
> +static struct net_bridge_mdb_entry *br_mdb_ip_get(
> + struct net_bridge_mdb_htable *mdb, __be32 dst)
> +{
> + return __br_mdb_ip_get(mdb, dst, br_ip_hash(mdb, dst));
> +}
> +
> +struct net_bridge_mdb_entry *br_mdb_get(struct net_bridge *br,
> + struct sk_buff *skb)
> +{
> + struct net_bridge_mdb_htable *mdb = br->mdb;
> +
> + if (!mdb || br->multicast_disabled)
> + return NULL;
> +
> + switch (skb->protocol) {
> + case htons(ETH_P_IP):
> + if (BR_INPUT_SKB_CB(skb)->igmp)
> + break;
> + return br_mdb_ip_get(mdb, ip_hdr(skb)->daddr);
> + }
> +
> + return NULL;
> +}
> +
> +static void br_mdb_free(struct rcu_head *head)
> +{
> + struct net_bridge_mdb_htable *mdb =
> + container_of(head, struct net_bridge_mdb_htable, rcu);
> + struct net_bridge_mdb_htable *old = mdb->old;
> +
> + mdb->old = NULL;
So one way to figure out when it is safe to do another resize is when
the ->old pointer is NULLed out here.
> + kfree(old->mhash);
> + kfree(old);
> +}
> +
> +static int br_mdb_copy(struct net_bridge_mdb_htable *new,
> + struct net_bridge_mdb_htable *old,
> + int elasticity)
> +{
> + struct net_bridge_mdb_entry *mp;
> + struct hlist_node *p;
> + int maxlen;
> + int len;
> + int i;
> +
> + for (i = 0; i < old->max; i++)
> + hlist_for_each_entry(mp, p, &old->mhash[i], hlist[old->ver])
> + hlist_add_head(&mp->hlist[new->ver],
> + &new->mhash[br_ip_hash(new, mp->addr)]);
> +
> + if (!elasticity)
> + return 0;
> +
> + maxlen = 0;
> + for (i = 0; i < new->max; i++) {
> + len = 0;
> + hlist_for_each_entry(mp, p, &new->mhash[i], hlist[new->ver])
> + len++;
> + if (len > maxlen)
> + maxlen = len;
> + }
> +
> + return maxlen > elasticity ? -EINVAL : 0;
> +}
> +
> +static void br_multicast_free_pg(struct rcu_head *head)
> +{
> + struct net_bridge_port_group *p =
> + container_of(head, struct net_bridge_port_group, rcu);
> +
> + kfree(p);
> +}
> +
> +static void br_multicast_free_group(struct rcu_head *head)
> +{
> + struct net_bridge_mdb_entry *mp =
> + container_of(head, struct net_bridge_mdb_entry, rcu);
> +
> + kfree(mp);
> +}
> +
> +static void br_multicast_group_expired(unsigned long data)
> +{
> + struct net_bridge_mdb_entry *mp = (void *)data;
> + struct net_bridge *br = mp->br;
> + struct net_bridge_mdb_htable *mdb;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) || timer_pending(&mp->timer))
> + goto out;
> +
> + if (!hlist_unhashed(&mp->mglist))
> + hlist_del_init(&mp->mglist);
> +
> + if (mp->ports)
> + goto out;
> +
> + mdb = br->mdb;
> + hlist_del_rcu(&mp->hlist[mdb->ver]);
Also protected by br->multicast_lock.
> + mdb->size--;
> +
> + del_timer(&mp->query_timer);
> + call_rcu_bh(&mp->rcu, br_multicast_free_group);
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static void br_multicast_del_pg(struct net_bridge *br,
> + struct net_bridge_port_group *pg)
> +{
> + struct net_bridge_mdb_htable *mdb = br->mdb;
> + struct net_bridge_mdb_entry *mp;
> + struct net_bridge_port_group *p;
> + struct net_bridge_port_group **pp;
> +
> + mp = br_mdb_ip_get(mdb, pg->addr);
> + if (WARN_ON(!mp))
> + return;
> +
> + for (pp = &mp->ports; (p = *pp); pp = &p->next) {
> + if (p != pg)
> + continue;
> +
> + *pp = p->next;
If I understand this code correctly, we are relying on the RCU-bh grace
period not completing until we process the next element. But we don't
appear to be in an RCU-bh read-side critical section.
I believe that you need an rcu_read_lock_bh() and an rcu_read_unlock_bh()
around this loop or in all callers. @@@
> + hlist_del_init(&p->mglist);
> + del_timer(&p->timer);
> + del_timer(&p->query_timer);
> + call_rcu_bh(&p->rcu, br_multicast_free_pg);
> +
> + if (!mp->ports && hlist_unhashed(&mp->mglist) &&
> + netif_running(br->dev))
> + mod_timer(&mp->timer, jiffies);
> +
> + return;
> + }
> +
> + WARN_ON(1);
> +}
> +
> +static void br_multicast_port_group_expired(unsigned long data)
> +{
> + struct net_bridge_port_group *pg = (void *)data;
> + struct net_bridge *br = pg->port->br;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) || timer_pending(&pg->timer) ||
> + hlist_unhashed(&pg->mglist))
> + goto out;
> +
> + br_multicast_del_pg(br, pg);
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static int br_mdb_rehash(struct net_bridge_mdb_htable **mdbp, int max,
> + int elasticity)
> +{
> + struct net_bridge_mdb_htable *old = *mdbp;
> + struct net_bridge_mdb_htable *mdb;
> + int err;
> +
> + mdb = kmalloc(sizeof(*mdb), GFP_ATOMIC);
> + if (!mdb)
> + return -ENOMEM;
> +
> + mdb->max = max;
> + mdb->old = old;
OK, so the current hash table points to the old one.
The way this is set up, it looks to be illegal for RCU readers to
traverse the ->old pointer, since it is NULLed after but one RCU
grace period.
> + mdb->mhash = kzalloc(max * sizeof(*mdb->mhash), GFP_ATOMIC);
> + if (!mdb->mhash) {
> + kfree(mdb);
> + return -ENOMEM;
> + }
> +
> + mdb->size = old ? old->size : 0;
> + mdb->ver = old ? old->ver ^ 1 : 0;
> +
> + if (!old || elasticity)
> + get_random_bytes(&mdb->secret, sizeof(mdb->secret));
> + else
> + mdb->secret = old->secret;
> +
> + if (!old)
> + goto out;
> +
> + err = br_mdb_copy(mdb, old, elasticity);
> + if (err) {
> + kfree(mdb->mhash);
> + kfree(mdb);
> + return err;
> + }
> +
> + call_rcu_bh(&mdb->rcu, br_mdb_free);
And we are using RCU-bh. OK.
> +
> +out:
> + rcu_assign_pointer(*mdbp, mdb);
Also protected by br->multicast_lock.
> +
> + return 0;
> +}
> +
> +static struct sk_buff *br_multicast_alloc_query(struct net_bridge *br,
> + __be32 group)
> +{
> + struct sk_buff *skb;
> + struct igmphdr *ih;
> + struct ethhdr *eth;
> + struct iphdr *iph;
> +
> + skb = netdev_alloc_skb_ip_align(br->dev, sizeof(*eth) + sizeof(*iph) +
> + sizeof(*ih) + 4);
> + if (!skb)
> + goto out;
> +
> + skb->protocol = htons(ETH_P_IP);
> +
> + skb_reset_mac_header(skb);
> + eth = eth_hdr(skb);
> +
> + memcpy(eth->h_source, br->dev->dev_addr, 6);
> + eth->h_dest[0] = 1;
> + eth->h_dest[1] = 0;
> + eth->h_dest[2] = 0x5e;
> + eth->h_dest[3] = 0;
> + eth->h_dest[4] = 0;
> + eth->h_dest[5] = 1;
> + eth->h_proto = htons(ETH_P_IP);
> + skb_put(skb, sizeof(*eth));
> +
> + skb_set_network_header(skb, skb->len);
> + iph = ip_hdr(skb);
> +
> + iph->version = 4;
> + iph->ihl = 6;
> + iph->tos = 0xc0;
> + iph->tot_len = htons(sizeof(*iph) + sizeof(*ih) + 4);
> + iph->id = 0;
> + iph->frag_off = htons(IP_DF);
> + iph->ttl = 1;
> + iph->protocol = IPPROTO_IGMP;
> + iph->saddr = 0;
> + iph->daddr = htonl(INADDR_ALLHOSTS_GROUP);
> + ((u8 *)&iph[1])[0] = IPOPT_RA;
> + ((u8 *)&iph[1])[1] = 4;
> + ((u8 *)&iph[1])[2] = 0;
> + ((u8 *)&iph[1])[3] = 0;
> + ip_send_check(iph);
> + skb_put(skb, 24);
> +
> + skb_set_transport_header(skb, skb->len);
> + ih = igmp_hdr(skb);
> + ih->type = IGMP_HOST_MEMBERSHIP_QUERY;
> + ih->code = (group ? br->multicast_last_member_interval :
> + br->multicast_query_response_interval) /
> + (HZ / IGMP_TIMER_SCALE);
> + ih->group = group;
> + ih->csum = 0;
> + ih->csum = ip_compute_csum((void *)ih, sizeof(struct igmphdr));
> + skb_put(skb, sizeof(*ih));
> +
> + __skb_pull(skb, sizeof(*eth));
> +
> +out:
> + return skb;
> +}
> +
> +static void br_multicast_send_group_query(struct net_bridge_mdb_entry *mp)
> +{
> + struct net_bridge *br = mp->br;
> + struct sk_buff *skb;
> +
> + skb = br_multicast_alloc_query(br, mp->addr);
> + if (!skb)
> + goto timer;
> +
> + netif_rx(skb);
> +
> +timer:
> + if (++mp->queries_sent < br->multicast_last_member_count)
> + mod_timer(&mp->query_timer,
> + jiffies + br->multicast_last_member_interval);
> +}
> +
> +static void br_multicast_group_query_expired(unsigned long data)
> +{
> + struct net_bridge_mdb_entry *mp = (void *)data;
> + struct net_bridge *br = mp->br;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) || hlist_unhashed(&mp->mglist) ||
> + mp->queries_sent >= br->multicast_last_member_count)
> + goto out;
> +
> + br_multicast_send_group_query(mp);
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static void br_multicast_send_port_group_query(struct net_bridge_port_group *pg)
> +{
> + struct net_bridge_port *port = pg->port;
> + struct net_bridge *br = port->br;
> + struct sk_buff *skb;
> +
> + skb = br_multicast_alloc_query(br, pg->addr);
> + if (!skb)
> + goto timer;
> +
> + br_deliver(port, skb);
> +
> +timer:
> + if (++pg->queries_sent < br->multicast_last_member_count)
> + mod_timer(&pg->query_timer,
> + jiffies + br->multicast_last_member_interval);
> +}
> +
> +static void br_multicast_port_group_query_expired(unsigned long data)
> +{
> + struct net_bridge_port_group *pg = (void *)data;
> + struct net_bridge_port *port = pg->port;
> + struct net_bridge *br = port->br;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) || hlist_unhashed(&pg->mglist) ||
> + pg->queries_sent >= br->multicast_last_member_count)
> + goto out;
> +
> + br_multicast_send_port_group_query(pg);
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static struct net_bridge_mdb_entry *br_multicast_get_group(
> + struct net_bridge *br, struct net_bridge_port *port, __be32 group,
> + int hash)
> +{
> + struct net_bridge_mdb_htable *mdb = br->mdb;
> + struct net_bridge_mdb_entry *mp;
> + struct hlist_node *p;
> + unsigned count = 0;
> + unsigned max;
> + int elasticity;
> + int err;
> +
> + hlist_for_each_entry(mp, p, &mdb->mhash[hash], hlist[mdb->ver]) {
> + count++;
> + if (unlikely(group == mp->addr)) {
> + return mp;
> + }
> + }
> +
> + elasticity = 0;
> + max = mdb->max;
> +
> + if (unlikely(count > br->hash_elasticity && count)) {
> + if (net_ratelimit())
> + printk(KERN_INFO "%s: Multicast hash table "
> + "chain limit reached: %s\n",
> + br->dev->name, port ? port->dev->name :
> + br->dev->name);
> +
> + elasticity = br->hash_elasticity;
> + }
> +
> + if (mdb->size >= max) {
> + max *= 2;
> + if (unlikely(max >= br->hash_max)) {
> + printk(KERN_WARNING "%s: Multicast hash table maximum "
> + "reached, disabling snooping: %s, %d\n",
> + br->dev->name, port ? port->dev->name :
> + br->dev->name,
> + max);
> + err = -E2BIG;
> +disable:
> + br->multicast_disabled = 1;
> + goto err;
> + }
> + }
> +
> + if (max > mdb->max || elasticity) {
> + if (mdb->old) {
And here is the ->old check, as required.
> + if (net_ratelimit())
> + printk(KERN_INFO "%s: Multicast hash table "
> + "on fire: %s\n",
> + br->dev->name, port ? port->dev->name :
> + br->dev->name);
> + err = -EEXIST;
> + goto err;
> + }
> +
> + err = br_mdb_rehash(&br->mdb, max, elasticity);
> + if (err) {
> + printk(KERN_WARNING "%s: Cannot rehash multicast "
> + "hash table, disabling snooping: "
> + "%s, %d, %d\n",
> + br->dev->name, port ? port->dev->name :
> + br->dev->name,
> + mdb->size, err);
> + goto disable;
> + }
> +
> + err = -EAGAIN;
> + goto err;
> + }
> +
> + return NULL;
> +
> +err:
> + mp = ERR_PTR(err);
> + return mp;
> +}
> +
> +static struct net_bridge_mdb_entry *br_multicast_new_group(
> + struct net_bridge *br, struct net_bridge_port *port, __be32 group)
> +{
> + struct net_bridge_mdb_htable *mdb = br->mdb;
> + struct net_bridge_mdb_entry *mp;
> + int hash;
> +
> + if (!mdb) {
> + if (br_mdb_rehash(&br->mdb, BR_HASH_SIZE, 0))
> + return NULL;
> + goto rehash;
> + }
> +
> + hash = br_ip_hash(mdb, group);
> + mp = br_multicast_get_group(br, port, group, hash);
> + switch (PTR_ERR(mp)) {
> + case 0:
> + break;
> +
> + case -EAGAIN:
> +rehash:
> + mdb = br->mdb;
> + hash = br_ip_hash(mdb, group);
> + break;
> +
> + default:
> + goto out;
> + }
> +
> + mp = kzalloc(sizeof(*mp), GFP_ATOMIC);
> + if (unlikely(!mp))
> + goto out;
> +
> + mp->br = br;
> + mp->addr = group;
> + setup_timer(&mp->timer, br_multicast_group_expired,
> + (unsigned long)mp);
> + setup_timer(&mp->query_timer, br_multicast_group_query_expired,
> + (unsigned long)mp);
> +
> + hlist_add_head_rcu(&mp->hlist[mdb->ver], &mdb->mhash[hash]);
Protected by br->multicast_lock, so OK.
> + mdb->size++;
> +
> +out:
> + return mp;
> +}
> +
> +static int br_multicast_add_group(struct net_bridge *br,
> + struct net_bridge_port *port, __be32 group)
> +{
> + struct net_bridge_mdb_entry *mp;
> + struct net_bridge_port_group *p;
> + struct net_bridge_port_group **pp;
> + unsigned long now = jiffies;
> + int err;
> +
> + if (ipv4_is_local_multicast(group))
> + return 0;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) ||
> + (port && port->state == BR_STATE_DISABLED))
> + goto out;
> +
> + mp = br_multicast_new_group(br, port, group);
> + err = PTR_ERR(mp);
> + if (unlikely(IS_ERR(mp) || !mp))
> + goto err;
> +
> + if (!port) {
> + hlist_add_head(&mp->mglist, &br->mglist);
> + mod_timer(&mp->timer, now + br->multicast_membership_interval);
> + goto out;
> + }
> +
> + for (pp = &mp->ports; (p = *pp); pp = &p->next) {
> + if (p->port == port)
> + goto found;
> + if ((unsigned long)p->port < (unsigned long)port)
> + break;
> + }
> +
> + p = kzalloc(sizeof(*p), GFP_ATOMIC);
> + err = -ENOMEM;
> + if (unlikely(!p))
> + goto err;
> +
> + p->addr = group;
> + p->port = port;
> + p->next = *pp;
> + hlist_add_head(&p->mglist, &port->mglist);
> + setup_timer(&p->timer, br_multicast_port_group_expired,
> + (unsigned long)p);
> + setup_timer(&p->query_timer, br_multicast_port_group_query_expired,
> + (unsigned long)p);
> +
> + rcu_assign_pointer(*pp, p);
Also protected by br->multicast_lock.
> +
> +found:
> + mod_timer(&p->timer, now + br->multicast_membership_interval);
> +out:
> + err = 0;
> +
> +err:
> + spin_unlock(&br->multicast_lock);
> + return err;
> +}
> +
> +static void br_multicast_router_expired(unsigned long data)
> +{
> + struct net_bridge_port *port = (void *)data;
> + struct net_bridge *br = port->br;
> +
> + spin_lock(&br->multicast_lock);
> + if (port->multicast_router != 1 ||
> + timer_pending(&port->multicast_router_timer) ||
> + hlist_unhashed(&port->rlist))
> + goto out;
> +
> + hlist_del_init_rcu(&port->rlist);
Also protected by br->multicast_lock.
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static void br_multicast_local_router_expired(unsigned long data)
> +{
> +}
> +
> +static void br_multicast_send_query(struct net_bridge *br,
> + struct net_bridge_port *port, u32 sent)
> +{
> + unsigned long time;
> + struct sk_buff *skb;
> +
> + if (!netif_running(br->dev) || br->multicast_disabled ||
> + timer_pending(&br->multicast_querier_timer))
> + return;
> +
> + skb = br_multicast_alloc_query(br, 0);
> + if (!skb)
> + goto timer;
> +
> + if (port) {
> + __skb_push(skb, sizeof(struct ethhdr));
> + skb->dev = port->dev;
> + NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_OUT, skb, NULL, skb->dev,
> + dev_queue_xmit);
> + } else
> + netif_rx(skb);
> +
> +timer:
> + time = jiffies;
> + time += sent < br->multicast_startup_query_count ?
> + br->multicast_startup_query_interval :
> + br->multicast_query_interval;
> + mod_timer(port ? &port->multicast_query_timer :
> + &br->multicast_query_timer, time);
> +}
> +
> +static void br_multicast_port_query_expired(unsigned long data)
> +{
> + struct net_bridge_port *port = (void *)data;
> + struct net_bridge *br = port->br;
> +
> + spin_lock(&br->multicast_lock);
> + if (port && (port->state == BR_STATE_DISABLED ||
> + port->state == BR_STATE_BLOCKING))
> + goto out;
> +
> + if (port->multicast_startup_queries_sent <
> + br->multicast_startup_query_count)
> + port->multicast_startup_queries_sent++;
> +
> + br_multicast_send_query(port->br, port,
> + port->multicast_startup_queries_sent);
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +void br_multicast_add_port(struct net_bridge_port *port)
> +{
> + port->multicast_router = 1;
> +
> + setup_timer(&port->multicast_router_timer, br_multicast_router_expired,
> + (unsigned long)port);
> + setup_timer(&port->multicast_query_timer,
> + br_multicast_port_query_expired, (unsigned long)port);
> +}
> +
> +void br_multicast_del_port(struct net_bridge_port *port)
> +{
> + del_timer_sync(&port->multicast_router_timer);
> +}
> +
> +void br_multicast_enable_port(struct net_bridge_port *port)
> +{
> + struct net_bridge *br = port->br;
> +
> + spin_lock(&br->multicast_lock);
> + if (br->multicast_disabled || !netif_running(br->dev))
> + goto out;
> +
> + port->multicast_startup_queries_sent = 0;
> +
> + if (try_to_del_timer_sync(&port->multicast_query_timer) >= 0 ||
> + del_timer(&port->multicast_query_timer))
> + mod_timer(&port->multicast_query_timer, jiffies);
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +void br_multicast_disable_port(struct net_bridge_port *port)
> +{
> + struct net_bridge *br = port->br;
> + struct net_bridge_port_group *pg;
> + struct hlist_node *p, *n;
> +
> + spin_lock(&br->multicast_lock);
> + hlist_for_each_entry_safe(pg, p, n, &port->mglist, mglist)
> + br_multicast_del_pg(br, pg);
> +
> + if (!hlist_unhashed(&port->rlist))
> + hlist_del_init_rcu(&port->rlist);
Also protected by br->multicast_lock.
> + del_timer(&port->multicast_router_timer);
> + del_timer(&port->multicast_query_timer);
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static int br_multicast_igmp3_report(struct net_bridge *br,
> + struct net_bridge_port *port,
> + struct sk_buff *skb)
> +{
> + struct igmpv3_report *ih;
> + struct igmpv3_grec *grec;
> + int i;
> + int len;
> + int num;
> + int type;
> + int err = 0;
> + __be32 group;
> +
> + if (!pskb_may_pull(skb, sizeof(*ih)))
> + return -EINVAL;
> +
> + ih = igmpv3_report_hdr(skb);
> + num = ntohs(ih->ngrec);
> + len = sizeof(*ih);
> +
> + for (i = 0; i < num; i++) {
> + len += sizeof(*grec);
> + if (!pskb_may_pull(skb, len))
> + return -EINVAL;
> +
> + grec = (void *)(skb->data + len);
> + group = grec->grec_mca;
> + type = grec->grec_type;
> +
> + len += grec->grec_nsrcs * 4;
> + if (!pskb_may_pull(skb, len))
> + return -EINVAL;
> +
> + /* We treat this as an IGMPv2 report for now. */
> + switch (type) {
> + case IGMPV3_MODE_IS_INCLUDE:
> + case IGMPV3_MODE_IS_EXCLUDE:
> + case IGMPV3_CHANGE_TO_INCLUDE:
> + case IGMPV3_CHANGE_TO_EXCLUDE:
> + case IGMPV3_ALLOW_NEW_SOURCES:
> + case IGMPV3_BLOCK_OLD_SOURCES:
> + break;
> +
> + default:
> + continue;
> + }
> +
> + err = br_multicast_add_group(br, port, group);
> + if (err)
> + break;
> + }
> +
> + return err;
> +}
> +
> +static void br_multicast_mark_router(struct net_bridge *br,
> + struct net_bridge_port *port)
> +{
> + unsigned long now = jiffies;
> + struct hlist_node *p;
> + struct hlist_node **h;
> +
> + if (!port) {
> + if (br->multicast_router == 1)
> + mod_timer(&br->multicast_router_timer,
> + now + br->multicast_querier_interval);
> + return;
> + }
> +
> + if (port->multicast_router != 1)
> + return;
> +
> + if (!hlist_unhashed(&port->rlist))
> + goto timer;
> +
> + for (h = &br->router_list.first;
> + (p = *h) &&
> + (unsigned long)container_of(p, struct net_bridge_port, rlist) >
> + (unsigned long)port;
> + h = &p->next)
> + ;
> +
> + port->rlist.pprev = h;
> + port->rlist.next = p;
> + rcu_assign_pointer(*h, &port->rlist);
Also protected by br->multicast_lock.
> + if (p)
> + p->pprev = &port->rlist.next;
> +
> +timer:
> + mod_timer(&port->multicast_router_timer,
> + now + br->multicast_querier_interval);
> +}
> +
> +static void br_multicast_query_received(struct net_bridge *br,
> + struct net_bridge_port *port,
> + __be32 saddr)
> +{
> + if (saddr)
> + mod_timer(&br->multicast_querier_timer,
> + jiffies + br->multicast_querier_interval);
> + else if (timer_pending(&br->multicast_querier_timer))
> + return;
> +
> + br_multicast_mark_router(br, port);
> +}
> +
> +static int br_multicast_query(struct net_bridge *br,
> + struct net_bridge_port *port,
> + struct sk_buff *skb)
> +{
> + struct iphdr *iph = ip_hdr(skb);
> + struct igmphdr *ih = igmp_hdr(skb);
> + struct net_bridge_mdb_entry *mp;
> + struct igmpv3_query *ih3;
> + struct net_bridge_port_group *p;
> + struct net_bridge_port_group **pp;
> + unsigned long max_delay;
> + unsigned long now = jiffies;
> + __be32 group;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) ||
> + (port && port->state == BR_STATE_DISABLED))
> + goto out;
> +
> + br_multicast_query_received(br, port, iph->saddr);
> +
> + group = ih->group;
> +
> + if (skb->len == sizeof(*ih)) {
> + max_delay = ih->code * (HZ / IGMP_TIMER_SCALE);
> +
> + if (!max_delay) {
> + max_delay = 10 * HZ;
> + group = 0;
> + }
> + } else {
> + if (!pskb_may_pull(skb, sizeof(struct igmpv3_query)))
> + return -EINVAL;
> +
> + ih3 = igmpv3_query_hdr(skb);
> + if (ih3->nsrcs)
> + return 0;
> +
> + max_delay = ih3->code ? 1 :
> + IGMPV3_MRC(ih3->code) * (HZ / IGMP_TIMER_SCALE);
> + }
> +
> + if (!group)
> + goto out;
> +
> + mp = br_mdb_ip_get(br->mdb, group);
> + if (!mp)
> + goto out;
> +
> + max_delay *= br->multicast_last_member_count;
> +
> + if (!hlist_unhashed(&mp->mglist) &&
> + (timer_pending(&mp->timer) ?
> + time_after(mp->timer.expires, now + max_delay) :
> + try_to_del_timer_sync(&mp->timer) >= 0))
> + mod_timer(&mp->timer, now + max_delay);
> +
> + for (pp = &mp->ports; (p = *pp); pp = &p->next) {
> + if (timer_pending(&p->timer) ?
> + time_after(p->timer.expires, now + max_delay) :
> + try_to_del_timer_sync(&p->timer) >= 0)
> + mod_timer(&mp->timer, now + max_delay);
> + }
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> + return 0;
> +}
> +
> +static void br_multicast_leave_group(struct net_bridge *br,
> + struct net_bridge_port *port,
> + __be32 group)
> +{
> + struct net_bridge_mdb_htable *mdb;
> + struct net_bridge_mdb_entry *mp;
> + struct net_bridge_port_group *p;
> + unsigned long now;
> + unsigned long time;
> +
> + if (ipv4_is_local_multicast(group))
> + return;
> +
> + spin_lock(&br->multicast_lock);
> + if (!netif_running(br->dev) ||
> + (port && port->state == BR_STATE_DISABLED) ||
> + timer_pending(&br->multicast_querier_timer))
> + goto out;
> +
> + mdb = br->mdb;
> + mp = br_mdb_ip_get(mdb, group);
> + if (!mp)
> + goto out;
> +
> + now = jiffies;
> + time = now + br->multicast_last_member_count *
> + br->multicast_last_member_interval;
> +
> + if (!port) {
> + if (!hlist_unhashed(&mp->mglist) &&
> + (timer_pending(&mp->timer) ?
> + time_after(mp->timer.expires, time) :
> + try_to_del_timer_sync(&mp->timer) >= 0)) {
> + mod_timer(&mp->timer, time);
> +
> + mp->queries_sent = 0;
> + mod_timer(&mp->query_timer, now);
> + }
> +
> + goto out;
> + }
> +
> + for (p = mp->ports; p; p = p->next) {
> + if (p->port != port)
> + continue;
> +
> + if (!hlist_unhashed(&p->mglist) &&
> + (timer_pending(&p->timer) ?
> + time_after(p->timer.expires, time) :
> + try_to_del_timer_sync(&p->timer) >= 0)) {
> + mod_timer(&p->timer, time);
> +
> + p->queries_sent = 0;
> + mod_timer(&p->query_timer, now);
> + }
> +
> + break;
> + }
> +
> +out:
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +static int br_multicast_ipv4_rcv(struct net_bridge *br,
> + struct net_bridge_port *port,
> + struct sk_buff *skb)
> +{
> + struct sk_buff *skb2 = skb;
> + struct iphdr *iph;
> + struct igmphdr *ih;
> + unsigned len;
> + unsigned offset;
> + int err;
> +
> + BR_INPUT_SKB_CB(skb)->igmp = 0;
> + BR_INPUT_SKB_CB(skb)->mrouters_only = 0;
> +
> + /* We treat OOM as packet loss for now. */
> + if (!pskb_may_pull(skb, sizeof(*iph)))
> + return -EINVAL;
> +
> + iph = ip_hdr(skb);
> +
> + if (iph->ihl < 5 || iph->version != 4)
> + return -EINVAL;
> +
> + if (!pskb_may_pull(skb, ip_hdrlen(skb)))
> + return -EINVAL;
> +
> + iph = ip_hdr(skb);
> +
> + if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
> + return -EINVAL;
> +
> + if (iph->protocol != IPPROTO_IGMP)
> + return 0;
> +
> + len = ntohs(iph->tot_len);
> + if (skb->len < len || len < ip_hdrlen(skb))
> + return -EINVAL;
> +
> + if (skb->len > len) {
> + skb2 = skb_clone(skb, GFP_ATOMIC);
> + if (!skb2)
> + return -ENOMEM;
> +
> + err = pskb_trim_rcsum(skb2, len);
> + if (err)
> + return err;
> + }
> +
> + len -= ip_hdrlen(skb2);
> + offset = skb_network_offset(skb2) + ip_hdrlen(skb2);
> + __skb_pull(skb2, offset);
> + skb_reset_transport_header(skb2);
> +
> + err = -EINVAL;
> + if (!pskb_may_pull(skb2, sizeof(*ih)))
> + goto out;
> +
> + iph = ip_hdr(skb2);
> +
> + switch (skb2->ip_summed) {
> + case CHECKSUM_COMPLETE:
> + if (!csum_fold(skb2->csum))
> + break;
> + /* fall through */
> + case CHECKSUM_NONE:
> + skb2->csum = 0;
> + if (skb_checksum_complete(skb2))
> + return -EINVAL;
> + }
> +
> + err = 0;
> +
> + BR_INPUT_SKB_CB(skb)->igmp = 1;
> + ih = igmp_hdr(skb2);
> +
> + switch (ih->type) {
> + case IGMP_HOST_MEMBERSHIP_REPORT:
> + case IGMPV2_HOST_MEMBERSHIP_REPORT:
> + BR_INPUT_SKB_CB(skb2)->mrouters_only = 1;
> + err = br_multicast_add_group(br, port, ih->group);
> + break;
> + case IGMPV3_HOST_MEMBERSHIP_REPORT:
> + err = br_multicast_igmp3_report(br, port, skb2);
> + break;
> + case IGMP_HOST_MEMBERSHIP_QUERY:
> + err = br_multicast_query(br, port, skb2);
> + break;
> + case IGMP_HOST_LEAVE_MESSAGE:
> + br_multicast_leave_group(br, port, ih->group);
> + break;
> + }
> +
> +out:
> + __skb_push(skb2, offset);
> + if (skb2 != skb)
> + kfree_skb(skb2);
> + return err;
> +}
> +
> +int br_multicast_rcv(struct net_bridge *br, struct net_bridge_port *port,
> + struct sk_buff *skb)
> +{
> + if (br->multicast_disabled)
> + return 0;
> +
> + switch (skb->protocol) {
> + case htons(ETH_P_IP):
> + return br_multicast_ipv4_rcv(br, port, skb);
> + }
> +
> + return 0;
> +}
> +
> +static void br_multicast_query_expired(unsigned long data)
> +{
> + struct net_bridge *br = (void *)data;
> +
> + spin_lock(&br->multicast_lock);
> + if (br->multicast_startup_queries_sent <
> + br->multicast_startup_query_count)
> + br->multicast_startup_queries_sent++;
> +
> + br_multicast_send_query(br, NULL, br->multicast_startup_queries_sent);
> +
> + spin_unlock(&br->multicast_lock);
> +}
> +
> +void br_multicast_init(struct net_bridge *br)
> +{
> + br->hash_elasticity = 4;
> + br->hash_max = 512;
> +
> + br->multicast_router = 1;
> + br->multicast_last_member_count = 2;
> + br->multicast_startup_query_count = 2;
> +
> + br->multicast_last_member_interval = HZ;
> + br->multicast_query_response_interval = 10 * HZ;
> + br->multicast_startup_query_interval = 125 * HZ / 4;
> + br->multicast_query_interval = 125 * HZ;
> + br->multicast_querier_interval = 255 * HZ;
> + br->multicast_membership_interval = 260 * HZ;
> +
> + spin_lock_init(&br->multicast_lock);
> + setup_timer(&br->multicast_router_timer,
> + br_multicast_local_router_expired, 0);
> + setup_timer(&br->multicast_querier_timer,
> + br_multicast_local_router_expired, 0);
> + setup_timer(&br->multicast_query_timer, br_multicast_query_expired,
> + (unsigned long)br);
> +}
> +
> +void br_multicast_open(struct net_bridge *br)
> +{
> + br->multicast_startup_queries_sent = 0;
> +
> + if (br->multicast_disabled)
> + return;
> +
> + mod_timer(&br->multicast_query_timer, jiffies);
> +}
> +
> +void br_multicast_stop(struct net_bridge *br)
> +{
> + struct net_bridge_mdb_htable *mdb;
> + struct net_bridge_mdb_entry *mp;
> + struct hlist_node *p, *n;
> + u32 ver;
> + int i;
> +
> + del_timer_sync(&br->multicast_router_timer);
> + del_timer_sync(&br->multicast_querier_timer);
> + del_timer_sync(&br->multicast_query_timer);
> +
> + spin_lock_bh(&br->multicast_lock);
> + mdb = br->mdb;
> + if (!mdb)
> + goto out;
> +
> + br->mdb = NULL;
> +
> + ver = mdb->ver;
> + for (i = 0; i < mdb->max; i++) {
> + hlist_for_each_entry_safe(mp, p, n, &mdb->mhash[i],
> + hlist[ver]) {
> + del_timer(&mp->timer);
> + del_timer(&mp->query_timer);
> + call_rcu_bh(&mp->rcu, br_multicast_free_group);
I don't see what prevents new readers from finding the net_bridge_mdb_entry
referenced by mp. Unless you stop new readers, an RCU grace period
cannot help you! @@@
> + }
> + }
> +
> + if (mdb->old) {
And another ->old check for teardown purposes?
> + spin_unlock_bh(&br->multicast_lock);
> + synchronize_rcu_bh();
Ah, but the above only guarantees that the grace period has elapsed,
not that all of the callbacks have been invoked, so...
> + spin_lock_bh(&br->multicast_lock);
> + WARN_ON(mdb->old);
This WARN_ON() can fire!!!
Suggested fix: s/synchronize_rcu_bh()/rcu_barrier_bh()/ @@@
> + }
> +
> + mdb->old = mdb;
I can't say I understand how making mdb->old point back at mdb helps...
Ah, clever -- this arrangement causes the RCU callback to free up the
whole mess. ;-)
> + call_rcu_bh(&mdb->rcu, br_mdb_free);
I don't see prevents new readers from finding the net_bridge_mdb_htable
referenced by mdb. Again, if new readers are permitted to reference
this structure, an RCU grace period won't help you -- you cannot start
the RCU grace period until after all new readers have been excluded.
> +
> +out:
> + spin_unlock_bh(&br->multicast_lock);
> +}
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 7b0aed5..0871775 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -57,6 +57,41 @@ struct net_bridge_fdb_entry
> unsigned char is_static;
> };
>
> +struct net_bridge_port_group {
> + struct net_bridge_port *port;
> + struct net_bridge_port_group *next;
> + struct hlist_node mglist;
> + struct rcu_head rcu;
> + struct timer_list timer;
> + struct timer_list query_timer;
> + __be32 addr;
> + u32 queries_sent;
> +};
> +
> +struct net_bridge_mdb_entry
> +{
> + struct hlist_node hlist[2];
> + struct hlist_node mglist;
> + struct net_bridge *br;
> + struct net_bridge_port_group *ports;
> + struct rcu_head rcu;
> + struct timer_list timer;
> + struct timer_list query_timer;
> + __be32 addr;
> + u32 queries_sent;
> +};
> +
> +struct net_bridge_mdb_htable
> +{
> + struct hlist_head *mhash;
> + struct rcu_head rcu;
> + struct net_bridge_mdb_htable *old;
> + u32 size;
> + u32 max;
> + u32 secret;
> + u32 ver;
> +};
> +
> struct net_bridge_port
> {
> struct net_bridge *br;
> @@ -84,6 +119,15 @@ struct net_bridge_port
>
> unsigned long flags;
> #define BR_HAIRPIN_MODE 0x00000001
> +
> +#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> + u32 multicast_startup_queries_sent;
> + unsigned char multicast_router;
> + struct timer_list multicast_router_timer;
> + struct timer_list multicast_query_timer;
> + struct hlist_head mglist;
> + struct hlist_node rlist;
> +#endif
> };
>
> struct net_bridge
> @@ -125,6 +169,35 @@ struct net_bridge
> unsigned char topology_change;
> unsigned char topology_change_detected;
>
> +#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> + unsigned char multicast_router;
> +
> + u8 multicast_disabled:1;
> +
> + u32 hash_elasticity;
> + u32 hash_max;
> +
> + u32 multicast_last_member_count;
> + u32 multicast_startup_queries_sent;
> + u32 multicast_startup_query_count;
> +
> + unsigned long multicast_last_member_interval;
> + unsigned long multicast_membership_interval;
> + unsigned long multicast_querier_interval;
> + unsigned long multicast_query_interval;
> + unsigned long multicast_query_response_interval;
> + unsigned long multicast_startup_query_interval;
> +
> + spinlock_t multicast_lock;
> + struct net_bridge_mdb_htable *mdb;
> + struct hlist_head router_list;
> + struct hlist_head mglist;
> +
> + struct timer_list multicast_router_timer;
> + struct timer_list multicast_querier_timer;
> + struct timer_list multicast_query_timer;
> +#endif
> +
> struct timer_list hello_timer;
> struct timer_list tcn_timer;
> struct timer_list topology_change_timer;
> @@ -134,6 +207,8 @@ struct net_bridge
>
> struct br_input_skb_cb {
> struct net_device *brdev;
> + int igmp;
> + int mrouters_only;
> };
>
> #define BR_INPUT_SKB_CB(__skb) ((struct br_input_skb_cb *)(__skb)->cb)
> @@ -205,6 +280,70 @@ extern struct sk_buff *br_handle_frame(struct net_bridge_port *p,
> extern int br_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
> extern int br_ioctl_deviceless_stub(struct net *net, unsigned int cmd, void __user *arg);
>
> +/* br_multicast.c */
> +#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> +extern int br_multicast_rcv(struct net_bridge *br,
> + struct net_bridge_port *port,
> + struct sk_buff *skb);
> +extern struct net_bridge_mdb_entry *br_mdb_get(struct net_bridge *br,
> + struct sk_buff *skb);
> +extern void br_multicast_add_port(struct net_bridge_port *port);
> +extern void br_multicast_del_port(struct net_bridge_port *port);
> +extern void br_multicast_enable_port(struct net_bridge_port *port);
> +extern void br_multicast_disable_port(struct net_bridge_port *port);
> +extern void br_multicast_init(struct net_bridge *br);
> +extern void br_multicast_open(struct net_bridge *br);
> +extern void br_multicast_stop(struct net_bridge *br);
> +#else
> +static inline int br_multicast_rcv(struct net_bridge *br,
> + struct net_bridge_port *port,
> + struct sk_buff *skb)
> +{
> + return 0;
> +}
> +
> +static inline struct net_bridge_mdb_entry *br_mdb_get(struct net_bridge *br,
> + struct sk_buff *skb)
> +{
> + return NULL;
> +}
> +
> +static inline void br_multicast_add_port(struct net_bridge_port *port)
> +{
> +}
> +
> +static inline void br_multicast_del_port(struct net_bridge_port *port)
> +{
> +}
> +
> +static inline void br_multicast_enable_port(struct net_bridge_port *port)
> +{
> +}
> +
> +static inline void br_multicast_disable_port(struct net_bridge_port *port)
> +{
> +}
> +
> +static inline void br_multicast_init(struct net_bridge *br)
> +{
> +}
> +
> +static inline void br_multicast_open(struct net_bridge *br)
> +{
> +}
> +
> +static inline void br_multicast_stop(struct net_bridge *br)
> +{
> +}
> +#endif
> +
> +static inline bool br_multicast_is_router(struct net_bridge *br)
> +{
> + return br->multicast_router == 2 ||
> + (br->multicast_router == 1 &&
> + timer_pending(&br->multicast_router_timer));
> +}
> +
> /* br_netfilter.c */
> #ifdef CONFIG_BRIDGE_NETFILTER
> extern int br_netfilter_init(void);
> --
> 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
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