Re: memory leakage in bridge(kernel-2.6.23.14)

On Wed, 16 Jan 2008 18:04:53 +0800
[off-list ref] 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);