[RFC 0/5] Fix mac addr enforcement on monitor interfaces

[RFC 0/5] Fix mac addr enforcement on monitor interfaces

[Jakub Kicinski]

From: Jakub Kicinski <kubakici@wp.pl>

It's still possible for users to hurt themselves 
by setting incompatible mac addresses on hardware
that needs conformance to addr_mask. This series
aims to fix that and improve address assignment to
active monitors.

Unfortunately there is a merge conflict in mac80211.git
and mac80211-next.git between:

ac20976dca mac80211: Allow single vif mac address change with addr_mask
and
31eba5bc56 mac80211: support active monitor interfaces

so I got confused and based the series on
wireless-testing.git. Is this acceptable?

Jakub Kicinski (5):
  mac80211: introduce __ieee80211_assign_perm_addr
  mac80211: return new mac address as soon as possible
  mac80211: always default to address compatible with mask
  mac80211: enforce address verification on monitors
  mac80211: assign right mac addr to active monitors

 net/mac80211/cfg.c         |   2 +-
 net/mac80211/ieee80211_i.h |   2 +-
 net/mac80211/iface.c       | 197 ++++++++++++++++++++++-----------------------
 net/mac80211/main.c        |   2 +-
 4 files changed, 99 insertions(+), 104 deletions(-)

-- 
1.8.1.4

[RFC 1/5] mac80211: introduce __ieee80211_assign_perm_addr

[Jakub Kicinski]

From: Jakub Kicinski <kubakici@wp.pl>

Move code from the default case of the big switch in
ieee80211_assign_perm_addr into a separate function.
It will be handy later.

