RE: [PATCH net-next 0/6] tcp_metrics: Network namespace bloat reduction v3

RE: [PATCH net-next 0/6] tcp_metrics: Network namespace bloat reduction v3

[Andrew Vagin]

Hello Eric,

We use deref_locked_genl(), but &genl_mutex isn't hold on this path.

[   13.215863] ===============================
[   13.216562] [ INFO: suspicious RCU usage. ]
[   13.217433] 4.0.0-rc3-next-20150316 #186 Not tainted
[   13.218550] -------------------------------
[   13.219496] net/ipv4/tcp_metrics.c:1060 suspicious rcu_dereference_protected() usage!
[   13.221213] other info that might help us debug this:
[   13.223050] rcu_scheduler_active = 1, debug_locks = 0
[   13.224491] 4 locks held by kworker/u8:2/62:
[   13.225461]  #0:  ("%s""netns"){.+.+.+}, at: [<ffffffff8109d9be>] process_one_work+0x15e/0x520
[   13.227600]  #1:  (net_cleanup_work){+.+.+.}, at: [<ffffffff8109d9be>] process_one_work+0x15e/0x520
[   13.229979]  #2:  (net_mutex){+.+.+.}, at: [<ffffffff8162cddc>] cleanup_net+0x8c/0x240
[   13.231858]  #3:  (tcp_metrics_lock){+.....}, at: [<ffffffff816ac14c>] tcp_metrics_flush_all+0x4c/0x170
[   13.234059] stack backtrace:
[   13.234975] CPU: 2 PID: 62 Comm: kworker/u8:2 Not tainted 4.0.0-rc3-next-20150316 #186
[   13.236689] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   13.237972] Workqueue: netns cleanup_net
[   13.238943]  0000000000000000 0000000002af7693 ffff880036d73c58 ffffffff8176a03d
[   13.240746]  0000000000000000 ffff88011a4d8000 ffff880036d73c88 ffffffff810d2867
[   13.242434]  ffff88011aa28000 ffff88011aa28000 ffff8800db620000 ffff8800db640000
[   13.244104] Call Trace:
[   13.244613]  [<ffffffff8176a03d>] dump_stack+0x45/0x57
[   13.245693]  [<ffffffff810d2867>] lockdep_rcu_suspicious+0xe7/0x120
[   13.247021]  [<ffffffff816ac264>] tcp_metrics_flush_all+0x164/0x170
[   13.248346]  [<ffffffff816ac27e>] tcp_net_metrics_exit+0xe/0x10
[   13.249619]  [<ffffffff8162bc49>] ops_exit_list.isra.4+0x39/0x60
[   13.250842]  [<ffffffff8162ceb0>] cleanup_net+0x160/0x240
[   13.251960]  [<ffffffff8109da2c>] process_one_work+0x1cc/0x520
[   13.253142]  [<ffffffff8109d9be>] ? process_one_work+0x15e/0x520
[   13.254411]  [<ffffffff8109e0db>] worker_thread+0x4b/0x470
[   13.255591]  [<ffffffff8109e090>] ? rescuer_thread+0x310/0x310
[   13.256753]  [<ffffffff8109e090>] ? rescuer_thread+0x310/0x310
[   13.257922]  [<ffffffff810a3a43>] kthread+0xf3/0x110
[   13.258948]  [<ffffffff810a3950>] ? kthread_create_on_node+0x240/0x240
[   13.260240]  [<ffffffff81774023>] ret_from_fork+0x53/0xb0
[   13.261272]  [<ffffffff810a3950>] ? kthread_create_on_node+0x240/0x240

[PATCH net-next] tcp_metrics: fix wrong lockdep annotations

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Changes in tcp_metric hash table are protected by tcp_metrics_lock
only, not by genl_mutex

Reported-by: Andrew Vagin <avagin@parallels.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 098a697b497e ("tcp_metrics: Use a single hash table for all network namespaces.")
---
 net/ipv4/tcp_metrics.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 366728cbee4a692394696f13e6aacf0331990eed..c7d34db9cfc99d85f6d21cf5bd7e718e80e404e7 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -1040,11 +1040,8 @@ out_free:
 	return ret;
 }
 
