Re: memory leakage in bridge(kernel-2.6.23.14)

Re: memory leakage in bridge(kernel-2.6.23.14)

[David Miller]

From: <wyb@topsec.com.cn>
Date: Wed, 16 Jan 2008 18:04:53 +0800

> In SMP, if a bridge fdb is being created when another CPU at the same time
> delete the bridge, this newly created fdb may incur a leakage:

netdev@vger.kernel.org (CC:'d) is the proper place to report
things like this.

'linux-net' is only for general user questions about the networking,
not for bug reports, patch postings, or developer discussion.  The
'netdev' list is for that.

Thank you.

> 
> CPU0:
> 
> static void del_nbp(struct net_bridge_port *p)
> {
> 	/*
> 	 * CPU1 enter br_fdb_update(), bridge port is still valid.
> 	 */
> 	......
> 	spin_lock_bh(&br->lock);
> 	br_stp_disable_port(p);
> 	spin_unlock_bh(&br->lock);
> 
> 	br_ifinfo_notify(RTM_DELLINK, p);
> 
> 	br_fdb_delete_by_port(br, p, 1);
> 
> 	/*
> 	 * CPU1 call fdb_create() for the being deleted bridge,
> 	 * a fdb would be add to bridge's fdb hash table, and will never
> 	 * be freed. because when deleting a bridge, linux flush fdb for
> each
> 	 * bridge port, but this newly created fdb belong to no bridge port
> 	 */
> 	......
> }
> 
> To fix this, fdb_create() should be changed to:
> {
> 	struct net_bridge_fdb_entry *fdb;
> 
> 	/*
> 	 * if the bridge port is deleted, then return.
> 	 */
> 	if (!(source->state == BR_STATE_LEARNING ||
> 	      source->state == BR_STATE_FORWARDING))
> 		return;
> 
> 	fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC);
> 	if (fdb) {
> 		memcpy(fdb->addr.addr, addr, ETH_ALEN);
> 		atomic_set(&fdb->use_count, 1);
> 		hlist_add_head_rcu(&fdb->hlist, head);
> 
> 		fdb->dst = source;
> 		fdb->is_local = is_local;
> 		fdb->is_static = is_local;
> 		fdb->ageing_timer = jiffies;
> 	}
> 	return fdb;
> }
> 
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: memory leakage in bridge(kernel-2.6.23.14)

[Stephen Hemminger]

On Wed, 16 Jan 2008 18:04:53 +0800
<wyb@topsec.com.cn> wrote:

> 
> In SMP, if a bridge fdb is being created when another CPU at the same time
> delete the bridge, this newly created fdb may incur a leakage:
> 
> CPU0:
> 
> static void del_nbp(struct net_bridge_port *p)
> {
> 	/*
> 	 * CPU1 enter br_fdb_update(), bridge port is still valid.
> 	 */
> 	......
> 	spin_lock_bh(&br->lock);
> 	br_stp_disable_port(p);
> 	spin_unlock_bh(&br->lock);
> 
> 	br_ifinfo_notify(RTM_DELLINK, p);
> 
> 	br_fdb_delete_by_port(br, p, 1);
> 
> 	/*
> 	 * CPU1 call fdb_create() for the being deleted bridge,
> 	 * a fdb would be add to bridge's fdb hash table, and will never
> 	 * be freed. because when deleting a bridge, linux flush fdb for
> each
> 	 * bridge port, but this newly created fdb belong to no bridge port
> 	 */
> 	......
> }
> 
> To fix this, fdb_create() should be changed to:
> {
> 	struct net_bridge_fdb_entry *fdb;
> 
> 	/*
> 	 * if the bridge port is deleted, then return.
> 	 */
> 	if (!(source->state == BR_STATE_LEARNING ||
> 	      source->state == BR_STATE_FORWARDING))
> 		return;
> 
> 	fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC);
> 	if (fdb) {
> 		memcpy(fdb->addr.addr, addr, ETH_ALEN);
> 		atomic_set(&fdb->use_count, 1);
> 		hlist_add_head_rcu(&fdb->hlist, head);
> 
> 		fdb->dst = source;
> 		fdb->is_local = is_local;
> 		fdb->is_static = is_local;
> 		fdb->ageing_timer = jiffies;
> 	}
> 	return fdb;
> }
> 

That check is not enough, since the state might change during
fdb_create.


I think fdb_delete_by_port needs to be moved after RCU barrier event.
Something like this (untested) patch.

--- a/net/bridge/br_if.c	2008-01-16 11:00:09.000000000 -0800
+++ b/net/bridge/br_if.c	2008-01-16 11:01:11.000000000 -0800
@@ -116,6 +116,9 @@ static void destroy_nbp_rcu(struct rcu_h
 {
 	struct net_bridge_port *p =
 			container_of(head, struct net_bridge_port, rcu);
+
+	br_fdb_delete_by_port(p->br, p, 1);
+
 	destroy_nbp(p);
 }
 
@@ -143,8 +146,6 @@ static void del_nbp(struct net_bridge_po
 
 	br_ifinfo_notify(RTM_DELLINK, p);
 
-	br_fdb_delete_by_port(br, p, 1);
-
 	list_del_rcu(&p->list);
 
 	rcu_assign_pointer(dev->br_port, NULL);