mac80211: fix sta_info mesh timer bug

I noticed a bug I introduced when mesh is enabled: sta_info_destroy()
will end up calling cancel_timer() on a timer that has never been
initialized because the timer is only initialized in mesh_plink_alloc(),
not in sta_info_alloc(). This patch moves the initialization of all mesh
related fields into sta_info_alloc(), adds a bit of sanity checking to
the cfg80211 handlers and sta_info_insert() and makes mesh_plink_alloc()
a static helper function that is only used from the mesh plink code.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Luis Carlos Cobo <luisca@cozybit.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 6263cfc148c029efa5b7572535869e060c1975d3..69238fa67bf2ce054d0d820f97403e26e0dbb3b0 100644 (file)
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -17,8 +17,6 @@
 #include "ieee80211_rate.h"
 #include "mesh.h"
 
-#define DEFAULT_RATES 0
-
 static enum ieee80211_if_types
 nl80211_type_to_mac80211_type(enum nl80211_iftype type)
 {
@@ -654,10 +652,13 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
        } else
                sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
-       if (ieee80211_vif_is_mesh(&sdata->vif))
-               sta = mesh_plink_alloc(sdata, mac, DEFAULT_RATES, GFP_KERNEL);
-       else
-               sta = sta_info_alloc(sdata, mac, GFP_KERNEL);
+       if (compare_ether_addr(mac, dev->dev_addr) == 0)
+               return -EINVAL;
+
+       if (is_multicast_ether_addr(mac))
+               return -EINVAL;
+
+       sta = sta_info_alloc(sdata, mac, GFP_KERNEL);
        if (!sta)
                return -ENOMEM;
 

diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
index aee0b9eb36e37e8d84020c25219c0eed95a09aae..add9b0ddda81e58ae51164ebd7d1422268f39d1d 100644 (file)
--- a/net/mac80211/mesh.h
+++ b/net/mac80211/mesh.h
@@ -232,8 +232,6 @@ void mesh_neighbour_update(u8 *hw_addr, u64 rates, struct net_device *dev,
 bool mesh_peer_accepts_plinks(struct ieee802_11_elems *ie,
                              struct net_device *dev);
 void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata);
-struct sta_info *mesh_plink_alloc(struct ieee80211_sub_if_data *sdata,
-                                 u8 *hw_addr, u64 rates, gfp_t gfp);
 void mesh_plink_broken(struct sta_info *sta);
 void mesh_plink_deactivate(struct sta_info *sta);
 int mesh_plink_open(struct sta_info *sta);

diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 85cb75d53c436a7eb45d5a0361addd8fc8b5c771..7f02ae8abe9091b27c7b395337b5afd94bb4f9c1 100644 (file)
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -88,40 +88,19 @@ static inline void mesh_plink_fsm_restart(struct sta_info *sta)
        sta->llid = sta->plid = sta->reason = sta->plink_retries = 0;
 }
 
