Date:   Mon, 6 Mar 2023 18:37:14 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Jaewan Kim <jaewan@google.com>
Cc:     gregkh@linuxfoundation.org, johannes@sipsolutions.net,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        kernel-team@android.com, adelva@google.com
Subject: Re: [PATCH v8 4/5] mac80211_hwsim: add PMSR abort support via virtio
Message-ID: <ZAYkypRT+mIdQr/v@corigine.com>
References: <20230302160310.923349-1-jaewan@google.com>
 <20230302160310.923349-5-jaewan@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230302160310.923349-5-jaewan@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ut48Q/va7swmRzILxiZR/0nAQBRaV5v8llrBmkIZbCQo8nzJa4s0OxH3e8Gb?=
 =?us-ascii?Q?Vv6JfGVPYcibYB/KpnVSktIInqQ7DBGerBGfIYj6CI/SbHYHWqnDsqUQKRSv?=
 =?us-ascii?Q?+J99ZsCu0Y2bUxPrnmzOOtzZgeKGKqUv1xrk6Tota3X4l/IMzETALblPyKFr?=
 =?us-ascii?Q?y3HGRZZqyuMMCHoTwzkH01N4lhtM5rtjJq6zaw+Or8MDW5tq6i8IpsQa5rBN?=
 =?us-ascii?Q?n/+A9maNlOMvMY5wqeJY8sJjMZrJg2koCnoFDWzeP95EYuPczD2X0pOL/XO4?=
 =?us-ascii?Q?JBSpWdpA8aGBGqCDsdGWqCWer8F2fh4Of20JZOaTkVPONsSyPWlpxJRlxu2/?=
 =?us-ascii?Q?n80V9o7biurpkCzogSfE/NYhcxB1u1Ag1jpLyxes3DqCey3NiDKDr5W9MJ5y?=
 =?us-ascii?Q?jLp1T/7YuaWZasao+Mm8vZDIHfdUM6XwRe4a3C00XNgz3d/l7mkhVh0J6Km1?=
 =?us-ascii?Q?uV4ezxQ5YMVtS8IcSRq5ugD4Es4fzm+My6mz5fIYFrMYjRyUN8+BgmRXNg7S?=
 =?us-ascii?Q?1JeZ6vmD6sQdFS659cVr8PhFteTJF86kKWavBcaSLLZUkqs9FF/vAFkEsC17?=
 =?us-ascii?Q?4oOevgLOFUFSkfE+lPJTwZ0q7V667r2mnt0f7UEVV5wzAUIEn6knj7U88iLa?=
 =?us-ascii?Q?5Ob16MdV2S+0Sng0kp0YyUj1U02KqJkL9VMuazI2R0eCoPQffBG88RdzjuBe?=
 =?us-ascii?Q?D0Z7xQLbGRkuBPKcnEQwVhZnurWS/k6V6Wlv1WKnO5TjmSeV6YeO8Gdxawzr?=
 =?us-ascii?Q?VUn8A4H68anhqVs7ENvpyPo9MfsUirzzZii7TFJ5zQc2ogJS2oRTMWfrLC8h?=
 =?us-ascii?Q?Jn/rrNL0f/NOHexUSiCiMiyy2K0KP+80AyY+ZOGNfUjG3DgXq1k8uI9zJfoW?=
 =?us-ascii?Q?cHZUlOZqViuncHL3P8WCY7NXS5cokTfn71pEwIno21FOTzfAnO4qnRj1kqbE?=
 =?us-ascii?Q?8bBb6XQf/BAbvKhYf6NV9cPxnx0C2Lmp/OfIO4CMFnDbDHAILuOmEdTSNv0P?=
 =?us-ascii?Q?3niSggpykvRcAMm7Tv4gyXrmhgY+3FNHu+SlBUZL884SEkVH9//j0NU9G5H8?=
 =?us-ascii?Q?+ZNAlSi9vuLBquMuFS3UkyZaWyBUB8UrA2XtZ7g4MkBKUP7Rm7MY0ziA7t6f?=
 =?us-ascii?Q?Ue5A/wx8EL3Z37h5ZUcsFuQOV2pedyMAFFx2Nr41KPmV63Z9p84soC/Szrs/?=
 =?us-ascii?Q?rEiCzoKUtm4o8i9H2aUZWFO6tivyv6FrDq18WY6S5FyQFVTpDCunjvkZnfof?=
 =?us-ascii?Q?65FTZmkiBhElRJHQ2QWTPvRDvSuMiMKvVjSfuiFERyEN3M8iLgrguJJVxBHt?=
 =?us-ascii?Q?DIw3LRrI7K9gXRX5sAUpcQRgb7XaD63c49sFe4UZGfo4LpPFKkbZT210oxuc?=
 =?us-ascii?Q?vFdOR8PeKqnolps0z7aOV6Bklf2log0f/ndYrxLOvo1YDsvNaiAq5YT2PV1A?=
 =?us-ascii?Q?xRneTZ/IepR7T13tw9Z8grvajxwKW+koLrORsLJd8HXTa4H6kNDNYYEXY+6j?=
 =?us-ascii?Q?5AT5dGiRDKu4bCaY7twU20ok1YvmCSTuvfMHqmp7EUQtNYKD2IAiWqAV+8jk?=
 =?us-ascii?Q?Y2dYJaZzopi1wqMQVk03CKlqQtMh0pIDoOygRfqUHT+G4xk9neuBK9vTLDTJ?=
 =?us-ascii?Q?uitutxJVXcBkYNL4qjOtI9zJGYMy3P26ZfZsEJyBS2sfsaQn4OPB+WP+Jqp6?=
 =?us-ascii?Q?lxLAVQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 45d3cbf4-2747-48f1-9fc7-08db1e696ff4
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2023 17:37:20.3790
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YbqAVUM6pv4A3GlQlBClZ7GVqBgRZJSZ7lf9tGWUo+j+JHC3a2r9I9xKGQx2osr0zuZQ93u7IRLTIWHokD755PyTnCCO++VllXjWnS7CdFg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR13MB3699
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

