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

[PATCH wpan v2 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

Changes in v2:
* Fixes lines with upsteam commit hashes rather than local
  hashes. Everything else is exactly the same.

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 v2 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: ed3557c947e1 ("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 v2 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: ed3557c947e1 ("ieee802154: Add support for user scanning requests")
Fixes: 9bc114504b07 ("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 v2 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: ed3557c947e1 ("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 v2 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: 3accf4762734 ("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 v2 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)'

Fixes: 3accf4762734 ("mac802154: Handle basic beaconing")
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 v2 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: ed3557c947e1 ("ieee802154: Add support for user scanning requests")
Fixes: 9bc114504b07 ("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 v2 0/6] ieee802154: Scan/Beacon fixes

[Miquel Raynal]

Hello Jakub, Stefan, Alexander,

miquel.raynal@bootlin.com wrote on Tue, 14 Feb 2023 14:50:29 +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.

I know it's only been 3 working days since I sent this series but as we
are approaching the closing of net-next and Stefan's MR was paused
until these fixes arrived, I wanted to check whether these changes
might be satisfying enough, in particular Jakub, if you found the
answers you asked for.

I mainly want to avoid the "Stefan waits for Alexander who waits for
Jakub who waits for Stefan" dependency chain :)

Thanks a lot for all the feedback anyway!
Miquèl

> Changes in v2:
> * Fixes lines with upsteam commit hashes rather than local
>   hashes. Everything else is exactly the same.
> 
> 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(-)
> 

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

[Stefan Schmidt]

Hello Miquel.

On 17.02.23 10:10, Miquel Raynal wrote:
> Hello Jakub, Stefan, Alexander,
> 
> miquel.raynal@bootlin.com wrote on Tue, 14 Feb 2023 14:50:29 +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.
> 
> I know it's only been 3 working days since I sent this series but as we
> are approaching the closing of net-next and Stefan's MR was paused
> until these fixes arrived, I wanted to check whether these changes
> might be satisfying enough, in particular Jakub, if you found the
> answers you asked for.
> 
> I mainly want to avoid the "Stefan waits for Alexander who waits for
> Jakub who waits for Stefan" dependency chain :)

I just reviewed and tested them and have no problem to take them in. For 
patches 1 and 2 I would prefer an ack from Jakub to make sure we covered 
all of this review feedback before. Let's hope we can get these on 
Monday or Tuesday. Once we have them in I will re-spin a new pull 
request for all the changes.

regards
Stefan Schmidt

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

[Miquel Raynal]

Hi Stefan,

stefan@datenfreihafen.org wrote on Sat, 18 Feb 2023 18:20:22 +0100:

> Hello Miquel.
> 
> On 17.02.23 10:10, Miquel Raynal wrote:
> > Hello Jakub, Stefan, Alexander,
> > 
> > miquel.raynal@bootlin.com wrote on Tue, 14 Feb 2023 14:50:29 +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.  
> > 
> > I know it's only been 3 working days since I sent this series but as we
> > are approaching the closing of net-next and Stefan's MR was paused
> > until these fixes arrived, I wanted to check whether these changes
> > might be satisfying enough, in particular Jakub, if you found the
> > answers you asked for.
> > 
> > I mainly want to avoid the "Stefan waits for Alexander who waits for
> > Jakub who waits for Stefan" dependency chain :)  
> 
> I just reviewed and tested them and have no problem to take them in. For patches 1 and 2 I would prefer an ack from Jakub to make sure we covered all of this review feedback before. Let's hope we can get these on Monday or Tuesday. Once we have them in I will re-spin a new pull request for all the changes.

Thanks a lot!

Miquèl

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

[Jakub Kicinski]

On Sat, 18 Feb 2023 18:20:22 +0100 Stefan Schmidt wrote:
> I just reviewed and tested them and have no problem to take them in. For 
> patches 1 and 2 I would prefer an ack from Jakub to make sure we covered 
> all of this review feedback before. 

Sorry I was away, yes, patches 1 and 2 LGTM!

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

[Stefan Schmidt]

Hello.

On 14.02.23 14:50, Miquel Raynal wrote:
> 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
> 
> Changes in v2:
> * Fixes lines with upsteam commit hashes rather than local
>    hashes. Everything else is exactly the same.
> 
> 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(-)


These patches have been applied to the wpan-next tree and will be
part of the next pull request to net-next. Thanks!

regards
Stefan Schmidt

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

[Stefan Schmidt]

Hello Jakub.

On 20.02.23 18:59, Jakub Kicinski wrote:
> On Sat, 18 Feb 2023 18:20:22 +0100 Stefan Schmidt wrote:
>> I just reviewed and tested them and have no problem to take them in. For
>> patches 1 and 2 I would prefer an ack from Jakub to make sure we covered
>> all of this review feedback before.
> 
> Sorry I was away, yes, patches 1 and 2 LGTM!

Thanks. Patches are applied now and will come in an updated pull request 
later today or tomorrow.

regards
Stefan Schmidt

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

[Eric Dumazet]

On Tue, Feb 14, 2023 at 2:50 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> 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: ed3557c947e1 ("ieee802154: Add support for user scanning requests")
> Fixes: 9bc114504b07 ("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])) {

syzbot crashes hosts by _not_ adding NL802154_ATTR_SCAN_TYPE attribute.

Did you mean to write :

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 2215f576ee3788f74ea175b046d05d285bac752d..d8f4379d4fa68b5b07bb2c45cd74d4b73213c107
100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1412,7 +1412,7 @@ static int nl802154_trigger_scan(struct sk_buff
*skb, struct genl_info *info)
                return -EOPNOTSUPP;
        }

-       if (!nla_get_u8(info->attrs[NL802154_ATTR_SCAN_TYPE])) {
+       if (!info->attrs[NL802154_ATTR_SCAN_TYPE]) {
                NL_SET_ERR_MSG(info->extack, "Malformed request,
missing scan type");
                return -EINVAL;
        }


> +               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
>

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

[Miquel Raynal]

Hi Eric,

edumazet@google.com wrote on Mon, 27 Feb 2023 21:41:51 +0100:

> On Tue, Feb 14, 2023 at 2:50 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > 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: ed3557c947e1 ("ieee802154: Add support for user scanning requests")
> > Fixes: 9bc114504b07 ("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])) {
> 
> syzbot crashes hosts by _not_ adding NL802154_ATTR_SCAN_TYPE attribute.

I was looking at Sanan Hasanov's e-mail, I believe this is the same
breakage that is being reported?
Link: https://lore.kernel.org/netdev/IA1PR07MB98302CDCC21F6BA664FB7298ABAF9@IA1PR07MB9830.namprd07.prod.outlook.com/

> Did you mean to write :
> 
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index 2215f576ee3788f74ea175b046d05d285bac752d..d8f4379d4fa68b5b07bb2c45cd74d4b73213c107
> 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -1412,7 +1412,7 @@ static int nl802154_trigger_scan(struct sk_buff
> *skb, struct genl_info *info)
>                 return -EOPNOTSUPP;
>         }
> 
> -       if (!nla_get_u8(info->attrs[NL802154_ATTR_SCAN_TYPE])) {
> +       if (!info->attrs[NL802154_ATTR_SCAN_TYPE]) {

That is absolutely what I intended to do, I will send a fix immediately.

>                 NL_SET_ERR_MSG(info->extack, "Malformed request,
> missing scan type");
>                 return -EINVAL;
>         }
> 

Thanks a lot,
Miquèl