| Message ID | 1370612179-24385-5-git-send-email-moorray3@wp.pl (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- 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 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 -- 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 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 -- 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 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 -- 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 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 -- 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 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 -- 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
> > 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 -- 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 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 -- 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 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;