[PATCH wpan 0/6] ieee802154: Scan/Beacon fixes

[PATCH wpan 0/6] ieee802154: Scan/Beacon fixes

[Miquel Raynal]

Hello,

Following Jakub's review on Stefan's MR, a number of changes were
requested for him in order to pull the patches in net. In the mean time,
a couple of discussions happened with Alexander (return codes for
monitor scans and transmit helper used for beacons).

Hopefully this series addresses everything.

Thanks,
Miquèl

Miquel Raynal (6):
  ieee802154: Use netlink policies when relevant on scan parameters
  ieee802154: Convert scan error messages to extack
  ieee802154: Change error code on monitor scan netlink request
  mac802154: Send beacons using the MLME Tx path
  mac802154: Fix an always true condition
  ieee802154: Drop device trackers

 net/ieee802154/nl802154.c | 125 ++++++++++++++------------------------
 net/mac802154/scan.c      |  25 ++++++--
 2 files changed, 65 insertions(+), 85 deletions(-)

-- 
2.34.1

[PATCH wpan 1/6] ieee802154: Use netlink policies when relevant on scan parameters

[Miquel Raynal]

Instead of open-coding scan parameters (page, channels, duration, etc),
let's use the existing NLA_POLICY* macros. This help greatly reducing
the error handling and clarifying the overall logic.

Fixes: 45755ce4bf46 ("ieee802154: Add support for user scanning requests")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/nl802154.c | 84 +++++++++++++--------------------------
 1 file changed, 28 insertions(+), 56 deletions(-)

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 0d9becd678e3..64fa811e1f0b 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -187,8 +187,8 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
 
 	[NL802154_ATTR_WPAN_DEV] = { .type = NLA_U64 },
 
-	[NL802154_ATTR_PAGE] = { .type = NLA_U8, },
-	[NL802154_ATTR_CHANNEL] = { .type = NLA_U8, },
+	[NL802154_ATTR_PAGE] = NLA_POLICY_MAX(NLA_U8, IEEE802154_MAX_PAGE),
+	[NL802154_ATTR_CHANNEL] = NLA_POLICY_MAX(NLA_U8, IEEE802154_MAX_CHANNEL),
 
 	[NL802154_ATTR_TX_POWER] = { .type = NLA_S32, },
 
@@ -221,13 +221,19 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
 
 	[NL802154_ATTR_COORDINATOR] = { .type = NLA_NESTED },
 
-	[NL802154_ATTR_SCAN_TYPE] = { .type = NLA_U8 },
-	[NL802154_ATTR_SCAN_CHANNELS] = { .type = NLA_U32 },
-	[NL802154_ATTR_SCAN_PREAMBLE_CODES] = { .type = NLA_U64 },
-	[NL802154_ATTR_SCAN_MEAN_PRF] = { .type = NLA_U8 },
-	[NL802154_ATTR_SCAN_DURATION] = { .type = NLA_U8 },
-	[NL802154_ATTR_SCAN_DONE_REASON] = { .type = NLA_U8 },
-	[NL802154_ATTR_BEACON_INTERVAL] = { .type = NLA_U8 },
+	[NL802154_ATTR_SCAN_TYPE] =
+		NLA_POLICY_RANGE(NLA_U8, NL802154_SCAN_ED, NL802154_SCAN_RIT_PASSIVE),
+	[NL802154_ATTR_SCAN_CHANNELS] =
+		NLA_POLICY_MASK(NLA_U32, GENMASK(IEEE802154_MAX_CHANNEL, 0)),
+	[NL802154_ATTR_SCAN_PREAMBLE_CODES] = { .type = NLA_REJECT },
+	[NL802154_ATTR_SCAN_MEAN_PRF] = { .type = NLA_REJECT },
+	[NL802154_ATTR_SCAN_DURATION] =
+		NLA_POLICY_MAX(NLA_U8, IEEE802154_MAX_SCAN_DURATION),
+	[NL802154_ATTR_SCAN_DONE_REASON] =
+		NLA_POLICY_RANGE(NLA_U8, NL802154_SCAN_DONE_REASON_FINISHED,
+				 NL802154_SCAN_DONE_REASON_ABORTED),
+	[NL802154_ATTR_BEACON_INTERVAL] =
+		NLA_POLICY_MAX(NLA_U8, IEEE802154_MAX_SCAN_DURATION),
 
 #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL
 	[NL802154_ATTR_SEC_ENABLED] = { .type = NLA_U8, },
@@ -1423,51 +1429,23 @@ static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 		goto free_request;
 	}
 
