linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] mac80211: fix rx monitor filter refcounters
@ 2010-09-28 16:36 Christian Lamparter
  2010-09-29  7:10 ` Johannes Berg
  0 siblings, 1 reply; 7+ messages in thread
From: Christian Lamparter @ 2010-09-28 16:36 UTC (permalink / raw)
  To: linux-wireless; +Cc: Johannes Berg

This patch fixes a refcounter & commit bug when monitor
rx flags are changed by:
	iw dev moni set monitor [new flags]

while interface is up.

---
Is there a sane way to do that?

Introduced by: 8cc9a73914 - Jan 31 19:48:23 2008 
"mac80211: Use monitor configuration flags"
---
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c981604..c156936 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -68,8 +68,47 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
 		 params && params->use_4addr >= 0)
 		sdata->u.mgd.use_4addr = params->use_4addr;
 
-	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
+	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
+		struct ieee80211_local *local = sdata->local;
+		u32 changed_flags = sdata->u.mntr_flags ^ *flags;
+
 		sdata->u.mntr_flags = *flags;
+		if (changed_flags & MONITOR_FLAG_FCSFAIL) {
+			if (*flags & MONITOR_FLAG_FCSFAIL)
+				local->fif_fcsfail++;
+			else
+				local->fif_fcsfail--;
+		}
+		if (changed_flags & MONITOR_FLAG_PLCPFAIL) {
+			if (*flags & MONITOR_FLAG_PLCPFAIL)
+				local->fif_plcpfail++;
+			else
+				local->fif_plcpfail--;
+		}
+		if (changed_flags & MONITOR_FLAG_COOK_FRAMES) {
+			if (*flags & MONITOR_FLAG_COOK_FRAMES)
+				local->cooked_mntrs++;
+			else
+				local->cooked_mntrs--;
+		}
+		if (changed_flags & MONITOR_FLAG_OTHER_BSS) {
+			if (*flags & MONITOR_FLAG_OTHER_BSS)
+				local->fif_other_bss++;
+			else
+				local->fif_other_bss--;
+		}
+		if (changed_flags & MONITOR_FLAG_CONTROL) {
+			if (*flags & MONITOR_FLAG_CONTROL) {
+				local->fif_pspoll++;
+				local->fif_control++;
+			} else {
+				local->fif_pspoll--;
+				local->fif_control--;
+			}
+		}
+
+		ieee80211_configure_filter(local);
+	}
 
 	return 0;
 }

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

* Re: [RFC] mac80211: fix rx monitor filter refcounters
  2010-09-28 16:36 [RFC] mac80211: fix rx monitor filter refcounters Christian Lamparter
@ 2010-09-29  7:10 ` Johannes Berg
  2010-09-29 19:57   ` [RFC v2] " Christian Lamparter
  0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2010-09-29  7:10 UTC (permalink / raw)
  To: Christian Lamparter; +Cc: linux-wireless

On Tue, 2010-09-28 at 18:36 +0200, Christian Lamparter wrote:
> This patch fixes a refcounter & commit bug when monitor
> rx flags are changed by:
> 	iw dev moni set monitor [new flags]
> 
> while interface is up.
> 
> ---
> Is there a sane way to do that?

Is this not sane enough? Looks OK to me, even if it adds a bit of code.

> -	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
> +	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
> +		struct ieee80211_local *local = sdata->local;
> +		u32 changed_flags = sdata->u.mntr_flags ^ *flags;
> +
>  		sdata->u.mntr_flags = *flags;
> +		if (changed_flags & MONITOR_FLAG_FCSFAIL) {
> +			if (*flags & MONITOR_FLAG_FCSFAIL)
> +				local->fif_fcsfail++;
> +			else
> +				local->fif_fcsfail--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_PLCPFAIL) {
> +			if (*flags & MONITOR_FLAG_PLCPFAIL)
> +				local->fif_plcpfail++;
> +			else
> +				local->fif_plcpfail--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_COOK_FRAMES) {
> +			if (*flags & MONITOR_FLAG_COOK_FRAMES)
> +				local->cooked_mntrs++;
> +			else
> +				local->cooked_mntrs--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_OTHER_BSS) {
> +			if (*flags & MONITOR_FLAG_OTHER_BSS)
> +				local->fif_other_bss++;
> +			else
> +				local->fif_other_bss--;
> +		}
> +		if (changed_flags & MONITOR_FLAG_CONTROL) {
> +			if (*flags & MONITOR_FLAG_CONTROL) {
> +				local->fif_pspoll++;
> +				local->fif_control++;
> +			} else {
> +				local->fif_pspoll--;
> +				local->fif_control--;
> +			}
> +		}

Although, come to think of it, one could do something like this:

static void adjust_flags(local, flags, offset)
{
#define ADJUST(_flg, _fif)	do { 		\
	if (flags & MONITOR_FLAG_#_flg)		\
		local->fif_#_fif += offset;	\
	} while (0)

	ADJUST(FCSFAIL, fcsfail);
	ADJUST(PLCPFAIL, plcpfail);
	ADJUST(CONTROL, control);
	ADJUST(CONTROL, pspoll);
	ADJUST(OTHER_BSS, other_bss);
#undef ADJUST
}

and then we can have four callers of this function.

Two here:
adjust_flags(local, sdata->u.mntr_flags, -1);
adjust_flags(local, *flags, 1)
sdata->u.mntr_flags = *flags;

and the same two in ieee80211_do_open / ieee80211_do_stop.

johannes


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

* [RFC v2] mac80211: fix rx monitor filter refcounters
  2010-09-29  7:10 ` Johannes Berg
