diff mbox

[2/2] ath10k: support MAC address randomization in scan

Message ID 1522379640-6442-3-git-send-email-cjhuang@codeaurora.org (mailing list archive)
State Accepted
Commit 60e1d0fb290197fe505dff6e4e3b7e4d258dbf60
Delegated to: Kalle Valo
Headers show

Commit Message

Carl Huang March 30, 2018, 3:14 a.m. UTC
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(+)

Comments

Brian Norris April 12, 2018, 8:59 p.m. UTC | #1
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
Carl Huang April 13, 2018, 6:53 a.m. UTC | #2
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
Kalle Valo April 13, 2018, 11:28 a.m. UTC | #3
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?
Arend van Spriel April 13, 2018, 9:13 p.m. UTC | #4
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
Carl Huang April 16, 2018, 5:16 a.m. UTC | #5
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
Carl Huang April 16, 2018, 5:17 a.m. UTC | #6
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
Kalle Valo April 16, 2018, 11:32 a.m. UTC | #7
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?
Kalle Valo April 16, 2018, 1:42 p.m. UTC | #8
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.
Brian Norris April 17, 2018, 12:28 a.m. UTC | #9
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
Carl Huang April 17, 2018, 7:29 a.m. UTC | #10
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
Arend van Spriel April 17, 2018, 8:22 a.m. UTC | #11
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
Brian Norris April 17, 2018, 4:07 p.m. UTC | #12
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
Arend van Spriel April 17, 2018, 9:49 p.m. UTC | #13
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
Brian Norris April 17, 2018, 10:26 p.m. UTC | #14
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
Dan Williams April 18, 2018, 2:35 a.m. UTC | #15
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
Arend van Spriel April 18, 2018, 8:29 a.m. UTC | #16
+ 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
Johannes Berg April 18, 2018, 9:58 a.m. UTC | #17
> > > 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
Kalle Valo April 20, 2018, 10:30 a.m. UTC | #18
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 mbox

Patch

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