aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMichal Ostrowski <mostrows@earthlink.net>2007-04-20 19:59:24 -0400
committerDavid S. Miller <davem@sunset.davemloft.net>2007-04-26 01:29:21 -0400
commit42dc9cd54b7290f862874a2544e50395e5719985 (patch)
treef37f4465f6adbdcdc9b961aa867919a3970190ce
parent202a03acf9994076055df40ae093a5c5474ad0bd (diff)
[PPPOE]: Fix device tear-down notification.
pppoe_flush_dev() kicks all sockets bound to a device that is going down. In doing so, locks must be taken in the right order consistently (sock lock, followed by the pppoe_hash_lock). However, the scan process is based on us holding the sock lock. So, when something is found in the scan we must release the lock we're holding and grab the sock lock. This patch fixes race conditions between this code and pppoe_release(), both of which perform similar functions but would naturally prefer to grab locks in opposing orders. Both code paths are now going after these locks in a consistent manner. pppoe_hash_lock protects the contents of the "pppox_sock" objects that reside inside the hash. Thus, NULL'ing out the pppoe_dev field should be done under the protection of this lock. Signed-off-by: Michal Ostrowski <mostrows@earthlink.net> Signed-off-by: David S. Miller <davem@davemloft.net>
-rw-r--r--drivers/net/pppoe.c87
1 files changed, 50 insertions, 37 deletions
diff --git a/drivers/net/pppoe.c b/drivers/net/pppoe.c
index bc4fc30bc85e..6f98834e6ace 100644
--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -241,54 +241,53 @@ static inline struct pppox_sock *delete_item(unsigned long sid, char *addr, int
241static void pppoe_flush_dev(struct net_device *dev) 241static void pppoe_flush_dev(struct net_device *dev)
242{ 242{
243 int hash; 243 int hash;
244
245 BUG_ON(dev == NULL); 244 BUG_ON(dev == NULL);
246 245
247 read_lock_bh(&pppoe_hash_lock); 246 write_lock_bh(&pppoe_hash_lock);
248 for (hash = 0; hash < PPPOE_HASH_SIZE; hash++) { 247 for (hash = 0; hash < PPPOE_HASH_SIZE; hash++) {
249 struct pppox_sock *po = item_hash_table[hash]; 248 struct pppox_sock *po = item_hash_table[hash];
250 249
251 while (po != NULL) { 250 while (po != NULL) {
252 if (po->pppoe_dev == dev) { 251 struct sock *sk = sk_pppox(po);
253 struct sock *sk = sk_pppox(po); 252 if (po->pppoe_dev != dev) {
254 253 po = po->next;
255 sock_hold(sk); 254 continue;
256 po->pppoe_dev = NULL; 255 }
256 po->pppoe_dev = NULL;
257 dev_put(dev);
257 258
258 /* We hold a reference to SK, now drop the
259 * hash table lock so that we may attempt
260 * to lock the socket (which can sleep).
261 */
262 read_unlock_bh(&pppoe_hash_lock);
263 259
264 lock_sock(sk); 260 /* We always grab the socket lock, followed by the
261 * pppoe_hash_lock, in that order. Since we should
262 * hold the sock lock while doing any unbinding,
263 * we need to release the lock we're holding.
264 * Hold a reference to the sock so it doesn't disappear
265 * as we're jumping between locks.
266 */
265 267
266 if (sk->sk_state & 268 sock_hold(sk);
267 (PPPOX_CONNECTED | PPPOX_BOUND)) {
268 pppox_unbind_sock(sk);
269 dev_put(dev);
270 sk->sk_state = PPPOX_ZOMBIE;
271 sk->sk_state_change(sk);
272 }
273 269
274 release_sock(sk); 270 write_unlock_bh(&pppoe_hash_lock);
271 lock_sock(sk);
275 272
276 sock_put(sk); 273 if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND)) {
274 pppox_unbind_sock(sk);
275 sk->sk_state = PPPOX_ZOMBIE;
276 sk->sk_state_change(sk);
277 }
277 278
278 read_lock_bh(&pppoe_hash_lock); 279 release_sock(sk);
280 sock_put(sk);
279 281
280 /* Now restart from the beginning of this 282 /* Restart scan at the beginning of this hash chain.
281 * hash chain. We always NULL out pppoe_dev 283 * While the lock was dropped the chain contents may
282 * so we are guaranteed to make forward 284 * have changed.
283 * progress. 285 */
284 */ 286 write_lock_bh(&pppoe_hash_lock);
285 po = item_hash_table[hash]; 287 po = item_hash_table[hash];
286 continue;
287 }
288 po = po->next;
289 } 288 }
290 } 289 }
291 read_unlock_bh(&pppoe_hash_lock); 290 write_unlock_bh(&pppoe_hash_lock);
292} 291}
293 292
294static int pppoe_device_event(struct notifier_block *this, 293static int pppoe_device_event(struct notifier_block *this,
@@ -504,28 +503,42 @@ static int pppoe_release(struct socket *sock)
504 if (!sk) 503 if (!sk)
505 return 0; 504 return 0;
506 505
507 if (sock_flag(sk, SOCK_DEAD)) 506 lock_sock(sk);
507 if (sock_flag(sk, SOCK_DEAD)){
508 release_sock(sk);
508 return -EBADF; 509 return -EBADF;
510 }
509 511
510 pppox_unbind_sock(sk); 512 pppox_unbind_sock(sk);
511 513
512 /* Signal the death of the socket. */ 514 /* Signal the death of the socket. */
513 sk->sk_state = PPPOX_DEAD; 515 sk->sk_state = PPPOX_DEAD;
514 516
517
518 /* Write lock on hash lock protects the entire "po" struct from
519 * concurrent updates via pppoe_flush_dev. The "po" struct should
520 * be considered part of the hash table contents, thus protected
521 * by the hash table lock */
522 write_lock_bh(&pppoe_hash_lock);
523
515 po = pppox_sk(sk); 524 po = pppox_sk(sk);
516 if (po->pppoe_pa.sid) { 525 if (po->pppoe_pa.sid) {
517 delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote, po->pppoe_ifindex); 526 __delete_item(po->pppoe_pa.sid,
527 po->pppoe_pa.remote, po->pppoe_ifindex);
518 } 528 }
519 529
520 if (po->pppoe_dev) 530 if (po->pppoe_dev) {
521 dev_put(po->pppoe_dev); 531 dev_put(po->pppoe_dev);
532 po->pppoe_dev = NULL;
533 }
522 534
523 po->pppoe_dev = NULL; 535 write_unlock_bh(&pppoe_hash_lock);
524 536
525 sock_orphan(sk); 537 sock_orphan(sk);
526 sock->sk = NULL; 538 sock->sk = NULL;
527 539
528 skb_queue_purge(&sk->sk_receive_queue); 540 skb_queue_purge(&sk->sk_receive_queue);
541 release_sock(sk);
529 sock_put(sk); 542 sock_put(sk);
530 543
531 return 0; 544 return 0;