[PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

Subsystems: networking [general], networking [ipv4/ipv6], the rest

STALE5207d

7 messages, 4 authors, 2012-06-06 · open the first message on its own page

[PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: Gao feng <hidden>
Date: 2012-06-05 07:52:17

now inetpeer doesn't support namespace,the information will
be leaking across namespace.

this patch move the global vars v4_peers and v6_peers to
netns_ipv4 and netns_ipv6 as a field peers.

add struct pernet_operations inetpeer_ops to initial pernet
inetpeer data.

and change family_to_base and inet_getpeer to support namespace.

Signed-off-by: Gao feng <redacted>
---
 include/net/inetpeer.h   |   10 ++++---
 include/net/netns/ipv4.h |    1 +
 include/net/netns/ipv6.h |    1 +
 net/ipv4/inetpeer.c      |   68 +++++++++++++++++++++++++++++++++------------
 net/ipv4/route.c         |    2 +-
 5 files changed, 59 insertions(+), 23 deletions(-)
diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index b94765e..4a50449 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -72,7 +72,9 @@ static inline bool inet_metrics_new(const struct inet_peer *p)
 }
 
 /* can be called with or without local BH being disabled */
-struct inet_peer	*inet_getpeer(const struct inetpeer_addr *daddr, int create);
+struct inet_peer *inet_getpeer(struct net *net,
+			       const struct inetpeer_addr *daddr,
+			       int create);
 
 static inline struct inet_peer *inet_getpeer_v4(__be32 v4daddr, int create)
 {
@@ -80,7 +82,7 @@ static inline struct inet_peer *inet_getpeer_v4(__be32 v4daddr, int create)
 
 	daddr.addr.a4 = v4daddr;
 	daddr.family = AF_INET;
-	return inet_getpeer(&daddr, create);
+	return inet_getpeer(&init_net, &daddr, create);
 }
 
 static inline struct inet_peer *inet_getpeer_v6(const struct in6_addr *v6daddr, int create)
@@ -89,14 +91,14 @@ static inline struct inet_peer *inet_getpeer_v6(const struct in6_addr *v6daddr,
 
 	*(struct in6_addr *)daddr.addr.a6 = *v6daddr;
 	daddr.family = AF_INET6;
-	return inet_getpeer(&daddr, create);
+	return inet_getpeer(&init_net, &daddr, create);
 }
 
 /* can be called from BH context or outside */
 extern void inet_putpeer(struct inet_peer *p);
 extern bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout);
 
-extern void inetpeer_invalidate_tree(int family);
+extern void inetpeer_invalidate_tree(struct net *net, int family);
 
 /*
  * temporary check to make sure we dont access rid, ip_id_count, tcp_ts,
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index bbd023a..0855e09 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -31,6 +31,7 @@ struct netns_ipv4 {
 	struct sock		**icmp_sk;
 	struct sock		*tcp_sock;
 
+	struct inet_peer_base	*peers;
 	struct netns_frags	frags;
 #ifdef CONFIG_NETFILTER
 	struct xt_table		*iptable_filter;
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index b42be53..df0a545 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -33,6 +33,7 @@ struct netns_ipv6 {
 	struct netns_sysctl_ipv6 sysctl;
 	struct ipv6_devconf	*devconf_all;
 	struct ipv6_devconf	*devconf_dflt;
+	struct inet_peer_base	*peers;
 	struct netns_frags	frags;
 #ifdef CONFIG_NETFILTER
 	struct xt_table		*ip6table_filter;
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index d4d61b6..0dbf1c8 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -88,18 +88,6 @@ struct inet_peer_base {
 	int		total;
 };
 
-static struct inet_peer_base v4_peers = {
-	.root		= peer_avl_empty_rcu,
-	.lock		= __SEQLOCK_UNLOCKED(v4_peers.lock),
-	.total		= 0,
-};
-
-static struct inet_peer_base v6_peers = {
-	.root		= peer_avl_empty_rcu,
-	.lock		= __SEQLOCK_UNLOCKED(v6_peers.lock),
-	.total		= 0,
-};
-
 #define PEER_MAXDEPTH 40 /* sufficient for about 2^27 nodes */
 
 /* Exported for sysctl_net_ipv4.  */