-	if (info->attrs[NL802154_ATTR_PAGE]) {
+	/* Use current page by default */
+	if (info->attrs[NL802154_ATTR_PAGE])
 		request->page = nla_get_u8(info->attrs[NL802154_ATTR_PAGE]);
-		if (request->page > IEEE802154_MAX_PAGE) {
-			pr_err("Invalid page %d > %d\n",
-			       request->page, IEEE802154_MAX_PAGE);
-			err = -EINVAL;
-			goto free_request;
-		}
-	} else {
-		/* Use current page by default */
+	else
 		request->page = wpan_phy->current_page;
-	}
 
-	if (info->attrs[NL802154_ATTR_SCAN_CHANNELS]) {
+	/* Scan all supported channels by default */
+	if (info->attrs[NL802154_ATTR_SCAN_CHANNELS])
 		request->channels = nla_get_u32(info->attrs[NL802154_ATTR_SCAN_CHANNELS]);
-		if (request->channels >= BIT(IEEE802154_MAX_CHANNEL + 1)) {
-			pr_err("Invalid channels bitfield %x ≥ %lx\n",
-			       request->channels,
-			       BIT(IEEE802154_MAX_CHANNEL + 1));
-			err = -EINVAL;
-			goto free_request;
-		}
-	} else {
-		/* Scan all supported channels by default */
+	else
 		request->channels = wpan_phy->supported.channels[request->page];
-	}
 
-	if (info->attrs[NL802154_ATTR_SCAN_PREAMBLE_CODES] ||
-	    info->attrs[NL802154_ATTR_SCAN_MEAN_PRF]) {
-		pr_err("Preamble codes and mean PRF not supported yet\n");
-		err = -EINVAL;
-		goto free_request;
-	}
-
-	if (info->attrs[NL802154_ATTR_SCAN_DURATION]) {
+	/* Use maximum duration order by default */
+	if (info->attrs[NL802154_ATTR_SCAN_DURATION])
 		request->duration = nla_get_u8(info->attrs[NL802154_ATTR_SCAN_DURATION]);
-		if (request->duration > IEEE802154_MAX_SCAN_DURATION) {
-			pr_err("Duration is out of range\n");
-			err = -EINVAL;
-			goto free_request;
-		}
-	} else {
-		/* Use maximum duration order by default */
+	else
 		request->duration = IEEE802154_MAX_SCAN_DURATION;
-	}
 
 	if (wpan_dev->netdev)
 		dev_hold(wpan_dev->netdev);
@@ -1614,17 +1592,11 @@ nl802154_send_beacons(struct sk_buff *skb, struct genl_info *info)
 	request->wpan_dev = wpan_dev;
 	request->wpan_phy = wpan_phy;
 
-	if (info->attrs[NL802154_ATTR_BEACON_INTERVAL]) {
+	/* Use maximum duration order by default */
+	if (info->attrs[NL802154_ATTR_BEACON_INTERVAL])
 		request->interval = nla_get_u8(info->attrs[NL802154_ATTR_BEACON_INTERVAL]);
-		if (request->interval > IEEE802154_MAX_SCAN_DURATION) {
-			pr_err("Interval is out of range\n");
-			err = -EINVAL;
-			goto free_request;
-		}
-	} else {
-		/* Use maximum duration order by default */
+	else
 		request->interval = IEEE802154_MAX_SCAN_DURATION;
-	}
 
 	if (wpan_dev->netdev)
 		dev_hold(wpan_dev->netdev);
@@ -1640,7 +1612,7 @@ nl802154_send_beacons(struct sk_buff *skb, struct genl_info *info)
 free_device:
 	if (wpan_dev->netdev)
 		dev_put(wpan_dev->netdev);
-free_request:
+
 	kfree(request);
 
 	return err;
-- 
2.34.1

[PATCH wpan 2/6] ieee802154: Convert scan error messages to extack

[Miquel Raynal]

Instead of printing error messages in the kernel log, let's use extack.
When there is a netlink error returned that could be further specified
with a string, use extack as well.

Apply this logic to the very recent scan/beacon infrastructure.

Fixes: 45755ce4bf46 ("ieee802154: Add support for user scanning requests")
Fixes: 7ed3b259eca1 ("ieee802154: Add support for user beaconing requests")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/nl802154.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 64fa811e1f0b..d3b6e9e80941 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1407,9 +1407,15 @@ static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 	u8 type;
 	int err;
 
-	/* Monitors are not allowed to perform scans */
-	if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
+	if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR) {
+		NL_SET_ERR_MSG(info->extack, "Monitors are not allowed to perform scans");
 		return -EPERM;
+	}
+
+	if (!nla_get_u8(info->attrs[NL802154_ATTR_SCAN_TYPE])) {
+		NL_SET_ERR_MSG(info->extack, "Malformed request, missing scan type");
+		return -EINVAL;
+	}
 
 	request = kzalloc(sizeof(*request), GFP_KERNEL);
 	if (!request)
