Skip to contentSkip to conversation

: Fix race between clusterip_config_find_get and _entry_put.

PATCH Applied Pavel Emelyanov 2 participants 2 messages Apr 12, 2008 – Apr 13, 2008

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : Fix race between clusterip_config_find_get and _entry_put.

Consider we are putting a clusterip_config entry with the "entries" count == 1, and on the other CPU there's a clusterip_config_find_get in progress:

CPU1: CPU2:
clusterip_config_entry_put: clusterip_config_find_get:

if (atomic_dec_and_test(&c->entries)) { 
	/* true */
							read_lock_bh(&clusterip_lock);
							c = __clusterip_config_find(clusterip);
							/* found - it's still in list */
							...
							atomic_inc(&c->entries);
							read_unlock_bh(&clusterip_lock);

	write_lock_bh(&clusterip_lock);
	list_del(&c->list);
	write_unlock_bh(&clusterip_lock);
	...
	dev_put(c->dev);

Oops! We have an entry returned by the clusterip_config_find_get, which is a) not in list b) has a stale dev pointer.

The problems will happen when the CPU2 will release the entry - it will remove it from the list for the 2nd time, thus spoiling it, and will put a stale dev pointer.

The fix is to make atomic_dec_and_test under the clusterip_lock.

---

net/ipv4/netfilter/ipt_CLUSTERIP.c +3 -1
index 52926c8..a12dd32 100644
@@ -82,8 +82,8 @@ clusterip_config_put(struct clusterip_config *c)
 static inline void
 clusterip_config_entry_put(struct clusterip_config *c)
 {
+	write_lock_bh(&clusterip_lock);
 	if (atomic_dec_and_test(&c->entries)) {
-		write_lock_bh(&clusterip_lock);
 		list_del(&c->list);
 		write_unlock_bh(&clusterip_lock);
 
@@ -96,7 +96,9 @@ clusterip_config_entry_put(struct clusterip_config *c)
 #ifdef CONFIG_PROC_FS
 		remove_proc_entry(c->pde->name, c->pde->parent);
 #endif
+		return;
 	}
+	write_unlock_bh(&clusterip_lock);
 }
 
 static struct clusterip_config *
#1 lore raw

[Please remeber to CC netfilter-devel]

Quoted 21 lines · Pavel Emelyanov

Consider we are putting a clusterip_config entry with the "entries" count == 1, and on the other CPU there's a clusterip_config_find_get in progress:

CPU1: CPU2:
clusterip_config_entry_put: clusterip_config_find_get:

if (atomic_dec_and_test(&c->entries)) { 
	/* true */
							read_lock_bh(&clusterip_lock);
							c = __clusterip_config_find(clusterip);
							/* found - it's still in list */
							...
							atomic_inc(&c->entries);
							read_unlock_bh(&clusterip_lock);

	write_lock_bh(&clusterip_lock);
	list_del(&c->list);
	write_unlock_bh(&clusterip_lock);
	...
	dev_put(c->dev);
Quoted 8 lines

Oops! We have an entry returned by the clusterip_config_find_get, which is a) not in list b) has a stale dev pointer.

The problems will happen when the CPU2 will release the entry - it will remove it from the list for the 2nd time, thus spoiling it, and will put a stale dev pointer.

The fix is to make atomic_dec_and_test under the clusterip_lock.

I guess an alternative would have been to use atomic_inc_not_zero in clusterip_config_find_get. But this is only called very rarely, so it doesn't really matter.

Applied, thanks.