| Message ID | 20130521171018.238056b2@north (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);