@@ -1424,7 +1430,7 @@ static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 		request->type = type;
 		break;
 	default:
-		pr_err("Unsupported scan type: %d\n", type);
+		NL_SET_ERR_MSG_FMT(info->extack, "Unsupported scan type: %d", type);
 		err = -EINVAL;
 		goto free_request;
 	}
@@ -1576,12 +1582,13 @@ nl802154_send_beacons(struct sk_buff *skb, struct genl_info *info)
 	struct cfg802154_beacon_request *request;
 	int err;
 
-	/* Only coordinators can send beacons */
-	if (wpan_dev->iftype != NL802154_IFTYPE_COORD)
+	if (wpan_dev->iftype != NL802154_IFTYPE_COORD) {
+		NL_SET_ERR_MSG(info->extack, "Only coordinators can send beacons");
 		return -EOPNOTSUPP;
+	}
 
 	if (wpan_dev->pan_id == cpu_to_le16(IEEE802154_PANID_BROADCAST)) {
-		pr_err("Device is not part of any PAN\n");
+		NL_SET_ERR_MSG(info->extack, "Device is not part of any PAN");
 		return -EPERM;
 	}
 
-- 
2.34.1

[PATCH wpan 3/6] ieee802154: Change error code on monitor scan netlink request

[Miquel Raynal]

Returning EPERM gives the impression that "right now" it is not
possible, but "later" it could be, while what we want to express is the
fact that this is not currently supported at all (might change in the
future). So let's return EOPNOTSUPP instead.

Fixes: 45755ce4bf46 ("ieee802154: Add support for user scanning requests")
Suggested-by: Alexander Aring <aahringo@redhat.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/nl802154.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index d3b6e9e80941..8ee7d2ef55ee 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1409,7 +1409,7 @@ static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 
 	if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR) {
 		NL_SET_ERR_MSG(info->extack, "Monitors are not allowed to perform scans");
-		return -EPERM;
+		return -EOPNOTSUPP;
 	}
 
 	if (!nla_get_u8(info->attrs[NL802154_ATTR_SCAN_TYPE])) {
-- 
2.34.1

[PATCH wpan 4/6] mac802154: Send beacons using the MLME Tx path

[Miquel Raynal]

Using ieee802154_subif_start_xmit() to bypass the net queue when
sending beacons is broken because it does not acquire the
HARD_TX_LOCK(), hence not preventing datagram buffers to be smashed by
beacons upon contention situation. Using the mlme_tx helper is not the
best fit either but at least it is not buggy and has little-to-no
performance hit. More details are given in the comment explaining this
choice in the code.

Fixes: 18fa625292e9 ("mac802154: Handle basic beaconing")
Reported-by: Alexander Aring <aahringo@redhat.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/scan.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index 8f98efec7753..fff41e59099e 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -326,7 +326,25 @@ static int mac802154_transmit_beacon(struct ieee802154_local *local,
 		return ret;
 	}
 
-	return ieee802154_subif_start_xmit(skb, sdata->dev);
+	/* Using the MLME transmission helper for sending beacons is a bit
+	 * overkill because we do not really care about the final outcome.
+	 *
+	 * Even though, going through the whole net stack with a regular
+	 * dev_queue_xmit() is not relevant either because we want beacons to be
+	 * sent "now" rather than go through the whole net stack scheduling
+	 * (qdisc & co).
+	 *
+	 * Finally, using ieee802154_subif_start_xmit() would only be an option
+	 * if we had a generic transmit helper which would acquire the
+	 * HARD_TX_LOCK() to prevent buffer handling conflicts with regular
+	 * packets.
+	 *
+	 * So for now we keep it simple and send beacons with our MLME helper,
+	 * even if it stops the ieee802154 queue entirely during these
+	 * transmissions, wich anyway does not have a huge impact on the
+	 * performances given the current design of the stack.
+	 */
+	return ieee802154_mlme_tx(local, sdata, skb);
 }
 
 void mac802154_beacon_worker(struct work_struct *work)
-- 
2.34.1

[PATCH wpan 5/6] mac802154: Fix an always true condition

[Miquel Raynal]

At this stage we simply do not care about the delayed work value,
because active scan is not yet supported, so we can blindly queue
another work once a beacon has been sent.

It fixes a smatch warning:
    mac802154_beacon_worker() warn: always true condition
    '(local->beacon_interval >= 0) => (0-u32max >= 0)'

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/scan.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index fff41e59099e..9b0933a185eb 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -383,9 +383,8 @@ void mac802154_beacon_worker(struct work_struct *work)
 		dev_err(&sdata->dev->dev,
 			"Beacon could not be transmitted (%d)\n", ret);
 
