[PATCH wpan-next 0/5] ieee802154: Association tweaks

[PATCH wpan-next 0/5] ieee802154: Association tweaks

[Miquel Raynal]

Hello,

Last serie on this area to clarify a few points and avoid to confuse the
network too much. I believe more will come while using this stack, but
that's a first round I kept aside. Nothing particularly problematic
here, just a few clarifications.

Thanks,
Miquèl

Miquel Raynal (5):
  mac80254: Provide real PAN coordinator info in beacons
  mac802154: Use the PAN coordinator parameter when stamping packets
  mac802154: Only allow PAN controllers to process association requests
  ieee802154: Avoid confusing changes after associating
  mac802154: Avoid new associations while disassociating

 include/net/cfg802154.h   |  4 +++-
 net/ieee802154/nl802154.c | 30 ++++++++++++++++++------------
 net/ieee802154/pan.c      |  8 +++++++-
 net/mac802154/cfg.c       | 11 ++++++++---
 net/mac802154/rx.c        | 11 +++++++----
 net/mac802154/scan.c      | 10 ++++++++--
 6 files changed, 51 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH wpan-next 1/5] mac80254: Provide real PAN coordinator info in beacons

[Miquel Raynal]

Sending a beacon is a way to advertise a PAN, but also ourselves as
coordinator in the PAN. There is only one PAN coordinator in a PAN, this
is the device without parent (not associated itself to an "upper"
coordinator). Instead of blindly saying that we are the PAN coordinator,
let's actually use our internal information to fill this field.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index 7597072aed57..5873da634fb4 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -466,6 +466,7 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
 				  struct cfg802154_beacon_request *request)
 {
 	struct ieee802154_local *local = sdata->local;
+	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
 
 	ASSERT_RTNL();
 
@@ -495,8 +496,7 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
 		local->beacon.mac_pl.superframe_order = request->interval;
 	local->beacon.mac_pl.final_cap_slot = 0xf;
 	local->beacon.mac_pl.battery_life_ext = 0;
-	/* TODO: Fill this field with the coordinator situation in the network */
-	local->beacon.mac_pl.pan_coordinator = 1;
+	local->beacon.mac_pl.pan_coordinator = !wpan_dev->parent;
 	local->beacon.mac_pl.assoc_permit = 1;
 
 	if (request->interval == IEEE802154_ACTIVE_SCAN_DURATION)
-- 
2.34.1

Re: [PATCH wpan-next 1/5] mac80254: Provide real PAN coordinator info in beacons

[Stefan Schmidt]

Hello.

On 28.11.23 12:16, Miquel Raynal wrote:
> Sending a beacon is a way to advertise a PAN, but also ourselves as
> coordinator in the PAN. There is only one PAN coordinator in a PAN, this
> is the device without parent (not associated itself to an "upper"
> coordinator). Instead of blindly saying that we are the PAN coordinator,
> let's actually use our internal information to fill this field.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/mac802154/scan.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> index 7597072aed57..5873da634fb4 100644
> --- a/net/mac802154/scan.c
> +++ b/net/mac802154/scan.c
> @@ -466,6 +466,7 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
>   				  struct cfg802154_beacon_request *request)
>   {
>   	struct ieee802154_local *local = sdata->local;
> +	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
>   
>   	ASSERT_RTNL();
>   
> @@ -495,8 +496,7 @@ int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
>   		local->beacon.mac_pl.superframe_order = request->interval;
>   	local->beacon.mac_pl.final_cap_slot = 0xf;
>   	local->beacon.mac_pl.battery_life_ext = 0;
> -	/* TODO: Fill this field with the coordinator situation in the network */
> -	local->beacon.mac_pl.pan_coordinator = 1;
> +	local->beacon.mac_pl.pan_coordinator = !wpan_dev->parent;
>   	local->beacon.mac_pl.assoc_permit = 1;
>   
>   	if (request->interval == IEEE802154_ACTIVE_SCAN_DURATION)

Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH wpan-next 1/5] mac80254: Provide real PAN coordinator info in beacons

