[PATCH] cfg80211: fix set_monitor_enabled

[PATCH] cfg80211: fix set_monitor_enabled

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When bringing monitor mode up with a driver using the
mac80211 virtual monitor interface this resulted in a
warning because cfg80211_update_iface_num() is called
from PRE_UP, which causes it to call mac80211 and the
low-level driver before the device is started.

For the case that another interface is added and the
monitor interface should be removed, this is correct.
However, in the case where a monitor interface is
added, it's not correct as the above.

To fix this, we need to split up the cases and track
whether or not "only monitor" is active so that the
code can correctly call into the driver when things
change.

Reported-by: Mohammed Shafi <shafi.wireless@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/core.c |   62 ++++++++++++++++++++++++++++++++++++++-------------
 net/wireless/core.h |   10 ++++++++-
 net/wireless/util.c |    8 +++++--
 3 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 0557bb1..b63c450 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -766,30 +766,56 @@ static void cfg80211_init_mon_chan(struct cfg80211_registered_device *rdev)
 }
 
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
-			       enum nl80211_iftype iftype, int num)
+			       enum nl80211_iftype iftype,
+			       enum cfg80211_iface_action action)
 {
-	bool has_monitors_only_old = cfg80211_has_monitors_only(rdev);
-	bool has_monitors_only_new;
+	bool monitors_only;
+	int num;
 
 	ASSERT_RTNL();
 
+	switch (action) {
+	case CFG80211_IFACE_DOWN:
+		num = -1;
+		break;
+	case CFG80211_IFACE_PRE_UP:
+		num = 1;
+		break;
+	case CFG80211_IFACE_UP:
+		num = 0;
+		break;
+	default:
+		WARN_ON(1);
+		return;
+	}
+
 	rdev->num_running_ifaces += num;
 	if (iftype == NL80211_IFTYPE_MONITOR)
 		rdev->num_running_monitor_ifaces += num;
 
-	has_monitors_only_new = cfg80211_has_monitors_only(rdev);
-	if (has_monitors_only_new != has_monitors_only_old) {
+	monitors_only = cfg80211_has_monitors_only(rdev);
+
+	if (monitors_only && rdev->monitor_only_enabled)
+		return;
+
+	if (!monitors_only && !rdev->monitor_only_enabled)
+		return;
+
+	if (monitors_only) {
+		/* Do it in CFG80211_IFACE_UP instead */
+		if (action == CFG80211_IFACE_PRE_UP)
+			return;
 		if (rdev->ops->set_monitor_enabled)
-			rdev->ops->set_monitor_enabled(&rdev->wiphy,
-						       has_monitors_only_new);
-
-		if (!has_monitors_only_new) {
-			rdev->monitor_channel = NULL;
-			rdev->monitor_channel_type = NL80211_CHAN_NO_HT;
-		} else {
-			cfg80211_init_mon_chan(rdev);
-		}
+			rdev->ops->set_monitor_enabled(&rdev->wiphy, true);
+		cfg80211_init_mon_chan(rdev);
+	} else {
+		rdev->monitor_channel = NULL;
+		rdev->monitor_channel_type = NL80211_CHAN_NO_HT;
+		if (rdev->ops->set_monitor_enabled)
+			rdev->ops->set_monitor_enabled(&rdev->wiphy, false);
 	}
+
+	rdev->monitor_only_enabled = monitors_only;
 }
 
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
@@ -895,7 +921,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		wdev->beacon_interval = 0;
 		break;
 	case NETDEV_DOWN:
-		cfg80211_update_iface_num(rdev, wdev->iftype, -1);
+		cfg80211_update_iface_num(rdev, wdev->iftype,
+					  CFG80211_IFACE_DOWN);
 		dev_hold(dev);
 		queue_work(cfg80211_wq, &wdev->cleanup_work);
 		break;
@@ -912,6 +939,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 			mutex_unlock(&rdev->devlist_mtx);
 			dev_put(dev);
 		}