@ 2010-09-29 19:57   ` Christian Lamparter
  2010-09-29 20:00     ` Johannes Berg
  0 siblings, 1 reply; 7+ messages in thread
From: Christian Lamparter @ 2010-09-29 19:57 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On Wednesday 29 September 2010 09:10:18 Johannes Berg wrote:
> On Tue, 2010-09-28 at 18:36 +0200, Christian Lamparter wrote:
> > This patch fixes a refcounter & commit bug when monitor
> > rx flags are changed by:
> > 	iw dev moni set monitor [new flags]
> > 
> > while interface is up.
> > 
> > ---
> > Is there a sane way to do that?
> 
> Is this not sane enough? Looks OK to me, even if it adds a bit of code.
> 

It's about MONITOR_FLAG_COOK_FRAMES. This flag gives me headaches.
I wish we could make this flag "const" and don't allow it be changed
by iw dev wlanX set monitor. Another alternative would be to move
the "sdata->u.mntr_flag -> fif_* processing" into
ieee80211_configure_filter. 

or: 
---
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c981604..1ffe266 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -68,9 +68,61 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
 		 params && params->use_4addr >= 0)
 		sdata->u.mgd.use_4addr = params->use_4addr;
 
-	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
+	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
+		struct ieee80211_local *local = sdata->local;
+		u32 changed_flags;
+		u32 old_flags;
+		u32 hw_reconf_flags = 0;
+
+		old_flags = sdata->u.mntr_flags;
+		changed_flags = old_flags ^ *flags;
+		if (!(old_flags & MONITOR_FLAG_COOK_FRAMES))
+			ieee80211_adjust_monitor_flags(sdata, -1);
+
+		if (changed_flags & MONITOR_FLAG_COOK_FRAMES) {
+			if (*flags & MONITOR_FLAG_COOK_FRAMES) {
+				local->cooked_mntrs++;
+				local->monitors--;
+			} else {
+				local->monitors++;
+				local->cooked_mntrs--;
+
+				changed_flags |=
+					old_flags & ~MONITOR_FLAG_COOK_FRAMES;
+			}
+
+			switch (local->monitors) {
+			case 0:
+				local->hw.conf.flags &=
+					~IEEE80211_CONF_MONITOR;
+				hw_reconf_flags |=
+					IEEE80211_CONF_CHANGE_MONITOR;
+				break;
+
+			case 1:
+				local->hw.conf.flags |=
+					IEEE80211_CONF_MONITOR;
+				hw_reconf_flags |=
+					 IEEE80211_CONF_CHANGE_MONITOR;
+				break;
+
+			default:
+				break;
+			}
+		}
+
 		sdata->u.mntr_flags = *flags;
 
