| Message ID | 20230504215247.581443-1-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] hwsim: remove 'optimization' sending to only known MACs | expand |
Hi James, On 5/4/23 16:52, James Prestwood wrote: > Based on the comment hwsim would only send frames to known MACs > which on its face seems reasonable. But in reality there shouldn't > ever be frames going to unknown MACs, at least not unknown to the > kernel. We can hit this case, though, when using network namespaces. > Each namespace is siloed and hwsim instances only know about radios > in that namespace. First, can you explain what is the motivation behind this series? Second, if we're passing unicast frames, and the unicast frame is addressed to a radio in a different namespace, how does that radio get the frame? We don't have it in our radio_info queue? The comment for the code you're taking out makes it clear that the intent is for unicast frames to be sent only only to the radio with the target receiver address. And the optimization is simply about not sending frames to radios with addresses we know do not have the target address. What you're doing here seems to indicate that our implementation (and the comment) is completely bogus. And we can simply send any frame to any radio instance (once) and the frames will be forwarded automagically. If so, then you might want to explain this better and fix the code to reflect this. > > In order to support hwsim and namespaces, this restriction is > being removed. > --- > tools/hwsim.c | 33 --------------------------------- > 1 file changed, 33 deletions(-) > Regards, -Denis
Hi Denis, On 5/7/23 4:21 PM, Denis Kenzior wrote: > Hi James, > > On 5/4/23 16:52, James Prestwood wrote: >> Based on the comment hwsim would only send frames to known MACs >> which on its face seems reasonable. But in reality there shouldn't >> ever be frames going to unknown MACs, at least not unknown to the >> kernel. We can hit this case, though, when using network namespaces. >> Each namespace is siloed and hwsim instances only know about radios >> in that namespace. > > First, can you explain what is the motivation behind this series? I'd like to enable hwsim to work with namespaces. First in order to test the roam before netconfig changes. And maybe future tests would need this. > > Second, if we're passing unicast frames, and the unicast frame is > addressed to a radio in a different namespace, how does that radio get > the frame? We don't have it in our radio_info queue? The namespace bars hwsim from seeing radios in other namespaces via GET_WIPHY. So yes, the radios are not in the radio_info queue. But since the radios are on the same kernel any MAC the kernel knows about is accepted when sending a frame, regardless of namespace. > > The comment for the code you're taking out makes it clear that the > intent is for unicast frames to be sent only only to the radio with the > target receiver address. And the optimization is simply about not > sending frames to radios with addresses we know do not have the target > address. > > What you're doing here seems to indicate that our implementation (and > the comment) is completely bogus. And we can simply send any frame to > any radio instance (once) and the frames will be forwarded > automagically. If so, then you might want to explain this better and > fix the code to reflect this. Maybe I described this badly, but I was trying to make 2 points: - Historically, before namespaces, hwsim should never be in a situation where a frame is addressed to an unknown recipient. Hostapd won't do this and IWD won't do this. So all that code was doing is looping through the radios. Maybe there is some case I haven't thought of, in which case the kernel would drop the frame anyways. - Then with the introduction of namespaces we actually do hit this case of having unknown destination addresses. Not because they are bogus, but because hwsim simply cannot know about radios in other namespaces. Thanks, James > >> >> In order to support hwsim and namespaces, this restriction is >> being removed. >> --- >> tools/hwsim.c | 33 --------------------------------- >> 1 file changed, 33 deletions(-) >> > > Regards, > -Denis
Hi James, >> Second, if we're passing unicast frames, and the unicast frame is addressed to >> a radio in a different namespace, how does that radio get the frame? We don't >> have it in our radio_info queue? > > The namespace bars hwsim from seeing radios in other namespaces via GET_WIPHY. > So yes, the radios are not in the radio_info queue. But since the radios are on > the same kernel any MAC the kernel knows about is accepted when sending a frame, > regardless of namespace. > I get this part. What bothers me is that the current dispatch code seems to be completely wrong? >> >> The comment for the code you're taking out makes it clear that the intent is >> for unicast frames to be sent only only to the radio with the target receiver >> address. And the optimization is simply about not sending frames to radios >> with addresses we know do not have the target address. >> >> What you're doing here seems to indicate that our implementation (and the >> comment) is completely bogus. And we can simply send any frame to any radio >> instance (once) and the frames will be forwarded automagically. If so, then >> you might want to explain this better and fix the code to reflect this. > > Maybe I described this badly, but I was trying to make 2 points: > > - Historically, before namespaces, hwsim should never be in a situation where > a frame is addressed to an unknown recipient. Hostapd won't do this and IWD > won't do this. So all that code was doing is looping through the radios. Maybe > there is some case I haven't thought of, in which case the kernel would drop the > frame anyways. Sure. To put it another way, hwsim works today by doing roughly the following: Suppose we have a setup with a single namespace, with 3 radios. Radio A with address aa Radio B with address bb Radio C with address cc When a _unicast_ frame with target 'cc' is seen by hwsim, it loops over A, B, C and compares the addresses. If the addresses do not match, then no HWSIM_CMD_FRAME is issued for that radio. So we'd only issue HWSIM_CMD_FRAME with Radio C's HWSIM_ATTR_ADDR_RECEIVER. What you're doing in this patch is to now issue the frame to each and every known radio. So suppose you change this to 2 namespaces, with Radio A+B in namespace A, Radio C in namespace C, etc. Are you running 2 instances of hwsim? One per namespace? How does it help if namespace A receives a frame with target 'CC'? It doesn't know about Radio C, so how would radio C even receive the message? And if it somehow does anyway, then it would seem like the above dispatch logic is bogus in the first place? > > - Then with the introduction of namespaces we actually do hit this case of > having unknown destination addresses. Not because they are bogus, but because > hwsim simply cannot know about radios in other namespaces. > Right, I get that. But this is not what my question is about :) Regards, -Denis
Hi Denis, On 5/8/23 11:05 AM, Denis Kenzior wrote: > Hi James, > >>> Second, if we're passing unicast frames, and the unicast frame is >>> addressed to a radio in a different namespace, how does that radio >>> get the frame? We don't have it in our radio_info queue? >> >> The namespace bars hwsim from seeing radios in other namespaces via >> GET_WIPHY. So yes, the radios are not in the radio_info queue. But >> since the radios are on the same kernel any MAC the kernel knows about >> is accepted when sending a frame, regardless of namespace. >> > > I get this part. What bothers me is that the current dispatch code > seems to be completely wrong? > >>> >>> The comment for the code you're taking out makes it clear that the >>> intent is for unicast frames to be sent only only to the radio with >>> the target receiver address. And the optimization is simply about >>> not sending frames to radios with addresses we know do not have the >>> target address. >>> >>> What you're doing here seems to indicate that our implementation (and >>> the comment) is completely bogus. And we can simply send any frame >>> to any radio instance (once) and the frames will be forwarded >>> automagically. If so, then you might want to explain this better and >>> fix the code to reflect this. >> >> Maybe I described this badly, but I was trying to make 2 points: >> >> - Historically, before namespaces, hwsim should never be in a >> situation where a frame is addressed to an unknown recipient. Hostapd >> won't do this and IWD won't do this. So all that code was doing is >> looping through the radios. Maybe there is some case I haven't thought >> of, in which case the kernel would drop the frame anyways. > > Sure. To put it another way, hwsim works today by doing roughly the > following: > > Suppose we have a setup with a single namespace, with 3 radios. > > Radio A with address aa > Radio B with address bb > Radio C with address cc > > When a _unicast_ frame with target 'cc' is seen by hwsim, it loops over > A, B, C and compares the addresses. If the addresses do not match, then > no HWSIM_CMD_FRAME is issued for that radio. So we'd only issue > HWSIM_CMD_FRAME with Radio C's HWSIM_ATTR_ADDR_RECEIVER. Ah ok, I see where you're coming from now. I overlooked that part. I can re-work this and if the destination is a known radio then only forward to that radio. If the destination is to an unknown radio I think we have to forward it regardless. > > What you're doing in this patch is to now issue the frame to each and > every known radio. > > So suppose you change this to 2 namespaces, with Radio A+B in namespace > A, Radio C in namespace C, etc. Are you running 2 instances of hwsim? > One per namespace? Yes, a separate hwsim instance per-namespace. > > How does it help if namespace A receives a frame with target 'CC'? It > doesn't know about Radio C, so how would radio C even receive the > message? And if it somehow does anyway, then it would seem like the > above dispatch logic is bogus in the first place? If a client in namespace A sends a frame to target CC then the hwsim instance in namespace A gets this frame. It doesn't know about target CC but the kernel does. So it forwards into the kernel and target CC receives it. I don't think there is anything wrong with the dispatch logic. There is just a disconnect between what information hwsim knows vs the kernel. I think tweaking the optimization I removed (described above) effectively keeps things the same (assuming clients are sane and don't send frames to completely bogus addresses). > >> >> - Then with the introduction of namespaces we actually do hit this >> case of having unknown destination addresses. Not because they are >> bogus, but because hwsim simply cannot know about radios in other >> namespaces. >> > > Right, I get that. But this is not what my question is about :) > > Regards, > -Denis
Hi James, >> >> Suppose we have a setup with a single namespace, with 3 radios. >> >> Radio A with address aa >> Radio B with address bb >> Radio C with address cc >> >> When a _unicast_ frame with target 'cc' is seen by hwsim, it loops over A, B, >> C and compares the addresses. If the addresses do not match, then no >> HWSIM_CMD_FRAME is issued for that radio. So we'd only issue HWSIM_CMD_FRAME >> with Radio C's HWSIM_ATTR_ADDR_RECEIVER. > > Ah ok, I see where you're coming from now. I overlooked that part. I can re-work > this and if the destination is a known radio then only forward to that radio. If > the destination is to an unknown radio I think we have to forward it regardless. > Well, that's the part that is unclear to me. See below. >> >> How does it help if namespace A receives a frame with target 'CC'? It doesn't >> know about Radio C, so how would radio C even receive the message? And if it >> somehow does anyway, then it would seem like the above dispatch logic is bogus >> in the first place? > > If a client in namespace A sends a frame to target CC then the hwsim instance in > namespace A gets this frame. It doesn't know about target CC but the kernel > does. So it forwards into the kernel and target CC receives it. So suppose client in namespace A sends something via Radio A to Radio C. hwsim instance in namespace A will loop over the radios it knows about, which are A and B. A is the source radio, so that should be ignored. So with your proposed logic, hwsim in namespace A will send the frame to radio B with the target being Radio C. But that would seem to be pointless? If sending to radio B regardless of the target address is fine, why do we even bother with the dispatch logic at all? Are you sure it isn't being sent to hwsim instances in all namespaces? Regards, -Denis
Hi James, > > Ok, actually I see your confusion now. Removing that block of code would send > the frame to all *known* radios only... So this actually should not work, but it > does. Yep! Exactly. > > Its like the kernel is disregarding ATTR_RECEIVER_ADDR and just looking at the > frame contents. Anyways, I'll investigate. > Ok, great. Regards, -Denis
On 5/8/23 11:55 AM, James Prestwood wrote: > Hi Denis, > > On 5/8/23 11:05 AM, Denis Kenzior wrote: >> Hi James, >> >>>> Second, if we're passing unicast frames, and the unicast frame is >>>> addressed to a radio in a different namespace, how does that radio >>>> get the frame? We don't have it in our radio_info queue? >>> >>> The namespace bars hwsim from seeing radios in other namespaces via >>> GET_WIPHY. So yes, the radios are not in the radio_info queue. But >>> since the radios are on the same kernel any MAC the kernel knows >>> about is accepted when sending a frame, regardless of namespace. >>> >> >> I get this part. What bothers me is that the current dispatch code >> seems to be completely wrong? >> >>>> >>>> The comment for the code you're taking out makes it clear that the >>>> intent is for unicast frames to be sent only only to the radio with >>>> the target receiver address. And the optimization is simply about >>>> not sending frames to radios with addresses we know do not have the >>>> target address. >>>> >>>> What you're doing here seems to indicate that our implementation >>>> (and the comment) is completely bogus. And we can simply send any >>>> frame to any radio instance (once) and the frames will be forwarded >>>> automagically. If so, then you might want to explain this better >>>> and fix the code to reflect this. >>> >>> Maybe I described this badly, but I was trying to make 2 points: >>> >>> - Historically, before namespaces, hwsim should never be in a >>> situation where a frame is addressed to an unknown recipient. Hostapd >>> won't do this and IWD won't do this. So all that code was doing is >>> looping through the radios. Maybe there is some case I haven't >>> thought of, in which case the kernel would drop the frame anyways. >> >> Sure. To put it another way, hwsim works today by doing roughly the >> following: >> >> Suppose we have a setup with a single namespace, with 3 radios. >> >> Radio A with address aa >> Radio B with address bb >> Radio C with address cc >> >> When a _unicast_ frame with target 'cc' is seen by hwsim, it loops >> over A, B, C and compares the addresses. If the addresses do not >> match, then no HWSIM_CMD_FRAME is issued for that radio. So we'd only >> issue HWSIM_CMD_FRAME with Radio C's HWSIM_ATTR_ADDR_RECEIVER. > > Ah ok, I see where you're coming from now. I overlooked that part. I can > re-work this and if the destination is a known radio then only forward > to that radio. If the destination is to an unknown radio I think we have > to forward it regardless. > >> >> What you're doing in this patch is to now issue the frame to each and >> every known radio. >> >> So suppose you change this to 2 namespaces, with Radio A+B in >> namespace A, Radio C in namespace C, etc. Are you running 2 instances >> of hwsim? One per namespace? > > Yes, a separate hwsim instance per-namespace. > >> >> How does it help if namespace A receives a frame with target 'CC'? It >> doesn't know about Radio C, so how would radio C even receive the >> message? And if it somehow does anyway, then it would seem like the >> above dispatch logic is bogus in the first place? > > If a client in namespace A sends a frame to target CC then the hwsim > instance in namespace A gets this frame. It doesn't know about target CC > but the kernel does. So it forwards into the kernel and target CC > receives it. > > I don't think there is anything wrong with the dispatch logic. There is > just a disconnect between what information hwsim knows vs the kernel. I > think tweaking the optimization I removed (described above) effectively > keeps things the same (assuming clients are sane and don't send frames > to completely bogus addresses). Ok, actually I see your confusion now. Removing that block of code would send the frame to all *known* radios only... So this actually should not work, but it does. Its like the kernel is disregarding ATTR_RECEIVER_ADDR and just looking at the frame contents. Anyways, I'll investigate. > > >> >>> >>> - Then with the introduction of namespaces we actually do hit this >>> case of having unknown destination addresses. Not because they are >>> bogus, but because hwsim simply cannot know about radios in other >>> namespaces. >>> >> >> Right, I get that. But this is not what my question is about :) >> >> Regards, >> -Denis
Hi Denis, On 5/8/23 12:01 PM, Denis Kenzior wrote: > Hi James, > >> >> Ok, actually I see your confusion now. Removing that block of code >> would send the frame to all *known* radios only... So this actually >> should not work, but it does. > > Yep! Exactly. > >> >> Its like the kernel is disregarding ATTR_RECEIVER_ADDR and just >> looking at the frame contents. Anyways, I'll investigate. >> > > Ok, great. Just an update here. The reason hwsim was behaving like this was because DEL_WIPHY isn't handled but DEL_INTERFACE is. So when hwsim starts it dumps and tracks all the radios but when a radio gets moved into another namespace only the interfaces get cleaned up. Then a frame comes and hwsim won't find a matching interface->addr to send the frame, and bail out. But when I removed that block of code it wouldn't bail out and since the radios still existed in the queue the frame could be sent out to all radios including "removed" radios. Since the kernel doesn't care about namespaces those "removed" radios still got the frames. So at least that's sorted out. How we want to support namespaces is the question. A few options: - Peek into the frame at the destination address and use that as ATTR_RECEIVER, but this won't work with address randomization for phys outside of the root namespace. IMO this is probably fine for testing purposes, if we know about it ahead of time. - Add support to hwsim to move radios to other namespaces, and keep track of those radios (similar to what we erroneously have today). This is fragile though because you no longer can remove radios/interfaces, nor do you know if they have been removed. Thanks, James > > Regards, > -Denis
Hi James, > Just an update here. The reason hwsim was behaving like this was because > DEL_WIPHY isn't handled but DEL_INTERFACE is. > Funny. > So when hwsim starts it dumps and tracks all the radios but when a radio gets > moved into another namespace only the interfaces get cleaned up. Then a frame > comes and hwsim won't find a matching interface->addr to send the frame, and > bail out. But when I removed that block of code it wouldn't bail out and since > the radios still existed in the queue the frame could be sent out to all radios > including "removed" radios. Since the kernel doesn't care about namespaces those > "removed" radios still got the frames. Ok, that makes sense now. > > So at least that's sorted out. How we want to support namespaces is the > question. A few options: > > - Peek into the frame at the destination address and use that as ATTR_RECEIVER, > but this won't work with address randomization for phys outside of the root > namespace. IMO this is probably fine for testing purposes, if we know about it > ahead of time. You can try to see whether HWSIM_CMD_ADD_MAC_ADDR works? See commit 5cc58a9ecfa1 ("mac80211_hwsim: notify wmediumd of used MAC addresses") > > - Add support to hwsim to move radios to other namespaces, and keep track of > those radios (similar to what we erroneously have today). This is fragile though > because you no longer can remove radios/interfaces, nor do you know if they have > been removed. Hmm, but radios are namespace independent. They can only be added/removed via HWSIM_CMD_ADD/DEL_RADIO, no? Since phys are moved wholesale across namespaces (you can't only move a given interface), you could assume that once a radio is created and populated, its interfaces do not change for the duration of the test, even if they're moved to a different namespace. Or if the HWSIM_CMD_ADD_MAC_ADDR API works, that might make things even simpler. Regards, -Denis
Hi Denis, On 6/26/23 7:31 PM, Denis Kenzior wrote: > Hi James, > >> Just an update here. The reason hwsim was behaving like this was >> because DEL_WIPHY isn't handled but DEL_INTERFACE is. >> > > Funny. > >> So when hwsim starts it dumps and tracks all the radios but when a >> radio gets moved into another namespace only the interfaces get >> cleaned up. Then a frame comes and hwsim won't find a matching >> interface->addr to send the frame, and bail out. But when I removed >> that block of code it wouldn't bail out and since the radios still >> existed in the queue the frame could be sent out to all radios >> including "removed" radios. Since the kernel doesn't care about >> namespaces those "removed" radios still got the frames. > > Ok, that makes sense now. > >> >> So at least that's sorted out. How we want to support namespaces is >> the question. A few options: >> >> - Peek into the frame at the destination address and use that as >> ATTR_RECEIVER, but this won't work with address randomization for phys >> outside of the root namespace. IMO this is probably fine for testing >> purposes, if we know about it ahead of time. > > You can try to see whether HWSIM_CMD_ADD_MAC_ADDR works? See commit > 5cc58a9ecfa1 ("mac80211_hwsim: notify wmediumd of used MAC addresses") I hadn't seen that before, but looking at it they don't expose that as an API, its only for internal use for scan address randomization and monitor interfaces (see mac80211_hwsim_config_mac_nl()). > >> >> - Add support to hwsim to move radios to other namespaces, and keep >> track of those radios (similar to what we erroneously have today). >> This is fragile though because you no longer can remove >> radios/interfaces, nor do you know if they have been removed. > > Hmm, but radios are namespace independent. They can only be > added/removed via HWSIM_CMD_ADD/DEL_RADIO, no? Since phys are moved > wholesale across namespaces (you can't only move a given interface), you > could assume that once a radio is created and populated, its interfaces > do not change for the duration of the test, even if they're moved to a > different namespace. For testing yes this is probably fine. It may require some adaptation in hwsim to do it better from a test-runner perspective. Currently we just use ip to create/delete namespaces and move radios. It may make more sense to add this to hwsim so there is one path. Then at least when hwsim gets a DEL_WIPHY event it doesn't have to assume the radio was moved (I'm thinking if we ever added hotplug tests this could be important). I'll pursue this path. > > Or if the HWSIM_CMD_ADD_MAC_ADDR API works, that might make things even > simpler. > > Regards, > -Denis
Hi James, >> You can try to see whether HWSIM_CMD_ADD_MAC_ADDR works? See commit >> 5cc58a9ecfa1 ("mac80211_hwsim: notify wmediumd of used MAC addresses") > > I hadn't seen that before, but looking at it they don't expose that as an API, > its only for internal use for scan address randomization and monitor interfaces > (see mac80211_hwsim_config_mac_nl()). > It is used as an unsolicited event / notification. Yeah I know, HWSIM_CMD_* prefix is confusing ;) It looks like we should start using this since most of our autotests are forced to include the following main.conf: [Scan] DisableMacAddressRandomization=true Having support for the above event would allow us to get rid of this hack. <snip> >> >> Hmm, but radios are namespace independent. They can only be added/removed via >> HWSIM_CMD_ADD/DEL_RADIO, no? Since phys are moved wholesale across namespaces >> (you can't only move a given interface), you could assume that once a radio is >> created and populated, its interfaces do not change for the duration of the >> test, even if they're moved to a different namespace. > > For testing yes this is probably fine. It may require some adaptation in hwsim > to do it better from a test-runner perspective. Currently we just use ip to > create/delete namespaces and move radios. It may make more sense to add this to > hwsim so there is one path. Then at least when hwsim gets a DEL_WIPHY event it It shouldn't matter really who invokes the namespace move. hwsim would know whether it is a hot-unplug or a namespace move by virtue of being the one who triggers HWSIM_CMD_DEL_RADIO. > doesn't have to assume the radio was moved (I'm thinking if we ever added > hotplug tests this could be important). We should add these, but the above still stands. hwsim is the only thing in our tests that triggers HWSIM_CMD_ADD_RADIO. Regards, -Denis
Hi Denis, On 6/27/23 11:00 AM, Denis Kenzior wrote: > Hi James, > >>> You can try to see whether HWSIM_CMD_ADD_MAC_ADDR works? See commit >>> 5cc58a9ecfa1 ("mac80211_hwsim: notify wmediumd of used MAC addresses") >> >> I hadn't seen that before, but looking at it they don't expose that as >> an API, its only for internal use for scan address randomization and >> monitor interfaces (see mac80211_hwsim_config_mac_nl()). >> > > It is used as an unsolicited event / notification. Yeah I know, > HWSIM_CMD_* prefix is confusing ;) > > It looks like we should start using this since most of our autotests are > forced to include the following main.conf: > > [Scan] > DisableMacAddressRandomization=true > > Having support for the above event would allow us to get rid of this hack. IIRC Tim started adding that into tests a long time ago, and had said improves reliability. If I remove it (e.g. in testPSK-roam) things start breaking, but its very random and not repeatable, some pass and some fail between runs. I would expect all hwsim tests to fail if we are dropping all frames with and unknown TX or RX address so something else must be going on here. But indeed, this *should* work if we handle these events similarly to wmediumd. > > <snip> > >>> >>> Hmm, but radios are namespace independent. They can only be >>> added/removed via HWSIM_CMD_ADD/DEL_RADIO, no? Since phys are moved >>> wholesale across namespaces (you can't only move a given interface), >>> you could assume that once a radio is created and populated, its >>> interfaces do not change for the duration of the test, even if >>> they're moved to a different namespace. >> >> For testing yes this is probably fine. It may require some adaptation >> in hwsim to do it better from a test-runner perspective. Currently we >> just use ip to create/delete namespaces and move radios. It may make >> more sense to add this to hwsim so there is one path. Then at least >> when hwsim gets a DEL_WIPHY event it > > It shouldn't matter really who invokes the namespace move. hwsim would > know whether it is a hot-unplug or a namespace move by virtue of being > the one who triggers HWSIM_CMD_DEL_RADIO. > >> doesn't have to assume the radio was moved (I'm thinking if we ever >> added hotplug tests this could be important). > > We should add these, but the above still stands. hwsim is the only > thing in our tests that triggers HWSIM_CMD_ADD_RADIO. Yeah true, I'll just flag the radio as moved but keep it around for processing frames. > > Regards, > -Denis
Hi James, >> It looks like we should start using this since most of our autotests are >> forced to include the following main.conf: >> >> [Scan] >> DisableMacAddressRandomization=true >> >> Having support for the above event would allow us to get rid of this hack. > IIRC Tim started adding that into tests a long time ago, and had said improves > reliability. If I remove it (e.g. in testPSK-roam) things start breaking, but > its very random and not repeatable, some pass and some fail between runs. Yep. It is very random because the active scans via probe requests fail, but the beacons still come in. So it is very much timing/luck dependent whether a scan picks up the network the test is interested in. We started noticing the failures shortly after adding scan address randomization support. 'git blame' pointed the finger, so we added this setting to work around the issue. Seems blindly obvious now, but we didn't recognize the link between address randomization and hwsim at that time. > > I would expect all hwsim tests to fail if we are dropping all frames with and > unknown TX or RX address so something else must be going on here. But indeed, > this *should* work if we handle these events similarly to wmediumd. See above about beacons. Regards, -Denis
On 6/27/23 11:00 AM, Denis Kenzior wrote: > Hi James, > >>> You can try to see whether HWSIM_CMD_ADD_MAC_ADDR works? See commit >>> 5cc58a9ecfa1 ("mac80211_hwsim: notify wmediumd of used MAC addresses") >> >> I hadn't seen that before, but looking at it they don't expose that as >> an API, its only for internal use for scan address randomization and >> monitor interfaces (see mac80211_hwsim_config_mac_nl()). >> > > It is used as an unsolicited event / notification. Yeah I know, > HWSIM_CMD_* prefix is confusing ;) > > It looks like we should start using this since most of our autotests are > forced to include the following main.conf: > > [Scan] > DisableMacAddressRandomization=true > > Having support for the above event would allow us to get rid of this hack. > > <snip> > >>> >>> Hmm, but radios are namespace independent. They can only be >>> added/removed via HWSIM_CMD_ADD/DEL_RADIO, no? Since phys are moved >>> wholesale across namespaces (you can't only move a given interface), >>> you could assume that once a radio is created and populated, its >>> interfaces do not change for the duration of the test, even if >>> they're moved to a different namespace. >> >> For testing yes this is probably fine. It may require some adaptation >> in hwsim to do it better from a test-runner perspective. Currently we >> just use ip to create/delete namespaces and move radios. It may make >> more sense to add this to hwsim so there is one path. Then at least >> when hwsim gets a DEL_WIPHY event it > > It shouldn't matter really who invokes the namespace move. hwsim would > know whether it is a hot-unplug or a namespace move by virtue of being > the one who triggers HWSIM_CMD_DEL_RADIO. So one other caveat with this is that IWD creates its interface while inside the namespace. So we actually cannot track that. Only thing we could do is force the interface creation ahead of time, and make namespace'd iwd's use the default interface. > >> doesn't have to assume the radio was moved (I'm thinking if we ever >> added hotplug tests this could be important). > > We should add these, but the above still stands. hwsim is the only > thing in our tests that triggers HWSIM_CMD_ADD_RADIO. > > Regards, > -Denis
Hi James, > > So one other caveat with this is that IWD creates its interface while inside the > namespace. So we actually cannot track that. Only thing we could do is force the > interface creation ahead of time, and make namespace'd iwd's use the default > interface. > Yep. Since iwd will have to handle multi-mode devices and multiple interfaces, there's really not much we can do in the general case. This is why we need to pay attention to HWSIM_CMD_ADD_MAC_ADDR. Looking at that kernel commit again, it seems to already invoke mac80211_hwsim_config_mac_nl() on add_interface and remove_interface. So maybe everything needed to handle interface creation/removal inside namespaces is already there? If not, then any missing event generation should be fixed in the kernel. Regards, -Denis
Hi Denis, On 6/28/23 7:49 AM, Denis Kenzior wrote: > Hi James, > >> >> So one other caveat with this is that IWD creates its interface while >> inside the namespace. So we actually cannot track that. Only thing we >> could do is force the interface creation ahead of time, and make >> namespace'd iwd's use the default interface. >> > > Yep. Since iwd will have to handle multi-mode devices and multiple > interfaces, there's really not much we can do in the general case. This > is why we need to pay attention to HWSIM_CMD_ADD_MAC_ADDR. > > Looking at that kernel commit again, it seems to already invoke > mac80211_hwsim_config_mac_nl() on add_interface and remove_interface. > So maybe everything needed to handle interface creation/removal inside > namespaces is already there? If not, then any missing event generation > should be fixed in the kernel. Unfortunately once you move the radio into another namespace you lose any events associated with that radio, due to the PID of hwsim being associated with a distinct namespace (I think). So ADD/DEL_MAC_ADDR won't help here. But still, its nice getting the ADD/DEL_MAC_ADDR events purely for supporting address randomization. > > Regards, > -Denis
Hi James, > > Unfortunately once you move the radio into another namespace you lose any events > associated with that radio, due to the PID of hwsim being associated with a > distinct namespace (I think). So ADD/DEL_MAC_ADDR won't help here. Which events though? You should always be getting HWSIM_CMD_ events for _all_ radios since only a single hwsim instance is registered, no? Aren't these all you need for the purposes of sending packets to the right radio? The NL80211 WIPHY events aren't really needed. We only use these for informational purposes (only the name is used, I think?) I guess we also use this information for the radio_ap_only() optimization we do, but we could probably solve this another way. Regards, -Denis
Hi Denis, On 6/28/23 8:40 AM, Denis Kenzior wrote: > Hi James, > >> >> Unfortunately once you move the radio into another namespace you lose >> any events associated with that radio, due to the PID of hwsim being >> associated with a distinct namespace (I think). So ADD/DEL_MAC_ADDR >> won't help here. > > Which events though? You should always be getting HWSIM_CMD_ events for > _all_ radios since only a single hwsim instance is registered, no? > Aren't these all you need for the purposes of sending packets to the > right radio? ADD/DEL_MAC_ADDR, which don't come once the radio is moved to another namespace (as far as I can tell with testing). I think it comes down to port ID: u32 _portid = READ_ONCE(data->wmediumd); ... hwsim_unicast_netgroup(data, skb, _portid); One other potential (but much more complex) solution would be to use multiple hwsim instances that communicate, making a sort of distributed hwsim environment. I'm working on a similar concept which could (partially) be upstreamed, at least the hwsim parts. We could provide hwsim a socket/FD to communicate with other hwsim instances. hwsim would behave the same as it does now unless it receives a frame which doesn't match to any known radios. In which case it send that frame out to the external socket. It would also receive on that socket, and pass the frame back to its local hwsim socket. I have it now where I just use the netlink protocol between hwsim instances, so you can receive on the remote socket/FD and send that directly back to the kernel for the local instance. This is similar enough that some of what I've done already could be upstreamed and solve this namespace problem. Thanks, James > > The NL80211 WIPHY events aren't really needed. We only use these for > informational purposes (only the name is used, I think?) I guess we > also use this information for the radio_ap_only() optimization we do, > but we could probably solve this another way. > > Regards, > -Denis
Hi James, >>> Unfortunately once you move the radio into another namespace you lose any >>> events associated with that radio, due to the PID of hwsim being associated >>> with a distinct namespace (I think). So ADD/DEL_MAC_ADDR won't help here. >> >> Which events though? You should always be getting HWSIM_CMD_ events for _all_ >> radios since only a single hwsim instance is registered, no? Aren't these all >> you need for the purposes of sending packets to the right radio? > > ADD/DEL_MAC_ADDR, which don't come once the radio is moved to another namespace > (as far as I can tell with testing). > > I think it comes down to port ID: > > u32 _portid = READ_ONCE(data->wmediumd); > ... > hwsim_unicast_netgroup(data, skb, _portid); Then this may be a conversation you have to start on linux-wireless. I would have thought that there should only be a single wmediumd instance on a system regardless of namespaces, but maybe I'm wrong here. Regards, -Denis
Hi Denis, On 6/28/23 9:25 AM, Denis Kenzior wrote: > Hi James, > >>>> Unfortunately once you move the radio into another namespace you >>>> lose any events associated with that radio, due to the PID of hwsim >>>> being associated with a distinct namespace (I think). So >>>> ADD/DEL_MAC_ADDR won't help here. >>> >>> Which events though? You should always be getting HWSIM_CMD_ events >>> for _all_ radios since only a single hwsim instance is registered, >>> no? Aren't these all you need for the purposes of sending packets to >>> the right radio? >> >> ADD/DEL_MAC_ADDR, which don't come once the radio is moved to another >> namespace (as far as I can tell with testing). >> >> I think it comes down to port ID: >> >> u32 _portid = READ_ONCE(data->wmediumd); >> ... >> hwsim_unicast_netgroup(data, skb, _portid); > > Then this may be a conversation you have to start on linux-wireless. I > would have thought that there should only be a single wmediumd instance > on a system regardless of namespaces, but maybe I'm wrong here. Yeah, I'll do that as well. This would simplify things if the namespaces didn't come into play. Nevertheless, was the distributed hwsim concept something you'd be interested in accepting upstream? Like I said, I'm already doing it and I'm happy to re-use hwsim and extend to support this external socket concept. > Regards, > -Denis
Hi James, >> Then this may be a conversation you have to start on linux-wireless. I would >> have thought that there should only be a single wmediumd instance on a system >> regardless of namespaces, but maybe I'm wrong here. > > Yeah, I'll do that as well. This would simplify things if the namespaces didn't > come into play. Check 100cb9ff40e0 ("mac80211_hwsim: Allow managing radios from non-initial namespaces") f21e4d8ed16b ("mac80211_hwsim: Allow wmediumd to attach to radios created in its netns") From those two commits I get the impression that the intent is to allow a single wmediumd across namespaces. If for some reason the ADD_MAC_ADDR messages are not being sent, then I would look into fixing that first. > > Nevertheless, was the distributed hwsim concept something you'd be interested in > accepting upstream? Like I said, I'm already doing it and I'm happy to re-use > hwsim and extend to support this external socket concept. I'm not against it, but as you point out, it sounds super complicated. So I'd have to look into it more once you are ready to share something. Regards, -Denis
Hi Denis, On 6/28/23 9:57 AM, Denis Kenzior wrote: > Hi James, > >>> Then this may be a conversation you have to start on linux-wireless. >>> I would have thought that there should only be a single wmediumd >>> instance on a system regardless of namespaces, but maybe I'm wrong here. >> >> Yeah, I'll do that as well. This would simplify things if the >> namespaces didn't come into play. > > Check > 100cb9ff40e0 ("mac80211_hwsim: Allow managing radios from non-initial > namespaces") > f21e4d8ed16b ("mac80211_hwsim: Allow wmediumd to attach to radios > created in its netns") Yeah, the second one does mention frames specifically, which I am getting from radios outside the namespace. But I don't see a difference in how ADD/DEL_MAC_ADDR works. But yeah, this is something I'll look into. > > From those two commits I get the impression that the intent is to allow > a single wmediumd across namespaces. If for some reason the > ADD_MAC_ADDR messages are not being sent, then I would look into fixing > that first. > >> >> Nevertheless, was the distributed hwsim concept something you'd be >> interested in accepting upstream? Like I said, I'm already doing it >> and I'm happy to re-use hwsim and extend to support this external >> socket concept. > > I'm not against it, but as you point out, it sounds super complicated. > So I'd have to look into it more once you are ready to share something. Ok fair enough. > > Regards, > -Denis >
Hi James, On Wed, 28 Jun 2023 at 18:14, James Prestwood <prestwoj@gmail.com> wrote: > One other potential (but much more complex) solution would be to use > multiple hwsim instances that communicate, making a sort of distributed > hwsim environment. I'm working on a similar concept which could > (partially) be upstreamed, at least the hwsim parts. Another option would be to occasionally dump information about phys and radios in all namespaces. While this is complicated it may be a little easier than having multiple hwsim instances because you can move threads in a single process into network namespaces. I.e. you can, for example, share an l_queue between temporary threads each of which moves into one network namespace, opens a netlink socket, dumps the phy information and exits. Or you can keep the threads alive to track netlink events but locking becomes more complicated. Best regards
Hi Andrew, Hi Andrew, On 6/28/23 4:19 PM, Andrew Zaborowski wrote: > Hi James, > > On Wed, 28 Jun 2023 at 18:14, James Prestwood <prestwoj@gmail.com> wrote: >> One other potential (but much more complex) solution would be to use >> multiple hwsim instances that communicate, making a sort of distributed >> hwsim environment. I'm working on a similar concept which could >> (partially) be upstreamed, at least the hwsim parts. > > Another option would be to occasionally dump information about phys > and radios in all namespaces. While this is complicated it may be a > little easier than having multiple hwsim instances because you can > move threads in a single process into network namespaces. I.e. you > can, for example, share an l_queue between temporary threads each of > which moves into one network namespace, opens a netlink socket, dumps > the phy information and exits. Or you can keep the threads alive to > track netlink events but locking becomes more complicated. Its good to hear from you :) Yeah that could work too. I just already have this concept working for other purposes, where the instances don't share the same kernel. So it makes some sense to upstream this and have the communication be generalized to an FD that could be Unix, UDP, TCP, etc. It may not be useful to test-runner, but since I used hwsim as a base for it I feel like it should be upstreamed if possible. I'll be sharing this soon. For now, and purely test-runner purposes, watching for ADD/DEL_MAC_ADDR events seems to be enough. > > Best regards
diff --git a/tools/hwsim.c b/tools/hwsim.c index 47352ad4..10a9db5f 100644 --- a/tools/hwsim.c +++ b/tools/hwsim.c @@ -1501,43 +1501,10 @@ static void process_frame(struct hwsim_frame *frame) struct send_frame_info *send_info; bool drop = drop_mcast; uint32_t delay = 0; - const struct l_queue_entry *i; if (radio == frame->src_radio) continue; - /* - * The kernel hwsim medium passes multicast frames to all - * radios that are on the same frequency as this frame but - * the netlink medium API only lets userspace pass frames to - * radios by known hardware address. It does check that the - * receiving radio is on the same frequency though so we can - * send to all known addresses. - * - * If the frame's Receiver Address (RA) is a multicast - * address, then send the frame to every radio that is - * registered. If it's a unicast address then optimize - * by only forwarding the frame to the radios that have - * at least one interface with this specific address. - */ - if (!util_is_broadcast_address(frame->dst_ether_addr)) { - for (i = l_queue_get_entries(interface_info); - i; i = i->next) { - struct interface_info_rec *interface = i->data; - - if (interface->radio_rec != radio) - continue; - - if (!memcmp(interface->addr, - frame->dst_ether_addr, - ETH_ALEN)) - break; - } - - if (!i) - continue; - } - process_rules(frame->src_radio, radio, frame, false, &drop, &delay);