[Alexander Aring]

Hi,

On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Sending a beacon is a way to advertise a PAN, but also ourselves as
> coordinator in the PAN. There is only one PAN coordinator in a PAN, this
> is the device without parent (not associated itself to an "upper"
> coordinator). Instead of blindly saying that we are the PAN coordinator,
> let's actually use our internal information to fill this field.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Alexander Aring <aahringo@redhat.com>

- Alex

Re: [PATCH wpan-next 1/5] mac80254: Provide real PAN coordinator info in beacons

[Miquel Raynal]

On Tue, 2023-11-28 at 11:16:51 UTC, Miquel Raynal wrote:
> Sending a beacon is a way to advertise a PAN, but also ourselves as
> coordinator in the PAN. There is only one PAN coordinator in a PAN, this
> is the device without parent (not associated itself to an "upper"
> coordinator). Instead of blindly saying that we are the PAN coordinator,
> let's actually use our internal information to fill this field.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> Acked-by: Alexander Aring <aahringo@redhat.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git master.

Miquel

[PATCH wpan-next 2/5] mac802154: Use the PAN coordinator parameter when stamping packets

[Miquel Raynal]

ACKs come with the source and destination address empty, this has been
clarified already. But there is something else: if the destination
address is empty but the source address is valid, it may be a way to
reach the PAN coordinator. Either the device receiving this frame is the
PAN coordinator itself and should process what it just received
(PACKET_HOST) or it is not and may, if supported, relay the packet as it
is targeted to another device in the network.

