Skip to contentSkip to conversation

neighbour: Fixed race condition at tbl->nht

PATCH Applied Michel Machado 3 participants 3 messages Feb 21, 2012

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Michel Machado

author
#0 lore raw

PATCH 1/1 neighbour: Fixed race condition at tbl->nht

When the fixed race condition happens:

1. While function neigh_periodic_work scans the neighbor hash table pointed by field tbl->nht, it unlocks and locks tbl->lock between buckets in order to call cond_resched.

2. Assume that function neigh_periodic_work calls cond_resched, that is, the lock tbl->lock is available, and function neigh_hash_grow runs.

3. Once function neigh_hash_grow finishes, and RCU calls neigh_hash_free_rcu, the original struct neigh_hash_table that function neigh_periodic_work was using doesn't exist anymore.

4. Once back at neigh_periodic_work, whenever the old struct neigh_hash_table is accessed, things can go badly.

Signed-off-by Michel Machado <[email protected]>
CC "David S. Miller" <[email protected]>
CC Eric Dumazet <[email protected]>

---

net/core/neighbour.c +2 -0
index e287346..2a83914 100644
@@ -826,6 +826,8 @@ next_elt:
 		write_unlock_bh(&tbl->lock);
 		cond_resched();
 		write_lock_bh(&tbl->lock);
+		nht = rcu_dereference_protected(tbl->nht,
+						lockdep_is_held(&tbl->lock));
 	}
 	/* Cycle through all hash buckets every base_reachable_time/2 ticks.
 	 * ARP entry timeouts range from 1/2 base_reachable_time to 3/2
Acked-by on v1 1/1
#1 lore raw
Quoted 27 lines · Michel Machado

When the fixed race condition happens:

1. While function neigh_periodic_work scans the neighbor hash table pointed by field tbl->nht, it unlocks and locks tbl->lock between buckets in order to call cond_resched.

2. Assume that function neigh_periodic_work calls cond_resched, that is, the lock tbl->lock is available, and function neigh_hash_grow runs.

3. Once function neigh_hash_grow finishes, and RCU calls neigh_hash_free_rcu, the original struct neigh_hash_table that function neigh_periodic_work was using doesn't exist anymore.

4. Once back at neigh_periodic_work, whenever the old struct neigh_hash_table is accessed, things can go badly.

Signed-off-by Michel Machado <[email protected]>
CC "David S. Miller" <[email protected]>
CC Eric Dumazet <[email protected]>

---

net/core/neighbour.c +0 -0
index e287346..2a83914 100644

net/core/neighbour.c next_elt: · line 833

Quoted 8 lines
net/core/neighbour.c +2 -0
@@ -826,6 +826,8 @@ next_elt:
 		write_unlock_bh(&tbl->lock);
 		cond_resched();
 		write_lock_bh(&tbl->lock);
+		nht = rcu_dereference_protected(tbl->nht,
+						lockdep_is_held(&tbl->lock));
 	}
 	/* Cycle through all hash buckets every base_reachable_time/2 ticks.
 	 * ARP entry timeouts range from 1/2 base_reachable_time to 3/2

Nice catch !

Bug introduced in 2.6.37 in commit d6bf781712a (net neigh: RCU conversion of neigh hash table)

Acked-by Eric Dumazet <[email protected]>
maintainer Applied
#2 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 21 Feb 2012 22:25:22 +0100

Quoted 18 lines · Michel Machado
Quoted 17 lines

When the fixed race condition happens:

1. While function neigh_periodic_work scans the neighbor hash table pointed by field tbl->nht, it unlocks and locks tbl->lock between buckets in order to call cond_resched.

2. Assume that function neigh_periodic_work calls cond_resched, that is, the lock tbl->lock is available, and function neigh_hash_grow runs.

3. Once function neigh_hash_grow finishes, and RCU calls neigh_hash_free_rcu, the original struct neigh_hash_table that function neigh_periodic_work was using doesn't exist anymore.

4. Once back at neigh_periodic_work, whenever the old struct neigh_hash_table is accessed, things can go badly.

Signed-off-by Michel Machado <[email protected]>

...

Quoted 6 lines

Nice catch !

Bug introduced in 2.6.37 in commit d6bf781712a (net neigh: RCU conversion of neigh hash table)

Acked-by Eric Dumazet <[email protected]>

Applied, and queued up for -stable, thanks.