No functional changes.

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 net/mac80211/iface.c | 169 ++++++++++++++++++++++++++-------------------------
 1 file changed, 87 insertions(+), 82 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index a7eba16..dbee397 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1425,8 +1425,8 @@ int ieee80211_if_change_type(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
-				       u8 *perm_addr, enum nl80211_iftype type)
+static void __ieee80211_assign_perm_addr(struct ieee80211_local *local,
+					 u8 *perm_addr)
 {
 	struct ieee80211_sub_if_data *sdata;
 	u64 mask, start, addr, val, inc;
@@ -1434,6 +1434,90 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
 	u8 tmp_addr[ETH_ALEN];
 	int i;
 
+	/* assign a new address if possible -- try n_addresses first */
+	for (i = 0; i < local->hw.wiphy->n_addresses; i++) {
+		bool used = false;
+
+		list_for_each_entry(sdata, &local->interfaces, list) {
+			if (memcmp(local->hw.wiphy->addresses[i].addr,
+				   sdata->vif.addr, ETH_ALEN) == 0) {
+				used = true;
+				break;
+			}
+		}
+
+		if (!used) {
+			memcpy(perm_addr, local->hw.wiphy->addresses[i].addr,
+			       ETH_ALEN);
+			break;
+		}
+	}
+
+	/* try mask if available */
+	if (is_zero_ether_addr(local->hw.wiphy->addr_mask))
+		return;
+
+	m = local->hw.wiphy->addr_mask;
+	mask =	((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
+		((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
+		((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
+
+	if (__ffs64(mask) + hweight64(mask) != fls64(mask)) {
+		/* not a contiguous mask ... not handled now! */
+		pr_info("not contiguous\n");
+		return;
+	}
+
+	/*
+	 * Pick address of existing interface in case user changed
+	 * MAC address manually, default to perm_addr.
+	 */
+	m = local->hw.wiphy->perm_addr;
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
+			continue;
+		m = sdata->vif.addr;
+		break;
+	}
+	start = ((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
+		((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
+		((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
+
+	inc = 1ULL<<__ffs64(mask);
+	val = (start & mask);
+	addr = (start & ~mask) | (val & mask);
+	do {
+		bool used = false;
+
+		tmp_addr[5] = addr >> 0*8;
+		tmp_addr[4] = addr >> 1*8;
+		tmp_addr[3] = addr >> 2*8;
+		tmp_addr[2] = addr >> 3*8;
+		tmp_addr[1] = addr >> 4*8;
+		tmp_addr[0] = addr >> 5*8;
+
+		val += inc;
+
+		list_for_each_entry(sdata, &local->interfaces, list) {
+			if (memcmp(tmp_addr, sdata->vif.addr, ETH_ALEN) == 0) {
+				used = true;
+				break;
+			}
+		}
+
+		if (!used) {
+			memcpy(perm_addr, tmp_addr, ETH_ALEN);
+			break;
+		}
+		addr = (start & ~mask) | (val & mask);
+	} while (addr != start);
+}
+
+static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
+				       u8 *perm_addr, enum nl80211_iftype type)
+{
+	struct ieee80211_sub_if_data *sdata;
+
 	/* default ... something at least */
 	memcpy(perm_addr, local->hw.wiphy->perm_addr, ETH_ALEN);
 
@@ -1472,86 +1556,7 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
 		}
 		/* otherwise fall through */
 	default:
-		/* assign a new address if possible -- try n_addresses first */
-		for (i = 0; i < local->hw.wiphy->n_addresses; i++) {
-			bool used = false;
-
-			list_for_each_entry(sdata, &local->interfaces, list) {
-				if (memcmp(local->hw.wiphy->addresses[i].addr,
-					   sdata->vif.addr, ETH_ALEN) == 0) {
-					used = true;
-					break;
-				}
-			}
-
-			if (!used) {
-				memcpy(perm_addr,
-				       local->hw.wiphy->addresses[i].addr,
-				       ETH_ALEN);
-				break;
-			}
-		}
-
-		/* try mask if available */
-		if (is_zero_ether_addr(local->hw.wiphy->addr_mask))
-			break;
-
-		m = local->hw.wiphy->addr_mask;
-		mask =	((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
-			((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
-			((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
-
-		if (__ffs64(mask) + hweight64(mask) != fls64(mask)) {
-			/* not a contiguous mask ... not handled now! */
-			pr_info("not contiguous\n");
-			break;
-		}
-
-		/*
-		 * Pick address of existing interface in case user changed
-		 * MAC address manually, default to perm_addr.
-		 */
-		m = local->hw.wiphy->perm_addr;
-		list_for_each_entry(sdata, &local->interfaces, list) {
-			if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
-				continue;
-			m = sdata->vif.addr;
-			break;
-		}
-		start = ((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
-			((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
-			((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
-
-		inc = 1ULL<<__ffs64(mask);
-		val = (start & mask);
-		addr = (start & ~mask) | (val & mask);
-		do {
-			bool used = false;
-
-			tmp_addr[5] = addr >> 0*8;
-			tmp_addr[4] = addr >> 1*8;
-			tmp_addr[3] = addr >> 2*8;
-			tmp_addr[2] = addr >> 3*8;
-			tmp_addr[1] = addr >> 4*8;
-			tmp_addr[0] = addr >> 5*8;
-
-			val += inc;
-
-			list_for_each_entry(sdata, &local->interfaces, list) {
-				if (memcmp(tmp_addr, sdata->vif.addr,
-							ETH_ALEN) == 0) {
-					used = true;
-					break;
-				}
-			}
-
-			if (!used) {
-				memcpy(perm_addr, tmp_addr, ETH_ALEN);
-				break;
-			}
-			addr = (start & ~mask) | (val & mask);
-		} while (addr != start);
-
+		__ieee80211_assign_perm_addr(local, perm_addr);
 		break;
 	}
 
-- 
1.8.1.4

[RFC 2/5] mac80211: return new mac address as soon as possible

[Jakub Kicinski]

From: Jakub Kicinski <kubakici@wp.pl>

Once one of wiphy->addresses is found to be unused
we can return from __ieee80211_assign_perm_addr and
use it. The same for masked addresses.

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 net/mac80211/iface.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index dbee397..38898b3 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1449,7 +1449,7 @@ static void __ieee80211_assign_perm_addr(struct ieee80211_local *local,
 		if (!used) {
 			memcpy(perm_addr, local->hw.wiphy->addresses[i].addr,
 			       ETH_ALEN);
-			break;
+			return;
 		}
 	}
 
@@ -1507,10 +1507,12 @@ static void __ieee80211_assign_perm_addr(struct ieee80211_local *local,
 
 		if (!used) {
 			memcpy(perm_addr, tmp_addr, ETH_ALEN);
-			break;
+			return;
 		}
 		addr = (start & ~mask) | (val & mask);
 	} while (addr != start);
+
+	pr_debug("no free address found - using default\n");
 }
 
 static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
-- 
1.8.1.4

[RFC 3/5] mac80211: always default to address compatible with mask

[Jakub Kicinski]

From: Jakub Kicinski <kubakici@wp.pl>

Currently default mac address for new interfaces
is set to perm_addr. It can be addr_mask-wise
incompatible with other addresses if user have
changed address of existing interfaces manualy.

This is especially important after introduction
of active monitors. We have to make sure that
monitor interfaces have mac addresses that fall
into addr_mask, otherwise hardware may not
acknowledge frames.

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 net/mac80211/iface.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 38898b3..1f26980 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1468,17 +1468,7 @@ static void __ieee80211_assign_perm_addr(struct ieee80211_local *local,
 		return;
 	}
 
-	/*
-	 * Pick address of existing interface in case user changed
-	 * MAC address manually, default to perm_addr.
-	 */
-	m = local->hw.wiphy->perm_addr;
-	list_for_each_entry(sdata, &local->interfaces, list) {
-		if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
-			continue;
-		m = sdata->vif.addr;
-		break;
-	}
+	m = perm_addr;
 	start = ((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
 		((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
 		((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
@@ -1529,6 +1519,15 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
 
 	mutex_lock(&local->iflist_mtx);
 
+	/* After manual mac change default addr may no longer fit into mask. */
+	if (!is_zero_ether_addr(local->hw.wiphy->addr_mask) &&
+	    !list_empty(&local->interfaces)) {
+		sdata = list_first_entry(&local->interfaces,
+					 struct ieee80211_sub_if_data,
+					 list);
+		memcpy(perm_addr, sdata->vif.addr, ETH_ALEN);
+	}
+
 	switch (type) {
 	case NL80211_IFTYPE_MONITOR:
 		/* doesn't matter */
-- 
1.8.1.4

[RFC 4/5] mac80211: enforce address verification on monitors

[Jakub Kicinski]

From: Jakub Kicinski <kubakici@wp.pl>

Mac address of passive monitor have to treated
the same as address any other interface because
type of interface can be changed or monitor can
be later put into active mode.

Refusing to change interface type because of
mismatch in mac addresses would likely be
confusing to users.

This requirement only applies to hardware that
sets addr_mask and need to have all addresses
kept within it.

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 net/mac80211/iface.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 1f26980..96bfafa 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -159,8 +159,7 @@ static int ieee80211_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
-static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr,
-				bool check_dup)
+static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr)
 {
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_sub_if_data *iter;
@@ -181,18 +180,11 @@ static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr,
 		((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
 		((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
 
-	if (!check_dup)
-		return ret;
-
 	mutex_lock(&local->iflist_mtx);
 	list_for_each_entry(iter, &local->interfaces, list) {
 		if (iter == sdata)
 			continue;
 
-		if (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
-		    !(sdata->u.mntr_flags & MONITOR_FLAG_ACTIVE))
-			continue;
-
 		m = iter->vif.addr;
 		tmp =	((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
 			((u64)m[2] << 3*8) | ((u64)m[3] << 2*8) |
@@ -212,17 +204,12 @@ static int ieee80211_change_mac(struct net_device *dev, void *addr)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct sockaddr *sa = addr;
-	bool check_dup = true;
 	int ret;
 
 	if (ieee80211_sdata_running(sdata))
 		return -EBUSY;
 
-	if (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
-	    !(sdata->u.mntr_flags & MONITOR_FLAG_ACTIVE))
-		check_dup = false;
-
-	ret = ieee80211_verify_mac(sdata, sa->sa_data, check_dup);
+	ret = ieee80211_verify_mac(sdata, sa->sa_data);
 	if (ret)
 		return ret;
 
-- 
1.8.1.4

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Felix Fietkau]

On 2013-06-07 3:36 PM, Jakub Kicinski wrote:
> From: Jakub Kicinski <kubakici@wp.pl>
> 
> Mac address of passive monitor have to treated
> the same as address any other interface because
> type of interface can be changed or monitor can
> be later put into active mode.
It can't. There's checks preventing that. I'd prefer to leave that part
of the code as it is, the MAC address for normal monitor interfaces
should be ignored completely.

- Felix

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Jakub Kiciński]

On Fri, 07 Jun 2013 15:49:39 +0200, Felix Fietkau wrote:
> On 2013-06-07 3:36 PM, Jakub Kicinski wrote:
>> From: Jakub Kicinski <kubakici@wp.pl>
>> 
>> Mac address of passive monitor have to treated
>> the same as address any other interface because
>> type of interface can be changed or monitor can
>> be later put into active mode.
> It can't. There's checks preventing that. I'd prefer to leave that part
> of the code as it is, the MAC address for normal monitor interfaces
> should be ignored completely.

I must have misread the code regarding active monitors then.
Main motivation behind this patch was the first part though
- user can change type of interface and once ignored address
of monitor now becomes address of AP, sta etc. 

  -- kuba

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Felix Fietkau]

On 2013-06-07 4:04 PM, Jakub Kiciński wrote:
> On Fri, 07 Jun 2013 15:49:39 +0200, Felix Fietkau wrote:
>> On 2013-06-07 3:36 PM, Jakub Kicinski wrote:
>>> From: Jakub Kicinski <kubakici@wp.pl>
>>> 
>>> Mac address of passive monitor have to treated
>>> the same as address any other interface because
>>> type of interface can be changed or monitor can
>>> be later put into active mode.
>> It can't. There's checks preventing that. I'd prefer to leave that part
>> of the code as it is, the MAC address for normal monitor interfaces
>> should be ignored completely.
> 
> I must have misread the code regarding active monitors then.
> Main motivation behind this patch was the first part though
> - user can change type of interface and once ignored address
> of monitor now becomes address of AP, sta etc. 
I'm pretty sure that switching from monitor to something else is also
not supported.

- Felix

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Jakub Kiciński]

On Fri, 07 Jun 2013 16:26:48 +0200, Felix Fietkau wrote:
>On 2013-06-07 4:04 PM, Jakub Kiciński wrote:
>> On Fri, 07 Jun 2013 15:49:39 +0200, Felix Fietkau wrote:
>>> On 2013-06-07 3:36 PM, Jakub Kicinski wrote:
>>>> From: Jakub Kicinski <kubakici@wp.pl>
>>>> 
>>>> Mac address of passive monitor have to treated
>>>> the same as address any other interface because
>>>> type of interface can be changed or monitor can
>>>> be later put into active mode.
>>> It can't. There's checks preventing that. I'd prefer to leave that part
>>> of the code as it is, the MAC address for normal monitor interfaces
>>> should be ignored completely.
>> 
>> I must have misread the code regarding active monitors then.
>> Main motivation behind this patch was the first part though
>> - user can change type of interface and once ignored address
>> of monitor now becomes address of AP, sta etc. 
> I'm pretty sure that switching from monitor to something else is also
> not supported.

Ok, let me explain what I mean to prevent by showing a command
trace - running unpatched wireless-testing and patched iw [1].

# lsmod | grep mac8
# modprobe rt2800usb
# iw dev wlan0 interface add wlan0-1 type managed
# ip link
75: wlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
    link/ether 00:4f:6a:06:57:90 brd ff:ff:ff:ff:ff:ff
76: wlan0-1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN qlen 1000
    link/ether 00:4f:6a:06:57:91 brd ff:ff:ff:ff:ff:ff

Now I can start two hostapd on those interfaces and
everything works just fine. 

# iw dev wlan0-1 set type monitor
# ip link set dev wlan0-1 address 00:00:fa:22:7c:00
# iw dev wlan0-1 set type managed
# ip link
75: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
    link/ether 00:4f:6a:06:57:90 brd ff:ff:ff:ff:ff:ff
76: wlan0-1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
    link/ether 00:00:fa:22:7c:00 brd ff:ff:ff:ff:ff:ff

If I start hostapd on both interfaces now the one on wlan0-1
will not work correctly (hw won't ack frames).

Also I think it's possible to change active flag on a monitor
while it's down (check in net/mac80211/cfg.c:75 only applies
to interfaces that are up):

# iw dev wlan0 interface add wlan0-1 type monitor flags active
# iw dev wlan0-1 set monitor none
# iw dev wlan0-1 set monitor active
# iw dev wlan0-1 set monitor none

  -- kuba

[1] http://moorray.hopto.org/iw_active_monitors.patch

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Johannes Berg]

On Fri, 2013-06-07 at 18:42 +0200, Jakub Kiciński wrote:

> Now I can start two hostapd on those interfaces and
> everything works just fine. 
> 
> # iw dev wlan0-1 set type monitor
> # ip link set dev wlan0-1 address 00:00:fa:22:7c:00
> # iw dev wlan0-1 set type managed
> # ip link
> 75: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
>     link/ether 00:4f:6a:06:57:90 brd ff:ff:ff:ff:ff:ff
> 76: wlan0-1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
>     link/ether 00:00:fa:22:7c:00 brd ff:ff:ff:ff:ff:ff
> 
> If I start hostapd on both interfaces now the one on wlan0-1
> will not work correctly (hw won't ack frames).
> 
> Also I think it's possible to change active flag on a monitor
> while it's down (check in net/mac80211/cfg.c:75 only applies
> to interfaces that are up):

I think we should "just" move ieee80211_verify_mac() into do_open().
Semantically anyway, I'm clearly handwaving a bit. But I would argue
that you can set any MAC address that you like, as long as you don't
bring the interface up, hence the verification really shouldn't be done
when you assign the address but when you bring it up.

Consider also this. Say you have this scenario:

address mask: 00:00:00:00:00:03
wlan0: 02:00:00:00:00:00
wlan1: 02:00:00:00:00:01

Now you want to change to
wlan0: 03:00:00:00:00:00
wlan1: 03:00:00:00:00:01

It seems that right now you can't do this at all, which also seems
wrong.

johannes

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Jakub Kiciński]

On Tue, 11 Jun 2013 13:46:15 +0200, Johannes Berg wrote:
>On Fri, 2013-06-07 at 18:42 +0200, Jakub Kiciński wrote:
>
>> Now I can start two hostapd on those interfaces and
>> everything works just fine. 
>> 
>> # iw dev wlan0-1 set type monitor
>> # ip link set dev wlan0-1 address 00:00:fa:22:7c:00
>> # iw dev wlan0-1 set type managed
>> # ip link
>> 75: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
>>     link/ether 00:4f:6a:06:57:90 brd ff:ff:ff:ff:ff:ff
>> 76: wlan0-1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
>>     link/ether 00:00:fa:22:7c:00 brd ff:ff:ff:ff:ff:ff
>> 
>> If I start hostapd on both interfaces now the one on wlan0-1
>> will not work correctly (hw won't ack frames).
>> 
>> Also I think it's possible to change active flag on a monitor
>> while it's down (check in net/mac80211/cfg.c:75 only applies
>> to interfaces that are up):
>
> I think we should "just" move ieee80211_verify_mac() into do_open().
> Semantically anyway, I'm clearly handwaving a bit. But I would argue
> that you can set any MAC address that you like, as long as you don't
> bring the interface up, hence the verification really shouldn't be done
> when you assign the address but when you bring it up.

I've considered this initially. Two reasons that made me
think the current approach is cleaner are:
 - it's nice when user gets the error during the action that
   puts system in inconsistent state not some time later. I
   personally hate to get vague EBUSY and have to figure out
   what's wrong.
 - suppose there are two interfaces, both down with
   incompatible addresses. User adds third ifc, what address
   should we assign to it?

Besides who would care what address does passive monitor
have? ;)

> Consider also this. Say you have this scenario:
>
> address mask: 00:00:00:00:00:03
> wlan0: 02:00:00:00:00:00
> wlan1: 02:00:00:00:00:01
>
> Now you want to change to
> wlan0: 03:00:00:00:00:00
> wlan1: 03:00:00:00:00:01
>
> It seems that right now you can't do this at all, which also seems
> wrong.

Right but I believe user would understand what is the problem
here and just remove and recreate one of the interfaces. They
have to be down to change MAC addr anyway.

Obviously this change is no matter for a big discussion. I
already feel like stealing your time a bit, because only
rt2x00 cares about this stuff anyway (I think). So please
let me know if my reasoning convinces you and if not I will
be happy to rewrite this the way you suggest.

  -- kuba

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Johannes Berg]

> > I think we should "just" move ieee80211_verify_mac() into do_open().
> > Semantically anyway, I'm clearly handwaving a bit. But I would argue
> > that you can set any MAC address that you like, as long as you don't
> > bring the interface up, hence the verification really shouldn't be done
> > when you assign the address but when you bring it up.
> 
> I've considered this initially. Two reasons that made me
> think the current approach is cleaner are:
>  - it's nice when user gets the error during the action that
>    puts system in inconsistent state not some time later. I
>    personally hate to get vague EBUSY and have to figure out
>    what's wrong.
>  - suppose there are two interfaces, both down with
>    incompatible addresses. User adds third ifc, what address
>    should we assign to it?

Right now you can assign the same addresses to multiple interfaces and
then you can't bring them up. This happens for example if there are no
more addresses to assign.

> Besides who would care what address does passive monitor
> have? ;)

Well those obviously don't matter, which is really the problem here, no?

> > address mask: 00:00:00:00:00:03
> > wlan0: 02:00:00:00:00:00
> > wlan1: 02:00:00:00:00:01
> >
> > Now you want to change to
> > wlan0: 03:00:00:00:00:00
> > wlan1: 03:00:00:00:00:01
> >
> > It seems that right now you can't do this at all, which also seems
> > wrong.
> 
> Right but I believe user would understand what is the problem
> here and just remove and recreate one of the interfaces. They
> have to be down to change MAC addr anyway.

Dunno, you could also argue the other way around, that since they're
down the MAC address really shouldn't matter at all...

johannes

Re: [RFC 4/5] mac80211: enforce address verification on monitors

[Jakub Kiciński]

On Tue, 11 Jun 2013 17:14:12 +0200, Johannes Berg wrote:
>
>>> I think we should "just" move ieee80211_verify_mac() into do_open().
>>> Semantically anyway, I'm clearly handwaving a bit. But I would argue
>>> that you can set any MAC address that you like, as long as you don't
>>> bring the interface up, hence the verification really shouldn't be done
>>> when you assign the address but when you bring it up.
>> 
>> I've considered this initially. Two reasons that made me
>> think the current approach is cleaner are:
>>  - it's nice when user gets the error during the action that
>>    puts system in inconsistent state not some time later. I
>>    personally hate to get vague EBUSY and have to figure out
>>    what's wrong.
>>  - suppose there are two interfaces, both down with
>>    incompatible addresses. User adds third ifc, what address
>>    should we assign to it?
>
> Right now you can assign the same addresses to multiple interfaces and
> then you can't bring them up. This happens for example if there are no
> more addresses to assign.

I didn't realise that. I will move the check into do_open() path as
suggested then.

  -- kuba 

[RFC 5/5] mac80211: assign right mac addr to active monitors

[Jakub Kicinski]

From: Jakub Kicinski <kubakici@wp.pl>

Active monitors should have a valid mac
address as they are supposed to be able
to ack incoming frames.

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 net/mac80211/cfg.c         |  2 +-
 net/mac80211/ieee80211_i.h |  2 +-
 net/mac80211/iface.c       | 12 +++++++-----
 net/mac80211/main.c        |  2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 3062210..123b35f 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -31,7 +31,7 @@ static struct wireless_dev *ieee80211_add_iface(struct wiphy *wiphy,
 	struct ieee80211_sub_if_data *sdata;
 	int err;
 
-	err = ieee80211_if_add(local, name, &wdev, type, params);
+	err = ieee80211_if_add(local, name, &wdev, type, flags, params);
 	if (err)
 		return ERR_PTR(err);
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 923e177..a2fa214 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1345,7 +1345,7 @@ int ieee80211_iface_init(void);
 void ieee80211_iface_exit(void);
 int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 		     struct wireless_dev **new_wdev, enum nl80211_iftype type,
-		     struct vif_params *params);
+		     u32 *flags, struct vif_params *params);
 int ieee80211_if_change_type(struct ieee80211_sub_if_data *sdata,
 			     enum nl80211_iftype type);
 void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 96bfafa..16fc8283 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1493,7 +1493,8 @@ static void __ieee80211_assign_perm_addr(struct ieee80211_local *local,
 }
 
 static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
-				       u8 *perm_addr, enum nl80211_iftype type)
+				       u8 *perm_addr, enum nl80211_iftype type,
+				       u32 *flags)
 {
 	struct ieee80211_sub_if_data *sdata;
 
@@ -1517,7 +1518,8 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
 
 	switch (type) {
 	case NL80211_IFTYPE_MONITOR:
-		/* doesn't matter */
+		if (flags && (*flags & MONITOR_FLAG_ACTIVE))
+			__ieee80211_assign_perm_addr(local, perm_addr);
 		break;
 	case NL80211_IFTYPE_WDS:
 	case NL80211_IFTYPE_AP_VLAN:
@@ -1563,7 +1565,7 @@ static void ieee80211_cleanup_sdata_stas_wk(struct work_struct *wk)
 
 int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 		     struct wireless_dev **new_wdev, enum nl80211_iftype type,
-		     struct vif_params *params)
+		     u32 *flags, struct vif_params *params)
 {
 	struct net_device *ndev = NULL;
 	struct ieee80211_sub_if_data *sdata = NULL;
@@ -1583,7 +1585,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 
 		sdata->dev = NULL;
 		strlcpy(sdata->name, name, IFNAMSIZ);
-		ieee80211_assign_perm_addr(local, wdev->address, type);
+		ieee80211_assign_perm_addr(local, wdev->address, type, flags);
 		memcpy(sdata->vif.addr, wdev->address, ETH_ALEN);
 	} else {
 		if (local->hw.queues >= IEEE80211_NUM_ACS)
@@ -1611,7 +1613,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
 			return ret;
 		}
 
-		ieee80211_assign_perm_addr(local, ndev->perm_addr, type);
+		ieee80211_assign_perm_addr(local, ndev->perm_addr, type, flags);
 		memcpy(ndev->dev_addr, ndev->perm_addr, ETH_ALEN);
 		SET_NETDEV_DEV(ndev, wiphy_dev(local->hw.wiphy));
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 1998f14..edaca0f 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -968,7 +968,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 	/* add one default STA interface if supported */
 	if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION)) {
 		result = ieee80211_if_add(local, "wlan%d", NULL,
-					  NL80211_IFTYPE_STATION, NULL);
+					  NL80211_IFTYPE_STATION, NULL, NULL);
 		if (result)
 			wiphy_warn(local->hw.wiphy,
 				   "Failed to add default virtual iface\n");
-- 
1.8.1.4