Right now we do not support relaying so the packet should be dropped in
the first place, but the stamping looks more accurate this way.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/rx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 0024341ef9c5..e40a988d6c80 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -156,12 +156,15 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 
 	switch (mac_cb(skb)->dest.mode) {
 	case IEEE802154_ADDR_NONE:
-		if (hdr->source.mode != IEEE802154_ADDR_NONE)
-			/* FIXME: check if we are PAN coordinator */
-			skb->pkt_type = PACKET_OTHERHOST;
-		else
+		if (hdr->source.mode == IEEE802154_ADDR_NONE)
 			/* ACK comes with both addresses empty */
 			skb->pkt_type = PACKET_HOST;
+		else if (!wpan_dev->parent)
+			/* No dest means PAN coordinator is the recipient */
+			skb->pkt_type = PACKET_HOST;
+		else
+			/* We are not the PAN coordinator, just relaying */
+			skb->pkt_type = PACKET_OTHERHOST;
 		break;
 	case IEEE802154_ADDR_LONG:
 		if (mac_cb(skb)->dest.pan_id != span &&
-- 
2.34.1

Re: [PATCH wpan-next 2/5] mac802154: Use the PAN coordinator parameter when stamping packets

[Stefan Schmidt]

Hello.

On 28.11.23 12:16, Miquel Raynal wrote:
> ACKs come with the source and destination address empty, this has been
> clarified already. But there is something else: if the destination
> address is empty but the source address is valid, it may be a way to
> reach the PAN coordinator. Either the device receiving this frame is the
> PAN coordinator itself and should process what it just received
> (PACKET_HOST) or it is not and may, if supported, relay the packet as it
> is targeted to another device in the network.
> 
> Right now we do not support relaying so the packet should be dropped in
> the first place, but the stamping looks more accurate this way.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/mac802154/rx.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 0024341ef9c5..e40a988d6c80 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -156,12 +156,15 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>   
>   	switch (mac_cb(skb)->dest.mode) {
>   	case IEEE802154_ADDR_NONE:
> -		if (hdr->source.mode != IEEE802154_ADDR_NONE)
> -			/* FIXME: check if we are PAN coordinator */
> -			skb->pkt_type = PACKET_OTHERHOST;
> -		else
> +		if (hdr->source.mode == IEEE802154_ADDR_NONE)
>   			/* ACK comes with both addresses empty */
>   			skb->pkt_type = PACKET_HOST;
> +		else if (!wpan_dev->parent)
> +			/* No dest means PAN coordinator is the recipient */
> +			skb->pkt_type = PACKET_HOST;
> +		else
> +			/* We are not the PAN coordinator, just relaying */
> +			skb->pkt_type = PACKET_OTHERHOST;
>   		break;
>   	case IEEE802154_ADDR_LONG:
>   		if (mac_cb(skb)->dest.pan_id != span &&

Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH wpan-next 2/5] mac802154: Use the PAN coordinator parameter when stamping packets

[Alexander Aring]

Hi,

On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> ACKs come with the source and destination address empty, this has been
> clarified already. But there is something else: if the destination
> address is empty but the source address is valid, it may be a way to
> reach the PAN coordinator. Either the device receiving this frame is the
> PAN coordinator itself and should process what it just received
> (PACKET_HOST) or it is not and may, if supported, relay the packet as it
> is targeted to another device in the network.
>
> Right now we do not support relaying so the packet should be dropped in
> the first place, but the stamping looks more accurate this way.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/mac802154/rx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 0024341ef9c5..e40a988d6c80 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -156,12 +156,15 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>
>         switch (mac_cb(skb)->dest.mode) {
>         case IEEE802154_ADDR_NONE:
> -               if (hdr->source.mode != IEEE802154_ADDR_NONE)
> -                       /* FIXME: check if we are PAN coordinator */
> -                       skb->pkt_type = PACKET_OTHERHOST;
> -               else
> +               if (hdr->source.mode == IEEE802154_ADDR_NONE)
>                         /* ACK comes with both addresses empty */
>                         skb->pkt_type = PACKET_HOST;
> +               else if (!wpan_dev->parent)
> +                       /* No dest means PAN coordinator is the recipient */
> +                       skb->pkt_type = PACKET_HOST;
> +               else
> +                       /* We are not the PAN coordinator, just relaying */
> +                       skb->pkt_type = PACKET_OTHERHOST;
>                 break;
>         case IEEE802154_ADDR_LONG:
>                 if (mac_cb(skb)->dest.pan_id != span &&

So if I understand it correctly, the "wpan_dev->parent" check acts
like a "forwarding" setting on an IP capable interface here? The
"forwarding" setting changes the interface to act as a router, which
is fine... but we have a difference here with the actual hardware and
the address filtering setting which we don't have in e.g. ethernet. My
concern is here that this code is probably interface type specific,
e.g. node vs coordinator type and currently we handle both in one
receive part.

I am fine with that and probably it is just a thing to change in future...

- Alex

Re: [PATCH wpan-next 2/5] mac802154: Use the PAN coordinator parameter when stamping packets

[Alexander Aring]

Hi,

On Thu, Dec 14, 2023 at 9:46 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > ACKs come with the source and destination address empty, this has been
> > clarified already. But there is something else: if the destination
> > address is empty but the source address is valid, it may be a way to
> > reach the PAN coordinator. Either the device receiving this frame is the
> > PAN coordinator itself and should process what it just received
> > (PACKET_HOST) or it is not and may, if supported, relay the packet as it
> > is targeted to another device in the network.
> >
> > Right now we do not support relaying so the packet should be dropped in
> > the first place, but the stamping looks more accurate this way.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Alexander Aring <aahringo@redhat.com>

- Alex

Re: [PATCH wpan-next 2/5] mac802154: Use the PAN coordinator parameter when stamping packets

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Thu, 14 Dec 2023 21:46:06 -0500:

> Hi,
> 
> On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > ACKs come with the source and destination address empty, this has been
> > clarified already. But there is something else: if the destination
> > address is empty but the source address is valid, it may be a way to
> > reach the PAN coordinator. Either the device receiving this frame is the
> > PAN coordinator itself and should process what it just received
> > (PACKET_HOST) or it is not and may, if supported, relay the packet as it
> > is targeted to another device in the network.
> >
> > Right now we do not support relaying so the packet should be dropped in
> > the first place, but the stamping looks more accurate this way.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  net/mac802154/rx.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > index 0024341ef9c5..e40a988d6c80 100644
> > --- a/net/mac802154/rx.c
> > +++ b/net/mac802154/rx.c
> > @@ -156,12 +156,15 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> >
> >         switch (mac_cb(skb)->dest.mode) {
> >         case IEEE802154_ADDR_NONE:
> > -               if (hdr->source.mode != IEEE802154_ADDR_NONE)
> > -                       /* FIXME: check if we are PAN coordinator */
> > -                       skb->pkt_type = PACKET_OTHERHOST;
> > -               else
> > +               if (hdr->source.mode == IEEE802154_ADDR_NONE)
> >                         /* ACK comes with both addresses empty */
> >                         skb->pkt_type = PACKET_HOST;
> > +               else if (!wpan_dev->parent)
> > +                       /* No dest means PAN coordinator is the recipient */
> > +                       skb->pkt_type = PACKET_HOST;
> > +               else
> > +                       /* We are not the PAN coordinator, just relaying */
> > +                       skb->pkt_type = PACKET_OTHERHOST;
> >                 break;
> >         case IEEE802154_ADDR_LONG:
> >                 if (mac_cb(skb)->dest.pan_id != span &&  
> 
> So if I understand it correctly, the "wpan_dev->parent" check acts
> like a "forwarding" setting on an IP capable interface here? The

Kind of, yes, in this case having a parent means we are not the top
level PAN coordinator.

> "forwarding" setting changes the interface to act as a router, which
> is fine... 

I think there is no true "router" role but depending on the frame
construction (dest field) we might sometimes act as a router. This is
not supported in Linux, just a feature of the spec.

> but we have a difference here with the actual hardware and
> the address filtering setting which we don't have in e.g. ethernet. My
> concern is here that this code is probably interface type specific,
> e.g. node vs coordinator type and currently we handle both in one
> receive part.
> 
> I am fine with that and probably it is just a thing to change in future...

That is true and probably will need adaptations if/when we come to this
feature. What we do here however is just stamping the packet, in a
manner that is more accurate. So in practice all type of interfaces may
want to do that. However the handling of the packet later in the
stack will be interface specific, I agree.

Thanks,
Miquèl

Re: [PATCH wpan-next 2/5] mac802154: Use the PAN coordinator parameter when stamping packets

[Miquel Raynal]

On Tue, 2023-11-28 at 11:16:52 UTC, Miquel Raynal wrote:
> ACKs come with the source and destination address empty, this has been
> clarified already. But there is something else: if the destination
> address is empty but the source address is valid, it may be a way to
> reach the PAN coordinator. Either the device receiving this frame is the
> PAN coordinator itself and should process what it just received
> (PACKET_HOST) or it is not and may, if supported, relay the packet as it
> is targeted to another device in the network.
> 
> Right now we do not support relaying so the packet should be dropped in
> the first place, but the stamping looks more accurate this way.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> Acked-by: Alexander Aring <aahringo@redhat.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git master.

Miquel

[PATCH wpan-next 3/5] mac802154: Only allow PAN controllers to process association requests

[Miquel Raynal]

It is not very clear in the specification whether simple coordinators
are allowed or not to answer to association requests themselves. As
there is no synchronization mechanism, it is probably best to rely on
the relay feature of these coordinators and avoid processing them in
this case.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/scan.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index 5873da634fb4..1c0eeaa76560 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -781,6 +781,12 @@ int mac802154_process_association_req(struct ieee802154_sub_if_data *sdata,
 		 unlikely(dest->short_addr != wpan_dev->short_addr))
 		return -ENODEV;
 
+	if (wpan_dev->parent) {
+		dev_dbg(&sdata->dev->dev,
+			"Ignoring ASSOC REQ, not the PAN coordinator\n");
+		return -ENODEV;
+	}
+
 	mutex_lock(&wpan_dev->association_lock);
 
 	memcpy(&assoc_req_pl, skb->data, sizeof(assoc_req_pl));
-- 
2.34.1

Re: [PATCH wpan-next 3/5] mac802154: Only allow PAN controllers to process association requests

[Stefan Schmidt]

Hello.

On 28.11.23 12:16, Miquel Raynal wrote:
> It is not very clear in the specification whether simple coordinators
> are allowed or not to answer to association requests themselves. As
> there is no synchronization mechanism, it is probably best to rely on
> the relay feature of these coordinators and avoid processing them in
> this case.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/mac802154/scan.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
> index 5873da634fb4..1c0eeaa76560 100644
> --- a/net/mac802154/scan.c
> +++ b/net/mac802154/scan.c
> @@ -781,6 +781,12 @@ int mac802154_process_association_req(struct ieee802154_sub_if_data *sdata,
>   		 unlikely(dest->short_addr != wpan_dev->short_addr))
>   		return -ENODEV;
>   
> +	if (wpan_dev->parent) {
> +		dev_dbg(&sdata->dev->dev,
> +			"Ignoring ASSOC REQ, not the PAN coordinator\n");
> +		return -ENODEV;
> +	}
> +
>   	mutex_lock(&wpan_dev->association_lock);
>   
>   	memcpy(&assoc_req_pl, skb->data, sizeof(assoc_req_pl));

Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH wpan-next 3/5] mac802154: Only allow PAN controllers to process association requests

[Alexander Aring]

Hi,

On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> It is not very clear in the specification whether simple coordinators
> are allowed or not to answer to association requests themselves. As
> there is no synchronization mechanism, it is probably best to rely on
> the relay feature of these coordinators and avoid processing them in
> this case.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Alexander Aring <aahringo@redhat.com>

- Alex

Re: [PATCH wpan-next 3/5] mac802154: Only allow PAN controllers to process association requests

[Miquel Raynal]

On Tue, 2023-11-28 at 11:16:53 UTC, Miquel Raynal wrote:
> It is not very clear in the specification whether simple coordinators
> are allowed or not to answer to association requests themselves. As
> there is no synchronization mechanism, it is probably best to rely on
> the relay feature of these coordinators and avoid processing them in
> this case.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> Acked-by: Alexander Aring <aahringo@redhat.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git master.

Miquel

[PATCH wpan-next 4/5] ieee802154: Avoid confusing changes after associating

[Miquel Raynal]

Once associated with any device, we are part of a PAN (with a specific
PAN ID), and we are expected to be present on a particular
channel. Let's avoid confusing other devices by preventing any PAN
ID/channel change once associated.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/nl802154.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index e4d290d0e0a0..5c73b5fcadc0 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1087,6 +1087,15 @@ static int nl802154_set_pan_id(struct sk_buff *skb, struct genl_info *info)
 
 	pan_id = nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
 
+	/* Only allow changing the PAN ID when the device has no more
+	 * associations ongoing to avoid confusing peers.
+	 */
+	if (cfg802154_device_is_associated(wpan_dev)) {
+		NL_SET_ERR_MSG(info->extack,
+			       "Existing associations, changing PAN ID forbidden");
+		return -EINVAL;
+	}
+
 	return rdev_set_pan_id(rdev, wpan_dev, pan_id);
 }
 
@@ -1113,20 +1122,17 @@ static int nl802154_set_short_addr(struct sk_buff *skb, struct genl_info *info)
 
 	short_addr = nla_get_le16(info->attrs[NL802154_ATTR_SHORT_ADDR]);
 
-	/* TODO
-	 * I am not sure about to check here on broadcast short_addr.
-	 * Broadcast is a valid setting, comment from 802.15.4:
-	 * A value of 0xfffe indicates that the device has
-	 * associated but has not been allocated an address. A
-	 * value of 0xffff indicates that the device does not
-	 * have a short address.
-	 *
-	 * I think we should allow to set these settings but
-	 * don't allow to allow socket communication with it.
+	/* The short address only has a meaning when part of a PAN, after a
+	 * proper association procedure. However, we want to still offer the
+	 * possibility to create static networks so changing the short address
+	 * is only allowed when not already associated to other devices with
+	 * the official handshake.
 	 */
-	if (short_addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC) ||
-	    short_addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST))
+	if (cfg802154_device_is_associated(wpan_dev)) {
+		NL_SET_ERR_MSG(info->extack,
+			       "Existing associations, changing short address forbidden");
 		return -EINVAL;
+	}
 
 	return rdev_set_short_addr(rdev, wpan_dev, short_addr);
 }
-- 
2.34.1

Re: [PATCH wpan-next 4/5] ieee802154: Avoid confusing changes after associating

[Stefan Schmidt]

Hello.

On 28.11.23 12:16, Miquel Raynal wrote:
> Once associated with any device, we are part of a PAN (with a specific
> PAN ID), and we are expected to be present on a particular
> channel. Let's avoid confusing other devices by preventing any PAN
> ID/channel change once associated.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/ieee802154/nl802154.c | 30 ++++++++++++++++++------------
>   1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index e4d290d0e0a0..5c73b5fcadc0 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -1087,6 +1087,15 @@ static int nl802154_set_pan_id(struct sk_buff *skb, struct genl_info *info)
>   
>   	pan_id = nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
>   
> +	/* Only allow changing the PAN ID when the device has no more
> +	 * associations ongoing to avoid confusing peers.
> +	 */
> +	if (cfg802154_device_is_associated(wpan_dev)) {
> +		NL_SET_ERR_MSG(info->extack,
> +			       "Existing associations, changing PAN ID forbidden");
> +		return -EINVAL;
> +	}
> +
>   	return rdev_set_pan_id(rdev, wpan_dev, pan_id);
>   }
>   
> @@ -1113,20 +1122,17 @@ static int nl802154_set_short_addr(struct sk_buff *skb, struct genl_info *info)
>   
>   	short_addr = nla_get_le16(info->attrs[NL802154_ATTR_SHORT_ADDR]);
>   
> -	/* TODO
> -	 * I am not sure about to check here on broadcast short_addr.
> -	 * Broadcast is a valid setting, comment from 802.15.4:
> -	 * A value of 0xfffe indicates that the device has
> -	 * associated but has not been allocated an address. A
> -	 * value of 0xffff indicates that the device does not
> -	 * have a short address.
> -	 *
> -	 * I think we should allow to set these settings but
> -	 * don't allow to allow socket communication with it.
> +	/* The short address only has a meaning when part of a PAN, after a
> +	 * proper association procedure. However, we want to still offer the
> +	 * possibility to create static networks so changing the short address
> +	 * is only allowed when not already associated to other devices with
> +	 * the official handshake.
>   	 */
> -	if (short_addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC) ||
> -	    short_addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST))
> +	if (cfg802154_device_is_associated(wpan_dev)) {
> +		NL_SET_ERR_MSG(info->extack,
> +			       "Existing associations, changing short address forbidden");
>   		return -EINVAL;
> +	}
>   
>   	return rdev_set_short_addr(rdev, wpan_dev, short_addr);
>   }

Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH wpan-next 4/5] ieee802154: Avoid confusing changes after associating

