Re: BUG: scheduling while atomic: ifconfig/0x00000002/4170
From: Paul E. McKenney <hidden>
Date: 2007-09-06 15:46:29
Also in:
lkml, netdev
On Thu, Sep 06, 2007 at 03:36:55PM +0200, Johannes Berg wrote:
On Thu, 2007-09-06 at 20:36 +0800, Herbert Xu wrote:quoted
Yeah I think they're all under RTNL too. So you don't need to take the lock here at all since you should already have the RTNL.Ok, this patch gets rid of the lock. I'd appreciate if you could give it a quick look for obvious RCU abuse as I haven't tested it. It also doesn't apply against net-2.6.24 because I based it after the patches I have queued with John for net-2.6.24.
Looks good to me from an RCU viewpoint. I cannot claim familiarity with this code. I therefore especially like the indications of where RTNL is held and not!!! Some questions below based on a quick scan. And a global question: should the comments about RTNL being held be replaced by ASSERT_RTNL()? Thanx, Paul
quoted hunk ↗ jump to hunk
johannes--- netdev-2.6.orig/net/mac80211/ieee80211.c 2007-09-06 15:35:23.324447686 +0200 +++ netdev-2.6/net/mac80211/ieee80211.c 2007-09-06 15:35:23.394447686 +0200@@ -90,8 +90,9 @@ static struct dev_mc_list *ieee80211_get /* start of iteration, both unassigned */ if (!mcd->cur && !mcd->sdata) { - mcd->sdata = list_entry(local->sub_if_list.next, - struct ieee80211_sub_if_data, list); + mcd->sdata = list_entry( + rcu_dereference(local->interfaces.next), + struct ieee80211_sub_if_data, list); mcd->cur = mcd->sdata->dev->mc_list; }@@ -100,10 +101,11 @@ static struct dev_mc_list *ieee80211_get while (!mcd->cur) { /* reached end of interface list? */ - if (mcd->sdata->list.next == &local->sub_if_list) + if (rcu_dereference(mcd->sdata->list.next) == + &local->interfaces) break; /* otherwise try next interface */ - mcd->sdata = list_entry(mcd->sdata->list.next, + mcd->sdata = list_entry(rcu_dereference(mcd->sdata->list.next), struct ieee80211_sub_if_data, list); mcd->cur = mcd->sdata->dev->mc_list; }@@ -145,9 +147,10 @@ static void ieee80211_configure_filter(s /* * We can iterate through the device list for the multicast - * address list so need to lock it. + * address list so need to be in a RCU read-side section, + * the RTNL isn't held in this function. */ - read_lock(&local->sub_if_lock); + rcu_read_lock(); /* be a bit nasty */ new_flags |= (1<<31);@@ -163,7 +166,7 @@ static void ieee80211_configure_filter(s WARN_ON(mcd.cur); local->filter_flags = new_flags & ~(1<<31); - read_unlock(&local->sub_if_lock); + rcu_read_unlock(); netif_tx_unlock(local->mdev); }@@ -176,14 +179,13 @@ static int ieee80211_master_open(struct struct ieee80211_sub_if_data *sdata; int res = -EOPNOTSUPP; - read_lock(&local->sub_if_lock); - list_for_each_entry(sdata, &local->sub_if_list, list) { + /* we hold the RTNL here so can safely walk the list */ + list_for_each_entry(sdata, &local->interfaces, list) { if (sdata->dev != dev && netif_running(sdata->dev)) { res = 0; break; } } - read_unlock(&local->sub_if_lock); return res; }@@ -192,11 +194,10 @@ static int ieee80211_master_stop(struct struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr); struct ieee80211_sub_if_data *sdata; - read_lock(&local->sub_if_lock); - list_for_each_entry(sdata, &local->sub_if_list, list) + /* we hold the RTNL here so can safely walk the list */ + list_for_each_entry(sdata, &local->interfaces, list) if (sdata->dev != dev && netif_running(sdata->dev)) dev_close(sdata->dev); - read_unlock(&local->sub_if_lock); return 0; }@@ -430,8 +431,8 @@ static int ieee80211_open(struct net_dev sdata = IEEE80211_DEV_TO_SUB_IF(dev); - read_lock(&local->sub_if_lock); - list_for_each_entry(nsdata, &local->sub_if_list, list) { + /* we hold the RTNL here so can safely walk the list */ + list_for_each_entry(nsdata, &local->interfaces, list) { struct net_device *ndev = nsdata->dev; if (ndev != dev && ndev != local->mdev && netif_running(ndev) &&@@ -440,10 +441,8 @@ static int ieee80211_open(struct net_dev * check whether it may have the same address */ if (!identical_mac_addr_allowed(sdata->type, - nsdata->type)) { - read_unlock(&local->sub_if_lock); + nsdata->type)) return -ENOTUNIQ; - } /* * can only add VLANs to enabled APs@@ -454,7 +453,6 @@ static int ieee80211_open(struct net_dev sdata->u.vlan.ap = nsdata; } } - read_unlock(&local->sub_if_lock); switch (sdata->type) { case IEEE80211_IF_TYPE_WDS:@@ -575,14 +573,13 @@ static int ieee80211_stop(struct net_dev sdata->u.sta.state = IEEE80211_DISABLED; del_timer_sync(&sdata->u.sta.timer); /* - * Holding the sub_if_lock for writing here blocks - * out the receive path and makes sure it's not - * currently processing a packet that may get - * added to the queue. + * When we get here, the interface is marked down. + * Call synchronize_rcu() to wait for the RX path + * should it be using the interface and enqueuing + * frames at this very time on another CPU. */ - write_lock_bh(&local->sub_if_lock); + synchronize_rcu(); skb_queue_purge(&sdata->u.sta.skb_queue); - write_unlock_bh(&local->sub_if_lock); if (!local->ops->hw_scan && local->scan_dev == sdata->dev) {@@ -1134,9 +1131,9 @@ void ieee80211_tx_status(struct ieee8021 rthdr->data_retries = status->retry_count; - read_lock(&local->sub_if_lock); + rcu_read_lock(); monitors = local->monitors; - list_for_each_entry(sdata, &local->sub_if_list, list) { + list_for_each_entry_rcu(sdata, &local->interfaces, list) { /* * Using the monitors counter is possibly racy, but * if the value is wrong we simply either clone the skb@@ -1152,7 +1149,7 @@ void ieee80211_tx_status(struct ieee8021 continue; monitors--; if (monitors) - skb2 = skb_clone(skb, GFP_KERNEL); + skb2 = skb_clone(skb, GFP_ATOMIC); else skb2 = NULL; skb->dev = sdata->dev;@@ -1167,7 +1164,7 @@ void ieee80211_tx_status(struct ieee8021 } } out: - read_unlock(&local->sub_if_lock); + rcu_read_unlock(); if (skb) dev_kfree_skb(skb); }@@ -1255,8 +1252,7 @@ struct ieee80211_hw *ieee80211_alloc_hw( INIT_LIST_HEAD(&local->modes_list); - rwlock_init(&local->sub_if_lock); - INIT_LIST_HEAD(&local->sub_if_list); + INIT_LIST_HEAD(&local->interfaces); INIT_DELAYED_WORK(&local->scan_work, ieee80211_sta_scan_work); ieee80211_rx_bss_list_init(mdev);@@ -1275,7 +1271,8 @@ struct ieee80211_hw *ieee80211_alloc_hw( sdata->u.ap.force_unicast_rateidx = -1; sdata->u.ap.max_ratectrl_rateidx = -1; ieee80211_if_sdata_init(sdata); - list_add_tail(&sdata->list, &local->sub_if_list); + /* no RCU needed since we're still during init phase */ + list_add_tail(&sdata->list, &local->interfaces); tasklet_init(&local->tx_pending_tasklet, ieee80211_tx_pending, (unsigned long)local);@@ -1434,7 +1431,6 @@ void ieee80211_unregister_hw(struct ieee { struct ieee80211_local *local = hw_to_local(hw); struct ieee80211_sub_if_data *sdata, *tmp; - struct list_head tmp_list; int i; tasklet_kill(&local->tx_pending_tasklet);@@ -1448,11 +1444,12 @@ void ieee80211_unregister_hw(struct ieee if (local->apdev) ieee80211_if_del_mgmt(local); - write_lock_bh(&local->sub_if_lock); - list_replace_init(&local->sub_if_list, &tmp_list); - write_unlock_bh(&local->sub_if_lock); - - list_for_each_entry_safe(sdata, tmp, &tmp_list, list) + /* + * At this point, interface list manipulations are fine + * because the driver cannot be handing us frames any + * more and the tasklet is killed. + */ + list_for_each_entry_safe(sdata, tmp, &local->interfaces, list) __ieee80211_if_del(local, sdata); rtnl_unlock(); --- netdev-2.6.orig/net/mac80211/ieee80211_i.h 2007-09-06 15:35:23.334447686 +0200 +++ netdev-2.6/net/mac80211/ieee80211_i.h 2007-09-06 15:35:23.404447686 +0200@@ -481,9 +481,8 @@ struct ieee80211_local { ieee80211_rx_handler *rx_handlers; ieee80211_tx_handler *tx_handlers; - rwlock_t sub_if_lock; /* Protects sub_if_list. Cannot be taken under - * sta_bss_lock or sta_lock. */ - struct list_head sub_if_list; + struct list_head interfaces; + int sta_scanning; int scan_channel_idx; enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state; --- netdev-2.6.orig/net/mac80211/ieee80211_iface.c 2007-09-06 15:35:23.334447686 +0200 +++ netdev-2.6/net/mac80211/ieee80211_iface.c 2007-09-06 15:35:23.404447686 +0200@@ -79,16 +79,15 @@ int ieee80211_if_add(struct net_device * ieee80211_debugfs_add_netdev(sdata); ieee80211_if_set_type(ndev, type); - write_lock_bh(&local->sub_if_lock); + /* we're under RTNL so all this is fine */ if (unlikely(local->reg_state == IEEE80211_DEV_UNREGISTERED)) { - write_unlock_bh(&local->sub_if_lock); __ieee80211_if_del(local, sdata); return -ENODEV; } - list_add(&sdata->list, &local->sub_if_list); + list_add_tail_rcu(&sdata->list, &local->interfaces);
The _rcu is required because this list isn't protected by RTNL?
quoted hunk ↗ jump to hunk
+ if (new_dev) *new_dev = ndev; - write_unlock_bh(&local->sub_if_lock); return 0;@@ -226,22 +225,22 @@ void ieee80211_if_reinit(struct net_devi /* Remove all virtual interfaces that use this BSS * as their sdata->bss */ struct ieee80211_sub_if_data *tsdata, *n; - LIST_HEAD(tmp_list); - write_lock_bh(&local->sub_if_lock);
This code is also protected by RTNL?
quoted hunk ↗ jump to hunk
- list_for_each_entry_safe(tsdata, n, &local->sub_if_list, list) { + list_for_each_entry_safe(tsdata, n, &local->interfaces, list) { if (tsdata != sdata && tsdata->bss == &sdata->u.ap) { printk(KERN_DEBUG "%s: removing virtual " "interface %s because its BSS interface" " is being removed\n", sdata->dev->name, tsdata->dev->name); - list_move_tail(&tsdata->list, &tmp_list); + list_del_rcu(&tsdata->list); + /* + * We have lots of time and can afford + * to sync for each interface + */ + synchronize_rcu(); + __ieee80211_if_del(local, tsdata); } } - write_unlock_bh(&local->sub_if_lock); - - list_for_each_entry_safe(tsdata, n, &tmp_list, list) - __ieee80211_if_del(local, tsdata); kfree(sdata->u.ap.beacon_head); kfree(sdata->u.ap.beacon_tail);@@ -318,18 +317,16 @@ int ieee80211_if_remove(struct net_devic ASSERT_RTNL();
I -like- this!!! ;-)
quoted hunk ↗ jump to hunk
- write_lock_bh(&local->sub_if_lock); - list_for_each_entry_safe(sdata, n, &local->sub_if_list, list) { + list_for_each_entry_safe(sdata, n, &local->interfaces, list) { if ((sdata->type == id || id == -1) && strcmp(name, sdata->dev->name) == 0 && sdata->dev != local->mdev) { - list_del(&sdata->list); - write_unlock_bh(&local->sub_if_lock); + list_del_rcu(&sdata->list); + synchronize_rcu(); __ieee80211_if_del(local, sdata); return 0; } } - write_unlock_bh(&local->sub_if_lock); return -ENODEV; }--- netdev-2.6.orig/net/mac80211/ieee80211_sta.c 2007-09-06 15:35:23.224447686 +0200 +++ netdev-2.6/net/mac80211/ieee80211_sta.c 2007-09-06 15:35:23.414447686 +0200@@ -2659,8 +2659,8 @@ void ieee80211_scan_completed(struct iee memset(&wrqu, 0, sizeof(wrqu)); wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL); - read_lock(&local->sub_if_lock); - list_for_each_entry(sdata, &local->sub_if_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(sdata, &local->interfaces, list) { /* No need to wake the master device. */ if (sdata->dev == local->mdev)@@ -2674,7 +2674,7 @@ void ieee80211_scan_completed(struct iee netif_wake_queue(sdata->dev); } - read_unlock(&local->sub_if_lock); + rcu_read_unlock(); sdata = IEEE80211_DEV_TO_SUB_IF(dev); if (sdata->type == IEEE80211_IF_TYPE_IBSS) {@@ -2811,8 +2811,8 @@ static int ieee80211_sta_start_scan(stru local->sta_scanning = 1; - read_lock(&local->sub_if_lock); - list_for_each_entry(sdata, &local->sub_if_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(sdata, &local->interfaces, list) { /* Don't stop the master interface, otherwise we can't transmit * probes! */@@ -2824,7 +2824,7 @@ static int ieee80211_sta_start_scan(stru (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED)) ieee80211_send_nullfunc(local, sdata, 1); } - read_unlock(&local->sub_if_lock); + rcu_read_unlock(); if (ssid) { local->scan_ssid_len = ssid_len; --- netdev-2.6.orig/net/mac80211/rx.c 2007-09-06 15:35:23.214447686 +0200 +++ netdev-2.6/net/mac80211/rx.c 2007-09-06 15:35:23.414447686 +0200@@ -1385,8 +1385,9 @@ void __ieee80211_rx(struct ieee80211_hw } /* - * key references are protected using RCU and this requires that - * we are in a read-site RCU section during receive processing + * key references and virtual interfaces are protected using RCU + * and this requires that we are in a read-side RCU section during + * receive processing */ rcu_read_lock();@@ -1441,8 +1442,7 @@ void __ieee80211_rx(struct ieee80211_hw bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len); - read_lock(&local->sub_if_lock); - list_for_each_entry(sdata, &local->sub_if_list, list) { + list_for_each_entry_rcu(sdata, &local->interfaces, list) { rx.flags |= IEEE80211_TXRXD_RXRA_MATCH; if (!netif_running(sdata->dev))@@ -1494,7 +1494,6 @@ void __ieee80211_rx(struct ieee80211_hw &rx, sta); } else dev_kfree_skb(skb); - read_unlock(&local->sub_if_lock); end: rcu_read_unlock(); --- netdev-2.6.orig/net/mac80211/tx.c 2007-09-06 15:35:23.074447686 +0200 +++ netdev-2.6/net/mac80211/tx.c 2007-09-06 15:35:23.424447686 +0200@@ -291,8 +291,12 @@ static void purge_old_ps_buffers(struct struct ieee80211_sub_if_data *sdata; struct sta_info *sta; - read_lock(&local->sub_if_lock); - list_for_each_entry(sdata, &local->sub_if_list, list) { + /* + * virtual interfaces are protected by RCU + */ + rcu_read_lock(); + + list_for_each_entry_rcu(sdata, &local->interfaces, list) { struct ieee80211_if_ap *ap; if (sdata->dev == local->mdev || sdata->type != IEEE80211_IF_TYPE_AP)@@ -305,7 +309,7 @@ static void purge_old_ps_buffers(struct } total += skb_queue_len(&ap->ps_bc_buf); } - read_unlock(&local->sub_if_lock); + rcu_read_unlock(); read_lock_bh(&local->sta_lock); list_for_each_entry(sta, &local->sta_list, list) { -To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html