@@ -153,6 +141,45 @@ static void inetpeer_gc_worker(struct work_struct *work)
 	schedule_delayed_work(&gc_work, gc_delay);
 }
 
+static int __net_init inetpeer_net_init(struct net *net)
+{
+
+	net->ipv4.peers = kzalloc(sizeof(struct inet_peer_base),
+				  GFP_KERNEL);
+	if (net->ipv4.peers == NULL)
+		return -ENOMEM;
+
+	net->ipv4.peers->root = peer_avl_empty_rcu;
+	seqlock_init(&net->ipv4.peers->lock);
+
+	net->ipv6.peers = kzalloc(sizeof(struct inet_peer_base),
+				  GFP_KERNEL);
+	if (net->ipv6.peers == NULL)
+		goto out_ipv6;
+
+	net->ipv6.peers->root = peer_avl_empty_rcu;
+	seqlock_init(&net->ipv6.peers->lock);
+
+	return 0;
+out_ipv6:
+	kfree(net->ipv4.peers);
+	return -ENOMEM;
+}
+
+static void __net_exit inetpeer_net_exit(struct net *net)
+{
+	inetpeer_invalidate_tree(net, AF_INET);
+	kfree(net->ipv4.peers);
+
+	inetpeer_invalidate_tree(net, AF_INET6);
+	kfree(net->ipv6.peers);
+}
+
+static struct pernet_operations inetpeer_ops = {
+	.init = inetpeer_net_init,
+	.exit = inetpeer_net_exit,
+};
+
 /* Called from ip_output.c:ip_init  */
 void __init inet_initpeers(void)
 {
@@ -177,6 +204,7 @@ void __init inet_initpeers(void)
 			NULL);
 
 	INIT_DELAYED_WORK_DEFERRABLE(&gc_work, inetpeer_gc_worker);
+	register_pernet_subsys(&inetpeer_ops);
 }
 
 static int addr_compare(const struct inetpeer_addr *a,
@@ -401,9 +429,10 @@ static void unlink_from_pool(struct inet_peer *p, struct inet_peer_base *base,
 	call_rcu(&p->rcu, inetpeer_free_rcu);
 }
 
-static struct inet_peer_base *family_to_base(int family)
+static struct inet_peer_base *family_to_base(struct net *net,
+					     int family)
 {
-	return family == AF_INET ? &v4_peers : &v6_peers;
+	return family == AF_INET ? net->ipv4.peers : net->ipv6.peers;
 }
 
 /* perform garbage collect on all items stacked during a lookup */
@@ -443,10 +472,12 @@ static int inet_peer_gc(struct inet_peer_base *base,
 	return cnt;
 }
 
-struct inet_peer *inet_getpeer(const struct inetpeer_addr *daddr, int create)
+struct inet_peer *inet_getpeer(struct net *net,
+			       const struct inetpeer_addr *daddr,
+			       int create)
 {
 	struct inet_peer __rcu **stack[PEER_MAXDEPTH], ***stackptr;
-	struct inet_peer_base *base = family_to_base(daddr->family);
+	struct inet_peer_base *base = family_to_base(net, daddr->family);
 	struct inet_peer *p;
 	unsigned int sequence;
 	int invalidated, gccnt = 0;
@@ -560,10 +591,10 @@ bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout)
 }
 EXPORT_SYMBOL(inet_peer_xrlim_allow);
 
-void inetpeer_invalidate_tree(int family)
+void inetpeer_invalidate_tree(struct net *net, int family)
 {
 	struct inet_peer *old, *new, *prev;
-	struct inet_peer_base *base = family_to_base(family);
+	struct inet_peer_base *base = family_to_base(net, family);
 
 	write_seqlock_bh(&base->lock);
 
@@ -586,3 +617,3 @@ out:
 	write_sequnlock_bh(&base->lock);
 }
 EXPORT_SYMBOL(inetpeer_invalidate_tree);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index ffcb3b0..e5b18b8 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -938,7 +938,7 @@ static void rt_cache_invalidate(struct net *net)
 
 	get_random_bytes(&shuffle, sizeof(shuffle));
 	atomic_add(shuffle + 1U, &net->ipv4.rt_genid);
