OpenVZ Forum - RDF feed
type: channel
items: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
items: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
items: [PATCH 3/5] net: Make the netlink methods in rtnetlink handle multiple network namespaces
items: [PATCH 4/5] net: Make AF_PACKET handle multiple network namespaces
items: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
items: Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
items: Re: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
items: Re: Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
items: Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace

[PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: Before I can enable rtnetlink to work in all network namespaces I need to be certain that something won't break. So this patch deliberately disables all of the rtnletlink methods in everything except the initial network namespace. After the methods have been audited this extra check can be disabled. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- net/bridge/br_netlink.c | 9 +++++++++ net/core/fib_rules.c | 11 +++++++++++ net/core/neighbour.c | 18 ++++++++++++++++++ net/core/rtnetlink.c | 19 +++++++++++++++++++ net/decnet/dn_dev.c | 12 ++++++++++++ net/decnet/dn_fib.c | 8 ++++++++ net/decnet/dn_route.c | 8 ++++++++ net/decnet/dn_table.c | 4 ++++ net/ipv4/devinet.c | 12 ++++++++++++ net/ipv4/fib_frontend.c | 12 ++++++++++++ net/ipv4/route.c | 4 ++++ net/ipv6/addrconf.c | 31 +++++++++++++++++++++++++++++++ net/ipv6/ip6_fib.c | 4 ++++ net/ipv6/route.c | 12 ++++++++++++ net/sched/act_api.c | 10 ++++++++++ net/sched/cls_api.c | 10 ++++++++++ net/sched/sch_api.c | 21 +++++++++++++++++++++ 17 files changed, 205 insertions(+), 0 deletions(-) diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 53ab8e0..a4ffa2b 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -13,6 +13,7 @@ #include <linux/kernel.h> #include <net/rtnetlink.h> #include <net/net_namespace.h> +#include <net/sock.h> #include "br_private.h" static inline size_t br_nlmsg_size(void) @@ -107,9 +108,13 @@ errout: */ static int br_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) { + struct net *net = skb->sk->sk_net; struct net_device *dev; int idx; + if (net != &init_net) + return 0; + idx = 0; for_each_netdev(&init_net, dev) { /* not a bridge port */ @@ -135,12 +140,16 @@ skip: */ static int br_rtm_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) { + struct net *net = skb->sk->sk_net; struct ifinfomsg *ifm; struct nlattr *protinfo; struct net_device *dev; struct net_bridge_port *p; u8 new_state; + if (net != &init_net) + return -EINVAL; + if …
creator: ebiederm
date: 2007-09-29T01:00:37-00:00

[PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: After this patch none of the netlink callback support anything except the initial network namespace but the rtnetlink infrastructure now handles multiple network namespaces. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- include/linux/rtnetlink.h | 8 ++-- include/net/net_namespace.h | 3 + net/bridge/br_netlink.c | 4 +- net/core/fib_rules.c | 4 +- net/core/neighbour.c | 4 +- net/core/rtnetlink.c | 96 +++++++++++++++++++++++++++++++++++++------ net/decnet/dn_dev.c | 4 +- net/decnet/dn_route.c | 2 +- net/decnet/dn_table.c | 4 +- net/ipv4/devinet.c | 4 +- net/ipv4/fib_semantics.c | 4 +- net/ipv4/ipmr.c | 4 +- net/ipv4/route.c | 2 +- net/ipv6/addrconf.c | 14 +++--- net/ipv6/route.c | 6 +- net/sched/act_api.c | 8 ++-- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 4 +- net/wireless/wext.c | 5 ++- 19 files changed, 129 insertions(+), 53 deletions(-) diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h index 9c21e45..518247e 100644 --- a/include/linux/rtnetlink.h +++ b/include/linux/rtnetlink.h @@ -582,11 +582,11 @@ extern int __rtattr_parse_nested_compat(struct rtattr *tb[], int maxattr, ({ data = RTA_PAYLOAD(rta) >= len ? RTA_DATA(rta) : NULL; \ __rtattr_parse_nested_compat(tb, max, rta, len); }) -extern int rtnetlink_send(struct sk_buff *skb, u32 pid, u32 group, int echo); -extern int rtnl_unicast(struct sk_buff *skb, u32 pid); -extern int rtnl_notify(struct sk_buff *skb, u32 pid, u32 group, +extern int rtnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, u32 group, int echo); +extern int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid); +extern int rtnl_notify(struct sk_buff *skb, struct net *net, u32 pid, u32 group, struct nlmsghdr *nlh, gfp_t flags); -extern void rtnl_set_sk_err(u32 group, int error); +extern void rtnl_set_sk_err(struct net *net, u32 group, int error); extern int rtnetlink_put_metrics(struct sk_buff *skb, u32 *metrics); extern int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id, u…
creator: ebiederm
date: 2007-09-29T01:02:11-00:00

[PATCH 3/5] net: Make the netlink methods in rtnetlink handle multiple network namespaces
type: item
description: After the previous prep work this just consists of removing checks limiting the code to work in the initial network namespace, and updating rtmsg_ifinfo so we can generate events for devices in something other then the initial network namespace. Referring to network other network devices like the IFLA_LINK and IFLA_MASTER attributes do, gets interesting if those network devices happen to be in other network namespaces. Currently ifindex numbers are allocated globally so I have taken the path of least resistance and not still report the information even though the devices they are talking about are invisible. If applications start getting confused or when ifindex numbers become local to the network namespace we may need to do something different in the future. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- net/core/rtnetlink.c | 27 +++------------------------ 1 files changed, 3 insertions(+), 24 deletions(-) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index fc49104..809a9fb 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -741,9 +741,6 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) int s_idx = cb->args[0]; struct net_device *dev; - if (net != &init_net) - return 0; - idx = 0; for_each_netdev(net, dev) { if (idx < s_idx) @@ -946,9 +943,6 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) struct nlattr *tb[IFLA_MAX+1]; char ifname[IFNAMSIZ]; - if (net != &init_net) - return -EINVAL; - err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy); if (err < 0) goto errout; @@ -997,9 +991,6 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) struct nlattr *tb[IFLA_MAX+1]; int err; - if (net != &init_net) - return -EINVAL; - err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy); if (err < 0) return err; @@ -1081,9 +1072,6 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg) struct nlattr *linkinfo[IF…
creator: ebiederm
date: 2007-09-29T01:04:35-00:00

[PATCH 4/5] net: Make AF_PACKET handle multiple network namespaces
type: item
description: This is done by making packet_sklist_lock and packet_sklist per network namespace and adding an additional filter condition on received packets to ensure they came from the proper network namespace. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- include/net/net_namespace.h | 4 + net/packet/af_packet.c | 129 +++++++++++++++++++++++++++--------------- 2 files changed, 87 insertions(+), 46 deletions(-) diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h index f75607a..5e9fb47 100644 --- a/include/net/net_namespace.h +++ b/include/net/net_namespace.h @@ -32,6 +32,10 @@ struct net { struct hlist_head *dev_index_head; struct sock *rtnl; /* rtnetlink socket */ + + /* List of all packet sockets. */ + rwlock_t packet_sklist_lock; + struct hlist_head packet_sklist; }; #ifdef CONFIG_NET diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index e11000a..1c3a5a8 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -135,10 +135,6 @@ dev->hard_header == NULL (ll header is added by device, we cannot control it) packet classifier depends on it. */ -/* List of all packet sockets. */ -static HLIST_HEAD(packet_sklist); -static DEFINE_RWLOCK(packet_sklist_lock); - static atomic_t packet_socks_nr; @@ -252,9 +248,6 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, struct struct sock *sk; struct sockaddr_pkt *spkt; - if (dev->nd_net != &init_net) - goto out; - /* * When we registered the protocol we saved the socket in the data * field for just this event. @@ -276,6 +269,9 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, struct if (skb->pkt_type == PACKET_LOOPBACK) goto out; + if (dev->nd_net != sk->sk_net) + goto out; + if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) goto oom; @@ -347,7 +343,7 @@ static int packet_sendmsg_spkt(struct kiocb *iocb, struct socket *sock, */ saddr->spkt_device[13] = 0; - dev = dev_get_by_name(&init_net, saddr->spkt_device); + dev = dev_get_by_name(…
creator: ebiederm
date: 2007-09-29T01:07:05-00:00

[PATCH 5/5] net: Make AF_UNIX per network namespace safe.
type: item
description: Because of the global nature of garbage collection, and because of the cost of per namespace hash tables unix_socket_table has been kept global. With a filter added on lookups so we don't see sockets from the wrong namespace. Currently I don't fold the namesapce into the hash so multiple namespaces using the same socket name will be guaranteed a hash collision. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- net/unix/af_unix.c | 113 +++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 86 insertions(+), 27 deletions(-) diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 10e7312..2792ef2 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -270,7 +270,8 @@ static inline void unix_insert_socket(struct hlist_head *list, struct sock *sk) spin_unlock(&unix_table_lock); } -static struct sock *__unix_find_socket_byname(struct sockaddr_un *sunname, +static struct sock *__unix_find_socket_byname(struct net *net, + struct sockaddr_un *sunname, int len, int type, unsigned hash) { struct sock *s; @@ -279,6 +280,9 @@ static struct sock *__unix_find_socket_byname(struct sockaddr_un *sunname, sk_for_each(s, node, &unix_socket_table[hash ^ type]) { struct unix_sock *u = unix_sk(s); + if (s->sk_net != net) + continue; + if (u->addr->len == len && !memcmp(u->addr->name, sunname, len)) goto found; @@ -288,21 +292,22 @@ found: return s; } -static inline struct sock *unix_find_socket_byname(struct sockaddr_un *sunname, +static inline struct sock *unix_find_socket_byname(struct net *net, + struct sockaddr_un *sunname, int len, int type, unsigned hash) { struct sock *s; spin_lock(&unix_table_lock); - s = __unix_find_socket_byname(sunname, len, type, hash); + s = __unix_find_socket_byname(net, sunname, len, type, hash); if (s) sock_hold(s); spin_unlock(&unix_table_lock); return s; } -static struct sock *unix_find_socket_byinode(struct inode *i) +static struct sock *unix_find_socket_byinode(struct net *net, struct inode *i) { struct sock *s; struct …
creator: ebiederm
date: 2007-09-29T01:08:05-00:00

Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Eric W. Biederman wrote: > void rtnl_unlock(void) > { > - mutex_unlock(&rtnl_mutex); > - if (rtnl && rtnl->sk_receive_queue.qlen) > + struct net *net; > + > + /* > + * Loop through all of the rtnl sockets until none of them (in > + * a live network namespace) have queue packets. > + * > + * We have to be careful with the locking here as > + * sk_data_ready aka rtnetlink_rcv takes the rtnl_mutex. > + * > + * To ensure the network namespace does not exit while > + * we are processing packets on it's rtnl socket we > + * grab a reference to the network namespace, ignoring > + * it if the network namespace has already exited. > + */ > +retry: > + for_each_net(net) { > + struct sock *rtnl = net->rtnl; > + > + if (!rtnl || !rtnl->sk_receive_queue.qlen) > + continue; > + > + if (!maybe_get_net(net)) > + continue; > + > + mutex_unlock(&rtnl_mutex); > rtnl->sk_data_ready(rtnl, 0); > + mutex_lock(&rtnl_mutex); > + put_net(net); > + goto retry; > + } > + mutex_unlock(&rtnl_mutex); > + > netdev_run_todo(); > } I'm wondering why this receive queue processing on unlock is still necessary today, we don't do trylock in rtnetlink_rcv anymore, so all senders will simply wait until the lock is released and then process the queue. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Patrick McHardy
date: 2007-09-29T15:44:35-00:00

Re: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
type: item
description: Eric W. Biederman wrote: > Because of the global nature of garbage collection, and because of the > cost of per namespace hash tables unix_socket_table has been kept > global. With a filter added on lookups so we don't see sockets from > the wrong namespace. > > Currently I don't fold the namesapce into the hash so multiple > namespaces using the same socket name will be guaranteed a hash > collision. That doesn't sound like a good thing :) Is there a reason for not avoiding the collisions? _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Patrick McHardy
date: 2007-09-29T15:47:56-00:00

Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Patrick McHardy <kaber@trash.net> writes: > I'm wondering why this receive queue processing on unlock is still > necessary today, we don't do trylock in rtnetlink_rcv anymore, so > all senders will simply wait until the lock is released and then > process the queue. Good question, I should probably look. I was lazy and didn't go back and audit why we were doing this. I just coded a routine that I was certain would work. It does appear that we are processing the queue with sk_data_read when we add a message, so this may be completely unnecessary. I will go back and look. If we can remove this bit things should be simpler. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-09-29T16:51:14-00:00

Re: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
type: item
description: Patrick McHardy <kaber@trash.net> writes: > Eric W. Biederman wrote: >> Because of the global nature of garbage collection, and because of the >> cost of per namespace hash tables unix_socket_table has been kept >> global. With a filter added on lookups so we don't see sockets from >> the wrong namespace. >> >> Currently I don't fold the namesapce into the hash so multiple >> namespaces using the same socket name will be guaranteed a hash >> collision. > > > That doesn't sound like a good thing :) Is there a reason for > not avoiding the collisions? Two reasons. Minimizing the size of the changes to make review easier, and I don't know if hash collisions are likely in practice or if they matter. I don't believe we can't physically collide and have the same inode because we make a node in the filesystem. The abstract domain is local to linux and so people don't use it as much. All of which boils down to. I don't see it matter a heck of a lot especially initially. So I did the traditional unix thing and started with a simple and stupid implementation. But it didn't quite feel right to me either so I documented it. Whipping up a patch to take the namespace into account in mkname doesn't look to hard though. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-09-29T17:03:05-00:00

Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Eric W. Biederman wrote: > Patrick McHardy <kaber@trash.net> writes: > > >>I'm wondering why this receive queue processing on unlock is still >>necessary today, we don't do trylock in rtnetlink_rcv anymore, so >>all senders will simply wait until the lock is released and then >>process the queue. > > > Good question, I should probably look. I was lazy and didn't go back > and audit why we were doing this. I just coded a routine that I was > certain would work. It does appear that we are processing the queue > with sk_data_read when we add a message, so this may be completely > unnecessary. I will go back and look. If we can remove this bit > things should be simpler. Maybe I can save you some time: we used to do down_trylock() for the rtnl mutex, so senders would simply return if someone else was already processing the queue *or* the rtnl was locked for some other reason. In the first case the process already processing the queue would also process the new messages, but if it the rtnl was locked for some other reason (for example during module registration) the message would sit in the queue until the next rtnetlink sendmsg call, which is why rtnl_unlock does queue processing. Commit 6756ae4b changed the down_trylock to mutex_lock, so senders will now simply wait until the mutex is released and then call netlink_run_queue themselves. This means its not needed anymore. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Patrick McHardy
date: 2007-09-29T17:48:03-00:00

Re: [PATCH 5/5] net: Make AF_UNIX per network namespace safe.
type: item
description: Eric W. Biederman wrote: > Patrick McHardy <kaber@trash.net> writes: > >>>Currently I don't fold the namesapce into the hash so multiple >>>namespaces using the same socket name will be guaranteed a hash >>>collision. >> >> >>That doesn't sound like a good thing :) Is there a reason for >>not avoiding the collisions? > > > Two reasons. Minimizing the size of the changes to make review > easier, and I don't know if hash collisions are likely in practice > or if they matter. I don't believe we can't physically collide and > have the same inode because we make a node in the filesystem. The > abstract domain is local to linux and so people don't use it as much. > > All of which boils down to. I don't see it matter a heck of a lot > especially initially. So I did the traditional unix thing and started > with a simple and stupid implementation. But it didn't quite feel > right to me either so I documented it. > > Whipping up a patch to take the namespace into account in mkname > doesn't look to hard though. It doesn't look like it would increase patch size significantly (about 4 more changed lines), but it could of course be done in a follow-up patch. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Patrick McHardy
date: 2007-09-29T17:50:14-00:00

Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Patrick McHardy <kaber@trash.net> writes: > Maybe I can save you some time: we used to do down_trylock() > for the rtnl mutex, so senders would simply return if someone > else was already processing the queue *or* the rtnl was locked > for some other reason. In the first case the process already > processing the queue would also process the new messages, but > if it the rtnl was locked for some other reason (for example > during module registration) the message would sit in the > queue until the next rtnetlink sendmsg call, which is why > rtnl_unlock does queue processing. Commit 6756ae4b changed > the down_trylock to mutex_lock, so senders will now simply wait > until the mutex is released and then call netlink_run_queue > themselves. This means its not needed anymore. Sounds reasonable. I started looking through the code paths and I currently cannot see anything that would leave a message on a kernel rtnl socket. However I did a quick test adding a WARN_ON if there were any messages found in the queue during rtnl_unlock and I found this code path getting invoked from linkwatch_event. So there is clearly something I don't understand, and it sounds at odds just a bit from your description. If we can remove the extra queue processing that would be great, as it looks like a nice way to simplify the locking and the special cases in the code. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-09-29T21:00:48-00:00

Re: Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Hmm, so it looks like we do not need this queue processing at all... Regards, Den Eric W. Biederman wrote: > Patrick McHardy <kaber@trash.net> writes: > >> Maybe I can save you some time: we used to do down_trylock() >> for the rtnl mutex, so senders would simply return if someone >> else was already processing the queue *or* the rtnl was locked >> for some other reason. In the first case the process already >> processing the queue would also process the new messages, but >> if it the rtnl was locked for some other reason (for example >> during module registration) the message would sit in the >> queue until the next rtnetlink sendmsg call, which is why >> rtnl_unlock does queue processing. Commit 6756ae4b changed >> the down_trylock to mutex_lock, so senders will now simply wait >> until the mutex is released and then call netlink_run_queue >> themselves. This means its not needed anymore. > > Sounds reasonable. > > I started looking through the code paths and I currently cannot > see anything that would leave a message on a kernel rtnl socket. > > However I did a quick test adding a WARN_ON if there were any messages > found in the queue during rtnl_unlock and I found this code path > getting invoked from linkwatch_event. So there is clearly something I > don't understand, and it sounds at odds just a bit from your > description. > > If we can remove the extra queue processing that would be great, > as it looks like a nice way to simplify the locking and the special > cases in the code. > > Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: dlunev
date: 2007-09-30T13:13:15-00:00

Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Eric W. Biederman wrote: > Patrick McHardy <kaber@trash.net> writes: > > >>Maybe I can save you some time: we used to do down_trylock() >>for the rtnl mutex, so senders would simply return if someone >>else was already processing the queue *or* the rtnl was locked >>for some other reason. In the first case the process already >>processing the queue would also process the new messages, but >>if it the rtnl was locked for some other reason (for example >>during module registration) the message would sit in the >>queue until the next rtnetlink sendmsg call, which is why >>rtnl_unlock does queue processing. Commit 6756ae4b changed >>the down_trylock to mutex_lock, so senders will now simply wait >>until the mutex is released and then call netlink_run_queue >>themselves. This means its not needed anymore. > > > Sounds reasonable. > > I started looking through the code paths and I currently cannot > see anything that would leave a message on a kernel rtnl socket. > > However I did a quick test adding a WARN_ON if there were any messages > found in the queue during rtnl_unlock and I found this code path > getting invoked from linkwatch_event. So there is clearly something I > don't understand, and it sounds at odds just a bit from your > description. That sounds like a bug. Did you place the WARN_ON before or after the mutex_unlock()? _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Patrick McHardy
date: 2007-09-30T15:39:54-00:00

Re: Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: Patrick McHardy wrote: > Eric W. Biederman wrote: >> Patrick McHardy <kaber@trash.net> writes: >> >> >>> Maybe I can save you some time: we used to do down_trylock() >>> for the rtnl mutex, so senders would simply return if someone >>> else was already processing the queue *or* the rtnl was locked >>> for some other reason. In the first case the process already >>> processing the queue would also process the new messages, but >>> if it the rtnl was locked for some other reason (for example >>> during module registration) the message would sit in the >>> queue until the next rtnetlink sendmsg call, which is why >>> rtnl_unlock does queue processing. Commit 6756ae4b changed >>> the down_trylock to mutex_lock, so senders will now simply wait >>> until the mutex is released and then call netlink_run_queue >>> themselves. This means its not needed anymore. >> >> Sounds reasonable. >> >> I started looking through the code paths and I currently cannot >> see anything that would leave a message on a kernel rtnl socket. >> >> However I did a quick test adding a WARN_ON if there were any messages >> found in the queue during rtnl_unlock and I found this code path >> getting invoked from linkwatch_event. So there is clearly something I >> don't understand, and it sounds at odds just a bit from your >> description. > > > That sounds like a bug. Did you place the WARN_ON before or after > the mutex_unlock()? The presence of the message in the queue during rtnl_unlock is quite possible as normal user->kernel message processing path for rtnl is the following: netlink_sendmsg netlink_unicast netlink_sendskb skb_queue_tail netlink_data_ready rtnetlink_rcv mutex_lock(&rtnl_mutex); netlink_run_queue(sk, qlen, &rtnetlink_rcv_msg); mutex_unlock(&rtnl_mutex); so, the presence of the packet in the rtnl queue on rtnl_unlock is normal race with a rtnetlink_rcv for me. Regards, Den _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org h…
creator: den
date: 2007-10-01T08:26:52-00:00

Re: Re: [PATCH 2/5] net: Make rtnetlink infrastructure network namespace aware
type: item
description: "Denis V. Lunev" <den@sw.ru> writes: > The presence of the message in the queue during rtnl_unlock is quite > possible as normal user->kernel message processing path for rtnl is the > following: > > netlink_sendmsg > netlink_unicast > netlink_sendskb > skb_queue_tail > netlink_data_ready > rtnetlink_rcv > mutex_lock(&rtnl_mutex); > netlink_run_queue(sk, qlen, &rtnetlink_rcv_msg); > mutex_unlock(&rtnl_mutex); > > so, the presence of the packet in the rtnl queue on rtnl_unlock is > normal race with a rtnetlink_rcv for me. Yes. That is what I saw in practice as well. Thanks for confirming this. It happened to reproducible because I had a dhcp client asking for a list of links in parallel with the actual link coming up during boot. Looking at netlink_unicast and netlink_broadcast I am generally convinced that we can remove the call of sk_data_ready in rtnl_unlock. I think those are the only two possible paths through there and I don't see how we could miss a processing a packet on the way through there. What would be nice is if we could figure out how to eliminate this race. As that would allow netlink packets to be processed synchronously and we could actually use current for security checks, and for getting the context of the calling process. Right now we are 99% of the way there but because of the above race the code must all be written as if netlink packets were coming in completely asynchronously. Which is unfortunate and a pain. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-10-01T08:45:07-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: Eric W. Biederman wrote: > Before I can enable rtnetlink to work in all network namespaces > I need to be certain that something won't break. So this > patch deliberately disables all of the rtnletlink methods in everything > except the initial network namespace. After the methods have been > audited this extra check can be disabled. > [...] > static int br_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > { > + struct net *net = skb->sk->sk_net; > struct net_device *dev; > int idx; > I've read some code today greping 'init_net.loopback_dev' and found interesting non-trivial for me issue. Network namespace is extracted from the packet in two different ways in TCP. This is a socket for outgoing path and a device for incoming. Though, there are some places called uniformly both from incoming and outgoing path. Typical example is netfilters. They are called uniformly all around the code. The prototype is the following: static unsigned int reject6_target(struct sk_buff **pskb, const struct net_device *in, const struct net_device *out, unsigned int hooknum, const struct xt_target *target, const void *targinfo); So, we are bound to the following options: - perform additional non-uniform hacks around to place 'struct net' into other and other structures like xt_target - add 7th parameter here and over - introduce an skb_net field in the 'struct sk_buff' making all code uniform, at least when we have an skb I think that this is not the last place with such a parameter list and we should make a decision at this point when the code in not mainline yet. As far as I understand, netfilters are not touched by the Eric and we can face some non-trivial problems there. So, if my point about uniformity is valid, this patchset looks wrong and should be re-worked :( Regards, Den
creator: den
date: 2007-10-10T12:33:50-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: Denis V. Lunev wrote: > Eric W. Biederman wrote: >> Before I can enable rtnetlink to work in all network namespaces >> I need to be certain that something won't break. So this >> patch deliberately disables all of the rtnletlink methods in everything >> except the initial network namespace. After the methods have been >> audited this extra check can be disabled. >> > [...] >> static int br_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) >> { >> + struct net *net = skb->sk->sk_net; >> struct net_device *dev; >> int idx; >> > > I've read some code today greping 'init_net.loopback_dev' and found > interesting non-trivial for me issue. > > Network namespace is extracted from the packet in two different ways in > TCP. This is a socket for outgoing path and a device for incoming. > Though, there are some places called uniformly both from incoming and > outgoing path. > > Typical example is netfilters. They are called uniformly all around the > code. The prototype is the following: > > static unsigned int reject6_target(struct sk_buff **pskb, > const struct net_device *in, > const struct net_device *out, > unsigned int hooknum, > const struct xt_target *target, > const void *targinfo); > Thanks Denis for auditing the code. As far as I see, struct net_device *in is NULL for outgoing traffic and struct net_device *out is NULL for ingress traffic. Except for the FORWARD rules where both are filled. If we are following network namespace semantic, we should not have two network devices belonging to two differents namespaces, right ? In this case, the following line of code should be sufficient to retrieve the network namespace, no ? struct net *net = in?in->nd_net:out->nd_net; > So, we are bound to the following options: > - perform additional non-uniform hacks around to place 'struct net' into > other and other structures like xt_target > - add 7th parameter here and over > - introduce an skb_net field in the 'struct sk_buff' making all code > uniform, at least w…
creator: Daniel Lezcano
date: 2007-10-10T14:05:26-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: Daniel Lezcano wrote: > struct net *net = in?in->nd_net:out->nd_net; > >> So, we are bound to the following options: >> - perform additional non-uniform hacks around to place 'struct net' into >> other and other structures like xt_target >> - add 7th parameter here and over >> - introduce an skb_net field in the 'struct sk_buff' making all code >> uniform, at least when we have an skb >> >> I think that this is not the last place with such a parameter list and >> we should make a decision at this point when the code in not mainline >> yet. >> >> As far as I understand, netfilters are not touched by the Eric and we >> can face some non-trivial problems there. > > In Eric's git tree: > http://git.kernel.org/?p=linux/kernel/git/ebiederm/linux-2.6-netns.git > > There are some modifications concerning > net/ipv4/netfiler/iptable_filter.c and at the ipt_hook function, there is: > > struct net *net = (in?in:out)->nd_net; > >> So, if my point about uniformity is valid, this patchset looks wrong and >> should be re-worked :( > > As Eric said, we want to build the network namespace step by step, > taking care of not breaking the init network namespace. > > If you want to make iptables per namespace or catch problems before the > code goes to Dave's tree, IMHO it will be more convenient to post to > containers@ the patches against netns49, where the modifications will be > in a network namespace big picture. > my point is somewhat another. Yes, this is enough for that place. If so, I must scatter these checks all around in the netfilters code. Brr. In forward chain the situation is different for Layer3 switching. Let's assume that we have an OpenVZ scheme, where the packet flows from socket to device and after that from device to device via forwarding path. You can't call skb_orphan on namespace switching as this breaks UDP flow regulation. Virtual network device is fast while real Ethernet is slow, packets will be dropped on queue in real device. So, the situation with packet…
creator: den
date: 2007-10-10T14:29:05-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: "Denis V. Lunev" <den@sw.ru> writes: > Eric W. Biederman wrote: >> Before I can enable rtnetlink to work in all network namespaces >> I need to be certain that something won't break. So this >> patch deliberately disables all of the rtnletlink methods in everything >> except the initial network namespace. After the methods have been >> audited this extra check can be disabled. >> > [...] >> static int br_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) >> { >> + struct net *net = skb->sk->sk_net; >> struct net_device *dev; >> int idx; >> > > I've read some code today greping 'init_net.loopback_dev' and found > interesting non-trivial for me issue. > > Network namespace is extracted from the packet in two different ways in > TCP. This is a socket for outgoing path and a device for incoming. > Though, there are some places called uniformly both from incoming and > outgoing path. > > Typical example is netfilters. They are called uniformly all around the > code. The prototype is the following: > > static unsigned int reject6_target(struct sk_buff **pskb, > const struct net_device *in, > const struct net_device *out, > unsigned int hooknum, > const struct xt_target *target, > const void *targinfo); > > So, we are bound to the following options: > - perform additional non-uniform hacks around to place 'struct net' into > other and other structures like xt_target > - add 7th parameter here and over > - introduce an skb_net field in the 'struct sk_buff' making all code > uniform, at least when we have an skb No. That bloats a sk_buff, changes the semantics of moving a skb around, and decreases performance (because we have to maintain the field on a fast path). There will not be a skb_net field. The entire concept of skb_net is a maintenance disaster. > I think that this is not the last place with such a parameter list and > we should make a decision at this point when the code in not mainline yet. Certainly that is what I have a proof of concept tree for. So w…
creator: ebiederm
date: 2007-10-10T19:37:04-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: Eric W. Biederman wrote: [..] >> - introduce an skb_net field in the 'struct sk_buff' making all code >> uniform, at least when we have an skb > > No. That bloats a sk_buff, changes the semantics of moving a skb > around, and decreases performance (because we have to maintain the > field on a fast path). > > There will not be a skb_net field. > > The entire concept of skb_net is a maintenance disaster. > >> I think that this is not the last place with such a parameter list and >> we should make a decision at this point when the code in not mainline yet. > > Certainly that is what I have a proof of concept tree for. So we can > see how these things look before we merge them. > >> As far as I understand, netfilters are not touched by the Eric and we >> can face some non-trivial problems there. > > No. In my proof of concept tree I should have working per network > namespace netfilter code. My intention was to just do enough to see > what the impact would be so most of the netfilter code (in my tree) > insists on running in the initial network namespace. But there are > a few pieces that are fully converted. Please take a look. > >> So, if my point about uniformity is valid, this patchset looks wrong and >> should be re-worked :( > > This patchset does need to get rebased on top of net-2.6.25 when it > opens and hopefully your patchset to remove the unnecessary work in > rtnl_unlock, and to really process netlink requests in process > context. I see a need for the more fundamental change you seem to > be advocating. I see that current patchset of RTNL code is attached. I'll start the next piece of work next week after some reaction from people. [..] > In the particular case of the netfilter hooks we don't have a > network namespace parameter laying around before we call NF_HOOK, > and the idiom "net = (in?in:out)->nd_net" seems perfectly accurate > so it seems reasonable to me to derive the network namespace that > way in generic code. Although thinking about this. We …
creator: den
date: 2007-10-11T06:35:58-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: "Denis V. Lunev" <den@sw.ru> writes: >> This patchset does need to get rebased on top of net-2.6.25 when it >> opens and hopefully your patchset to remove the unnecessary work in >> rtnl_unlock, and to really process netlink requests in process >> context. I see a need for the more fundamental change you seem to >> be advocating. Grr. That last sentence should have been I do not see a need for the more fundamental change you seem to be advocating. > I see that current patchset of RTNL code is attached. I'll start the > next piece of work next week after some reaction from people. > I understand your position. But still have some points :) > > First. A real-life usecase we have recently fixed in the OpenVZ. I have > described it in the previous post, but (may be) not in great details. > > UDP buffers management for outgoing traffic. > The packet carries over a destructor, which is called in skb_orphan in > the real networking device. This destructor allows to queue more > packets. There is no problem in the current implementation. But there is > one after namespace introduction in the following configuration: > [NS1] <-> [NS2] <-> [world] > There are two namespaces on the host. One of them is connected to the > outside world via another and the packet follows usual forwarding path > in NS2. Yes. We can't quite do that today because of the missing ipv4 support, but the basic infrastructure exists to describe this is already merged. > The problem: if we call skb_orphan in [NS1] outgoing device, there is a > great possibility to have a really huge packet drop in [NS2] on queuing > operation. Yes. Removing skb_orphan from veth to solve this looks like a pretty substantial hack. A clean solution is more likely to resemble ethernet pause frames so we temporarily plug the virtual device. Although there are issues with that as currently virtual devices don't have queues. > In OpenVZ we have added skb_orphan into receive path (tcp_v4_rcv, > __udp4_lib_rcv etc) and removed one…
creator: ebiederm
date: 2007-10-11T07:57:11-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: Eric W. Biederman wrote: > "Denis V. Lunev" <den@sw.ru> writes: > >>> This patchset does need to get rebased on top of net-2.6.25 when it >>> opens and hopefully your patchset to remove the unnecessary work in >>> rtnl_unlock, and to really process netlink requests in process >>> context. I see a need for the more fundamental change you seem to >>> be advocating. > > Grr. That last sentence should have been I do not see a need for the more > fundamental change you seem to be advocating. why? >> First. A real-life usecase we have recently fixed in the OpenVZ. I have >> described it in the previous post, but (may be) not in great details. >> >> UDP buffers management for outgoing traffic. >> The packet carries over a destructor, which is called in skb_orphan in >> the real networking device. This destructor allows to queue more >> packets. There is no problem in the current implementation. But there is >> one after namespace introduction in the following configuration: >> [NS1] <-> [NS2] <-> [world] >> There are two namespaces on the host. One of them is connected to the >> outside world via another and the packet follows usual forwarding path >> in NS2. > > Yes. We can't quite do that today because of the missing ipv4 support, > but the basic infrastructure exists to describe this is already > merged. > >> The problem: if we call skb_orphan in [NS1] outgoing device, there is a >> great possibility to have a really huge packet drop in [NS2] on queuing >> operation. > > Yes. Removing skb_orphan from veth to solve this looks like a > pretty substantial hack. A clean solution is more likely to resemble > ethernet pause frames so we temporarily plug the virtual device. > Although there are issues with that as currently virtual devices > don't have queues. > >> In OpenVZ we have added skb_orphan into receive path (tcp_v4_rcv, >> __udp4_lib_rcv etc) and removed one from our virtual devices. As >> unfortunate side effect we have packet in NS2 with a socket from NS1. > > That…
creator: den
date: 2007-10-11T12:17:19-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: "Denis V. Lunev" <den@sw.ru> writes: >> Grr. That last sentence should have been I do not see a need for the more >> fundamental change you seem to be advocating. > > why? Because I do not see the need. There are certainly details that can be improved, but you seem to be talking about ripping everything out, ignoring the reviews and the acceptance that has happened and starting from scratch all over again. I don't see the need to start again from scratch. > veth is not mainly affected, if it is connected to a bridge there will > be no problem. You are talking about decision to switch/choose VE on > level 2, I am told about layer 3, i.e. at the moment of routing/based on > IP. So, > > I do not understand how this will help me in my usecase. The macvlan > device transmit the packet into real device. OK. But it does not help if > I want to setup router in one namespace for another. > > Basically, queue stop will not help, as routing namespace can have two > interfaces, one fast and one slow. In your case we should stop accepting > the message based on the slowest one? You are proposing a solution that only works for namespaces, while the problem continues to exist in all other routing cases. Solving this in a namespace specific way seems to reduce the value of using namespaces for testing weird aspects of the networking stack. And except for OpenVZ this is not a common case for anyone yet, so it seems a seriously premature optimization. Given that it could only be one path having something like ECN for UDP to slow the transmitter down would be nice. The point of the macvlan example is that I believe that will be our common case when things network namespaces are deployed in the real world. Both because macvlans are easier to setup then routing or bridging, and because they perform better. Further this problem feels like an application bug to me. Going from a fast to a slow network when you are saturating the fast network with UDP packets will cause packets to be droppe…
creator: ebiederm
date: 2007-10-11T17:08:51-00:00

Re: [PATCH 1/5] net: Modify all rtnetlink methods to only work in the initial namespace
type: item
description: > If to achieve the above I have to use a solution that distresses > the embedded people my apologies. Sounds good for me :) Regards, Den
creator: dlunev
date: 2007-10-11T17:26:55-00:00