This patch below was added to the kernel around 2/24/2015
I am curious mostly about the first change: I thought the transmitter-addr
relates to the radio device, not the vdev (sta, ap, etc).
But, wouldn't using data from the header break that assumption?
Is there any actual advantage to having more than one address per
hwsim radio? It seems it complicates things for no particular
reason as far as I can tell?
Thanks,
Ben
mac80211_hwsim: support any address in userspace
Due to the checks in get_hwsim_data_ref_from_addr, wmediumd
was only able to use the second mac address (those starting with
0x42). This is confusing and needlessly limiting, so allow any
configured address.
Signed-off-by: Bob Copeland <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
-------------------- drivers/net/wireless/mac80211_hwsim.c --------------------
index 4a4c658..e259ee1 100644
@@ -906,8 +906,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
goto nla_put_failure;
}
- if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
- ETH_ALEN, data->addresses[1].addr))
+ if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER, ETH_ALEN, hdr->addr2))
goto nla_put_failure;
/* We get the skb->data */
@@ -2608,7 +2607,7 @@ static struct mac80211_hwsim_data *get_hwsim_data_ref_from_addr(const u8 *addr)
spin_lock_bh(&hwsim_radio_lock);
list_for_each_entry(data, &hwsim_radios, list) {
- if (memcmp(data->addresses[1].addr, addr, ETH_ALEN) == 0) {
+ if (mac80211_hwsim_addr_match(data, addr)) {
_found = true;
break;
}
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 12/16/2015 05:21 AM, [email protected] >> Bob Copeland wrote:
> On Tue, Dec 15, 2015 at 07:29:30PM -0800, Ben Greear wrote:
>> This patch below was added to the kernel around 2/24/2015
>>
>> I am curious mostly about the first change: I thought the transmitter-addr
>> relates to the radio device, not the vdev (sta, ap, etc).
>>
>> But, wouldn't using data from the header break that assumption?
>
> I'm not sure this assumption is correct. I have a hard time
> seeing the value in basing the transmitter addr attribute on some
> hardware address that may not even be used.
Imagine you have 20 virtual radios, and you want to send a beacon (or
other broadcast) to just one radio.
You can use the radio transmitter-addr to let the kernel know which
virtual radio receives the packet.
>
>> Is there any actual advantage to having more than one address per
>> hwsim radio? It seems it complicates things for no particular
>> reason as far as I can tell?
>
> As a practical matter: the radios already have two "hardware"
> addresses, and as reported in the commit log, only one of them
> worked with the netlink interface, and it wasn't even the default
> address.
>
> I suppose there's no real benefit to multi-vif on hwsim vs multiple
> phys, other than testing multi-vif support in the stack, but why not?
> I think this patch actually simplifies things.
As long as you do a mapping in wmediumd, there should be no problem
at all with multiple vifs on a radio. The HWSIM_ATTR_ADDR_TRANSMITTER
is just a key to let us know which radio we are talking about. It
never needs to be 'on the air'.
>
> Does this patch cause problems for your userspace implementation?
Yes, because I coded with the assumptions that the radio addr had nothing
to do with the vif addr.
Thanks,
Ben
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 12/16/2015 09:30 AM, Adam R. Welle wrote:
>
>> Well, it was always rather awkward since it was the *second* address :)
>
> Could somebody provide background information on why the decision was
> made to use a second address for the netlink frames instead of the same
> address as was used for the non-netlink frames?
I would be fine with always using the first address instead of the second,
in case that helps someone.
We could also set the address at creation time easily enough. Then it
could still be unique across many machines if you managed it.
Thanks,
Ben
>
> I too have an obscure user-space app which uses this API :)
>
> My app was written to account for both the old and new logic being debated,
> so I can cope either way with the flip of a switch. I would just like to
> say that for my purposes the patch worked better as I already intended for
> my app to track MAC address changes made by a user.
>
> I should note that I am passing these frames between virtual machines so
> the use of unique addresses (instead of hard-coded 0x42 addresses) as a key
> simplifies things when determining which radio transmitted a given frame
> and which radio needs to receive the frame. My app has always relied on the
> MAC address assigned by the user as a unique key across virtual machines.
> Also, I will point out that I am not using multiple vifs on a radio but if
> I were, I imagine that I would send a copy of the frame to each vif.
>
> Adam
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 12/16/2015 05:42 AM, Johannes Berg wrote:
> On Wed, 2015-12-16 at 05:35 -0800, Ben Greear wrote:
>
>> Well, the old code used it as a key, and the old documentation used
>> it as a key, so it is a bit of a regression to change the behaviour
>> now.
>
> But it's still used as a key, no? Just the value changed. If you treat
> it as a key then you'd just have it for a frame incoming and outgoing?
>
>>> I think this is pretty much a done deal by now though since I don't
>>> really want to break wmediumd.
>>
>> It was not the only user-space to use the API :)
>
> So you're saying you have code that broke? How did it break though, I
> don't really see it yet. Can you explain?
My code expected that the key was the MAC of the radio, not the
MAC of a vif. It set up mappings accordingly in the user-space
program.
And, if I change a vif's mac address, the previous 'key' is no longer
valid with the new patch. If I have multiple vifs on one radio
and want to send a broadcast pkt to that radio, then at best the
patched API is lame because you would have to specify the address
of one of the vifs on the radio, but it is not really destined for
just that vif.
A fair bit of code was written, and not just by me, against the
API that assumed the MAC of the radio was a unique key. The
code can be changed of course, but if Bob's change does not
really offer any advantage, then I think the patch should
be reverted.
If wmediumd needs more info, then I think adding a new netlink
message or attribute is the way to go..that way it should be fully
backwards compatible.
>> Well, that would be fine too. The nice thing about the
>> address,though, is that you can query it as part of
>> /sys/class/ieee..... and other already-implemented
>> interfaces. Finding the radio-id would require new API in this case,
>> which is a bit of work.
>
> Well, it was always rather awkward since it was the *second* address :)
Yes, it was always weird, but predictably so.
>
> You can query the ID/index already through the netlink API, or even
> from sysfs since the virtual device name is essentially
> sprintf(name, "hwsim%d", idx)
Good lord, please don't even suggest parsing the name. You can easily
rename a phy device. I do it all the time to keep a phyname associated
with a real radio through module reloads.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
PiA+IENvdWxkIHNvbWVib2R5IHByb3ZpZGUgYmFja2dyb3VuZCBpbmZvcm1hdGlvbiBvbiB3aHkg
dGhlIGRlY2lzaW9uIHdhcw0KPiA+IG1hZGUgdG8gdXNlIGEgc2Vjb25kIGFkZHJlc3MgZm9yIHRo
ZSBuZXRsaW5rIGZyYW1lcyBpbnN0ZWFkIG9mIHRoZQ0KPiA+IHNhbWUgYWRkcmVzcyBhcyB3YXMg
dXNlZCBmb3IgdGhlIG5vbi1uZXRsaW5rIGZyYW1lcz8NCj4gDQo+IEkgd291bGQgYmUgZmluZSB3
aXRoIGFsd2F5cyB1c2luZyB0aGUgZmlyc3QgYWRkcmVzcyBpbnN0ZWFkIG9mIHRoZQ0KPiBzZWNv
bmQsIGluIGNhc2UgdGhhdCBoZWxwcyBzb21lb25lLg0KPiANCj4gV2UgY291bGQgYWxzbyBzZXQg
dGhlIGFkZHJlc3MgYXQgY3JlYXRpb24gdGltZSBlYXNpbHkgZW5vdWdoLiAgVGhlbiBpdA0KPiBj
b3VsZCBzdGlsbCBiZSB1bmlxdWUgYWNyb3NzIG1hbnkgbWFjaGluZXMgaWYgeW91IG1hbmFnZWQg
aXQuDQoNCkkgYWN0dWFsbHkgYXBwbHkgYSBwYXRjaCBmb3IgdGhhdC4gSSBoYXZlIGFkZGVkIGEg
bW9kdWxlIHBhcmFtZXRlciB3aGljaA0KaXMgdXNlZCBhcyB0aGUgTUFDIGFkZHJlc3MgZm9yIHRo
ZSBmaXJzdCByYWRpbywgYW5kIEkgaW5jcmVtZW50IHRoZSBmaW5hbA0Kb2N0ZXQgZm9yIGVhY2gg
YWRkaXRpb25hbCByYWRpby4gVGhpcyBpcyBzbyB0aGF0IG15IHVzZXJzIGRvIG5vdCBoYXZlDQp0
byBtYW51YWxseSByZXNldCB0aGUgTUFDIGFkZHJlc3MgdG8gZ2V0IGEgdW5pcXVlIG9uZS4gSSBh
cHBseSB0aGUgYWRkcmVzcw0KdG8gYm90aCBhZGRyZXNzZXMgbWVudGlvbmVkIGFib3ZlIHRob3Vn
aCBJIG9ubHkgdXNlIHRoZSBuZXRsaW5rIGRhdGFwYXRoLg0KDQpJZiB0aGVyZSBpcyBpbnRlcmVz
dCBpbiB0aGlzIEkgY2FuIHN1Ym1pdCBpdCBhcyBhbiBSRkMgcGF0Y2ggdG9tb3Jyb3cuDQoNCkFk
YW0NCg0KDQotLQ0KQWRhbSBXZWxsZSA8YXJ3ZWxsZUBjZXJ0Lm9yZz4NCg0KDQoNCg0K
PiA+PiBXZSBjb3VsZCBhbHNvIHNldCB0aGUgYWRkcmVzcyBhdCBjcmVhdGlvbiB0aW1lIGVhc2ls
eSBlbm91Z2guICBUaGVuDQo+ID4+IGl0IGNvdWxkIHN0aWxsIGJlIHVuaXF1ZSBhY3Jvc3MgbWFu
eSBtYWNoaW5lcyBpZiB5b3UgbWFuYWdlZCBpdC4NCj4gPg0KPiA+IEkgYWN0dWFsbHkgYXBwbHkg
YSBwYXRjaCBmb3IgdGhhdC4gSSBoYXZlIGFkZGVkIGEgbW9kdWxlIHBhcmFtZXRlcg0KPiA+IHdo
aWNoIGlzIHVzZWQgYXMgdGhlIE1BQyBhZGRyZXNzIGZvciB0aGUgZmlyc3QgcmFkaW8sIGFuZCBJ
IGluY3JlbWVudA0KPiA+IHRoZSBmaW5hbCBvY3RldCBmb3IgZWFjaCBhZGRpdGlvbmFsIHJhZGlv
LiBUaGlzIGlzIHNvIHRoYXQgbXkgdXNlcnMgZG8NCj4gPiBub3QgaGF2ZSB0byBtYW51YWxseSBy
ZXNldCB0aGUgTUFDIGFkZHJlc3MgdG8gZ2V0IGEgdW5pcXVlIG9uZS4gSQ0KPiA+IGFwcGx5IHRo
ZSBhZGRyZXNzIHRvIGJvdGggYWRkcmVzc2VzIG1lbnRpb25lZCBhYm92ZSB0aG91Z2ggSSBvbmx5
IHVzZQ0KPiA+IHRoZSBuZXRsaW5rIGRhdGFwYXRoLg0KPiA+DQo+ID4gSWYgdGhlcmUgaXMgaW50
ZXJlc3QgaW4gdGhpcyBJIGNhbiBzdWJtaXQgaXQgYXMgYW4gUkZDIHBhdGNoIHRvbW9ycm93Lg0K
PiANCj4gSSdkIHJhdGhlciBzZWUgc3VwcG9ydCBmb3Igc3BlY2lmeWluZyBhbiBhcmJpdHJhcnkg
TUFDIHVwb24gY3JlYXRpb24NCj4gd2l0aCBhIG5ldGxpbmsgYXR0cmlidXRlIHRoYW4gYSBtb2R1
bGUgcGFyYW1ldGVyLi4uDQoNCkkgY2FuIHNlZSB0aGUgdXNlZnVsbmVzcyBpbiB5b3VyIHN1Z2dl
c3Rpb24gYXMgeW91IHdvdWxkIGJlIGFibGUgdG8gc3BlY2lmeSBub24tc2VxdWVudGlhbCBhZGRy
ZXNzZXMgdG8gdGhlIHJhZGlvcy4gSSBoYWRuJ3QgdGhvdWdodCBvZiB0aGF0IHNpbmNlIEkgbG9h
ZCB0aGUgbW9kdWxlIHdpdGggb25lIHJhZGlvIGFuZCBkbyBub3QgdXNlIHRoZSBuZXRsaW5rIEFQ
SSB0byBhZGQgb3IgcmVtb3ZlIHJhZGlvcy4NCgkNCkFkYW0NCg0KLS0NCg==
On Wed, 2015-12-16 at 06:11 -0800, Ben Greear wrote:
>
> > You can query the ID/index already through the netlink API, or even
> > from sysfs since the virtual device name is essentially
> > sprintf(name, "hwsim%d", idx)
>
> Good lord, please don't even suggest parsing the name. You can
> easily rename a phy device. I do it all the time to keep a phyname
> associated with a real radio through module reloads.
>
You're confusing the wiphy name and the hwsim virtual class device name
(which you can't touch) :)
johannes
On 12/16/2015 06:59 AM, Bob Copeland wrote:
> On Wed, Dec 16, 2015 at 03:15:40PM +0100, Johannes Berg wrote:
>> On Wed, 2015-12-16 at 06:11 -0800, Ben Greear wrote:
>>
>>> My code expected that the key was the MAC of the radio, not the
>>> MAC of a vif. It set up mappings accordingly in the user-space
>>> program.
>>
>> I guess you were trying to be much smarter than wmediumd :)
>>
>> Bob, any thoughts?
>
> So, now that I understand the argument, I see the value in having
> an unchanging key for each phy. I'm also pretty sure that it was by
> accident that it used to work that way. If we were designing the ABI
> from scratch, radio id would probably be better than a mac address for
> that purpose.
>
> Anyway, in the interest of not breaking userspace, I'm not opposed to
> reverting that patch, and perhaps adding some documentation on top to make
> it clear that the addr attributes have nothing to do with any mac addresses
> actually in use.
>
> For wmediumd users that would mean going back to the way it was previously,
> in which only the 42:xx mac addresses will work, until I can work out another
> way to do it. I think that would break the test_wmediumd.py in hostapd
> test suite in the meantime though.
>
Ok, thanks.
I'm fine with waiting a bit before reverting..its easy enough for me to carry
a private patch for a while.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Tue, Dec 15, 2015 at 07:29:30PM -0800, Ben Greear wrote:
> This patch below was added to the kernel around 2/24/2015
>
> I am curious mostly about the first change: I thought the transmitter-addr
> relates to the radio device, not the vdev (sta, ap, etc).
>
> But, wouldn't using data from the header break that assumption?
I'm not sure this assumption is correct. I have a hard time
seeing the value in basing the transmitter addr attribute on some
hardware address that may not even be used.
> Is there any actual advantage to having more than one address per
> hwsim radio? It seems it complicates things for no particular
> reason as far as I can tell?
As a practical matter: the radios already have two "hardware"
addresses, and as reported in the commit log, only one of them
worked with the netlink interface, and it wasn't even the default
address.
I suppose there's no real benefit to multi-vif on hwsim vs multiple
phys, other than testing multi-vif support in the stack, but why not?
I think this patch actually simplifies things.
Does this patch cause problems for your userspace implementation?
--
Bob Copeland %% http://bobcopeland.com/
On 12/16/2015 02:56 PM, Adam R. Welle wrote:
>>> Could somebody provide background information on why the decision was
>>> made to use a second address for the netlink frames instead of the
>>> same address as was used for the non-netlink frames?
>>
>> I would be fine with always using the first address instead of the
>> second, in case that helps someone.
>>
>> We could also set the address at creation time easily enough. Then it
>> could still be unique across many machines if you managed it.
>
> I actually apply a patch for that. I have added a module parameter which
> is used as the MAC address for the first radio, and I increment the final
> octet for each additional radio. This is so that my users do not have
> to manually reset the MAC address to get a unique one. I apply the address
> to both addresses mentioned above though I only use the netlink datapath.
>
> If there is interest in this I can submit it as an RFC patch tomorrow.
I'd rather see support for specifying an arbitrary MAC upon creation
with a netlink attribute than a module parameter...
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 12/16/2015 01:17 AM, Johannes Berg wrote:
> On Tue, 2015-12-15 at 19:29 -0800, Ben Greear wrote:
>> This patch below was added to the kernel around 2/24/2015
>>
>> I am curious mostly about the first change: I thought the
>> transmitter-addr relates to the radio device, not the vdev (sta, ap,
>> etc).
>
> It doesn't, even on real hardware.
No, I mean that the HWSIM_ATTR_ADDR_TRANSMITTER should relate to the
radio, and not the vdev, see the mac80211_hwsim.h:
* @HWSIM_ATTR_ADDR_TRANSMITTER: MAC address of the radio device that
* the frame was broadcasted from
>
>> But, wouldn't using data from the header break that assumption?
>>
>>
>> Is there any actual advantage to having more than one address per
>> hwsim radio? It seems it complicates things for no particular
>> reason as far as I can tell?
>>
>
> ??
> You can do this with any regular hardware that supports multiple
> virtual interfaces - each one of them gets its own address.
>
> I think you might be confused by how ath*k implements the address
> matching - as I understand it there it's a common address (which may or
> may not match the programmed hardware address) along with a mask.
> That's not true in general though.
Since we are asking user-space to provide HWSIM_ATTR_ADDR_TRANSMITTER,
then we can use that to find the radio device. Then, normal mac80211
logic can handle finding the vdevs (just as it does for ath9k).
And in this case, there is no reason to have more than one address
associated with the hwsim radio device. We could add a pretty simple
hash to keep the lookup near constant time instead of linear search
as the current behaviour is...
> The hwsim commit here just makes wmediumd able to behave properly when
> the user changed the vif interface address.
I think that wmediumd should keep it's own mapping of what radio
a vdev is on and use the proper hwsim radio addr for the HWSIM_ATTR_ADDR_TRANSMITTER
attribute.
Thanks,
Ben
>
> johannes
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Wed, Dec 16, 2015 at 02:14:18PM -0800, Ben Greear wrote:
> On 12/16/2015 09:30 AM, Adam R. Welle wrote:
> >
> >>Well, it was always rather awkward since it was the *second* address :)
> >
> >Could somebody provide background information on why the decision was
> >made to use a second address for the netlink frames instead of the same
> >address as was used for the non-netlink frames?
>
> I would be fine with always using the first address instead of the second,
> in case that helps someone.
It doesn't really, that would break old wmediumd.
> We could also set the address at creation time easily enough. Then it
> could still be unique across many machines if you managed it.
We already have a way to change mac addresses; adding a new API to change
the default address seems pointless to me.
--
Bob Copeland %% http://bobcopeland.com/
DQo+IFdlbGwsIGl0IHdhcyBhbHdheXMgcmF0aGVyIGF3a3dhcmQgc2luY2UgaXQgd2FzIHRoZSAq
c2Vjb25kKiBhZGRyZXNzIDopDQoNCkNvdWxkIHNvbWVib2R5IHByb3ZpZGUgYmFja2dyb3VuZCBp
bmZvcm1hdGlvbiBvbiB3aHkgdGhlIGRlY2lzaW9uIHdhcw0KbWFkZSB0byB1c2UgYSBzZWNvbmQg
YWRkcmVzcyBmb3IgdGhlIG5ldGxpbmsgZnJhbWVzIGluc3RlYWQgb2YgdGhlIHNhbWUNCmFkZHJl
c3MgYXMgd2FzIHVzZWQgZm9yIHRoZSBub24tbmV0bGluayBmcmFtZXM/IA0KDQpJIHRvbyBoYXZl
IGFuIG9ic2N1cmUgdXNlci1zcGFjZSBhcHAgd2hpY2ggdXNlcyB0aGlzIEFQSSA6KQ0KDQpNeSBh
cHAgd2FzIHdyaXR0ZW4gdG8gYWNjb3VudCBmb3IgYm90aCB0aGUgb2xkIGFuZCBuZXcgbG9naWMg
YmVpbmcgZGViYXRlZCwNCnNvIEkgY2FuIGNvcGUgZWl0aGVyIHdheSB3aXRoIHRoZSBmbGlwIG9m
IGEgc3dpdGNoLiBJIHdvdWxkIGp1c3QgbGlrZSB0bw0Kc2F5IHRoYXQgZm9yIG15IHB1cnBvc2Vz
IHRoZSBwYXRjaCB3b3JrZWQgYmV0dGVyIGFzIEkgYWxyZWFkeSBpbnRlbmRlZCBmb3INCm15IGFw
cCB0byB0cmFjayBNQUMgYWRkcmVzcyBjaGFuZ2VzIG1hZGUgYnkgYSB1c2VyLg0KDQpJIHNob3Vs
ZCBub3RlIHRoYXQgSSBhbSBwYXNzaW5nIHRoZXNlIGZyYW1lcyBiZXR3ZWVuIHZpcnR1YWwgbWFj
aGluZXMgc28NCnRoZSB1c2Ugb2YgdW5pcXVlIGFkZHJlc3NlcyAoaW5zdGVhZCBvZiBoYXJkLWNv
ZGVkIDB4NDIgYWRkcmVzc2VzKSBhcyBhIGtleQ0Kc2ltcGxpZmllcyB0aGluZ3Mgd2hlbiBkZXRl
cm1pbmluZyB3aGljaCByYWRpbyB0cmFuc21pdHRlZCBhIGdpdmVuIGZyYW1lDQphbmQgd2hpY2gg
cmFkaW8gbmVlZHMgdG8gcmVjZWl2ZSB0aGUgZnJhbWUuIE15IGFwcCBoYXMgYWx3YXlzIHJlbGll
ZCBvbiB0aGUNCk1BQyBhZGRyZXNzIGFzc2lnbmVkIGJ5IHRoZSB1c2VyIGFzIGEgdW5pcXVlIGtl
eSBhY3Jvc3MgdmlydHVhbCBtYWNoaW5lcy4NCkFsc28sIEkgd2lsbCBwb2ludCBvdXQgdGhhdCBJ
IGFtIG5vdCB1c2luZyBtdWx0aXBsZSB2aWZzIG9uIGEgcmFkaW8gYnV0IGlmDQpJIHdlcmUsIEkg
aW1hZ2luZSB0aGF0IEkgd291bGQgc2VuZCBhIGNvcHkgb2YgdGhlIGZyYW1lIHRvIGVhY2ggdmlm
Lg0KDQpBZGFtDQo=
On Wed, Dec 16, 2015 at 06:16:06AM -0800, Ben Greear wrote:
> Even so, I think it is more important to keep the kernel API stable than
> to work around issues in relatively obscure user-space apps.
Let's face it, everything using hwsim netlink api is already obscure :)
--
Bob Copeland %% http://bobcopeland.com/
On Wed, Dec 16, 2015 at 03:15:40PM +0100, Johannes Berg wrote:
> On Wed, 2015-12-16 at 06:11 -0800, Ben Greear wrote:
>
> > My code expected that the key was the MAC of the radio, not the
> > MAC of a vif.??It set up mappings accordingly in the user-space
> > program.
>
> I guess you were trying to be much smarter than wmediumd :)
>
> Bob, any thoughts?
So, now that I understand the argument, I see the value in having
an unchanging key for each phy. I'm also pretty sure that it was by
accident that it used to work that way. If we were designing the ABI
from scratch, radio id would probably be better than a mac address for
that purpose.
Anyway, in the interest of not breaking userspace, I'm not opposed to
reverting that patch, and perhaps adding some documentation on top to make
it clear that the addr attributes have nothing to do with any mac addresses
actually in use.
For wmediumd users that would mean going back to the way it was previously,
in which only the 42:xx mac addresses will work, until I can work out another
way to do it. I think that would break the test_wmediumd.py in hostapd
test suite in the meantime though.
--
Bob Copeland %% http://bobcopeland.com/
On Wed, Dec 16, 2015 at 05:30:20PM +0000, Adam R. Welle wrote:
>
> > Well, it was always rather awkward since it was the *second* address :)
>
> Could somebody provide background information on why the decision was
> made to use a second address for the netlink frames instead of the same
> address as was used for the non-netlink frames?
I can't; I believe it was done that way from the start but I don't really
know if there was a reason.
> I too have an obscure user-space app which uses this API :)
>
> My app was written to account for both the old and new logic being debated,
> so I can cope either way with the flip of a switch. I would just like to
> say that for my purposes the patch worked better as I already intended for
> my app to track MAC address changes made by a user.
In any case, we'll need to support that. However the addr attribute is
interpreted, the actual userspace app should accept any user defined address
as long as it can map it back to the attribute. It just may be the case
going forward that the attribute bears no obvious relationship to the
addresses used in frames.
> I should note that I am passing these frames between virtual machines so
> the use of unique addresses (instead of hard-coded 0x42 addresses) as a key
> simplifies things when determining which radio transmitted a given frame
> and which radio needs to receive the frame. My app has always relied on the
> MAC address assigned by the user as a unique key across virtual machines.
Sounds interesting - is this app open source?
> Also, I will point out that I am not using multiple vifs on a radio but if
> I were, I imagine that I would send a copy of the frame to each vif.
I guess this is what wmediumd would wind up doing too, but it would result in
duplicate frames at the receivers with multiple vifs.
--
Bob Copeland %% http://bobcopeland.com/
On Wed, 2015-12-16 at 06:11 -0800, Ben Greear wrote:
> My code expected that the key was the MAC of the radio, not the
> MAC of a vif. It set up mappings accordingly in the user-space
> program.
>
> And, if I change a vif's mac address, the previous 'key' is no longer
> valid with the new patch. If I have multiple vifs on one radio
> and want to send a broadcast pkt to that radio, then at best the
> patched API is lame because you would have to specify the address
> of one of the vifs on the radio, but it is not really destined for
> just that vif.
>
> A fair bit of code was written, and not just by me, against the
> API that assumed the MAC of the radio was a unique key. The
> code can be changed of course, but if Bob's change does not
> really offer any advantage, then I think the patch should
> be reverted.
>
I guess you were trying to be much smarter than wmediumd :)
Bob, any thoughts?
johannes
On Tue, 2015-12-15 at 19:29 -0800, Ben Greear wrote:
> This patch below was added to the kernel around 2/24/2015
>
> I am curious mostly about the first change: I thought the
> transmitter-addr relates to the radio device, not the vdev (sta, ap,
> etc).
It doesn't, even on real hardware.
> But, wouldn't using data from the header break that assumption?
>
>
> Is there any actual advantage to having more than one address per
> hwsim radio? It seems it complicates things for no particular
> reason as far as I can tell?
>
??
You can do this with any regular hardware that supports multiple
virtual interfaces - each one of them gets its own address.
I think you might be confused by how ath*k implements the address
matching - as I understand it there it's a common address (which may or
may not match the programmed hardware address) along with a mask.
That's not true in general though.
The hwsim commit here just makes wmediumd able to behave properly when
the user changed the vif interface address.
johannes
On Wed, Dec 16, 2015 at 05:27:44AM -0800, Ben Greear wrote:
> >Does this patch cause problems for your userspace implementation?
>
> Yes, because I coded with the assumptions that the radio addr had nothing
> to do with the vif addr.
I see -- but looking back at wmediumd history (before I had anything to
do with it) it seems it always had the opposite assumption -- e.g. the
ATTR_ADDR_TRANSMITTER mac address was compared against addresses in the
frame for delivery decisions.
--
Bob Copeland %% http://bobcopeland.com/
On 12/16/2015 05:25 AM, Johannes Berg wrote:
> On Wed, 2015-12-16 at 05:13 -0800, Ben Greear wrote:
>> On 12/16/2015 01:17 AM, Johannes Berg wrote:
>>> On Tue, 2015-12-15 at 19:29 -0800, Ben Greear wrote:
>>>> This patch below was added to the kernel around 2/24/2015
>>>>
>>>> I am curious mostly about the first change: I thought the
>>>> transmitter-addr relates to the radio device, not the vdev (sta,
>>>> ap,
>>>> etc).
>>>
>>> It doesn't, even on real hardware.
>>
>> No, I mean that the HWSIM_ATTR_ADDR_TRANSMITTER should relate to the
>> radio, and not the vdev, see the mac80211_hwsim.h:
>>
>> * @HWSIM_ATTR_ADDR_TRANSMITTER: MAC address of the radio device that
>> * the frame was broadcasted from
>
> I think that just means the documentation is misleading. Clearly, the
> TA (hdr->addr2) is intended and implemented. "radio device" is a bit of
> a misleading term when you have virtual interfaces.
Well, the old code used it as a key, and the old documentation used
it as a key, so it is a bit of a regression to change the behaviour
now.
>> Since we are asking user-space to provide HWSIM_ATTR_ADDR_TRANSMITTER,
>> then we can use that to find the radio device. Then, normal mac80211
>> logic can handle finding the vdevs (just as it does for ath9k).
>
> Oh.
>
> So you're basically arguing that we should treat it as a cookie, and on
> outgoing frames give the hardware address, and on incoming frames use
> it only to look up the struct mac80211_hwsim_data.
yes.
>
>> And in this case, there is no reason to have more than one address
>> associated with the hwsim radio device. We could add a pretty simple
>> hash to keep the lookup near constant time instead of linear search
>> as the current behaviour is...
>
> Yeah, ok, that would be (have been) doable I guess. We'd still have the
> real TA in the frame itself (hdr->addr2).
>
>> I think that wmediumd should keep it's own mapping of what radio
>> a vdev is on and use the proper hwsim radio addr for the
>> HWSIM_ATTR_ADDR_TRANSMITTER attribute.
>
> That's somewhat difficult for it, since it could only populate that
> mapping on actual TX frames.
It can also be managed by an outside entity to set up mappings
as needed. And, it could be made smarter to listen to wifi
netlink events or whatever.
And anyway, unless you are doing all passive scans, you should always
get at least some packet transmitted (beacon or probe request), right?
>
> I think this is pretty much a done deal by now though since I don't
> really want to break wmediumd.
It was not the only user-space to use the API :)
>
> If we wanted to go this route I think we should be more explicit and
> simply use the HWSIM_ATTR_RADIO_ID attribute. We could support that
> easily - just see if the RADIO_ID is present and look up the
> transmitter (or receiver btw) radio based on the RADIO_ID if present -
> that gives a clean path forward too since wmediumd can be taught to
> specify both knowing the kernel will prefer the radio ID if present.
Well, that would be fine too. The nice thing about the address,though,
is that you can query it as part of /sys/class/ieee..... and other already-implemented
interfaces. Finding the radio-id would require new API in this case, which
is a bit of work.
Thanks,
Ben
>
> johannes
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Wed, 2015-12-16 at 05:13 -0800, Ben Greear wrote:
> On 12/16/2015 01:17 AM, Johannes Berg wrote:
> > On Tue, 2015-12-15 at 19:29 -0800, Ben Greear wrote:
> > > This patch below was added to the kernel around 2/24/2015
> > >
> > > I am curious mostly about the first change: I thought the
> > > transmitter-addr relates to the radio device, not the vdev (sta,
> > > ap,
> > > etc).
> >
> > It doesn't, even on real hardware.
>
> No, I mean that the HWSIM_ATTR_ADDR_TRANSMITTER should relate to the
> radio, and not the vdev, see the mac80211_hwsim.h:
>
> * @HWSIM_ATTR_ADDR_TRANSMITTER: MAC address of the radio device that
> * the frame was broadcasted from
I think that just means the documentation is misleading. Clearly, the
TA (hdr->addr2) is intended and implemented. "radio device" is a bit of
a misleading term when you have virtual interfaces.
> Since we are asking user-space to provide HWSIM_ATTR_ADDR_TRANSMITTER,
> then we can use that to find the radio device. Then, normal mac80211
> logic can handle finding the vdevs (just as it does for ath9k).
Oh.
So you're basically arguing that we should treat it as a cookie, and on
outgoing frames give the hardware address, and on incoming frames use
it only to look up the struct mac80211_hwsim_data.
> And in this case, there is no reason to have more than one address
> associated with the hwsim radio device. We could add a pretty simple
> hash to keep the lookup near constant time instead of linear search
> as the current behaviour is...
Yeah, ok, that would be (have been) doable I guess. We'd still have the
real TA in the frame itself (hdr->addr2).
> I think that wmediumd should keep it's own mapping of what radio
> a vdev is on and use the proper hwsim radio addr for the
> HWSIM_ATTR_ADDR_TRANSMITTER attribute.
That's somewhat difficult for it, since it could only populate that
mapping on actual TX frames.
I think this is pretty much a done deal by now though since I don't
really want to break wmediumd.
If we wanted to go this route I think we should be more explicit and
simply use the HWSIM_ATTR_RADIO_ID attribute. We could support that
easily - just see if the RADIO_ID is present and look up the
transmitter (or receiver btw) radio based on the RADIO_ID if present -
that gives a clean path forward too since wmediumd can be taught to
specify both knowing the kernel will prefer the radio ID if present.
johannes
On Wed, 2015-12-16 at 05:35 -0800, Ben Greear wrote:
> Well, the old code used it as a key, and the old documentation used
> it as a key, so it is a bit of a regression to change the behaviour
> now.
But it's still used as a key, no? Just the value changed. If you treat
it as a key then you'd just have it for a frame incoming and outgoing?
> > I think this is pretty much a done deal by now though since I don't
> > really want to break wmediumd.
>
> It was not the only user-space to use the API :)
So you're saying you have code that broke? How did it break though, I
don't really see it yet. Can you explain?
> Well, that would be fine too. The nice thing about the
> address,though, is that you can query it as part of
> /sys/class/ieee..... and other already-implemented
> interfaces. Finding the radio-id would require new API in this case,
> which is a bit of work.
Well, it was always rather awkward since it was the *second* address :)
You can query the ID/index already through the netlink API, or even
from sysfs since the virtual device name is essentially
sprintf(name, "hwsim%d", idx)
johannes
> > I should note that I am passing these frames between virtual machines
> > so the use of unique addresses (instead of hard-coded 0x42 addresses)
> > as a key simplifies things when determining which radio transmitted a
> > given frame and which radio needs to receive the frame. My app has
> > always relied on the MAC address assigned by the user as a unique key
> > across virtual machines.
>
> Sounds interesting - is this app open source?
We intend for it to be but I am not sure how long it will take to make that
happen. I told Johannes a few months ago that I was using mac80211_hwsim and
a wmediumd-inspired app to train wireless pen-testing skills. I am using
VMWare as the hypervisor and everything has been working pretty well.
Adam
--
On 12/16/2015 05:57 AM, [email protected] >> Bob Copeland wrote:
> On Wed, Dec 16, 2015 at 05:27:44AM -0800, Ben Greear wrote:
>>> Does this patch cause problems for your userspace implementation?
>>
>> Yes, because I coded with the assumptions that the radio addr had nothing
>> to do with the vif addr.
>
> I see -- but looking back at wmediumd history (before I had anything to
> do with it) it seems it always had the opposite assumption -- e.g. the
> ATTR_ADDR_TRANSMITTER mac address was compared against addresses in the
> frame for delivery decisions.
Even so, I think it is more important to keep the kernel API stable than
to work around issues in relatively obscure user-space apps.
There could of course be more than just my app and wmediumd that uses
the kernel API....
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com