-	if (local->beacon_interval >= 0)
-		queue_delayed_work(local->mac_wq, &local->beacon_work,
-				   local->beacon_interval);
+	queue_delayed_work(local->mac_wq, &local->beacon_work,
+			   local->beacon_interval);
 }
 
 int mac802154_stop_beacons_locked(struct ieee802154_local *local,
-- 
2.34.1

[PATCH wpan 6/6] ieee802154: Drop device trackers

[Miquel Raynal]

In order to prevent a device from disappearing when a background job was
started, dev_hold() and dev_put() calls were made. During the
stabilization phase of the scan/beacon features, it was later decided
that removing the device while a background job was ongoing was a valid use
case, and we should instead stop the background job and then remove the
device, rather than prevent the device from being removed. This is what
is currently done, which means manually reference counting the device
during background jobs is no longer needed.

Fixes: 45755ce4bf46 ("ieee802154: Add support for user scanning requests")
Fixes: 7ed3b259eca1 ("ieee802154: Add support for user beaconing requests")
Reported-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/nl802154.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 8ee7d2ef55ee..88380606af2c 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1453,20 +1453,14 @@ static int nl802154_trigger_scan(struct sk_buff *skb, struct genl_info *info)
 	else
 		request->duration = IEEE802154_MAX_SCAN_DURATION;
 
-	if (wpan_dev->netdev)
-		dev_hold(wpan_dev->netdev);
-
 	err = rdev_trigger_scan(rdev, request);
 	if (err) {
 		pr_err("Failure starting scanning (%d)\n", err);
-		goto free_device;
+		goto free_request;
 	}
 
 	return 0;
 
-free_device:
-	if (wpan_dev->netdev)
-		dev_put(wpan_dev->netdev);
 free_request:
 	kfree(request);
 
@@ -1555,9 +1549,6 @@ int nl802154_scan_done(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
 	if (err == -ESRCH)
 		err = 0;
 
-	if (wpan_dev->netdev)
-		dev_put(wpan_dev->netdev);
-
 	return err;
 }
 EXPORT_SYMBOL_GPL(nl802154_scan_done);
@@ -1605,21 +1596,15 @@ nl802154_send_beacons(struct sk_buff *skb, struct genl_info *info)
 	else
 		request->interval = IEEE802154_MAX_SCAN_DURATION;
 
-	if (wpan_dev->netdev)
-		dev_hold(wpan_dev->netdev);
-
 	err = rdev_send_beacons(rdev, request);
 	if (err) {
 		pr_err("Failure starting sending beacons (%d)\n", err);
-		goto free_device;
+		goto free_request;
 	}
 
 	return 0;
 
-free_device:
-	if (wpan_dev->netdev)
-		dev_put(wpan_dev->netdev);
-
+free_request:
 	kfree(request);
 
 	return err;
@@ -1627,8 +1612,7 @@ nl802154_send_beacons(struct sk_buff *skb, struct genl_info *info)
 
 void nl802154_beaconing_done(struct wpan_dev *wpan_dev)
 {
-	if (wpan_dev->netdev)
-		dev_put(wpan_dev->netdev);
+	/* NOP */
 }
 EXPORT_SYMBOL_GPL(nl802154_beaconing_done);
 
-- 
2.34.1

Re: [PATCH wpan 0/6] ieee802154: Scan/Beacon fixes

[Miquel Raynal]

Hi all,

miquel.raynal@bootlin.com wrote on Mon, 13 Feb 2023 17:54:08 +0100:

> Hello,
> 
> Following Jakub's review on Stefan's MR, a number of changes were
> requested for him in order to pull the patches in net. In the mean time,
> a couple of discussions happened with Alexander (return codes for
> monitor scans and transmit helper used for beacons).
> 
> Hopefully this series addresses everything.

Looks like my 'Fixes' commit hash lookups lead to local tree hits rather
than matching the ones in the wpan-next tree, so these hashes won't
point at any valid commit once in the upstream trees. I am sending a v2
with nothing else than these commit hashes fixed.

Sorry for the noise.

Thanks,
Miquèl

> 
> Thanks,
> Miquèl
> 
> Miquel Raynal (6):
>   ieee802154: Use netlink policies when relevant on scan parameters
>   ieee802154: Convert scan error messages to extack
>   ieee802154: Change error code on monitor scan netlink request
>   mac802154: Send beacons using the MLME Tx path
>   mac802154: Fix an always true condition
>   ieee802154: Drop device trackers
> 
>  net/ieee802154/nl802154.c | 125 ++++++++++++++------------------------
>  net/mac802154/scan.c      |  25 ++++++--
>  2 files changed, 65 insertions(+), 85 deletions(-)
>