All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] mac80211: fix mesh deadlock
@ 2013-06-03 23:25 Thomas Pedersen
  2013-06-03 23:25 ` [PATCH 2/2] mac80211: update mesh beacon on workqueue Thomas Pedersen
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Pedersen @ 2013-06-03 23:25 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, open80211s, Thomas Pedersen

The patch "cfg80211/mac80211: use cfg80211 wdev mutex in
mac80211" introduced several deadlocks by converting the
ifmsh->mtx to wdev->mtx. Solve these by:

1. drop the cancel_work_sync() in ieee80211_stop_mesh().
   Instead make the mesh work conditional on whether the mesh
   is running or not.
2. lock the mesh work with sdata_lock() to protect beacon
   updates and prevent races with wdev->mesh_id_len or
   cfg80211.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/mesh.c       |   29 +++++++++++++++++------------
 net/mac80211/mesh_plink.c |    7 +------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 73a597b..d5faf91 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -579,9 +579,7 @@ static void ieee80211_mesh_housekeeping(struct ieee80211_sub_if_data *sdata)
 	mesh_path_expire(sdata);
 
 	changed = mesh_accept_plinks_update(sdata);
-	sdata_lock(sdata);
 	ieee80211_mbss_info_change_notify(sdata, changed);
-	sdata_unlock(sdata);
 
 	mod_timer(&ifmsh->housekeeping_timer,
 		  round_jiffies(jiffies +
@@ -788,12 +786,10 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
 	sdata->vif.bss_conf.enable_beacon = false;
 	clear_bit(SDATA_STATE_OFFCHANNEL_BEACON_STOPPED, &sdata->state);
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED);
-	sdata_lock(sdata);
 	bcn = rcu_dereference_protected(ifmsh->beacon,
 					lockdep_is_held(&sdata->wdev.mtx));
 	rcu_assign_pointer(ifmsh->beacon, NULL);
 	kfree_rcu(bcn, rcu_head);
-	sdata_unlock(sdata);
 
 	/* flush STAs and mpaths on this iface */
 	sta_info_flush(sdata);
@@ -806,14 +802,6 @@ void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
 	del_timer_sync(&sdata->u.mesh.housekeeping_timer);
 	del_timer_sync(&sdata->u.mesh.mesh_path_root_timer);
 	del_timer_sync(&sdata->u.mesh.mesh_path_timer);
-	/*
-	 * If the timer fired while we waited for it, it will have
-	 * requeued the work. Now the work will be running again
-	 * but will not rearm the timer again because it checks
-	 * whether the interface is running, which, at this point,
-	 * it no longer is.
-	 */
-	cancel_work_sync(&sdata->work);
 
 	local->fif_other_bss--;
 	atomic_dec(&local->iff_allmultis);
@@ -954,6 +942,12 @@ void ieee80211_mesh_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_mgmt *mgmt;
 	u16 stype;
 
+	sdata_lock(sdata);
+
+	/* mesh already went down */
+	if (!sdata->wdev.mesh_id_len)
+		goto out;
+
 	rx_status = IEEE80211_SKB_RXCB(skb);
 	mgmt = (struct ieee80211_mgmt *) skb->data;
 	stype = le16_to_cpu(mgmt->frame_control) & IEEE80211_FCTL_STYPE;
@@ -971,12 +965,20 @@ void ieee80211_mesh_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 		ieee80211_mesh_rx_mgmt_action(sdata, mgmt, skb->len, rx_status);
 		break;
 	}
+out:
+	sdata_unlock(sdata);
 }
 
 void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 
+	sdata_lock(sdata);
+
+	/* mesh already went down */
+	if (!sdata->wdev.mesh_id_len)
+		goto out;
+
 	if (ifmsh->preq_queue_len &&
 	    time_after(jiffies,
 		       ifmsh->last_preq + msecs_to_jiffies(ifmsh->mshcfg.dot11MeshHWMPpreqMinInterval)))
@@ -996,6 +998,9 @@ void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata)
 
 	if (test_and_clear_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags))
 		mesh_sync_adjust_tbtt(sdata);
+
+out:
+	sdata_unlock(sdata);
 }
 
 void ieee80211_mesh_notify_scan_completed(struct ieee80211_local *local)
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 6c4da99..09bebed 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -517,9 +517,7 @@ void mesh_neighbour_update(struct ieee80211_sub_if_data *sdata,
 	ieee80211_mps_frame_release(sta, elems);
 out:
 	rcu_read_unlock();
-	sdata_lock(sdata);
 	ieee80211_mbss_info_change_notify(sdata, changed);
-	sdata_unlock(sdata);
 }
 
 static void mesh_plink_timer(unsigned long data)
@@ -1070,9 +1068,6 @@ void mesh_rx_plink_frame(struct ieee80211_sub_if_data *sdata,
 
 	rcu_read_unlock();
 
-	if (changed) {
-		sdata_lock(sdata);
+	if (changed)
 		ieee80211_mbss_info_change_notify(sdata, changed);
-		sdata_unlock(sdata);
-	}
 }
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] mac80211: update mesh beacon on workqueue
  2013-06-03 23:25 [PATCH 1/2] mac80211: fix mesh deadlock Thomas Pedersen
@ 2013-06-03 23:25 ` Thomas Pedersen
  2013-06-10  2:41   ` Thomas Pedersen
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Pedersen @ 2013-06-03 23:25 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, open80211s, Thomas Pedersen

Fixes yet another deadlock on calling sta_info_flush()
with the sdata_lock() held. Should make it easier to
reason about locking in the future, since the sdata_lock()
is now held on all mesh work.

Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
---
 net/mac80211/ieee80211_i.h |    1 +
 net/mac80211/mesh.c        |   43 +++++++++++++++++++++++++++++++------------
 net/mac80211/mesh.h        |    2 ++
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 9eed6f1..b4937fc 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -542,6 +542,7 @@ struct ieee80211_if_mesh {
 	struct timer_list mesh_path_root_timer;
 
 	unsigned long wrkq_flags;
+	unsigned long mbss_changed;
 
 	u8 mesh_id[IEEE80211_MAX_MESH_ID_LEN];
 	size_t mesh_id_len;
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index d5faf91..288aad2 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -161,11 +161,8 @@ void mesh_sta_cleanup(struct sta_info *sta)
 		del_timer_sync(&sta->plink_timer);
 	}
 
-	if (changed) {
-		sdata_lock(sdata);
+	if (changed)
 		ieee80211_mbss_info_change_notify(sdata, changed);
-		sdata_unlock(sdata);
-	}
 }
 
 int mesh_rmc_init(struct ieee80211_sub_if_data *sdata)
@@ -719,14 +716,15 @@ ieee80211_mesh_rebuild_beacon(struct ieee80211_sub_if_data *sdata)
 void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata,
 				       u32 changed)
 {
-	if (sdata->vif.bss_conf.enable_beacon &&
-	    (changed & (BSS_CHANGED_BEACON |
-			BSS_CHANGED_HT |
-			BSS_CHANGED_BASIC_RATES |
-			BSS_CHANGED_BEACON_INT)))
-		if (ieee80211_mesh_rebuild_beacon(sdata))
-			return;
-	ieee80211_bss_info_change_notify(sdata, changed);
+	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
+	u32 bit;
+
+	/* if we race with running work, worst case this work becomes a noop */
+	for_each_set_bit(bit, (unsigned long *)&changed,
+			 sizeof(changed) * BITS_PER_BYTE)
+		set_bit(BIT(bit), &ifmsh->mbss_changed);
+	set_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags);
+	ieee80211_queue_work(&sdata->local->hw, &sdata->work);
 }
 
 int ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata)
@@ -969,6 +967,25 @@ out:
 	sdata_unlock(sdata);
 }
 
+static void mesh_bss_info_changed(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
+#define TEST_CHANGED_BIT(bit) \
+	test_and_clear_bit(bit, &ifmsh->mbss_changed)
+
+	u32 changed = TEST_CHANGED_BIT(BSS_CHANGED_BEACON) |
+		      TEST_CHANGED_BIT(BSS_CHANGED_HT) |
+		      TEST_CHANGED_BIT(BSS_CHANGED_BASIC_RATES) |
+		      TEST_CHANGED_BIT(BSS_CHANGED_BEACON_INT);
+#undef TEST_CHANGED_BIT
+
+	if (sdata->vif.bss_conf.enable_beacon && changed)
+		if (ieee80211_mesh_rebuild_beacon(sdata))
+			return;
+
+	ieee80211_bss_info_change_notify(sdata, changed);
+}
+
 void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
@@ -999,6 +1016,8 @@ void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata)
 	if (test_and_clear_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags))
 		mesh_sync_adjust_tbtt(sdata);
 
+	if (test_and_clear_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags))
+		mesh_bss_info_changed(sdata);
 out:
 	sdata_unlock(sdata);
 }
diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
index 8b4d9a3..be28f9b 100644
--- a/net/mac80211/mesh.h
+++ b/net/mac80211/mesh.h
@@ -57,6 +57,7 @@ enum mesh_path_flags {
  * grow
  * @MESH_WORK_ROOT: the mesh root station needs to send a frame
  * @MESH_WORK_DRIFT_ADJUST: time to compensate for clock drift relative to other
+ * @MESH_WORK_MBSS_CHANGED: rebuild beacon and notify driver of BSS changes
  * mesh nodes
  */
 enum mesh_deferred_task_flags {
@@ -65,6 +66,7 @@ enum mesh_deferred_task_flags {
 	MESH_WORK_GROW_MPP_TABLE,
 	MESH_WORK_ROOT,
 	MESH_WORK_DRIFT_ADJUST,
+	MESH_WORK_MBSS_CHANGED,
 };
 
 /**
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] mac80211: update mesh beacon on workqueue
  2013-06-03 23:25 ` [PATCH 2/2] mac80211: update mesh beacon on workqueue Thomas Pedersen