-	inetpeer_invalidate_tree(AF_INET);
+	inetpeer_invalidate_tree(net, AF_INET);
 }
 
 /*
-- 
1.7.7.6

Re: [PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: Eric Dumazet <hidden>
Date: 2012-06-05 08:57:12

On Tue, 2012-06-05 at 15:52 +0800, Gao feng wrote:
+static void __net_exit inetpeer_net_exit(struct net *net)
+{
+	inetpeer_invalidate_tree(net, AF_INET);
+	kfree(net->ipv4.peers);
+
+	inetpeer_invalidate_tree(net, AF_INET6);
+	kfree(net->ipv6.peers);
+}
+
Are we 1000% sure no code ever run in inetpeer land after this call ?

I would add
	net->ipv4.peers = NULL;
	net->ipv6.peers = NULL;

to catch NULL deref instead of strange errors, just in case.

By the way, I think we have a bug in inetpeer_gc_worker()

Steffen ?

We have no rcu grace period to make sure the following is safe :

if (!atomic_read(&p->refcnt)) {
	list_del(&p->gc_list);
	kmem_cache_free(peer_cachep, p);
}

I'll post a fix like :
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index d4d61b6..07731b5 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -137,7 +137,7 @@ static void inetpeer_gc_worker(struct work_struct *work)
 
 		n = list_entry(p->gc_list.next, struct inet_peer, gc_list);
 
-		if (!atomic_read(&p->refcnt)) {
+		if (atomic_cmpxchg(&p->refcnt, 0, -1) == 0) {
 			list_del(&p->gc_list);
 			kmem_cache_free(peer_cachep, p);
 		}

Re: [PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: Steffen Klassert <hidden>
Date: 2012-06-05 11:27:28

On Tue, Jun 05, 2012 at 10:57:06AM +0200, Eric Dumazet wrote:
On Tue, 2012-06-05 at 15:52 +0800, Gao feng wrote:
quoted
+static void __net_exit inetpeer_net_exit(struct net *net)
+{
+	inetpeer_invalidate_tree(net, AF_INET);
+	kfree(net->ipv4.peers);
+
+	inetpeer_invalidate_tree(net, AF_INET6);
+	kfree(net->ipv6.peers);
+}
+
Are we 1000% sure no code ever run in inetpeer land after this call ?

I would add
	net->ipv4.peers = NULL;
	net->ipv6.peers = NULL;

to catch NULL deref instead of strange errors, just in case.
I thought about that too, and I'm not absolutely sure.
The rest of this patch looks ok to me.
By the way, I think we have a bug in inetpeer_gc_worker()

Steffen ?

We have no rcu grace period to make sure the following is safe :

if (!atomic_read(&p->refcnt)) {
	list_del(&p->gc_list);
	kmem_cache_free(peer_cachep, p);
}
I think this is ok as it is. inetpeer_invalidate_tree()
unlinks the whole inetpeer tree from the inetpeer base and
adds it to a gc_list. These intetpeer entries are stale,
they can't be looked up again. So noone should increment the
refcount, they just wait until the refcount get zero.

Re: [PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: Eric Dumazet <hidden>
Date: 2012-06-05 12:00:35

On Tue, 2012-06-05 at 13:27 +0200, Steffen Klassert wrote:
quoted
By the way, I think we have a bug in inetpeer_gc_worker()

Steffen ?

We have no rcu grace period to make sure the following is safe :

if (!atomic_read(&p->refcnt)) {
	list_del(&p->gc_list);
	kmem_cache_free(peer_cachep, p);
}
I think this is ok as it is. inetpeer_invalidate_tree()
unlinks the whole inetpeer tree from the inetpeer base and
adds it to a gc_list. These intetpeer entries are stale,
they can't be looked up again. So noone should increment the
refcount, they just wait until the refcount get zero.
Its not OK, lookups are done under rcu.

Since there is no RCU grace period, the worker free the entries while
another cpus are doing their lookups.

Alternative would be to wait a RCU grace period before feeding them to
worker.

Re: [PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: Steffen Klassert <hidden>
Date: 2012-06-05 12:15:10

On Tue, Jun 05, 2012 at 02:00:30PM +0200, Eric Dumazet wrote:
On Tue, 2012-06-05 at 13:27 +0200, Steffen Klassert wrote:
quoted
quoted
By the way, I think we have a bug in inetpeer_gc_worker()

Steffen ?

We have no rcu grace period to make sure the following is safe :

if (!atomic_read(&p->refcnt)) {
	list_del(&p->gc_list);
	kmem_cache_free(peer_cachep, p);
}
I think this is ok as it is. inetpeer_invalidate_tree()
unlinks the whole inetpeer tree from the inetpeer base and
adds it to a gc_list. These intetpeer entries are stale,
they can't be looked up again. So noone should increment the
refcount, they just wait until the refcount get zero.
Its not OK, lookups are done under rcu.

Since there is no RCU grace period, the worker free the entries while
another cpus are doing their lookups.
Lookups are done under rcu, yes. But a lookup will not find
these stale entries because the whole interpeer tree is removed
from the inetpeer base before the worker is scheduled. A lookup
would have to cerate a new inetpeer entry in this case.

Re: [PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: Gao feng <hidden>
Date: 2012-06-05 12:29:35

于 2012年06月05日 16:57, Eric Dumazet 写道:
On Tue, 2012-06-05 at 15:52 +0800, Gao feng wrote:
quoted
+static void __net_exit inetpeer_net_exit(struct net *net)
+{
+	inetpeer_invalidate_tree(net, AF_INET);
+	kfree(net->ipv4.peers);
+
+	inetpeer_invalidate_tree(net, AF_INET6);
+	kfree(net->ipv6.peers);
+}
+
Are we 1000% sure no code ever run in inetpeer land after this call ?
I am not sure,I need more time to research it.
I just do kfree peers here without set NULL pointer is
beacuse there is the same code with fib6_main_tbl in fib6_net_exit
and it seems work well.

Anyway, I will research it.
quoted hunk
I would add
	net->ipv4.peers = NULL;
	net->ipv6.peers = NULL;

to catch NULL deref instead of strange errors, just in case.

By the way, I think we have a bug in inetpeer_gc_worker()

Steffen ?

We have no rcu grace period to make sure the following is safe :

if (!atomic_read(&p->refcnt)) {
	list_del(&p->gc_list);
	kmem_cache_free(peer_cachep, p);
}

I'll post a fix like :
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index d4d61b6..07731b5 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -137,7 +137,7 @@ static void inetpeer_gc_worker(struct work_struct *work)
 
 		n = list_entry(p->gc_list.next, struct inet_peer, gc_list);
 
-		if (!atomic_read(&p->refcnt)) {
+		if (atomic_cmpxchg(&p->refcnt, 0, -1) == 0) {
 			list_del(&p->gc_list);
 			kmem_cache_free(peer_cachep, p);
 		}



--
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
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH net-next v2 1/2] inetpeer: add namespace support for inetpeer

From: David Miller <davem@davemloft.net>
Date: 2012-06-06 17:43:24

From: Gao feng <redacted>
Date: Tue, 5 Jun 2012 15:52:17 +0800
now inetpeer doesn't support namespace,the information will
be leaking across namespace.

this patch move the global vars v4_peers and v6_peers to
netns_ipv4 and netns_ipv6 as a field peers.

add struct pernet_operations inetpeer_ops to initial pernet
inetpeer data.

and change family_to_base and inet_getpeer to support namespace.

Signed-off-by: Gao feng <redacted>
As stated yesterday we have to move the inetpeer tree roots into
the FIB rule entries to fix other bugs, and that as a result will
transparently fix this problem too.

So I'm dropping these two patches and will work on the mentioned
approach to this fix.

Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help