+		cfg80211_update_iface_num(rdev, wdev->iftype,
+					  CFG80211_IFACE_UP);
 		cfg80211_lock_rdev(rdev);
 		mutex_lock(&rdev->devlist_mtx);
 		wdev_lock(wdev);
@@ -1006,7 +1035,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		mutex_unlock(&rdev->devlist_mtx);
 		if (ret)
 			return notifier_from_errno(ret);
-		cfg80211_update_iface_num(rdev, wdev->iftype, 1);
+		cfg80211_update_iface_num(rdev, wdev->iftype,
+					  CFG80211_IFACE_PRE_UP);
 		break;
 	}
 
diff --git a/net/wireless/core.h b/net/wireless/core.h
index bac97da..b2b1db3 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -60,6 +60,7 @@ struct cfg80211_registered_device {
 	/* protected by RTNL only */
 	int num_running_ifaces;
 	int num_running_monitor_ifaces;
+	bool monitor_only_enabled;
 
 	struct ieee80211_channel *monitor_channel;
 	enum nl80211_channel_type monitor_channel_type;
@@ -480,8 +481,15 @@ int ieee80211_get_ratemask(struct ieee80211_supported_band *sband,
 int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 				 u32 beacon_int);
 
+enum cfg80211_iface_action {
+	CFG80211_IFACE_DOWN,
+	CFG80211_IFACE_PRE_UP,
+	CFG80211_IFACE_UP,
+};
+
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
-			       enum nl80211_iftype iftype, int num);
+			       enum nl80211_iftype iftype,
+			       enum cfg80211_iface_action action);
 
 #define CFG80211_MAX_NUM_DIFFERENT_CHANNELS 10
 
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 26f8cd3..37e9a3f 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -893,8 +893,12 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
 	}
 
 	if (!err && ntype != otype && netif_running(dev)) {
-		cfg80211_update_iface_num(rdev, ntype, 1);
-		cfg80211_update_iface_num(rdev, otype, -1);
+		cfg80211_update_iface_num(rdev, ntype,
+					  CFG80211_IFACE_DOWN);
+		cfg80211_update_iface_num(rdev, ntype,
+					  CFG80211_IFACE_PRE_UP);
+		cfg80211_update_iface_num(rdev, otype,
+					  CFG80211_IFACE_UP);
 	}
 
 	return err;
-- 
1.7.10.4

Re: [PATCH] cfg80211: fix set_monitor_enabled

[Eliad Peller]

On Thu, Jul 12, 2012 at 6:15 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> When bringing monitor mode up with a driver using the
> mac80211 virtual monitor interface this resulted in a
> warning because cfg80211_update_iface_num() is called
> from PRE_UP, which causes it to call mac80211 and the
> low-level driver before the device is started.
>
> For the case that another interface is added and the
> monitor interface should be removed, this is correct.
> However, in the case where a monitor interface is
> added, it's not correct as the above.
>
> To fix this, we need to split up the cases and track
> whether or not "only monitor" is active so that the
> code can correctly call into the driver when things
> change.
>
> Reported-by: Mohammed Shafi <shafi.wireless@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
why not always do it only on IFACE_UP?