-/**
- * mesh_plink_alloc - allocate a new mesh peer link
- *
- * @sdata: local mesh interface
- * @hw_addr: hardware address (ETH_ALEN length)
- * @rates: rates the mesh peer supports
- *
- * The initial state of the new plink is set to LISTEN
- *
- * Returns: NULL on error.
- */
-struct sta_info *mesh_plink_alloc(struct ieee80211_sub_if_data *sdata,
-                                 u8 *hw_addr, u64 rates, gfp_t gfp)
+static struct sta_info *mesh_plink_alloc(struct ieee80211_sub_if_data *sdata,
+                                        u8 *hw_addr, u64 rates)
 {
        struct ieee80211_local *local = sdata->local;
        struct sta_info *sta;
 
-       if (compare_ether_addr(hw_addr, sdata->dev->dev_addr) == 0)
-               /* never add ourselves as neighbours */
-               return NULL;
-
-       if (is_multicast_ether_addr(hw_addr))
-               return NULL;
-
        if (local->num_sta >= MESH_MAX_PLINKS)
                return NULL;
 
-       sta = sta_info_alloc(sdata, hw_addr, gfp);
+       sta = sta_info_alloc(sdata, hw_addr, GFP_ATOMIC);
        if (!sta)
                return NULL;
 
-       sta->plink_state = LISTEN;
-       spin_lock_init(&sta->plink_lock);
-       init_timer(&sta->plink_timer);
        sta->flags |= WLAN_STA_AUTHORIZED;
        sta->supp_rates[local->hw.conf.channel->band] = rates;
 
@@ -249,7 +228,7 @@ void mesh_neighbour_update(u8 *hw_addr, u64 rates, struct net_device *dev,
 
        sta = sta_info_get(local, hw_addr);
        if (!sta) {
-               sta = mesh_plink_alloc(sdata, hw_addr, rates, GFP_ATOMIC);
+               sta = mesh_plink_alloc(sdata, hw_addr, rates);
                if (!sta) {
                        rcu_read_unlock();
                        return;
@@ -518,7 +497,7 @@ void mesh_rx_plink_frame(struct net_device *dev, struct ieee80211_mgmt *mgmt,
                }
 
                rates = ieee80211_sta_get_rates(local, &elems, rx_status->band);
-               sta = mesh_plink_alloc(sdata, mgmt->sa, rates, GFP_ATOMIC);
+               sta = mesh_plink_alloc(sdata, mgmt->sa, rates);
                if (!sta) {
                        mpl_dbg("Mesh plink error: plink table full\n");
                        rcu_read_unlock();

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 42414b4415927e8a04d6904c6df6185069f9781b..909fa38edb6cfc50405e074b5621bf52ec89e9d2 100644 (file)
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -31,13 +31,12 @@
  * for faster lookup and a list for iteration. They are managed using
  * RCU, i.e. access to the list and hash table is protected by RCU.
  *
- * Upon allocating a STA info structure with sta_info_alloc() or
- * mesh_plink_alloc(), the caller owns that structure. It must then either
- * destroy it using sta_info_destroy() (which is pretty useless) or insert
- * it into the hash table using sta_info_insert() which demotes the reference
- * from ownership to a regular RCU-protected reference; if the function
- * is called without protection by an RCU critical section the reference
- * is instantly invalidated.
+ * Upon allocating a STA info structure with sta_info_alloc(), the caller owns
+ * that structure. It must then either destroy it using sta_info_destroy()
+ * (which is pretty useless) or insert it into the hash table using
+ * sta_info_insert() which demotes the reference from ownership to a regular
+ * RCU-protected reference; if the function is called without protection by an
+ * RCU critical section the reference is instantly invalidated.
  *
  * Because there are debugfs entries for each station, and adding those
  * must be able to sleep, it is also possible to "pin" a station entry,
@@ -248,6 +247,12 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
               wiphy_name(local->hw.wiphy), print_mac(mbuf, sta->addr));
 #endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
 
+#ifdef CONFIG_MAC80211_MESH
+       sta->plink_state = LISTEN;
+       spin_lock_init(&sta->plink_lock);
+       init_timer(&sta->plink_timer);
+#endif
+
        return sta;
 }
 
@@ -258,7 +263,19 @@ int sta_info_insert(struct sta_info *sta)
        unsigned long flags;
        DECLARE_MAC_BUF(mac);
 
-       WARN_ON(!netif_running(sdata->dev));
+       /*
+        * Can't be a WARN_ON because it can be triggered through a race:
+        * something inserts a STA (on one CPU) without holding the RTNL
+        * and another CPU turns off the net device.
+        */
+       if (unlikely(!netif_running(sdata->dev)))
+               return -ENETDOWN;
+
+       if (WARN_ON(compare_ether_addr(sta->addr, sdata->dev->dev_addr) == 0))
+               return -EINVAL;
+
+       if (WARN_ON(is_multicast_ether_addr(sta->addr)))
+               return -EINVAL;
 
        spin_lock_irqsave(&local->sta_lock, flags);
        /* check if STA exists already */