[Alexander Aring]

Hi,

On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Once associated with any device, we are part of a PAN (with a specific
> PAN ID), and we are expected to be present on a particular
> channel. Let's avoid confusing other devices by preventing any PAN
> ID/channel change once associated.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Alexander Aring <aahringo@redhat.com>

- Alex

Re: [PATCH wpan-next 4/5] ieee802154: Avoid confusing changes after associating

[Miquel Raynal]

On Tue, 2023-11-28 at 11:16:54 UTC, Miquel Raynal wrote:
> Once associated with any device, we are part of a PAN (with a specific
> PAN ID), and we are expected to be present on a particular
> channel. Let's avoid confusing other devices by preventing any PAN
> ID/channel change once associated.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> Acked-by: Alexander Aring <aahringo@redhat.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git master.

Miquel

[PATCH wpan-next 5/5] mac802154: Avoid new associations while disassociating

[Miquel Raynal]

While disassociating from a PAN ourselves, let's set the maximum number
of associations temporarily to zero to be sure no new device tries to
associate with us.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h |  4 +++-
 net/ieee802154/pan.c    |  8 +++++++-
 net/mac802154/cfg.c     | 11 ++++++++---
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index a64bbcd71f10..cd95711b12b8 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -589,8 +589,10 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev,
  * cfg802154_set_max_associations - Limit the number of future associations
  * @wpan_dev: the wpan device
  * @max: the maximum number of devices we accept to associate
