Message ID | 1522379640-6442-3-git-send-email-cjhuang@codeaurora.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Carl, On Fri, Mar 30, 2018 at 11:14:00AM +0800, Carl Huang wrote: > The ath10k reports the random_mac_addr capability to upper layer > based on the service bit firmware reported. Driver sets the > spoofed flag in scan_ctrl_flag to firmware if upper layer has > enabled this feature in scan request. > > Test with QCA6174 hw3.0 and firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1, > but QCA9377 is also affected. > > Signed-off-by: Carl Huang <cjhuang@codeaurora.org> > --- > drivers/net/wireless/ath/ath10k/mac.c | 17 +++++++++++++++++ > drivers/net/wireless/ath/ath10k/wmi-ops.h | 22 ++++++++++++++++++++++ > drivers/net/wireless/ath/ath10k/wmi-tlv.c | 25 +++++++++++++++++++++++++ > drivers/net/wireless/ath/ath10k/wmi-tlv.h | 11 +++++++++++ > drivers/net/wireless/ath/ath10k/wmi.c | 5 +++++ > drivers/net/wireless/ath/ath10k/wmi.h | 9 +++++++++ > 6 files changed, 89 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index ebb3f1b..c5cd5e5 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -5699,6 +5699,12 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw, > arg.scan_ctrl_flags |= WMI_SCAN_FLAG_PASSIVE; > } > > + if (req->flags & NL80211_SCAN_FLAG_RANDOM_ADDR) { > + arg.scan_ctrl_flags |= WMI_SCAN_ADD_SPOOFED_MAC_IN_PROBE_REQ; > + ether_addr_copy(arg.mac_addr.addr, req->mac_addr); > + ether_addr_copy(arg.mac_mask.addr, req->mac_addr_mask); > + } > + > if (req->n_channels) { > arg.n_channels = req->n_channels; > for (i = 0; i < arg.n_channels; i++) > @@ -8397,6 +8403,17 @@ int ath10k_mac_register(struct ath10k *ar) > goto err_dfs_detector_exit; > } > > + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { > + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); > + if (ret) { > + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); > + goto err_dfs_detector_exit; > + } > + > + ar->hw->wiphy->features |= > + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? > + } > + Brian
On 2018-04-13 04:59, Brian Norris wrote: > Hi Carl, > > On Fri, Mar 30, 2018 at 11:14:00AM +0800, Carl Huang wrote: >> The ath10k reports the random_mac_addr capability to upper layer >> based on the service bit firmware reported. Driver sets the >> spoofed flag in scan_ctrl_flag to firmware if upper layer has >> enabled this feature in scan request. >> >> Test with QCA6174 hw3.0 and >> firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1, >> but QCA9377 is also affected. >> >> Signed-off-by: Carl Huang <cjhuang@codeaurora.org> >> --- >> drivers/net/wireless/ath/ath10k/mac.c | 17 +++++++++++++++++ >> drivers/net/wireless/ath/ath10k/wmi-ops.h | 22 ++++++++++++++++++++++ >> drivers/net/wireless/ath/ath10k/wmi-tlv.c | 25 >> +++++++++++++++++++++++++ >> drivers/net/wireless/ath/ath10k/wmi-tlv.h | 11 +++++++++++ >> drivers/net/wireless/ath/ath10k/wmi.c | 5 +++++ >> drivers/net/wireless/ath/ath10k/wmi.h | 9 +++++++++ >> 6 files changed, 89 insertions(+) >> >> diff --git a/drivers/net/wireless/ath/ath10k/mac.c >> b/drivers/net/wireless/ath/ath10k/mac.c >> index ebb3f1b..c5cd5e5 100644 >> --- a/drivers/net/wireless/ath/ath10k/mac.c >> +++ b/drivers/net/wireless/ath/ath10k/mac.c >> @@ -5699,6 +5699,12 @@ static int ath10k_hw_scan(struct ieee80211_hw >> *hw, >> arg.scan_ctrl_flags |= WMI_SCAN_FLAG_PASSIVE; >> } >> >> + if (req->flags & NL80211_SCAN_FLAG_RANDOM_ADDR) { >> + arg.scan_ctrl_flags |= WMI_SCAN_ADD_SPOOFED_MAC_IN_PROBE_REQ; >> + ether_addr_copy(arg.mac_addr.addr, req->mac_addr); >> + ether_addr_copy(arg.mac_mask.addr, req->mac_addr_mask); >> + } >> + >> if (req->n_channels) { >> arg.n_channels = req->n_channels; >> for (i = 0; i < arg.n_channels; i++) >> @@ -8397,6 +8403,17 @@ int ath10k_mac_register(struct ath10k *ar) >> goto err_dfs_detector_exit; >> } >> >> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >> + if (ret) { >> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >> + goto err_dfs_detector_exit; >> + } >> + >> + ar->hw->wiphy->features |= >> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; > > Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? I'll add this flag too. > >> + } >> + > > > Brian
cjhuang@codeaurora.org writes: >>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >>> + if (ret) { >>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >>> + goto err_dfs_detector_exit; >>> + } >>> + >>> + ar->hw->wiphy->features |= >>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; >> >> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? > > I'll add this flag too. Are you going to send v2 or what's the plan?
On 4/13/2018 1:28 PM, Kalle Valo wrote: > cjhuang@codeaurora.org writes: > >>>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >>>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >>>> + if (ret) { >>>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >>>> + goto err_dfs_detector_exit; >>>> + } >>>> + >>>> + ar->hw->wiphy->features |= >>>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; >>> >>> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? >> >> I'll add this flag too. > > Are you going to send v2 or what's the plan? Maybe a stupid question, but does ath10k support scheduled scan? Regards, Arend
On 2018-04-14 05:13, Arend van Spriel wrote: > On 4/13/2018 1:28 PM, Kalle Valo wrote: >> cjhuang@codeaurora.org writes: >> >>>>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >>>>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >>>>> + if (ret) { >>>>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >>>>> + goto err_dfs_detector_exit; >>>>> + } >>>>> + >>>>> + ar->hw->wiphy->features |= >>>>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; >>>> >>>> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? >>> >>> I'll add this flag too. >> >> Are you going to send v2 or what's the plan? > > Maybe a stupid question, but does ath10k support scheduled scan? > The reason is AVL test case needs this flag to enable random mac address scan. Maybe Brian Can explain why this flag is necessary. Thanks, Carl
On 2018-04-13 19:28, Kalle Valo wrote: > cjhuang@codeaurora.org writes: > >>>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >>>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >>>> + if (ret) { >>>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >>>> + goto err_dfs_detector_exit; >>>> + } >>>> + >>>> + ar->hw->wiphy->features |= >>>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; >>> >>> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? >> >> I'll add this flag too. > > Are you going to send v2 or what's the plan? Yes I'll send V2. Thanks, Carl
cjhuang@codeaurora.org writes: > On 2018-04-14 05:13, Arend van Spriel wrote: >> On 4/13/2018 1:28 PM, Kalle Valo wrote: >>> cjhuang@codeaurora.org writes: >>> >>>>>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >>>>>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >>>>>> + if (ret) { >>>>>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >>>>>> + goto err_dfs_detector_exit; >>>>>> + } >>>>>> + >>>>>> + ar->hw->wiphy->features |= >>>>>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; >>>>> >>>>> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? >>>> >>>> I'll add this flag too. >>> >>> Are you going to send v2 or what's the plan? >> >> Maybe a stupid question, but does ath10k support scheduled scan? AFAICS ath10k does not support it (sched_scan_start() op). > The reason is AVL test case needs this flag to enable random mac > address scan. Maybe Brian Can explain why this flag is necessary. If ath10k does not support scheduled scan what's the point? Shouldn't the test case then be it fixed instead of making hacks in ath10k?
Carl Huang <cjhuang@codeaurora.org> wrote: > The ath10k reports the random_mac_addr capability to upper layer > based on the service bit firmware reported. Driver sets the > spoofed flag in scan_ctrl_flag to firmware if upper layer has > enabled this feature in scan request. > > Test with QCA6174 hw3.0 and firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1, > but QCA9377 is also affected. > > Signed-off-by: Carl Huang <cjhuang@codeaurora.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> This added a new checkpatch warning: drivers/net/wireless/ath/ath10k/wmi-ops.h:123: Alignment should match open parenthesis I fixed it in the pending branch.
Hi, On Mon, Apr 16, 2018 at 02:32:47PM +0300, Kalle Valo wrote: > cjhuang@codeaurora.org writes: > > On 2018-04-14 05:13, Arend van Spriel wrote: > >> On 4/13/2018 1:28 PM, Kalle Valo wrote: > >>> cjhuang@codeaurora.org writes: > >>> > >>>>>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { > >>>>>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); > >>>>>> + if (ret) { > >>>>>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); > >>>>>> + goto err_dfs_detector_exit; > >>>>>> + } > >>>>>> + > >>>>>> + ar->hw->wiphy->features |= > >>>>>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; > >>>>> > >>>>> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? > >>>> > >>>> I'll add this flag too. > >>> > >>> Are you going to send v2 or what's the plan? > >> > >> Maybe a stupid question, but does ath10k support scheduled scan? Not a stupid question. Sorry for not asking that first. > AFAICS ath10k does not support it (sched_scan_start() op). Right, that seems to be the case. > > The reason is AVL test case needs this flag to enable random mac > > address scan. Maybe Brian Can explain why this flag is necessary. I never actually claimed you *needed* this flag; I just wondered how you claimed to pass our test when you did not support this flag, since our network manager currently checks for both NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR and NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR before enabling MAC randomization in scans. But that's no matter to worry about here. > If ath10k does not support scheduled scan what's the point? Shouldn't > the test case then be it fixed instead of making hacks in ath10k? Indeed. We're trying to work that out right now. It looks like the status quo for looking for SCHED_SCAN support is to check if NL80211_CMD_START_SCHED_SCAN shows up in the command support list. (IOW, that's what wpa_supplicant does.) We'll probably need to imitate that. Brian
On 2018-04-16 21:42, Kalle Valo wrote: > Carl Huang <cjhuang@codeaurora.org> wrote: > >> The ath10k reports the random_mac_addr capability to upper layer >> based on the service bit firmware reported. Driver sets the >> spoofed flag in scan_ctrl_flag to firmware if upper layer has >> enabled this feature in scan request. >> >> Test with QCA6174 hw3.0 and >> firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1, >> but QCA9377 is also affected. >> >> Signed-off-by: Carl Huang <cjhuang@codeaurora.org> >> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > > This added a new checkpatch warning: > > drivers/net/wireless/ath/ath10k/wmi-ops.h:123: Alignment should match > open parenthesis > > I fixed it in the pending branch. Kvalo, as the flag issue NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR will be fixed from Brian side, I don't need to submit V2 for it. For this alignment warning and sparse warnings in [1/2], as you fixed them in the pending branch, so I needn't do anything, right? Thanks, Carl
On 4/17/2018 2:28 AM, Brian Norris wrote: > Hi, > > On Mon, Apr 16, 2018 at 02:32:47PM +0300, Kalle Valo wrote: >> cjhuang@codeaurora.org writes: >>> On 2018-04-14 05:13, Arend van Spriel wrote: >>>> On 4/13/2018 1:28 PM, Kalle Valo wrote: >>>>> cjhuang@codeaurora.org writes: >>>>> >>>>>>>> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { >>>>>>>> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); >>>>>>>> + if (ret) { >>>>>>>> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); >>>>>>>> + goto err_dfs_detector_exit; >>>>>>>> + } >>>>>>>> + >>>>>>>> + ar->hw->wiphy->features |= >>>>>>>> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; >>>>>>> >>>>>>> Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too? >>>>>> >>>>>> I'll add this flag too. >>>>> >>>>> Are you going to send v2 or what's the plan? >>>> >>>> Maybe a stupid question, but does ath10k support scheduled scan? > > Not a stupid question. Sorry for not asking that first. > >> AFAICS ath10k does not support it (sched_scan_start() op). > > Right, that seems to be the case. > >>> The reason is AVL test case needs this flag to enable random mac >>> address scan. Maybe Brian Can explain why this flag is necessary. > > I never actually claimed you *needed* this flag; I just wondered how you > claimed to pass our test when you did not support this flag, since our > network manager currently checks for both > NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR and > NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR before enabling MAC > randomization in scans. But that's no matter to worry about here. > >> If ath10k does not support scheduled scan what's the point? Shouldn't >> the test case then be it fixed instead of making hacks in ath10k? > > Indeed. We're trying to work that out right now. > > It looks like the status quo for looking for SCHED_SCAN support is to > check if NL80211_CMD_START_SCHED_SCAN shows up in the command support > list. (IOW, that's what wpa_supplicant does.) We'll probably need to > imitate that. I believe checking command support is not really recommended. Instead, you better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero (since kernel 4.12 that is). Regards, Arend
On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote: > On 4/17/2018 2:28 AM, Brian Norris wrote: > > It looks like the status quo for looking for SCHED_SCAN support is to > > check if NL80211_CMD_START_SCHED_SCAN shows up in the command support > > list. (IOW, that's what wpa_supplicant does.) We'll probably need to > > imitate that. > > I believe checking command support is not really recommended. Instead, you > better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero (since kernel > 4.12 that is). Why not? Command support checking is what wpa_supplicant is doing. I noticed NL80211_ATTR_SCHED_SCAN_MAX_REQS, but unfortunately, we have to support older kernels. It looks like randomization was added in v3.19, and as you point out, that's only available in v4.12. I welcome other alternatives if you have them to offer. Brian
On 4/17/2018 6:07 PM, Brian Norris wrote: > On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote: >> On 4/17/2018 2:28 AM, Brian Norris wrote: >>> It looks like the status quo for looking for SCHED_SCAN support is to >>> check if NL80211_CMD_START_SCHED_SCAN shows up in the command support >>> list. (IOW, that's what wpa_supplicant does.) We'll probably need to >>> imitate that. >> >> I believe checking command support is not really recommended. Instead, you >> better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero (since kernel >> 4.12 that is). > > Why not? Command support checking is what wpa_supplicant is doing. That's not really a good argument. A couple (or more) years ago wpa_supplicant was not doing nl80211 but wext and some other using driver private ioctls, but that did not make it the best approach. The START_SCHED_SCAN command is indeed still provided to user-space: @@ -1376,7 +1377,7 @@ static int nl80211_add_commands_unsplit(struct cfg80211_r CMD(tdls_mgmt, TDLS_MGMT); CMD(tdls_oper, TDLS_OPER); } - if (rdev->wiphy.flags & WIPHY_FLAG_SUPPORTS_SCHED_SCAN) + if (rdev->wiphy.max_sched_scan_reqs) CMD(sched_scan_start, START_SCHED_SCAN); CMD(probe_client, PROBE_CLIENT); CMD(set_noack_map, SET_NOACK_MAP); It was left in because existing user-space apps might depend on it. > I noticed NL80211_ATTR_SCHED_SCAN_MAX_REQS, but unfortunately, we have > to support older kernels. It looks like randomization was added in > v3.19, and as you point out, that's only available in v4.12. I figured that would be the issue so I dug up the kernel version. > I welcome other alternatives if you have them to offer. Nope. Not if you want to be kernel version agnostic. Regards, Arend
On Tue, Apr 17, 2018 at 2:49 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > On 4/17/2018 6:07 PM, Brian Norris wrote: >> On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote: >>> I believe checking command support is not really recommended. Instead, you >>> better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero (since kernel >>> 4.12 that is). >> >> >> Why not? Command support checking is what wpa_supplicant is doing. > > > That's not really a good argument. A couple (or more) years ago wpa_supplicant was not doing nl80211 but wext and some other using driver private ioctls, but that did not make it the best approach. I see what you're saying (though your comparison doesn't seem that fair either; private ioctls are nothing like a well-defined nl80211 support list), and I'm totally good on looking at the new flag eventually. But you still haven't answered my question ("why not?"). Is there a problem with the "supported commands" list? > The START_SCHED_SCAN command is indeed still provided to user-space: And as I see it, it probably needs to be for essentially forever. Or at least a significant amount of time after wpa_supplicant stops relying on it. (Hint: it's still using it today, with no reference to NL80211_ATTR_SCHED_SCAN_MAX_REQS.) There's a reason the kernel has ABI guarantees. I suspect you only get a chance to rewrite the world (WEXT -> nl80211) a few times in the life of kernel ABIs. Brian
On Tue, 2018-04-17 at 15:26 -0700, Brian Norris wrote: > On Tue, Apr 17, 2018 at 2:49 PM, Arend van Spriel > <arend.vanspriel@broadcom.com> wrote: > > On 4/17/2018 6:07 PM, Brian Norris wrote: > > > On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote: > > > > I believe checking command support is not really recommended. > > > > Instead, you > > > > better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero > > > > (since kernel > > > > 4.12 that is). > > > > > > > > > Why not? Command support checking is what wpa_supplicant is > > > doing. > > > > > > That's not really a good argument. A couple (or more) years ago > > wpa_supplicant was not doing nl80211 but wext and some other using > > driver private ioctls, but that did not make it the best approach. > > I see what you're saying (though your comparison doesn't seem that > fair either; private ioctls are nothing like a well-defined nl80211 > support list), and I'm totally good on looking at the new flag > eventually. But you still haven't answered my question ("why not?"). > Is there a problem with the "supported commands" list? > > > The START_SCHED_SCAN command is indeed still provided to user- > > space: > > And as I see it, it probably needs to be for essentially forever. Or > at least a significant amount of time after wpa_supplicant stops > relying on it. (Hint: it's still using it today, with no reference to > NL80211_ATTR_SCHED_SCAN_MAX_REQS.) There's a reason the kernel has > ABI > guarantees. I suspect you only get a chance to rewrite the world > (WEXT > -> nl80211) a few times in the life of kernel ABIs. It sometimes feels like wpa_supplicant gets treated as a static entity that can never be changed. In fact, send a patch to Jouni implementing the best practice, with a fallback to preserve compat for old kernels, and I'm sure he'd entertain it. Just because the supplicant does something a certain way, doesn't mean it's the *best* way, but it too evolves. Dan
+ Johannes (to confirm/correct my understanding regarding "supported commands") On 4/18/2018 4:35 AM, Dan Williams wrote: > On Tue, 2018-04-17 at 15:26 -0700, Brian Norris wrote: >> On Tue, Apr 17, 2018 at 2:49 PM, Arend van Spriel >> <arend.vanspriel@broadcom.com> wrote: >>> On 4/17/2018 6:07 PM, Brian Norris wrote: >>>> On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote: >>>>> I believe checking command support is not really recommended. >>>>> Instead, you >>>>> better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero >>>>> (since kernel >>>>> 4.12 that is). >>>> >>>> >>>> Why not? Command support checking is what wpa_supplicant is >>>> doing. >>> >>> >>> That's not really a good argument. A couple (or more) years ago >>> wpa_supplicant was not doing nl80211 but wext and some other using >>> driver private ioctls, but that did not make it the best approach. >> >> I see what you're saying (though your comparison doesn't seem that >> fair either; private ioctls are nothing like a well-defined nl80211 >> support list), and I'm totally good on looking at the new flag >> eventually. But you still haven't answered my question ("why not?"). >> Is there a problem with the "supported commands" list? My understanding is that in general the "supported commands" list is not well-maintained. Not every nl80211 command is represented in the list so user-space can not know whether it is missing or not supported. For this particular START_SCHED_SCAN command it can be used still and indeed probably for a long time. I just wanted to point out that it is not recommended for new user-space functionality. >>> The START_SCHED_SCAN command is indeed still provided to user- >>> space: >> >> And as I see it, it probably needs to be for essentially forever. Or >> at least a significant amount of time after wpa_supplicant stops >> relying on it. (Hint: it's still using it today, with no reference to >> NL80211_ATTR_SCHED_SCAN_MAX_REQS.) There's a reason the kernel has >> ABI >> guarantees. I suspect you only get a chance to rewrite the world >> (WEXT >> -> nl80211) a few times in the life of kernel ABIs. > > It sometimes feels like wpa_supplicant gets treated as a static entity > that can never be changed. In fact, send a patch to Jouni implementing > the best practice, with a fallback to preserve compat for old kernels, > and I'm sure he'd entertain it. Just because the supplicant does > something a certain way, doesn't mean it's the *best* way, but it too > evolves. I was actually considering to do that. The netlink messages are easily checked for presence of an attribute so deciding on whether to use the fallback is trivial. Regards, Arend
> > > Is there a problem with the "supported commands" list? > > My understanding is that in general the "supported commands" list is not > well-maintained. Not every nl80211 command is represented in the list so > user-space can not know whether it is missing or not supported. > For this particular START_SCHED_SCAN command it can be used still and > indeed probably for a long time. I just wanted to point out that it is > not recommended for new user-space functionality. We used to do it more, but most features are more complex than "command is supported" or are simpler than "these 5 commands are supported" (just a single bit to indicate the whole feature is enough), so we mostly use extended feature flags now. > > It sometimes feels like wpa_supplicant gets treated as a static entity > > that can never be changed. In fact, send a patch to Jouni implementing > > the best practice, with a fallback to preserve compat for old kernels, > > and I'm sure he'd entertain it. Just because the supplicant does > > something a certain way, doesn't mean it's the *best* way, but it too > > evolves. You also need to consider old supplicant on new kernels though? But I didn't really follow this part of the discussion, so you're probably taking that into account. johannes
cjhuang@codeaurora.org writes: > On 2018-04-16 21:42, Kalle Valo wrote: >> Carl Huang <cjhuang@codeaurora.org> wrote: >> >>> The ath10k reports the random_mac_addr capability to upper layer >>> based on the service bit firmware reported. Driver sets the >>> spoofed flag in scan_ctrl_flag to firmware if upper layer has >>> enabled this feature in scan request. >>> >>> Test with QCA6174 hw3.0 and >>> firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1, >>> but QCA9377 is also affected. >>> >>> Signed-off-by: Carl Huang <cjhuang@codeaurora.org> >>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> >> >> This added a new checkpatch warning: >> >> drivers/net/wireless/ath/ath10k/wmi-ops.h:123: Alignment should match >> open parenthesis >> >> I fixed it in the pending branch. > > Kvalo, as the flag issue NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR > will be fixed from Brian side, I don't need to submit V2 for it. > > For this alignment warning and sparse warnings in [1/2], as you fixed > them in the pending branch, so I needn't do anything, right? Correct, I'm planning to apply this soon.
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index ebb3f1b..c5cd5e5 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -5699,6 +5699,12 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw, arg.scan_ctrl_flags |= WMI_SCAN_FLAG_PASSIVE; } + if (req->flags & NL80211_SCAN_FLAG_RANDOM_ADDR) { + arg.scan_ctrl_flags |= WMI_SCAN_ADD_SPOOFED_MAC_IN_PROBE_REQ; + ether_addr_copy(arg.mac_addr.addr, req->mac_addr); + ether_addr_copy(arg.mac_mask.addr, req->mac_addr_mask); + } + if (req->n_channels) { arg.n_channels = req->n_channels; for (i = 0; i < arg.n_channels; i++) @@ -8397,6 +8403,17 @@ int ath10k_mac_register(struct ath10k *ar) goto err_dfs_detector_exit; } + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) { + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr); + if (ret) { + ath10k_err(ar, "failed to set prob req oui: %i\n", ret); + goto err_dfs_detector_exit; + } + + ar->hw->wiphy->features |= + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR; + } + ar->hw->wiphy->cipher_suites = cipher_suites; /* QCA988x and QCA6174 family chips do not support CCMP-256, GCMP-128 diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h index 86d083d..cf17d91 100644 --- a/drivers/net/wireless/ath/ath10k/wmi-ops.h +++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h @@ -118,6 +118,8 @@ struct wmi_ops { u32 value); struct sk_buff *(*gen_scan_chan_list)(struct ath10k *ar, const struct wmi_scan_chan_list_arg *arg); + struct sk_buff *(*gen_scan_prob_req_oui)(struct ath10k *ar, + u32 prob_req_oui); struct sk_buff *(*gen_beacon_dma)(struct ath10k *ar, u32 vdev_id, const void *bcn, size_t bcn_len, u32 bcn_paddr, bool dtim_zero, @@ -892,6 +894,26 @@ ath10k_wmi_scan_chan_list(struct ath10k *ar, } static inline int +ath10k_wmi_scan_prob_req_oui(struct ath10k *ar, const u8 mac_addr[ETH_ALEN]) +{ + struct sk_buff *skb; + u32 prob_req_oui; + + prob_req_oui = (((u32)mac_addr[0]) << 16) | + (((u32)mac_addr[1]) << 8) | mac_addr[2]; + + if (!ar->wmi.ops->gen_scan_prob_req_oui) + return -EOPNOTSUPP; + + skb = ar->wmi.ops->gen_scan_prob_req_oui(ar, prob_req_oui); + if (IS_ERR(skb)) + return PTR_ERR(skb); + + return ath10k_wmi_cmd_send(ar, skb, + ar->wmi.cmd->scan_prob_req_oui_cmdid); +} + +static inline int ath10k_wmi_peer_assoc(struct ath10k *ar, const struct wmi_peer_assoc_complete_arg *arg) { diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c index cb0d130..20e3fd4 100644 --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c @@ -1573,6 +1573,8 @@ ath10k_wmi_tlv_op_gen_start_scan(struct ath10k *ar, cmd->num_bssids = __cpu_to_le32(arg->n_bssids); cmd->ie_len = __cpu_to_le32(arg->ie_len); cmd->num_probes = __cpu_to_le32(3); + ether_addr_copy(cmd->mac_addr.addr, arg->mac_addr.addr); + ether_addr_copy(cmd->mac_mask.addr, arg->mac_mask.addr); /* FIXME: There are some scan flag inconsistencies across firmwares, * e.g. WMI-TLV inverts the logic behind the following flag. @@ -2420,6 +2422,27 @@ ath10k_wmi_tlv_op_gen_scan_chan_list(struct ath10k *ar, } static struct sk_buff * +ath10k_wmi_tlv_op_gen_scan_prob_req_oui(struct ath10k *ar, u32 prob_req_oui) +{ + struct wmi_scan_prob_req_oui_cmd *cmd; + struct wmi_tlv *tlv; + struct sk_buff *skb; + + skb = ath10k_wmi_alloc_skb(ar, sizeof(*tlv) + sizeof(*cmd)); + if (!skb) + return ERR_PTR(-ENOMEM); + + tlv = (void *)skb->data; + tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_SCAN_PROB_REQ_OUI_CMD); + tlv->len = __cpu_to_le16(sizeof(*cmd)); + cmd = (void *)tlv->value; + cmd->prob_req_oui = __cpu_to_le32(prob_req_oui); + + ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv scan prob req oui\n"); + return skb; +} + +static struct sk_buff * ath10k_wmi_tlv_op_gen_beacon_dma(struct ath10k *ar, u32 vdev_id, const void *bcn, size_t bcn_len, u32 bcn_paddr, bool dtim_zero, @@ -3366,6 +3389,7 @@ static struct wmi_cmd_map wmi_tlv_cmd_map = { .stop_scan_cmdid = WMI_TLV_STOP_SCAN_CMDID, .scan_chan_list_cmdid = WMI_TLV_SCAN_CHAN_LIST_CMDID, .scan_sch_prio_tbl_cmdid = WMI_TLV_SCAN_SCH_PRIO_TBL_CMDID, + .scan_prob_req_oui_cmdid = WMI_TLV_SCAN_PROB_REQ_OUI_CMDID, .pdev_set_regdomain_cmdid = WMI_TLV_PDEV_SET_REGDOMAIN_CMDID, .pdev_set_channel_cmdid = WMI_TLV_PDEV_SET_CHANNEL_CMDID, .pdev_set_param_cmdid = WMI_TLV_PDEV_SET_PARAM_CMDID, @@ -3734,6 +3758,7 @@ static const struct wmi_ops wmi_tlv_ops = { .gen_set_sta_ps = ath10k_wmi_tlv_op_gen_set_sta_ps, .gen_set_ap_ps = ath10k_wmi_tlv_op_gen_set_ap_ps, .gen_scan_chan_list = ath10k_wmi_tlv_op_gen_scan_chan_list, + .gen_scan_prob_req_oui = ath10k_wmi_tlv_op_gen_scan_prob_req_oui, .gen_beacon_dma = ath10k_wmi_tlv_op_gen_beacon_dma, .gen_pdev_set_wmm = ath10k_wmi_tlv_op_gen_pdev_set_wmm, .gen_request_stats = ath10k_wmi_tlv_op_gen_request_stats, diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.h b/drivers/net/wireless/ath/ath10k/wmi-tlv.h index 2889225..e8ec1c3 100644 --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.h +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.h @@ -1695,6 +1695,15 @@ struct wmi_tlv_scan_chan_list_cmd { __le32 num_scan_chans; } __packed; +struct wmi_scan_prob_req_oui_cmd { +/* OUI to be used in Probe Request frame when random MAC address is + * requested part of scan parameters. This is applied to both FW internal + * scans and host initiated scans. Host can request for random MAC address + * with WMI_SCAN_ADD_SPOOFED_MAC_IN_PROBE_REQ flag. + */ + __le32 prob_req_oui; +} __packed; + struct wmi_tlv_start_scan_cmd { struct wmi_start_scan_common common; __le32 burst_duration_ms; @@ -1703,6 +1712,8 @@ struct wmi_tlv_start_scan_cmd { __le32 num_ssids; __le32 ie_len; __le32 num_probes; + struct wmi_mac_addr mac_addr; + struct wmi_mac_addr mac_mask; } __packed; struct wmi_tlv_vdev_start_cmd { diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 4aca166..04b5606 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -41,6 +41,7 @@ static struct wmi_cmd_map wmi_cmd_map = { .stop_scan_cmdid = WMI_STOP_SCAN_CMDID, .scan_chan_list_cmdid = WMI_SCAN_CHAN_LIST_CMDID, .scan_sch_prio_tbl_cmdid = WMI_SCAN_SCH_PRIO_TBL_CMDID, + .scan_prob_req_oui_cmdid = WMI_CMD_UNSUPPORTED, .pdev_set_regdomain_cmdid = WMI_PDEV_SET_REGDOMAIN_CMDID, .pdev_set_channel_cmdid = WMI_PDEV_SET_CHANNEL_CMDID, .pdev_set_param_cmdid = WMI_PDEV_SET_PARAM_CMDID, @@ -205,6 +206,7 @@ static struct wmi_cmd_map wmi_10x_cmd_map = { .stop_scan_cmdid = WMI_10X_STOP_SCAN_CMDID, .scan_chan_list_cmdid = WMI_10X_SCAN_CHAN_LIST_CMDID, .scan_sch_prio_tbl_cmdid = WMI_CMD_UNSUPPORTED, + .scan_prob_req_oui_cmdid = WMI_CMD_UNSUPPORTED, .pdev_set_regdomain_cmdid = WMI_10X_PDEV_SET_REGDOMAIN_CMDID, .pdev_set_channel_cmdid = WMI_10X_PDEV_SET_CHANNEL_CMDID, .pdev_set_param_cmdid = WMI_10X_PDEV_SET_PARAM_CMDID, @@ -371,6 +373,7 @@ static struct wmi_cmd_map wmi_10_2_4_cmd_map = { .stop_scan_cmdid = WMI_10_2_STOP_SCAN_CMDID, .scan_chan_list_cmdid = WMI_10_2_SCAN_CHAN_LIST_CMDID, .scan_sch_prio_tbl_cmdid = WMI_CMD_UNSUPPORTED, + .scan_prob_req_oui_cmdid = WMI_CMD_UNSUPPORTED, .pdev_set_regdomain_cmdid = WMI_10_2_PDEV_SET_REGDOMAIN_CMDID, .pdev_set_channel_cmdid = WMI_10_2_PDEV_SET_CHANNEL_CMDID, .pdev_set_param_cmdid = WMI_10_2_PDEV_SET_PARAM_CMDID, @@ -537,6 +540,7 @@ static struct wmi_cmd_map wmi_10_4_cmd_map = { .stop_scan_cmdid = WMI_10_4_STOP_SCAN_CMDID, .scan_chan_list_cmdid = WMI_10_4_SCAN_CHAN_LIST_CMDID, .scan_sch_prio_tbl_cmdid = WMI_10_4_SCAN_SCH_PRIO_TBL_CMDID, + .scan_prob_req_oui_cmdid = WMI_CMD_UNSUPPORTED, .pdev_set_regdomain_cmdid = WMI_10_4_PDEV_SET_REGDOMAIN_CMDID, .pdev_set_channel_cmdid = WMI_10_4_PDEV_SET_CHANNEL_CMDID, .pdev_set_param_cmdid = WMI_10_4_PDEV_SET_PARAM_CMDID, @@ -1334,6 +1338,7 @@ static struct wmi_cmd_map wmi_10_2_cmd_map = { .stop_scan_cmdid = WMI_10_2_STOP_SCAN_CMDID, .scan_chan_list_cmdid = WMI_10_2_SCAN_CHAN_LIST_CMDID, .scan_sch_prio_tbl_cmdid = WMI_CMD_UNSUPPORTED, + .scan_prob_req_oui_cmdid = WMI_CMD_UNSUPPORTED, .pdev_set_regdomain_cmdid = WMI_10_2_PDEV_SET_REGDOMAIN_CMDID, .pdev_set_channel_cmdid = WMI_10_2_PDEV_SET_CHANNEL_CMDID, .pdev_set_param_cmdid = WMI_10_2_PDEV_SET_PARAM_CMDID, diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h index 6919618..3dd8f76 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.h +++ b/drivers/net/wireless/ath/ath10k/wmi.h @@ -767,6 +767,7 @@ struct wmi_cmd_map { u32 stop_scan_cmdid; u32 scan_chan_list_cmdid; u32 scan_sch_prio_tbl_cmdid; + u32 scan_prob_req_oui_cmdid; u32 pdev_set_regdomain_cmdid; u32 pdev_set_channel_cmdid; u32 pdev_set_param_cmdid; @@ -3144,6 +3145,8 @@ struct wmi_start_scan_arg { u16 channels[64]; struct wmi_ssid_arg ssids[WLAN_SCAN_PARAMS_MAX_SSID]; struct wmi_bssid_arg bssids[WLAN_SCAN_PARAMS_MAX_BSSID]; + struct wmi_mac_addr mac_addr; + struct wmi_mac_addr mac_mask; }; /* scan control flags */ @@ -3167,6 +3170,12 @@ struct wmi_start_scan_arg { */ #define WMI_SCAN_CONTINUE_ON_ERROR 0x80 +/* Use random MAC address for TA for Probe Request frame and add + * OUI specified by WMI_SCAN_PROB_REQ_OUI_CMDID to the Probe Request frame. + * if OUI is not set by WMI_SCAN_PROB_REQ_OUI_CMDID then the flag is ignored. + */ +#define WMI_SCAN_ADD_SPOOFED_MAC_IN_PROBE_REQ 0x1000 + /* WMI_SCAN_CLASS_MASK must be the same value as IEEE80211_SCAN_CLASS_MASK */ #define WMI_SCAN_CLASS_MASK 0xFF000000
The ath10k reports the random_mac_addr capability to upper layer based on the service bit firmware reported. Driver sets the spoofed flag in scan_ctrl_flag to firmware if upper layer has enabled this feature in scan request. Test with QCA6174 hw3.0 and firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1, but QCA9377 is also affected. Signed-off-by: Carl Huang <cjhuang@codeaurora.org> --- drivers/net/wireless/ath/ath10k/mac.c | 17 +++++++++++++++++ drivers/net/wireless/ath/ath10k/wmi-ops.h | 22 ++++++++++++++++++++++ drivers/net/wireless/ath/ath10k/wmi-tlv.c | 25 +++++++++++++++++++++++++ drivers/net/wireless/ath/ath10k/wmi-tlv.h | 11 +++++++++++ drivers/net/wireless/ath/ath10k/wmi.c | 5 +++++ drivers/net/wireless/ath/ath10k/wmi.h | 9 +++++++++ 6 files changed, 89 insertions(+)