netdev - Re: [PATCH v7 4/4] mac80211_hwsim: add PMSR report support via virtio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1fb20d096fa4ddb2995d58ad30be560699f9eaf9.camel@sipsolutions.net>
Date:   Wed, 15 Feb 2023 19:13:13 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Jaewan Kim <jaewan@...gle.com>, gregkh@...uxfoundation.org,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Cc:     kernel-team@...roid.com, adelva@...gle.com
Subject: Re: [PATCH v7 4/4] mac80211_hwsim: add PMSR report support via
 virtio

On Tue, 2023-02-07 at 08:54 +0000, Jaewan Kim wrote:
> PMSR (a.k.a. peer measurement) is generalized measurement between two
> devices with Wi-Fi support. And currently FTM (a.k.a. fine time measurement
> or flight time measurement) is the one and only measurement.
> 
> This change allows mac80211_hwsim to report PMSR result. The PMSR result
[...]
> This change adds HWSIM_CMD_REPORT_PMSR, HWSIM_ATTR_PMSR_RESULT. When
[...]
> To help receive the PMSR result detail, this CL also adds nla policies to

Generally you don't really need to talk about "this change" (or "this
CL"), just say it:

   Add the necessarily functionality to allow hwsim to report PMSR
   results. [...]

   To do that, add new hwsim netlink attributes ..._PMSR and
   ..._PMSR_RESULT.
   
(and I'd just remove the language about policies, that's obvious anyway)

> +	if (tb[NL80211_PMSR_PEER_ATTR_RESP]) {
> +		ret = mac80211_hwsim_parse_pmsr_resp(tb[NL80211_PMSR_PEER_ATTR_RESP], result, info);

I didn't comment about long lines again, but this one stands out :)

> @@ -5758,7 +6118,7 @@ static struct genl_family hwsim_genl_family __ro_after_init = {
>  	.module = THIS_MODULE,
>  	.small_ops = hwsim_ops,
>  	.n_small_ops = ARRAY_SIZE(hwsim_ops),
> -	.resv_start_op = HWSIM_CMD_DEL_MAC_ADDR + 1,
> +	.resv_start_op = __HWSIM_CMD_MAX,

No. This was intentional that way. If this causes your new userspace
trouble, please adjust it. Clearly, it's new userspace implementation -
that's why the resv_start_op is set that way, to force stricter
validation on new ops that userspace cannot have implemented since they
didn't exist at the time we set this.

johannes