+		if (!(*flags & MONITOR_FLAG_COOK_FRAMES))
+			ieee80211_adjust_monitor_flags(sdata, 1);
+
+		if (changed_flags)
+			ieee80211_configure_filter(local);
+
+		if (hw_reconf_flags)
+			ieee80211_hw_config(local, hw_reconf_flags);
+	}
+
 	return 0;
 }
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 945fbf2..f6a6d78 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1132,6 +1132,8 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata);
 void ieee80211_remove_interfaces(struct ieee80211_local *local);
 u32 __ieee80211_recalc_idle(struct ieee80211_local *local);
 void ieee80211_recalc_idle(struct ieee80211_local *local);
+void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
+				    const int offset);
 
 static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 6678573..d59b0be 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -148,6 +148,26 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
+void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
+				    const int offset)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 flags = sdata->u.mntr_flags;
+
+#define ADJUST(_f, _s)	do {					\
+	if (flags & MONITOR_FLAG_##_f)				\
+		local->fif_##_s += offset;				\
+	} while (0)
+
+	ADJUST(FCSFAIL, fcsfail);
+	ADJUST(PLCPFAIL, plcpfail);
+	ADJUST(CONTROL, control);
+	ADJUST(CONTROL, pspoll);
+	ADJUST(OTHER_BSS, other_bss);
+
+#undef ADJUST
+}
+
 /*
  * NOTE: Be very careful when changing this function, it must NOT return
  * an error on interface type changes that have been pre-checked, so most
@@ -240,17 +260,7 @@ static int ieee80211_do_open(struct net_device *dev, bool coming_up)
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
-		if (sdata->u.mntr_flags & MONITOR_FLAG_FCSFAIL)
-			local->fif_fcsfail++;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_PLCPFAIL)
-			local->fif_plcpfail++;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_CONTROL) {
-			local->fif_control++;
-			local->fif_pspoll++;
-		}
-		if (sdata->u.mntr_flags & MONITOR_FLAG_OTHER_BSS)
-			local->fif_other_bss++;
-
+		ieee80211_adjust_monitor_flags(sdata, 1);
 		ieee80211_configure_filter(local);
 
 		netif_carrier_on(dev);
@@ -477,17 +487,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
-		if (sdata->u.mntr_flags & MONITOR_FLAG_FCSFAIL)
-			local->fif_fcsfail--;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_PLCPFAIL)
-			local->fif_plcpfail--;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_CONTROL) {
-			local->fif_pspoll--;
-			local->fif_control--;
-		}
-		if (sdata->u.mntr_flags & MONITOR_FLAG_OTHER_BSS)
-			local->fif_other_bss--;
-
+		ieee80211_adjust_monitor_flags(sdata, -1);
 		ieee80211_configure_filter(local);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:

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

* Re: [RFC v2] mac80211: fix rx monitor filter refcounters
  2010-09-29 19:57   ` [RFC v2] " Christian Lamparter
@ 2010-09-29 20:00     ` Johannes Berg
  2010-10-01 21:53       ` [PATCH] " Christian Lamparter
  0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2010-09-29 20:00 UTC (permalink / raw)
  To: Christian Lamparter; +Cc: linux-wireless

On Wed, 2010-09-29 at 21:57 +0200, Christian Lamparter wrote:

> > > Is there a sane way to do that?
> > 
> > Is this not sane enough? Looks OK to me, even if it adds a bit of code.
> > 
> 
> It's about MONITOR_FLAG_COOK_FRAMES. This flag gives me headaches.
> I wish we could make this flag "const" and don't allow it be changed
> by iw dev wlanX set monitor. 

Ah. Then you can only specify it when creating an interface, right? Fine
with me, really.

> Another alternative would be to move
> the "sdata->u.mntr_flag -> fif_* processing" into
> ieee80211_configure_filter. 
> 
> or: 
> ---
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index c981604..1ffe266 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -68,9 +68,61 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
>  		 params && params->use_4addr >= 0)
>  		sdata->u.mgd.use_4addr = params->use_4addr;
>  
> -	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
> +	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
> +		struct ieee80211_local *local = sdata->local;
> +		u32 changed_flags;
> +		u32 old_flags;
> +		u32 hw_reconf_flags = 0;
> +
> +		old_flags = sdata->u.mntr_flags;
> +		changed_flags = old_flags ^ *flags;
> +		if (!(old_flags & MONITOR_FLAG_COOK_FRAMES))
> +			ieee80211_adjust_monitor_flags(sdata, -1);
> +
> +		if (changed_flags & MONITOR_FLAG_COOK_FRAMES) {
> +			if (*flags & MONITOR_FLAG_COOK_FRAMES) {
> +				local->cooked_mntrs++;
> +				local->monitors--;

Yeah, that's kinda ugly. I'm fine with not allowing the cook flag to
change, seems like a pretty special case anyway. Or, we can allow it to
change, but only while the interface is down, right?

johannes


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

* [PATCH] mac80211: fix rx monitor filter refcounters
  2010-09-29 20:00     ` Johannes Berg
@ 2010-10-01 21:53       ` Christian Lamparter
  2010-10-02  8:46         ` Johannes Berg
  0 siblings, 1 reply; 7+ messages in thread
From: Christian Lamparter @ 2010-10-01 21:53 UTC (permalink / raw)
  To: linux-wireless; +Cc: John W. Linville, Johannes Berg

This patch fixes a refcounter & commit bug when monitor
rx flags are changed by:
        iw dev moni set monitor [new flags]

while interface is up.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
On Wednesday 29 September 2010 22:00:39 Johannes wrote:
> On Wed, 2010-09-29 at 21:57 +0200, Christian wrote:
> > It's about MONITOR_FLAG_COOK_FRAMES. This flag gives me
> > headaches. I wish we could make this flag "const" and
> > don't allow it be changed by iw dev wlanX set monitor.
>  
> I'm fine with not allowing the cook flag to change,
> seems like a pretty special case anyway. Or, we can
> allow it to change, but only while the interface is
> down, right?
jup, works:

command failed: Function not implemented (-38), or should
we use a different error code like -EBUSY/-EOPNOTSUPP?
---
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c981604..554166b 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -68,8 +68,36 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
 		 params && params->use_4addr >= 0)
 		sdata->u.mgd.use_4addr = params->use_4addr;
 
-	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
-		sdata->u.mntr_flags = *flags;
+	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
+		struct ieee80211_local *local = sdata->local;
+
+		if (test_bit(SDATA_STATE_RUNNING, &sdata->state)) {
+			/*
+			 * Prohibit MONITOR_FLAG_COOK_FRAMES to be
+			 * changed while the interface is up.
+			 * Else we would need to add a lot of cruft
+			 * to update everything:
+			 *	cooked_mntrs, monitor and all fif_* counters
+			 *	reconfigure hardware
+			 */
+			if ((*flags & MONITOR_FLAG_COOK_FRAMES) !=
+			    (sdata->u.mntr_flags & MONITOR_FLAG_COOK_FRAMES))
+				return -ENOSYS;
+
+			ieee80211_adjust_monitor_flags(sdata, -1);
+			sdata->u.mntr_flags = *flags;
+			ieee80211_adjust_monitor_flags(sdata, 1);
+
+			ieee80211_configure_filter(local);
+		} else {
+			/*
+			 * Because the interface is down, ieee80211_do_stop
+			 * and ieee80211_do_open take care of "everything"
+			 * mentioned in the comment above.
+			 */
+			sdata->u.mntr_flags = *flags;
+		}
+	}
 
 	return 0;
 }
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 945fbf2..f6a6d78 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1132,6 +1132,8 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata);
 void ieee80211_remove_interfaces(struct ieee80211_local *local);
 u32 __ieee80211_recalc_idle(struct ieee80211_local *local);
 void ieee80211_recalc_idle(struct ieee80211_local *local);