On Thu, Mar 02, 2023 at 04:03:09PM +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.
> 
> Add necessary functionalities for mac80211_hwsim to abort previous PMSR
> request. The abortion request is sent to the wmedium where the PMSR request
> is actually handled.
> 
> In detail, add new mac80211_hwsim command HWSIM_CMD_ABORT_PMSR. When
> mac80211_hwsim receives the PMSR abortion request via
> ieee80211_ops.abort_pmsr, the received cfg80211_pmsr_request is resent to
> the wmediumd with command HWSIM_CMD_ABORT_PMSR and attribute
> HWSIM_ATTR_PMSR_REQUEST. The attribute is formatted as the same way as
> nl80211_pmsr_start() expects.
> 
> Signed-off-by: Jaewan Kim <jaewan@google.com>
> ---
> V7->V8: Rewrote commit msg
> V7: Initial commit (split from previously large patch)
> ---
>  drivers/net/wireless/mac80211_hwsim.c | 61 +++++++++++++++++++++++++++
>  drivers/net/wireless/mac80211_hwsim.h |  2 +
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index 691b83140d57..0d92a7e51057 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -3343,6 +3343,66 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw,
>  	return err;
>  }
>  
> +static void mac80211_hwsim_abort_pmsr(struct ieee80211_hw *hw,
> +				      struct ieee80211_vif *vif,
> +				      struct cfg80211_pmsr_request *request)
> +{
> +	struct mac80211_hwsim_data *data = hw->priv;
> +	u32 _portid = READ_ONCE(data->wmediumd);
> +	struct sk_buff *skb = NULL;
> +	int err = 0;
> +	void *msg_head;
> +	struct nlattr *pmsr;
> +
> +	if (!_portid && !hwsim_virtio_enabled)
> +		return;
> +
> +	mutex_lock(&data->mutex);
> +
> +	if (data->pmsr_request != request) {
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	if (err)
> +		return;

How can this occur?
And if it does, isn't the lock leaked?

> +
> +	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!skb)
> +		return;

I think the mutex needs to be unlocked here in this error path.

> +
> +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0, HWSIM_CMD_ABORT_PMSR);
> +
> +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER, ETH_ALEN, data->addresses[1].addr))

In the current scheme, I think err needs to be set here.

> +		goto out_err;
> +
> +	pmsr = nla_nest_start(skb, HWSIM_ATTR_PMSR_REQUEST);
> +	if (!pmsr) {
> +		err = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	err = mac80211_hwsim_send_pmsr_request(skb, request);
> +	if (err)

I think this error path needs to call nla_nest_cancel().

> +		goto out_err;
> +
> +	err = nla_nest_end(skb, pmsr);
> +	if (err)

I don't think is an error condition.

> +		goto out_err;
> +
> +	genlmsg_end(skb, msg_head);
> +	if (hwsim_virtio_enabled)
> +		hwsim_tx_virtio(data, skb);
> +	else
> +		hwsim_unicast_netgroup(data, skb, _portid);
> +
> +out_err:
> +	if (err && skb)
> +		nlmsg_free(skb);
> +
> +	mutex_unlock(&data->mutex);

I think it might be nicer to arrange this as:

	goto out_unlock;

err_nest_cancel:
	nla_nest_cancel(...);
err_free:
	nlmsg_free(skb);
out_unlock:
	mutex_unlock(&data->mutex);
}

> +}
> +
>  #define HWSIM_COMMON_OPS					\
>  	.tx = mac80211_hwsim_tx,				\
>  	.wake_tx_queue = ieee80211_handle_wake_tx_queue,	\
> @@ -3367,6 +3427,7 @@ static int mac80211_hwsim_start_pmsr(struct ieee80211_hw *hw,
>  	.get_et_stats = mac80211_hwsim_get_et_stats,		\
>  	.get_et_strings = mac80211_hwsim_get_et_strings,	\
>  	.start_pmsr = mac80211_hwsim_start_pmsr,		\
> +	.abort_pmsr = mac80211_hwsim_abort_pmsr,
>  
>  #define HWSIM_NON_MLO_OPS					\
>  	.sta_add = mac80211_hwsim_sta_add,			\

...