+ * @return: the old maximum value
  */
-void cfg802154_set_max_associations(struct wpan_dev *wpan_dev, unsigned int max);
+unsigned int cfg802154_set_max_associations(struct wpan_dev *wpan_dev,
+					    unsigned int max);
 
 /**
  * cfg802154_get_free_short_addr - Get a free address among the known devices
diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
index fb5b0af2ef68..249df7364b3e 100644
--- a/net/ieee802154/pan.c
+++ b/net/ieee802154/pan.c
@@ -94,10 +94,16 @@ __le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev)
 }
 EXPORT_SYMBOL_GPL(cfg802154_get_free_short_addr);
 
-void cfg802154_set_max_associations(struct wpan_dev *wpan_dev, unsigned int max)
+unsigned int cfg802154_set_max_associations(struct wpan_dev *wpan_dev,
+					    unsigned int max)
 {
+	unsigned int old_max;
+
 	lockdep_assert_held(&wpan_dev->association_lock);
 
+	old_max = wpan_dev->max_associations;
 	wpan_dev->max_associations = max;
+
+	return old_max;
 }
 EXPORT_SYMBOL_GPL(cfg802154_set_max_associations);
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 17e2032fac24..ef7f23af043f 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -389,6 +389,7 @@ static int mac802154_disassociate_from_parent(struct wpan_phy *wpan_phy,
 	struct ieee802154_local *local = wpan_phy_priv(wpan_phy);
 	struct ieee802154_pan_device *child, *tmp;
 	struct ieee802154_sub_if_data *sdata;
+	unsigned int max_assoc;
 	u64 eaddr;
 	int ret;
 
@@ -397,6 +398,7 @@ static int mac802154_disassociate_from_parent(struct wpan_phy *wpan_phy,
 	/* Start by disassociating all the children and preventing new ones to
 	 * attempt associations.
 	 */