+void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
+				    const int offset);
 
 static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 6678573..1300e88 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -148,6 +148,26 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
+void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
+				    const int offset)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 flags = sdata->u.mntr_flags;
+
+#define ADJUST(_f, _s)	do {					\
+	if (flags & MONITOR_FLAG_##_f)				\
+		local->fif_##_s += offset;			\
+	} while (0)
+
+	ADJUST(FCSFAIL, fcsfail);
+	ADJUST(PLCPFAIL, plcpfail);
+	ADJUST(CONTROL, control);
+	ADJUST(CONTROL, pspoll);
+	ADJUST(OTHER_BSS, other_bss);
+
+#undef ADJUST
+}
+
 /*
  * NOTE: Be very careful when changing this function, it must NOT return
  * an error on interface type changes that have been pre-checked, so most
@@ -240,17 +260,7 @@ static int ieee80211_do_open(struct net_device *dev, bool coming_up)
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
-		if (sdata->u.mntr_flags & MONITOR_FLAG_FCSFAIL)
-			local->fif_fcsfail++;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_PLCPFAIL)
-			local->fif_plcpfail++;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_CONTROL) {
-			local->fif_control++;
-			local->fif_pspoll++;
-		}
-		if (sdata->u.mntr_flags & MONITOR_FLAG_OTHER_BSS)
-			local->fif_other_bss++;
-
+		ieee80211_adjust_monitor_flags(sdata, 1);
 		ieee80211_configure_filter(local);
 
 		netif_carrier_on(dev);
@@ -477,17 +487,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
-		if (sdata->u.mntr_flags & MONITOR_FLAG_FCSFAIL)
-			local->fif_fcsfail--;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_PLCPFAIL)
-			local->fif_plcpfail--;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_CONTROL) {
-			local->fif_pspoll--;
-			local->fif_control--;
-		}
-		if (sdata->u.mntr_flags & MONITOR_FLAG_OTHER_BSS)
-			local->fif_other_bss--;
-
+		ieee80211_adjust_monitor_flags(sdata, -1);
 		ieee80211_configure_filter(local);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:

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

* Re: [PATCH] mac80211: fix rx monitor filter refcounters
  2010-10-01 21:53       ` [PATCH] " Christian Lamparter
@ 2010-10-02  8:46         ` Johannes Berg
  2010-10-02 11:17           ` [PATCH v2] " Christian Lamparter
  0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2010-10-02  8:46 UTC (permalink / raw)
  To: Christian Lamparter, linux-wireless; +Cc: John W. Linville

> > I'm fine with not allowing the cook flag to change,
> > seems like a pretty special case anyway. Or, we can
> > allow it to change, but only while the interface is
> > down, right?
> jup, works:
> 
> command failed: Function not implemented (-38), or should
> we use a different error code like -EBUSY/-EOPNOTSUPP?

I think typically I'd use ebusy, and I think it'd be more descriptive here?

> +        if (test_bit(SDATA_STATE_RUNNING, &sdata->state)) {

There's an inline for this, I think?

>   static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data

heh, by accident it's even in your patch :)

ACK with those two changes.

Johannes

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

* [PATCH v2] mac80211: fix rx monitor filter refcounters
  2010-10-02  8:46         ` Johannes Berg
@ 2010-10-02 11:17           ` Christian Lamparter
  0 siblings, 0 replies; 7+ messages in thread
From: Christian Lamparter @ 2010-10-02 11:17 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless, John W. Linville

This patch fixes an refcounting bug. Previously it
was possible to corrupt the per-device recv. filter
and monitor management counters when:
	iw dev wlanX set monitor [new flags]
was issued on an active monitor interface.

Acked-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
On Saturday 02 October 2010 10:46:27 Johannes Berg wrote:
> > +        if (test_bit(SDATA_STATE_RUNNING, &sdata->state)) {
> 
> There's an inline for this, I think?
> 
> >   static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data
> 
> heh, by accident it's even in your patch :)
but what an accident!
---
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c981604..9e63fc2 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -68,8 +68,36 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
 		 params && params->use_4addr >= 0)
 		sdata->u.mgd.use_4addr = params->use_4addr;
 
-	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags)
-		sdata->u.mntr_flags = *flags;
+	if (sdata->vif.type == NL80211_IFTYPE_MONITOR && flags) {
+		struct ieee80211_local *local = sdata->local;
+
+		if (ieee80211_sdata_running(sdata)) {
+			/*
+			 * Prohibit MONITOR_FLAG_COOK_FRAMES to be
+			 * changed while the interface is up.
+			 * Else we would need to add a lot of cruft
+			 * to update everything:
+			 *	cooked_mntrs, monitor and all fif_* counters
+			 *	reconfigure hardware
+			 */
+			if ((*flags & MONITOR_FLAG_COOK_FRAMES) !=
+			    (sdata->u.mntr_flags & MONITOR_FLAG_COOK_FRAMES))
+				return -EBUSY;
+
+			ieee80211_adjust_monitor_flags(sdata, -1);
+			sdata->u.mntr_flags = *flags;
+			ieee80211_adjust_monitor_flags(sdata, 1);
+
+			ieee80211_configure_filter(local);
+		} else {
+			/*
+			 * Because the interface is down, ieee80211_do_stop
+			 * and ieee80211_do_open take care of "everything"
+			 * mentioned in the comment above.
+			 */
+			sdata->u.mntr_flags = *flags;
+		}
+	}
 
 	return 0;
 }
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 945fbf2..f6a6d78 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1132,6 +1132,8 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata);
 void ieee80211_remove_interfaces(struct ieee80211_local *local);
 u32 __ieee80211_recalc_idle(struct ieee80211_local *local);
 void ieee80211_recalc_idle(struct ieee80211_local *local);