> @@ -893,8 +893,12 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
>         if (!err && ntype != otype && netif_running(dev)) {
> -               cfg80211_update_iface_num(rdev, ntype, 1);
> -               cfg80211_update_iface_num(rdev, otype, -1);
> +               cfg80211_update_iface_num(rdev, ntype,
> +                                         CFG80211_IFACE_DOWN);
> +               cfg80211_update_iface_num(rdev, ntype,
> +                                         CFG80211_IFACE_PRE_UP);
> +               cfg80211_update_iface_num(rdev, otype,
> +                                         CFG80211_IFACE_UP);

this sequence (down(new_type), pre_up (new_type), up(old_type))
doesn't make any sense to me :)

Eliad.

Re: [PATCH] cfg80211: fix set_monitor_enabled

[Johannes Berg]

On Thu, 2012-07-12 at 19:00 +0300, Eliad Peller wrote:
> On Thu, Jul 12, 2012 at 6:15 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > When bringing monitor mode up with a driver using the
> > mac80211 virtual monitor interface this resulted in a
> > warning because cfg80211_update_iface_num() is called
> > from PRE_UP, which causes it to call mac80211 and the
> > low-level driver before the device is started.
> >
> > For the case that another interface is added and the
> > monitor interface should be removed, this is correct.
> > However, in the case where a monitor interface is
> > added, it's not correct as the above.
> >
> > To fix this, we need to split up the cases and track
> > whether or not "only monitor" is active so that the
> > code can correctly call into the driver when things
> > change.

> why not always do it only on IFACE_UP?

Yeah .. thought so too at first, but it doesn't work, say you have this
situation:

 * up moni0 (monitor)
 * up wlan0 (managed)

Then the set_monitor_enabled(false) should happen before wlan0 is
brought up, otherwise mac80211/the driver will correctly reject wlan0
being brought up.


> > +               cfg80211_update_iface_num(rdev, ntype,
> > +                                         CFG80211_IFACE_DOWN);
> > +               cfg80211_update_iface_num(rdev, ntype,
> > +                                         CFG80211_IFACE_PRE_UP);
> > +               cfg80211_update_iface_num(rdev, otype,
> > +                                         CFG80211_IFACE_UP);
> 
> this sequence (down(new_type), pre_up (new_type), up(old_type))
> doesn't make any sense to me :)

Ouch. Thanks, will fix, it should obviously be down(new_type),
pre_up(new_type), up(new_type).

johannes

Re: [PATCH] cfg80211: fix set_monitor_enabled

[Eliad Peller]

On Thu, Jul 12, 2012 at 7:03 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-07-12 at 19:00 +0300, Eliad Peller wrote:
>> On Thu, Jul 12, 2012 at 6:15 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> >
>> > When bringing monitor mode up with a driver using the
>> > mac80211 virtual monitor interface this resulted in a
>> > warning because cfg80211_update_iface_num() is called
>> > from PRE_UP, which causes it to call mac80211 and the
>> > low-level driver before the device is started.
>> >
>> > For the case that another interface is added and the
>> > monitor interface should be removed, this is correct.
>> > However, in the case where a monitor interface is
>> > added, it's not correct as the above.
>> >
>> > To fix this, we need to split up the cases and track
>> > whether or not "only monitor" is active so that the
>> > code can correctly call into the driver when things
>> > change.
>
>> why not always do it only on IFACE_UP?
>
> Yeah .. thought so too at first, but it doesn't work, say you have this
> situation:
>
>  * up moni0 (monitor)
>  * up wlan0 (managed)
>
> Then the set_monitor_enabled(false) should happen before wlan0 is
> brought up, otherwise mac80211/the driver will correctly reject wlan0
> being brought up.
>
but you don't call set_monitor_enabled(false) on pre_up in your
patch... so i guess the driver will still get its add_interface
callback while the monitor is enabled?

afaict, the only effect of pre_up is updating
rdev->num_running_ifaces/rdev->num_running_monitor_ifaces, which don't
seem to matter much...

Eliad.

Re: [PATCH] cfg80211: fix set_monitor_enabled

[Johannes Berg]

On Thu, 2012-07-12 at 19:26 +0300, Eliad Peller wrote:

> >> why not always do it only on IFACE_UP?
> >
> > Yeah .. thought so too at first, but it doesn't work, say you have this
> > situation:
> >
> >  * up moni0 (monitor)
> >  * up wlan0 (managed)
> >
> > Then the set_monitor_enabled(false) should happen before wlan0 is
> > brought up, otherwise mac80211/the driver will correctly reject wlan0
> > being brought up.
> >
> but you don't call set_monitor_enabled(false) on pre_up in your
> patch...

It does, this is what happens when wlan0 is brought up:

> 	rdev->num_running_ifaces += num;
>         if (iftype == NL80211_IFTYPE_MONITOR)
>                 rdev->num_running_monitor_ifaces += num;

will inc num_running_ifaces by one

>         monitors_only = cfg80211_has_monitors_only(rdev);

will be false, while it was previously true

> 	if (monitors_only && rdev->monitor_only_enabled)
>                 return;

doesn't return, since monitors_only == false

> 	if (!monitors_only && !rdev->monitor_only_enabled)
>                 return;

doesn't return since monitor_only_enabled == true


> 	if (monitors_only) {

no, we'll go into else

> 	} else {
>                 rdev->monitor_channel = NULL;
>                 rdev->monitor_channel_type = NL80211_CHAN_NO_HT;
>                 if (rdev->ops->set_monitor_enabled)
>                         rdev->ops->set_monitor_enabled(&rdev->wiphy, false);
>         }

and call set_monitor_enabled(false)

> so i guess the driver will still get its add_interface
> callback while the monitor is enabled?

add_interface is a long time ago. I'm talking about bringing it up, and
PRE_UP happens before the ndo_open() is called for the interface.

> afaict, the only effect of pre_up is updating
> rdev->num_running_ifaces/rdev->num_running_monitor_ifaces, which don't
> seem to matter much...

No :-)