@ 2013-06-10  2:41   ` Thomas Pedersen
  2013-06-10  6:56     ` Johannes Berg
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Pedersen @ 2013-06-10  2:41 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, open80211s, Thomas Pedersen

On Mon, Jun 3, 2013 at 4:25 PM, Thomas Pedersen <thomas@cozybit.com> wrote:
> Fixes yet another deadlock on calling sta_info_flush()
> with the sdata_lock() held. Should make it easier to
> reason about locking in the future, since the sdata_lock()
> is now held on all mesh work.
>
> Signed-off-by: Thomas Pedersen <thomas@cozybit.com>
> ---

[...]

> +static void mesh_bss_info_changed(struct ieee80211_sub_if_data *sdata)
> +{
> +       struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
> +#define TEST_CHANGED_BIT(bit) \
> +       test_and_clear_bit(bit, &ifmsh->mbss_changed)
> +
> +       u32 changed = TEST_CHANGED_BIT(BSS_CHANGED_BEACON) |
> +                     TEST_CHANGED_BIT(BSS_CHANGED_HT) |
> +                     TEST_CHANGED_BIT(BSS_CHANGED_BASIC_RATES) |
> +                     TEST_CHANGED_BIT(BSS_CHANGED_BEACON_INT);
> +#undef TEST_CHANGED_BIT

Uhhh we should read all the bits here. Will fix and resubmit as v2.

--
Thomas

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] mac80211: update mesh beacon on workqueue
  2013-06-10  2:41   ` Thomas Pedersen
@ 2013-06-10  6:56     ` Johannes Berg
  2013-06-10 18:12       ` Thomas Pedersen
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Berg @ 2013-06-10  6:56 UTC (permalink / raw)
  To: Thomas Pedersen; +Cc: linux-wireless, open80211s

On Sun, 2013-06-09 at 19:41 -0700, Thomas Pedersen wrote:

> > +static void mesh_bss_info_changed(struct ieee80211_sub_if_data *sdata)
> > +{
> > +       struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
> > +#define TEST_CHANGED_BIT(bit) \
> > +       test_and_clear_bit(bit, &ifmsh->mbss_changed)
> > +
> > +       u32 changed = TEST_CHANGED_BIT(BSS_CHANGED_BEACON) |
> > +                     TEST_CHANGED_BIT(BSS_CHANGED_HT) |
> > +                     TEST_CHANGED_BIT(BSS_CHANGED_BASIC_RATES) |
> > +                     TEST_CHANGED_BIT(BSS_CHANGED_BEACON_INT);
> > +#undef TEST_CHANGED_BIT
> 
> Uhhh we should read all the bits here. Will fix and resubmit as v2.

Now that you're quoting it I'd also say that the macro isn't really
best, does removing it really make the code so much worse?

johannes


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] mac80211: update mesh beacon on workqueue
  2013-06-10  6:56     ` Johannes Berg