-#define deref_locked_genl(p)	\
-	rcu_dereference_protected(p, lockdep_genl_is_held() && \
-				     lockdep_is_held(&tcp_metrics_lock))
-
-#define deref_genl(p)	rcu_dereference_protected(p, lockdep_genl_is_held())
+#define deref_locked(p)	\
+	rcu_dereference_protected(p, lockdep_is_held(&tcp_metrics_lock))
 
 static void tcp_metrics_flush_all(struct net *net)
 {
@@ -1057,8 +1054,7 @@ static void tcp_metrics_flush_all(struct net *net)
 		struct tcp_metrics_block __rcu **pp;
 		spin_lock_bh(&tcp_metrics_lock);
 		pp = &hb->chain;
-		for (tm = deref_locked_genl(*pp); tm;
-		     tm = deref_locked_genl(*pp)) {
+		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
 			if (net_eq(tm_net(tm), net)) {
 				*pp = tm->tcpm_next;
 				kfree_rcu(tm, rcu_head);
@@ -1097,7 +1093,7 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	hb = tcp_metrics_hash + hash;
 	pp = &hb->chain;
 	spin_lock_bh(&tcp_metrics_lock);
-	for (tm = deref_locked_genl(*pp); tm; tm = deref_locked_genl(*pp)) {
+	for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
 		if (addr_same(&tm->tcpm_daddr, &daddr) &&
 		    (!src || addr_same(&tm->tcpm_saddr, &saddr)) &&
 		    net_eq(tm_net(tm), net)) {

Re: [PATCH net-next] tcp_metrics: fix wrong lockdep annotations

[Eric Dumazet]

On Mon, 2015-03-16 at 06:58 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Changes in tcp_metric hash table are protected by tcp_metrics_lock
> only, not by genl_mutex
> 
> Reported-by: Andrew Vagin <avagin@parallels.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 098a697b497e ("tcp_metrics: Use a single hash table for all network namespaces.")
> ---
>  net/ipv4/tcp_metrics.c |   12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)

I'll send a v2 : We also can use deref_locked() in tcp_new() to avoid
unnecessary barrier (as they are implied in rcu_dereference())
 

[PATCH v2 net-next] tcp_metrics: fix wrong lockdep annotations

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Changes in tcp_metric hash table are protected by tcp_metrics_lock
only, not by genl_mutex

While we are at it use deref_locked() instead of rcu_dereference()
in tcp_new() to avoid unnecessary barrier, as we hold tcp_metrics_lock
as well.
 
Reported-by: Andrew Vagin <avagin@parallels.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 098a697b497e ("tcp_metrics: Use a single hash table for all network namespaces.")
---
 net/ipv4/tcp_metrics.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 366728cbee4a692394696f13e6aacf0331990eed..5bef3513af77afc8943d99a107f74155b9979aaa 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -152,6 +152,9 @@ static void tcpm_check_stamp(struct tcp_metrics_block *tm, struct dst_entry *dst
 #define TCP_METRICS_RECLAIM_DEPTH	5
 #define TCP_METRICS_RECLAIM_PTR		(struct tcp_metrics_block *) 0x1UL
 
+#define deref_locked(p)	\
+	rcu_dereference_protected(p, lockdep_is_held(&tcp_metrics_lock))
+
 static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 					  struct inetpeer_addr *saddr,
 					  struct inetpeer_addr *daddr,
@@ -180,9 +183,9 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
 	if (unlikely(reclaim)) {
 		struct tcp_metrics_block *oldest;
 
-		oldest = rcu_dereference(tcp_metrics_hash[hash].chain);
-		for (tm = rcu_dereference(oldest->tcpm_next); tm;
-		     tm = rcu_dereference(tm->tcpm_next)) {
+		oldest = deref_locked(tcp_metrics_hash[hash].chain);
+		for (tm = deref_locked(oldest->tcpm_next); tm;
+		     tm = deref_locked(tm->tcpm_next)) {
 			if (time_before(tm->tcpm_stamp, oldest->tcpm_stamp))
 				oldest = tm;
 		}
@@ -1040,12 +1043,6 @@ out_free:
 	return ret;
 }
 
-#define deref_locked_genl(p)	\
-	rcu_dereference_protected(p, lockdep_genl_is_held() && \
-				     lockdep_is_held(&tcp_metrics_lock))
-
-#define deref_genl(p)	rcu_dereference_protected(p, lockdep_genl_is_held())
-
 static void tcp_metrics_flush_all(struct net *net)
 {
 	unsigned int max_rows = 1U << tcp_metrics_hash_log;
@@ -1057,8 +1054,7 @@ static void tcp_metrics_flush_all(struct net *net)
 		struct tcp_metrics_block __rcu **pp;
 		spin_lock_bh(&tcp_metrics_lock);
 		pp = &hb->chain;
-		for (tm = deref_locked_genl(*pp); tm;
-		     tm = deref_locked_genl(*pp)) {
+		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
 			if (net_eq(tm_net(tm), net)) {
 				*pp = tm->tcpm_next;
 				kfree_rcu(tm, rcu_head);
@@ -1097,7 +1093,7 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	hb = tcp_metrics_hash + hash;
 	pp = &hb->chain;
 	spin_lock_bh(&tcp_metrics_lock);
-	for (tm = deref_locked_genl(*pp); tm; tm = deref_locked_genl(*pp)) {
+	for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
 		if (addr_same(&tm->tcpm_daddr, &daddr) &&
 		    (!src || addr_same(&tm->tcpm_saddr, &saddr)) &&
 		    net_eq(tm_net(tm), net)) {

Re: [PATCH v2 net-next] tcp_metrics: fix wrong lockdep annotations

[Eric W. Biederman]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> From: Eric Dumazet <edumazet@google.com>
>
> Changes in tcp_metric hash table are protected by tcp_metrics_lock
> only, not by genl_mutex
>
> While we are at it use deref_locked() instead of rcu_dereference()
> in tcp_new() to avoid unnecessary barrier, as we hold tcp_metrics_lock
> as well.

Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

Good catch, and good improvement.  Thanks for catching and correcting
this.


> Reported-by: Andrew Vagin <avagin@parallels.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 098a697b497e ("tcp_metrics: Use a single hash table for all network namespaces.")
> ---
>  net/ipv4/tcp_metrics.c |   20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 366728cbee4a692394696f13e6aacf0331990eed..5bef3513af77afc8943d99a107f74155b9979aaa 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -152,6 +152,9 @@ static void tcpm_check_stamp(struct tcp_metrics_block *tm, struct dst_entry *dst
>  #define TCP_METRICS_RECLAIM_DEPTH	5
>  #define TCP_METRICS_RECLAIM_PTR		(struct tcp_metrics_block *) 0x1UL
>  
> +#define deref_locked(p)	\
> +	rcu_dereference_protected(p, lockdep_is_held(&tcp_metrics_lock))
> +
>  static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
>  					  struct inetpeer_addr *saddr,
>  					  struct inetpeer_addr *daddr,
> @@ -180,9 +183,9 @@ static struct tcp_metrics_block *tcpm_new(struct dst_entry *dst,
>  	if (unlikely(reclaim)) {
>  		struct tcp_metrics_block *oldest;
>  
> -		oldest = rcu_dereference(tcp_metrics_hash[hash].chain);
> -		for (tm = rcu_dereference(oldest->tcpm_next); tm;
> -		     tm = rcu_dereference(tm->tcpm_next)) {
> +		oldest = deref_locked(tcp_metrics_hash[hash].chain);
> +		for (tm = deref_locked(oldest->tcpm_next); tm;
> +		     tm = deref_locked(tm->tcpm_next)) {
>  			if (time_before(tm->tcpm_stamp, oldest->tcpm_stamp))
>  				oldest = tm;
>  		}
> @@ -1040,12 +1043,6 @@ out_free:
>  	return ret;
>  }
>  
> -#define deref_locked_genl(p)	\
> -	rcu_dereference_protected(p, lockdep_genl_is_held() && \
> -				     lockdep_is_held(&tcp_metrics_lock))
> -
> -#define deref_genl(p)	rcu_dereference_protected(p, lockdep_genl_is_held())
> -
>  static void tcp_metrics_flush_all(struct net *net)
>  {
>  	unsigned int max_rows = 1U << tcp_metrics_hash_log;
> @@ -1057,8 +1054,7 @@ static void tcp_metrics_flush_all(struct net *net)
>  		struct tcp_metrics_block __rcu **pp;
>  		spin_lock_bh(&tcp_metrics_lock);
>  		pp = &hb->chain;
> -		for (tm = deref_locked_genl(*pp); tm;
> -		     tm = deref_locked_genl(*pp)) {
> +		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
>  			if (net_eq(tm_net(tm), net)) {
>  				*pp = tm->tcpm_next;
>  				kfree_rcu(tm, rcu_head);
> @@ -1097,7 +1093,7 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
>  	hb = tcp_metrics_hash + hash;
>  	pp = &hb->chain;
>  	spin_lock_bh(&tcp_metrics_lock);
> -	for (tm = deref_locked_genl(*pp); tm; tm = deref_locked_genl(*pp)) {
> +	for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
>  		if (addr_same(&tm->tcpm_daddr, &daddr) &&
>  		    (!src || addr_same(&tm->tcpm_saddr, &saddr)) &&
>  		    net_eq(tm_net(tm), net)) {

Re: [PATCH v2 net-next] tcp_metrics: fix wrong lockdep annotations

[Julian Anastasov]

	Hello,

On Mon, 16 Mar 2015, Eric Dumazet wrote:

> From: Eric Dumazet <edumazet@google.com>
> 
> Changes in tcp_metric hash table are protected by tcp_metrics_lock
> only, not by genl_mutex

	So, can we also use .parallel_ops = true in
tcp_metrics_nl_family ? Looks like commands do not use
genl_mutex in such case.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH v2 net-next] tcp_metrics: fix wrong lockdep annotations

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 16 Mar 2015 07:14:34 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Changes in tcp_metric hash table are protected by tcp_metrics_lock
> only, not by genl_mutex
> 
> While we are at it use deref_locked() instead of rcu_dereference()
> in tcp_new() to avoid unnecessary barrier, as we hold tcp_metrics_lock
> as well.
>  
> Reported-by: Andrew Vagin <avagin@parallels.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 098a697b497e ("tcp_metrics: Use a single hash table for all network namespaces.")

Applied, thanks Eric.