* Re: [PATCH] mac80211: Allow single vif mac address change with addr_mask
2013-05-21 12:54 [PATCH] mac80211: Allow single vif mac address change with addr_mask Helmut Schaa
@ 2013-05-21 15:10 ` Jakub Kiciński
2013-05-22 8:06 ` Helmut Schaa
2013-05-21 15:51 ` R: " Alessandro Lannocca
2013-05-27 8:43 ` [PATCHv2] " Helmut Schaa
2 siblings, 1 reply; 8+ messages in thread
From: Jakub Kiciński @ 2013-05-21 15:10 UTC (permalink / raw)
To: Helmut Schaa
Cc: linux-wireless, johannes, IvDoorn, gwingerde, stf_xl,
Alessandro Lannocca, Bruno Randolf
On Tue, 21 May 2013 14:54:53 +0200, Helmut Schaa wrote:
> When changing the MAC address of a single vif mac80211 will check if the new
> address fits into the address mask specified by the driver. This only needs
> to be done when using multiple BSSIDs. Hence, check the new address only
> against all other vifs.
Oh, I see that you already posted this patch for review!
I think we should also take care of the way addresses for new
interfaces are chosen otherwise they'll just pick up
perm_addr and we will end up with incompatible MACs.
I created a patch for a user who reported this problem to
rt2x00 ML independently from Helmut.
--->8------------------
>From 63b32f4509d2c2bcf0ad791faa624b56a1947748 Mon Sep 17 00:00:00 2001
From: Jakub Kicinski <kubakici@wp.pl>
Date: Tue, 21 May 2013 13:05:43 +0200
Subject: [PATCH] mac80211: allow changing mac address on mac-masking devices
Some devices support multiple MAC addresses by
masking last few bits of the address. Address
compatibility has to be checked only when there
is more than one interface - otherwise we can
just overwrite current address.
While at it fix inverted flow at the end of
ieee80211_change_mac.
Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
net/mac80211/iface.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 60f1ce5..367aa3b 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -159,8 +159,9 @@ static int ieee80211_change_mtu(struct net_device *dev, int new_mtu)
return 0;
}
-static int ieee80211_verify_mac(struct ieee80211_local *local, u8 *addr)
+static int ieee80211_verify_mac(struct ieee80211_sub_if_data *target, u8 *addr)
{
+ struct ieee80211_local *local = target->local;
struct ieee80211_sub_if_data *sdata;
u64 new, mask, tmp;
u8 *m;
@@ -184,6 +185,8 @@ static int ieee80211_verify_mac(struct ieee80211_local *local, u8 *addr)
list_for_each_entry(sdata, &local->interfaces, list) {
if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
continue;
+ if (sdata == target)
+ continue;
m = sdata->vif.addr;
tmp = ((u64)m[0] << 5*8) | ((u64)m[1] << 4*8) |
@@ -209,14 +212,15 @@ static int ieee80211_change_mac(struct net_device *dev, void *addr)
if (ieee80211_sdata_running(sdata))
return -EBUSY;
- ret = ieee80211_verify_mac(sdata->local, sa->sa_data);
+ ret = ieee80211_verify_mac(sdata, sa->sa_data);
if (ret)
return ret;
ret = eth_mac_addr(dev, sa);
+ if (ret)
+ return ret;
- if (ret == 0)
- memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
+ memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
return ret;
}
@@ -1479,7 +1483,17 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
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);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] mac80211: Allow single vif mac address change with addr_mask
2013-05-21 15:10 ` Jakub Kiciński
@ 2013-05-22 8:06 ` Helmut Schaa
2013-05-22 12:42 ` Jakub Kiciński
0 siblings, 1 reply; 8+ messages in thread
From: Helmut Schaa @ 2013-05-22 8:06 UTC (permalink / raw)
To: Jakub Kiciński
Cc: linux-wireless, Johannes Berg, Ivo Van Doorn,
Gertjan van Wingerde, stf_xl@wp.pl, Alessandro Lannocca,
Bruno Randolf
On Tue, May 21, 2013 at 5:10 PM, Jakub Kiciński <moorray3@wp.pl> wrote:
> On Tue, 21 May 2013 14:54:53 +0200, Helmut Schaa wrote:
>> When changing the MAC address of a single vif mac80211 will check if the new
>> address fits into the address mask specified by the driver. This only needs
>> to be done when using multiple BSSIDs. Hence, check the new address only
>> against all other vifs.
>
> Oh, I see that you already posted this patch for review!
>
> I think we should also take care of the way addresses for new
> interfaces are chosen otherwise they'll just pick up
> perm_addr and we will end up with incompatible MACs.
Maybe you could send the assign_pem_addr changes as a follow up patch?
[...]
> @@ -1479,7 +1483,17 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
> 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);
This is only relevant if the driver registered a addr_mask with mac80211.
So, maybe you could only select a new address (!=perm_addr) if the
perm_addr is not covered by the addr_mask?
Helmut
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mac80211: Allow single vif mac address change with addr_mask
2013-05-22 8:06 ` Helmut Schaa
@ 2013-05-22 12:42 ` Jakub Kiciński
2013-05-23 13:32 ` Helmut Schaa
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Kiciński @ 2013-05-22 12:42 UTC (permalink / raw)
To: Helmut Schaa
Cc: linux-wireless, Johannes Berg, Ivo Van Doorn,
Gertjan van Wingerde, stf_xl@wp.pl, Alessandro Lannocca,
Bruno Randolf
On Wed, 22 May 2013 10:06:26 +0200, Helmut Schaa wrote:
> On Tue, May 21, 2013 at 5:10 PM, Jakub Kiciński <moorray3@wp.pl> wrote:
> > On Tue, 21 May 2013 14:54:53 +0200, Helmut Schaa wrote:
> >> When changing the MAC address of a single vif mac80211 will check if the new
> >> address fits into the address mask specified by the driver. This only needs
> >> to be done when using multiple BSSIDs. Hence, check the new address only
> >> against all other vifs.
> >
> > Oh, I see that you already posted this patch for review!
> >
> > I think we should also take care of the way addresses for new
> > interfaces are chosen otherwise they'll just pick up
> > perm_addr and we will end up with incompatible MACs.
>
> Maybe you could send the assign_pem_addr changes as a follow up patch?
I would rather not, I think it would be messy. Or is your
patch already merged somewhere?
Perhaps you could incorporate the change to assign_pem_addr
into your patch and send v2 adding my Sign-off?
> > @@ -1479,7 +1483,17 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
> > 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);
>
> This is only relevant if the driver registered a addr_mask with mac80211.
> So, maybe you could only select a new address (!=perm_addr) if the
> perm_addr is not covered by the addr_mask?
I'm not sure I understand all the internal logic, but if
driver doesn't set addr_mask it will always get one of
wiphy->addresses or perm_addr and leave on line 1468 [1]
before reaching this code.
[1]http://git.kernel.org/cgit/linux/kernel/git/jberg/mac80211-next.git/tree/net/mac80211/iface.c#n1468
-- Kuba
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mac80211: Allow single vif mac address change with addr_mask
2013-05-22 12:42 ` Jakub Kiciński
@ 2013-05-23 13:32 ` Helmut Schaa
0 siblings, 0 replies; 8+ messages in thread
From: Helmut Schaa @ 2013-05-23 13:32 UTC (permalink / raw)
To: Jakub Kiciński
Cc: linux-wireless, Johannes Berg, Ivo Van Doorn,
Gertjan van Wingerde, stf_xl@wp.pl, Alessandro Lannocca,
Bruno Randolf
On Wed, May 22, 2013 at 2:42 PM, Jakub Kiciński <moorray3@wp.pl> wrote:
> Perhaps you could incorporate the change to assign_pem_addr
> into your patch and send v2 adding my Sign-off?
Will do.
>> > @@ -1479,7 +1483,17 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
>> > 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);
>>
>> This is only relevant if the driver registered a addr_mask with mac80211.
>> So, maybe you could only select a new address (!=perm_addr) if the
>> perm_addr is not covered by the addr_mask?
>
> I'm not sure I understand all the internal logic, but if
> driver doesn't set addr_mask it will always get one of
> wiphy->addresses or perm_addr and leave on line 1468 [1]
> before reaching this code.
My fault. This is indeed correct.
Johannes, please drop this patch. I'll respin.
Thanks,
Helmut
^ permalink raw reply [flat|nested] 8+ messages in thread
* R: [PATCH] mac80211: Allow single vif mac address change with addr_mask
2013-05-21 12:54 [PATCH] mac80211: Allow single vif mac address change with addr_mask Helmut Schaa
2013-05-21 15:10 ` Jakub Kiciński
@ 2013-05-21 15:51 ` Alessandro Lannocca
2013-05-27 8:43 ` [PATCHv2] " Helmut Schaa
2 siblings, 0 replies; 8+ messages in thread
From: Alessandro Lannocca @ 2013-05-21 15:51 UTC (permalink / raw)
To: 'Helmut Schaa'; +Cc: linux-wireless
Tested right now against compat-drivers-2013-03-28-5-u and I can confirm it
works.
Thank you for your effort!
Alessandro Lannocca
-----Messaggio originale-----
Da: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
Inviato: martedì 21 maggio 2013 14.55
A: linux-wireless@vger.kernel.org
Cc: johannes@sipsolutions.net; IvDoorn@gmail.com; gwingerde@gmail.com;
stf_xl@wp.pl; Helmut Schaa; Alessandro Lannocca; Bruno Randolf
Oggetto: [PATCH] mac80211: Allow single vif mac address change with
addr_mask
When changing the MAC address of a single vif mac80211 will check if the new
address fits into the address mask specified by the driver. This only needs
to be done when using multiple BSSIDs. Hence, check the new address only
against all other vifs.
Resolves:
https://bugzilla.kernel.org/show_bug.cgi?id=57371
Reported-by: Alessandro Lannocca <alessandro.lannocca@gmail.com>
Cc: Alessandro Lannocca <alessandro.lannocca@gmail.com>
Cc: Bruno Randolf <br1@thinktube.com>
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
net/mac80211/iface.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index
60f1ce5..c1a8c79 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -159,9 +159,10 @@ static int ieee80211_change_mtu(struct net_device *dev,
int new_mtu)
return 0;
}
-static int ieee80211_verify_mac(struct ieee80211_local *local, u8 *addr)
+static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8
+*addr)
{
- struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_sub_if_data *iter;
u64 new, mask, tmp;
u8 *m;
int ret = 0;
@@ -181,11 +182,14 @@ static int ieee80211_verify_mac(struct ieee80211_local
*local, u8 *addr)
mutex_lock(&local->iflist_mtx);
- list_for_each_entry(sdata, &local->interfaces, list) {
- if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
+ list_for_each_entry(iter, &local->interfaces, list) {
+ if (iter == sdata)
+ continue;
+
+ if (iter->vif.type == NL80211_IFTYPE_MONITOR)
continue;
- m = sdata->vif.addr;
+ 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) |
((u64)m[4] << 1*8) | ((u64)m[5] << 0*8); @@ -209,7
+213,7 @@ static int ieee80211_change_mac(struct net_device *dev, void
*addr)
if (ieee80211_sdata_running(sdata))
return -EBUSY;
- ret = ieee80211_verify_mac(sdata->local, sa->sa_data);
+ ret = ieee80211_verify_mac(sdata, sa->sa_data);
if (ret)
return ret;
--
1.7.10.4
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCHv2] mac80211: Allow single vif mac address change with addr_mask
2013-05-21 12:54 [PATCH] mac80211: Allow single vif mac address change with addr_mask Helmut Schaa
2013-05-21 15:10 ` Jakub Kiciński
2013-05-21 15:51 ` R: " Alessandro Lannocca
@ 2013-05-27 8:43 ` Helmut Schaa
2013-05-27 8:55 ` Johannes Berg
2 siblings, 1 reply; 8+ messages in thread
From: Helmut Schaa @ 2013-05-27 8:43 UTC (permalink / raw)
To: linux-wireless
Cc: johannes, Helmut Schaa, Jakub Kicinski, Alessandro Lannocca,
Bruno Randolf
When changing the MAC address of a single vif mac80211 will check if
the new address fits into the address mask specified by the driver.
This only needs to be done when using multiple BSSIDs. Hence, check
the new address only against all other vifs.
Also fix the MAC address assignment on new interfaces if the user
changed the address of a vif such that perm_addr is not covered by
addr_mask anymore.
Resolves:
https://bugzilla.kernel.org/show_bug.cgi?id=57371
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
Reported-by: Alessandro Lannocca <alessandro.lannocca@gmail.com>
Cc: Alessandro Lannocca <alessandro.lannocca@gmail.com>
Cc: Bruno Randolf <br1@thinktube.com>
---
net/mac80211/iface.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 60f1ce5..3000cda 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -159,9 +159,10 @@ static int ieee80211_change_mtu(struct net_device *dev, int new_mtu)
return 0;
}
-static int ieee80211_verify_mac(struct ieee80211_local *local, u8 *addr)
+static int ieee80211_verify_mac(struct ieee80211_sub_if_data *sdata, u8 *addr)
{
- struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_sub_if_data *iter;
u64 new, mask, tmp;
u8 *m;
int ret = 0;
@@ -181,11 +182,14 @@ static int ieee80211_verify_mac(struct ieee80211_local *local, u8 *addr)
mutex_lock(&local->iflist_mtx);
- list_for_each_entry(sdata, &local->interfaces, list) {
- if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
+ list_for_each_entry(iter, &local->interfaces, list) {
+ if (iter == sdata)
continue;
- m = sdata->vif.addr;
+ if (iter->vif.type == NL80211_IFTYPE_MONITOR)
+ 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) |
((u64)m[4] << 1*8) | ((u64)m[5] << 0*8);
@@ -209,7 +213,7 @@ static int ieee80211_change_mac(struct net_device *dev, void *addr)
if (ieee80211_sdata_running(sdata))
return -EBUSY;
- ret = ieee80211_verify_mac(sdata->local, sa->sa_data);
+ ret = ieee80211_verify_mac(sdata, sa->sa_data);
if (ret)
return ret;
@@ -1479,7 +1483,17 @@ static void ieee80211_assign_perm_addr(struct ieee80211_local *local,
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);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCHv2] mac80211: Allow single vif mac address change with addr_mask
2013-05-27 8:43 ` [PATCHv2] " Helmut Schaa
@ 2013-05-27 8:55 ` Johannes Berg
0 siblings, 0 replies; 8+ messages in thread
From: Johannes Berg @ 2013-05-27 8:55 UTC (permalink / raw)
To: Helmut Schaa
Cc: linux-wireless, Jakub Kicinski, Alessandro Lannocca,
Bruno Randolf
On Mon, 2013-05-27 at 10:43 +0200, Helmut Schaa wrote:
> When changing the MAC address of a single vif mac80211 will check if
> the new address fits into the address mask specified by the driver.
> This only needs to be done when using multiple BSSIDs. Hence, check
> the new address only against all other vifs.
>
> Also fix the MAC address assignment on new interfaces if the user
> changed the address of a vif such that perm_addr is not covered by
> addr_mask anymore.
>
> Resolves:
> https://bugzilla.kernel.org/show_bug.cgi?id=57371
Applied for 3.10, thanks.
johannes
^ permalink raw reply [flat|nested] 8+ messages in thread