@ 2013-06-10 18:12       ` Thomas Pedersen
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Pedersen @ 2013-06-10 18:12 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, open80211s

On Sun, Jun 9, 2013 at 11:56 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sun, 2013-06-09 at 19:41 -0700, Thomas Pedersen wrote:
>
>> > +static void mesh_bss_info_changed(struct ieee80211_sub_if_data *sdata)
>> > +{
>> > +       struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
>> > +#define TEST_CHANGED_BIT(bit) \
>> > +       test_and_clear_bit(bit, &ifmsh->mbss_changed)
>> > +
>> > +       u32 changed = TEST_CHANGED_BIT(BSS_CHANGED_BEACON) |
>> > +                     TEST_CHANGED_BIT(BSS_CHANGED_HT) |
>> > +                     TEST_CHANGED_BIT(BSS_CHANGED_BASIC_RATES) |
>> > +                     TEST_CHANGED_BIT(BSS_CHANGED_BEACON_INT);
>> > +#undef TEST_CHANGED_BIT
>>
>> Uhhh we should read all the bits here. Will fix and resubmit as v2.
>
> Now that you're quoting it I'd also say that the macro isn't really
> best, does removing it really make the code so much worse?

No, it won't be there in v2. Thanks.

--
Thomas

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2013-06-10 18:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-03 23:25 [PATCH 1/2] mac80211: fix mesh deadlock Thomas Pedersen
2013-06-03 23:25 ` [PATCH 2/2] mac80211: update mesh beacon on workqueue Thomas Pedersen
2013-06-10  2:41   ` Thomas Pedersen
2013-06-10  6:56     ` Johannes Berg
2013-06-10 18:12       ` Thomas Pedersen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.