+	max_assoc = cfg802154_set_max_associations(wpan_dev, 0);
 	list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) {
 		ret = mac802154_send_disassociation_notif(sdata, child,
 							  IEEE802154_COORD_WISHES_DEVICE_TO_LEAVE);
@@ -429,14 +431,17 @@ static int mac802154_disassociate_from_parent(struct wpan_phy *wpan_phy,
 	if (local->hw.flags & IEEE802154_HW_AFILT) {
 		ret = drv_set_pan_id(local, wpan_dev->pan_id);
 		if (ret < 0)
-			return ret;
+			goto reset_mac_assoc;
 
 		ret = drv_set_short_addr(local, wpan_dev->short_addr);
 		if (ret < 0)
-			return ret;
+			goto reset_mac_assoc;
 	}
 
-	return 0;
+reset_mac_assoc:
+	cfg802154_set_max_associations(wpan_dev, max_assoc);
+
+	return ret;
 }
 
 static int mac802154_disassociate_child(struct wpan_phy *wpan_phy,
-- 
2.34.1

Re: [PATCH wpan-next 5/5] mac802154: Avoid new associations while disassociating

[Stefan Schmidt]

Hello.

On 28.11.23 12:16, Miquel Raynal wrote:
> While disassociating from a PAN ourselves, let's set the maximum number
> of associations temporarily to zero to be sure no new device tries to
> associate with us.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   include/net/cfg802154.h |  4 +++-
>   net/ieee802154/pan.c    |  8 +++++++-
>   net/mac802154/cfg.c     | 11 ++++++++---
>   3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index a64bbcd71f10..cd95711b12b8 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -589,8 +589,10 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev,
>    * cfg802154_set_max_associations - Limit the number of future associations
>    * @wpan_dev: the wpan device
>    * @max: the maximum number of devices we accept to associate
> + * @return: the old maximum value
>    */
> -void cfg802154_set_max_associations(struct wpan_dev *wpan_dev, unsigned int max);
> +unsigned int cfg802154_set_max_associations(struct wpan_dev *wpan_dev,
> +					    unsigned int max);
>   
>   /**
>    * cfg802154_get_free_short_addr - Get a free address among the known devices
> diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c
> index fb5b0af2ef68..249df7364b3e 100644
> --- a/net/ieee802154/pan.c
> +++ b/net/ieee802154/pan.c
> @@ -94,10 +94,16 @@ __le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev)
>   }
>   EXPORT_SYMBOL_GPL(cfg802154_get_free_short_addr);
>   
> -void cfg802154_set_max_associations(struct wpan_dev *wpan_dev, unsigned int max)
> +unsigned int cfg802154_set_max_associations(struct wpan_dev *wpan_dev,
> +					    unsigned int max)
>   {
> +	unsigned int old_max;
> +
>   	lockdep_assert_held(&wpan_dev->association_lock);
>   
> +	old_max = wpan_dev->max_associations;
>   	wpan_dev->max_associations = max;
> +
> +	return old_max;
>   }
>   EXPORT_SYMBOL_GPL(cfg802154_set_max_associations);
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index 17e2032fac24..ef7f23af043f 100644
> --- a/net/mac802154/cfg.c
> +++ b/net/mac802154/cfg.c
> @@ -389,6 +389,7 @@ static int mac802154_disassociate_from_parent(struct wpan_phy *wpan_phy,
>   	struct ieee802154_local *local = wpan_phy_priv(wpan_phy);
>   	struct ieee802154_pan_device *child, *tmp;
>   	struct ieee802154_sub_if_data *sdata;
> +	unsigned int max_assoc;
>   	u64 eaddr;
>   	int ret;
>   
> @@ -397,6 +398,7 @@ static int mac802154_disassociate_from_parent(struct wpan_phy *wpan_phy,
>   	/* Start by disassociating all the children and preventing new ones to
>   	 * attempt associations.
>   	 */
> +	max_assoc = cfg802154_set_max_associations(wpan_dev, 0);
>   	list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) {
>   		ret = mac802154_send_disassociation_notif(sdata, child,
>   							  IEEE802154_COORD_WISHES_DEVICE_TO_LEAVE);
> @@ -429,14 +431,17 @@ static int mac802154_disassociate_from_parent(struct wpan_phy *wpan_phy,
>   	if (local->hw.flags & IEEE802154_HW_AFILT) {
>   		ret = drv_set_pan_id(local, wpan_dev->pan_id);
>   		if (ret < 0)
> -			return ret;
> +			goto reset_mac_assoc;
>   
>   		ret = drv_set_short_addr(local, wpan_dev->short_addr);
>   		if (ret < 0)
> -			return ret;
> +			goto reset_mac_assoc;
>   	}
>   
> -	return 0;
> +reset_mac_assoc:
> +	cfg802154_set_max_associations(wpan_dev, max_assoc);
> +
> +	return ret;
>   }
>   
>   static int mac802154_disassociate_child(struct wpan_phy *wpan_phy,

Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>

regards
Stefan Schmidt

Re: [PATCH wpan-next 5/5] mac802154: Avoid new associations while disassociating

[Alexander Aring]

Hi,

On Tue, Nov 28, 2023 at 6:17 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> While disassociating from a PAN ourselves, let's set the maximum number
> of associations temporarily to zero to be sure no new device tries to
> associate with us.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Acked-by: Alexander Aring <aahringo@redhat.com>

- Alex

Re: [PATCH wpan-next 5/5] mac802154: Avoid new associations while disassociating

[Miquel Raynal]

On Tue, 2023-11-28 at 11:16:55 UTC, Miquel Raynal wrote:
> While disassociating from a PAN ourselves, let's set the maximum number
> of associations temporarily to zero to be sure no new device tries to
> associate with us.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org>
> Acked-by: Alexander Aring <aahringo@redhat.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/wpan/wpan-next.git master.

Miquel