+void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
+				    const int offset);
 
 static inline bool ieee80211_sdata_running(struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 6678573..1300e88 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -148,6 +148,26 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
+void ieee80211_adjust_monitor_flags(struct ieee80211_sub_if_data *sdata,
+				    const int offset)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 flags = sdata->u.mntr_flags;
+
+#define ADJUST(_f, _s)	do {					\
+	if (flags & MONITOR_FLAG_##_f)				\
+		local->fif_##_s += offset;			\
+	} while (0)
+
+	ADJUST(FCSFAIL, fcsfail);
+	ADJUST(PLCPFAIL, plcpfail);
+	ADJUST(CONTROL, control);
+	ADJUST(CONTROL, pspoll);
+	ADJUST(OTHER_BSS, other_bss);
+
+#undef ADJUST
+}
+
 /*
  * NOTE: Be very careful when changing this function, it must NOT return
  * an error on interface type changes that have been pre-checked, so most
@@ -240,17 +260,7 @@ static int ieee80211_do_open(struct net_device *dev, bool coming_up)
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
-		if (sdata->u.mntr_flags & MONITOR_FLAG_FCSFAIL)
-			local->fif_fcsfail++;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_PLCPFAIL)
-			local->fif_plcpfail++;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_CONTROL) {
-			local->fif_control++;
-			local->fif_pspoll++;
-		}
-		if (sdata->u.mntr_flags & MONITOR_FLAG_OTHER_BSS)
-			local->fif_other_bss++;
-
+		ieee80211_adjust_monitor_flags(sdata, 1);
 		ieee80211_configure_filter(local);
 
 		netif_carrier_on(dev);
@@ -477,17 +487,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
-		if (sdata->u.mntr_flags & MONITOR_FLAG_FCSFAIL)
-			local->fif_fcsfail--;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_PLCPFAIL)
-			local->fif_plcpfail--;
-		if (sdata->u.mntr_flags & MONITOR_FLAG_CONTROL) {
-			local->fif_pspoll--;
-			local->fif_control--;
-		}
-		if (sdata->u.mntr_flags & MONITOR_FLAG_OTHER_BSS)
-			local->fif_other_bss--;
-
+		ieee80211_adjust_monitor_flags(sdata, -1);
 		ieee80211_configure_filter(local);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:

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

end of thread, other threads:[~2010-10-02 11:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-28 16:36 [RFC] mac80211: fix rx monitor filter refcounters Christian Lamparter
2010-09-29  7:10 ` Johannes Berg
2010-09-29 19:57   ` [RFC v2] " Christian Lamparter
2010-09-29 20:00     ` Johannes Berg
2010-10-01 21:53       ` [PATCH] " Christian Lamparter
2010-10-02  8:46         ` Johannes Berg
2010-10-02 11:17           ` [PATCH v2] " Christian Lamparter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).