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 <[email protected]>
Cc: Alessandro Lannocca <[email protected]>
Cc: Bruno Randolf <[email protected]>
Signed-off-by: Helmut Schaa <[email protected]>
---
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
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:[email protected]]
Inviato: marted? 21 maggio 2013 14.55
A: [email protected]
Cc: [email protected]; [email protected]; [email protected];
[email protected]; 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 <[email protected]>
Cc: Alessandro Lannocca <[email protected]>
Cc: Bruno Randolf <[email protected]>
Signed-off-by: Helmut Schaa <[email protected]>
---
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
On Wed, May 22, 2013 at 2:42 PM, Jakub Kiciński <[email protected]> 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
On Wed, 22 May 2013 10:06:26 +0200, Helmut Schaa wrote:
> On Tue, May 21, 2013 at 5:10 PM, Jakub Kiciński <[email protected]> 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
On Tue, May 21, 2013 at 5:10 PM, Jakub Kiciński <[email protected]> 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
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
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 <[email protected]>
Signed-off-by: Jakub Kicinski <[email protected]>
Reported-by: Alessandro Lannocca <[email protected]>
Cc: Alessandro Lannocca <[email protected]>
Cc: Bruno Randolf <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
---
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