johannes

Re: [PATCH] cfg80211: fix set_monitor_enabled

[Eliad Peller]

On Thu, Jul 12, 2012 at 7:31 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2012-07-12 at 19:26 +0300, Eliad Peller wrote:
>
>> >> why not always do it only on IFACE_UP?
>> >
>> > Yeah .. thought so too at first, but it doesn't work, say you have this
>> > situation:
>> >
>> >  * up moni0 (monitor)
>> >  * up wlan0 (managed)
>> >
>> > Then the set_monitor_enabled(false) should happen before wlan0 is
>> > brought up, otherwise mac80211/the driver will correctly reject wlan0
>> > being brought up.
>> >
>> but you don't call set_monitor_enabled(false) on pre_up in your
>> patch...
>
> It does, this is what happens when wlan0 is brought up:
>

>>       if (monitors_only) {
>
> no, we'll go into else
>
i completely overlooked this if :)

>> afaict, the only effect of pre_up is updating
>> rdev->num_running_ifaces/rdev->num_running_monitor_ifaces, which don't
>> seem to matter much...
>
> No :-)
>
thanks for the detailed walkthrough :D

Eliad.

Re: [PATCH] cfg80211: fix set_monitor_enabled

[Johannes Berg]

On Thu, 2012-07-12 at 19:37 +0300, Eliad Peller wrote:
> On Thu, Jul 12, 2012 at 7:31 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2012-07-12 at 19:26 +0300, Eliad Peller wrote:
> >
> >> >> why not always do it only on IFACE_UP?
> >> >
> >> > Yeah .. thought so too at first, but it doesn't work, say you have this
> >> > situation:
> >> >
> >> >  * up moni0 (monitor)
> >> >  * up wlan0 (managed)
> >> >
> >> > Then the set_monitor_enabled(false) should happen before wlan0 is
> >> > brought up, otherwise mac80211/the driver will correctly reject wlan0
> >> > being brought up.
> >> >
> >> but you don't call set_monitor_enabled(false) on pre_up in your
> >> patch...
> >
> > It does, this is what happens when wlan0 is brought up:
> >
> 
> >>       if (monitors_only) {
> >
> > no, we'll go into else
> >
> i completely overlooked this if :)
> 
> >> afaict, the only effect of pre_up is updating
> >> rdev->num_running_ifaces/rdev->num_running_monitor_ifaces, which don't
> >> seem to matter much...
> >
> > No :-)
> >
> thanks for the detailed walkthrough :D

:-)

However, I just realised that it's still all broken. We must not do any
accounting in NETDEV_PRE_UP, because it might fail afterwards and we'd
never get NETDEV_UP, but also don't get NETDEV_DOWN or anything else.

It seems we can't fix it and will have to revert the monitor interface
tracking changes in cfg80211/mac80211.

johannes