| Message ID | 20230922155029.592018-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Associations between devices | expand |
Hi, On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Coordinators may have to handle association requests from peers which > want to join the PAN. The logic involves: > - Acknowledging the request (done by hardware) > - If requested, a random short address that is free on this PAN should > be chosen for the device. > - Sending an association response with the short address allocated for > the peer and expecting it to be ack'ed. > > If anything fails during this procedure, the peer is considered not > associated. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/net/cfg802154.h | 7 ++ > include/net/ieee802154_netdev.h | 6 ++ > net/ieee802154/core.c | 7 ++ > net/ieee802154/pan.c | 30 +++++++ > net/mac802154/ieee802154_i.h | 2 + > net/mac802154/rx.c | 8 ++ > net/mac802154/scan.c | 142 ++++++++++++++++++++++++++++++++ > 7 files changed, 202 insertions(+) > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > index 9b036ab20079..c844ae63bc04 100644 > --- a/include/net/cfg802154.h > +++ b/include/net/cfg802154.h > @@ -583,4 +583,11 @@ struct ieee802154_pan_device * > cfg802154_device_is_child(struct wpan_dev *wpan_dev, > struct ieee802154_addr *target); > > +/** > + * cfg802154_get_free_short_addr - Get a free address among the known devices > + * @wpan_dev: the wpan device > + * @return: a random short address expectedly unused on our PAN > + */ > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev); > + > #endif /* __NET_CFG802154_H */ > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h > index 16194356cfe7..4de858f9929e 100644 > --- a/include/net/ieee802154_netdev.h > +++ b/include/net/ieee802154_netdev.h > @@ -211,6 +211,12 @@ struct ieee802154_association_req_frame { > struct ieee802154_assoc_req_pl assoc_req_pl; > }; > > +struct ieee802154_association_resp_frame { > + struct ieee802154_hdr mhr; > + struct ieee802154_mac_cmd_pl mac_pl; > + struct ieee802154_assoc_resp_pl assoc_resp_pl; > +}; > + > struct ieee802154_disassociation_notif_frame { > struct ieee802154_hdr mhr; > struct ieee802154_mac_cmd_pl mac_pl; > diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c > index a08d75dd56ad..1670a71327a7 100644 > --- a/net/ieee802154/core.c > +++ b/net/ieee802154/core.c > @@ -200,11 +200,18 @@ EXPORT_SYMBOL(wpan_phy_free); > > static void cfg802154_free_peer_structures(struct wpan_dev *wpan_dev) > { > + struct ieee802154_pan_device *child, *tmp; > + > mutex_lock(&wpan_dev->association_lock); > > kfree(wpan_dev->parent); > wpan_dev->parent = NULL; > > + list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) { > + list_del(&child->node); > + kfree(child); > + } > + > mutex_unlock(&wpan_dev->association_lock); > } > > diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c > index 9e1f1973c294..e99c64054dcb 100644 > --- a/net/ieee802154/pan.c > +++ b/net/ieee802154/pan.c > @@ -73,3 +73,33 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev, > return NULL; > } > EXPORT_SYMBOL_GPL(cfg802154_device_is_child); > + > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev) > +{ > + struct ieee802154_pan_device *child; > + __le16 addr; > + > + lockdep_assert_held(&wpan_dev->association_lock); > + > + do { > + get_random_bytes(&addr, 2); > + if (addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST) || > + addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC)) > + continue; > + > + if (wpan_dev->short_addr == addr) > + continue; > + > + if (wpan_dev->parent && wpan_dev->parent->short_addr == addr) > + continue; > + > + list_for_each_entry(child, &wpan_dev->children, node) > + if (child->short_addr == addr) > + continue; > + > + break; > + } while (1); > + I still believe that this random 2 bytes and check if it's already being used is wrong here. We need something to use the next free available number according to the data we are storing here. However it is acceptable and can be changed later... - Alex
Hi Alexander, aahringo@redhat.com wrote on Sun, 24 Sep 2023 20:13:34 -0400: > Hi, > > On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Coordinators may have to handle association requests from peers which > > want to join the PAN. The logic involves: > > - Acknowledging the request (done by hardware) > > - If requested, a random short address that is free on this PAN should > > be chosen for the device. > > - Sending an association response with the short address allocated for > > the peer and expecting it to be ack'ed. > > > > If anything fails during this procedure, the peer is considered not > > associated. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > include/net/cfg802154.h | 7 ++ > > include/net/ieee802154_netdev.h | 6 ++ > > net/ieee802154/core.c | 7 ++ > > net/ieee802154/pan.c | 30 +++++++ > > net/mac802154/ieee802154_i.h | 2 + > > net/mac802154/rx.c | 8 ++ > > net/mac802154/scan.c | 142 ++++++++++++++++++++++++++++++++ > > 7 files changed, 202 insertions(+) > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > index 9b036ab20079..c844ae63bc04 100644 > > --- a/include/net/cfg802154.h > > +++ b/include/net/cfg802154.h > > @@ -583,4 +583,11 @@ struct ieee802154_pan_device * > > cfg802154_device_is_child(struct wpan_dev *wpan_dev, > > struct ieee802154_addr *target); > > > > +/** > > + * cfg802154_get_free_short_addr - Get a free address among the known devices > > + * @wpan_dev: the wpan device > > + * @return: a random short address expectedly unused on our PAN > > + */ > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev); > > + > > #endif /* __NET_CFG802154_H */ > > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h > > index 16194356cfe7..4de858f9929e 100644 > > --- a/include/net/ieee802154_netdev.h > > +++ b/include/net/ieee802154_netdev.h > > @@ -211,6 +211,12 @@ struct ieee802154_association_req_frame { > > struct ieee802154_assoc_req_pl assoc_req_pl; > > }; > > > > +struct ieee802154_association_resp_frame { > > + struct ieee802154_hdr mhr; > > + struct ieee802154_mac_cmd_pl mac_pl; > > + struct ieee802154_assoc_resp_pl assoc_resp_pl; > > +}; > > + > > struct ieee802154_disassociation_notif_frame { > > struct ieee802154_hdr mhr; > > struct ieee802154_mac_cmd_pl mac_pl; > > diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c > > index a08d75dd56ad..1670a71327a7 100644 > > --- a/net/ieee802154/core.c > > +++ b/net/ieee802154/core.c > > @@ -200,11 +200,18 @@ EXPORT_SYMBOL(wpan_phy_free); > > > > static void cfg802154_free_peer_structures(struct wpan_dev *wpan_dev) > > { > > + struct ieee802154_pan_device *child, *tmp; > > + > > mutex_lock(&wpan_dev->association_lock); > > > > kfree(wpan_dev->parent); > > wpan_dev->parent = NULL; > > > > + list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) { > > + list_del(&child->node); > > + kfree(child); > > + } > > + > > mutex_unlock(&wpan_dev->association_lock); > > } > > > > diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c > > index 9e1f1973c294..e99c64054dcb 100644 > > --- a/net/ieee802154/pan.c > > +++ b/net/ieee802154/pan.c > > @@ -73,3 +73,33 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev, > > return NULL; > > } > > EXPORT_SYMBOL_GPL(cfg802154_device_is_child); > > + > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev) > > +{ > > + struct ieee802154_pan_device *child; > > + __le16 addr; > > + > > + lockdep_assert_held(&wpan_dev->association_lock); > > + > > + do { > > + get_random_bytes(&addr, 2); > > + if (addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST) || > > + addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC)) > > + continue; > > + > > + if (wpan_dev->short_addr == addr) > > + continue; > > + > > + if (wpan_dev->parent && wpan_dev->parent->short_addr == addr) > > + continue; > > + > > + list_for_each_entry(child, &wpan_dev->children, node) > > + if (child->short_addr == addr) > > + continue; > > + > > + break; > > + } while (1); > > + > > I still believe that this random 2 bytes and check if it's already > being used is wrong here. We need something to use the next free > available number according to the data we are storing here. This issue I still have in mind is when you have this typology: device A -------> device B --------> device C <-------- device D (leaf) (coord) (PAN coord) (leaf) B associates with C A associates with B D associates with C If B and C run Linux's stack, they will always have the same short address. Yes this can be handled (realignment procedure). But any time this happens, you'll have a load of predictable realignments when A and D get in range with B or C. > However it is acceptable and can be changed later... Ok. Thanks, Miquèl
Hi, On Mon, Sep 25, 2023 at 3:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Sun, 24 Sep 2023 20:13:34 -0400: > > > Hi, > > > > On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > > > Coordinators may have to handle association requests from peers which > > > want to join the PAN. The logic involves: > > > - Acknowledging the request (done by hardware) > > > - If requested, a random short address that is free on this PAN should > > > be chosen for the device. > > > - Sending an association response with the short address allocated for > > > the peer and expecting it to be ack'ed. > > > > > > If anything fails during this procedure, the peer is considered not > > > associated. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > include/net/cfg802154.h | 7 ++ > > > include/net/ieee802154_netdev.h | 6 ++ > > > net/ieee802154/core.c | 7 ++ > > > net/ieee802154/pan.c | 30 +++++++ > > > net/mac802154/ieee802154_i.h | 2 + > > > net/mac802154/rx.c | 8 ++ > > > net/mac802154/scan.c | 142 ++++++++++++++++++++++++++++++++ > > > 7 files changed, 202 insertions(+) > > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > > index 9b036ab20079..c844ae63bc04 100644 > > > --- a/include/net/cfg802154.h > > > +++ b/include/net/cfg802154.h > > > @@ -583,4 +583,11 @@ struct ieee802154_pan_device * > > > cfg802154_device_is_child(struct wpan_dev *wpan_dev, > > > struct ieee802154_addr *target); > > > > > > +/** > > > + * cfg802154_get_free_short_addr - Get a free address among the known devices > > > + * @wpan_dev: the wpan device > > > + * @return: a random short address expectedly unused on our PAN > > > + */ > > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev); > > > + > > > #endif /* __NET_CFG802154_H */ > > > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h > > > index 16194356cfe7..4de858f9929e 100644 > > > --- a/include/net/ieee802154_netdev.h > > > +++ b/include/net/ieee802154_netdev.h > > > @@ -211,6 +211,12 @@ struct ieee802154_association_req_frame { > > > struct ieee802154_assoc_req_pl assoc_req_pl; > > > }; > > > > > > +struct ieee802154_association_resp_frame { > > > + struct ieee802154_hdr mhr; > > > + struct ieee802154_mac_cmd_pl mac_pl; > > > + struct ieee802154_assoc_resp_pl assoc_resp_pl; > > > +}; > > > + > > > struct ieee802154_disassociation_notif_frame { > > > struct ieee802154_hdr mhr; > > > struct ieee802154_mac_cmd_pl mac_pl; > > > diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c > > > index a08d75dd56ad..1670a71327a7 100644 > > > --- a/net/ieee802154/core.c > > > +++ b/net/ieee802154/core.c > > > @@ -200,11 +200,18 @@ EXPORT_SYMBOL(wpan_phy_free); > > > > > > static void cfg802154_free_peer_structures(struct wpan_dev *wpan_dev) > > > { > > > + struct ieee802154_pan_device *child, *tmp; > > > + > > > mutex_lock(&wpan_dev->association_lock); > > > > > > kfree(wpan_dev->parent); > > > wpan_dev->parent = NULL; > > > > > > + list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) { > > > + list_del(&child->node); > > > + kfree(child); > > > + } > > > + > > > mutex_unlock(&wpan_dev->association_lock); > > > } > > > > > > diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c > > > index 9e1f1973c294..e99c64054dcb 100644 > > > --- a/net/ieee802154/pan.c > > > +++ b/net/ieee802154/pan.c > > > @@ -73,3 +73,33 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev, > > > return NULL; > > > } > > > EXPORT_SYMBOL_GPL(cfg802154_device_is_child); > > > + > > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev) > > > +{ > > > + struct ieee802154_pan_device *child; > > > + __le16 addr; > > > + > > > + lockdep_assert_held(&wpan_dev->association_lock); > > > + > > > + do { > > > + get_random_bytes(&addr, 2); > > > + if (addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST) || > > > + addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC)) > > > + continue; > > > + > > > + if (wpan_dev->short_addr == addr) > > > + continue; > > > + > > > + if (wpan_dev->parent && wpan_dev->parent->short_addr == addr) > > > + continue; > > > + > > > + list_for_each_entry(child, &wpan_dev->children, node) > > > + if (child->short_addr == addr) > > > + continue; > > > + > > > + break; > > > + } while (1); > > > + > > > > I still believe that this random 2 bytes and check if it's already > > being used is wrong here. We need something to use the next free > > available number according to the data we are storing here. > > This issue I still have in mind is when you have this typology: > > device A -------> device B --------> device C <-------- device D > (leaf) (coord) (PAN coord) (leaf) > > B associates with C > A associates with B > D associates with C > > If B and C run Linux's stack, they will always have the same short > address. Yes this can be handled (realignment procedure). But any time > this happens, you'll have a load of predictable realignments when A and > D get in range with B or C. > I see that it can be "more" predictable, but what happens when there is the same short address case with the random number generator? It sounds to me like there needs to be a kind of duplicate address detection going on and then choose another one, if 802.15.4 even handles this case... I am also thinking that there is only one number left and the random generator runs multiple times to find the last one aka "it's random you can never be sure", when it always returns the same address. However, that's only my thoughts about it and hopefully can be improved in future. - Alex
Hi, On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Coordinators may have to handle association requests from peers which > want to join the PAN. The logic involves: > - Acknowledging the request (done by hardware) > - If requested, a random short address that is free on this PAN should > be chosen for the device. > - Sending an association response with the short address allocated for > the peer and expecting it to be ack'ed. > > If anything fails during this procedure, the peer is considered not > associated. I thought a coordinator can also reject requests for _any_ reason and it's very user specific whatever that reason is. If we have such a case (that it is very user specific what to do exactly) this should be able to be controlled by the user space to have there a logic to tell the kernel to accept or reject the association. However, I am fine with this solution, but I think we might want to change this behaviour in the future so that an application in the user space has the logic to tell the kernel to accept or reject an association. That would make sense? - Alex
Hi Alexander, aahringo@redhat.com wrote on Tue, 26 Sep 2023 21:31:09 -0400: > Hi, > > On Mon, Sep 25, 2023 at 3:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Sun, 24 Sep 2023 20:13:34 -0400: > > > > > Hi, > > > > > > On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > > > > > Coordinators may have to handle association requests from peers which > > > > want to join the PAN. The logic involves: > > > > - Acknowledging the request (done by hardware) > > > > - If requested, a random short address that is free on this PAN should > > > > be chosen for the device. > > > > - Sending an association response with the short address allocated for > > > > the peer and expecting it to be ack'ed. > > > > > > > > If anything fails during this procedure, the peer is considered not > > > > associated. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > include/net/cfg802154.h | 7 ++ > > > > include/net/ieee802154_netdev.h | 6 ++ > > > > net/ieee802154/core.c | 7 ++ > > > > net/ieee802154/pan.c | 30 +++++++ > > > > net/mac802154/ieee802154_i.h | 2 + > > > > net/mac802154/rx.c | 8 ++ > > > > net/mac802154/scan.c | 142 ++++++++++++++++++++++++++++++++ > > > > 7 files changed, 202 insertions(+) > > > > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > > > index 9b036ab20079..c844ae63bc04 100644 > > > > --- a/include/net/cfg802154.h > > > > +++ b/include/net/cfg802154.h > > > > @@ -583,4 +583,11 @@ struct ieee802154_pan_device * > > > > cfg802154_device_is_child(struct wpan_dev *wpan_dev, > > > > struct ieee802154_addr *target); > > > > > > > > +/** > > > > + * cfg802154_get_free_short_addr - Get a free address among the known devices > > > > + * @wpan_dev: the wpan device > > > > + * @return: a random short address expectedly unused on our PAN > > > > + */ > > > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev); > > > > + > > > > #endif /* __NET_CFG802154_H */ > > > > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h > > > > index 16194356cfe7..4de858f9929e 100644 > > > > --- a/include/net/ieee802154_netdev.h > > > > +++ b/include/net/ieee802154_netdev.h > > > > @@ -211,6 +211,12 @@ struct ieee802154_association_req_frame { > > > > struct ieee802154_assoc_req_pl assoc_req_pl; > > > > }; > > > > > > > > +struct ieee802154_association_resp_frame { > > > > + struct ieee802154_hdr mhr; > > > > + struct ieee802154_mac_cmd_pl mac_pl; > > > > + struct ieee802154_assoc_resp_pl assoc_resp_pl; > > > > +}; > > > > + > > > > struct ieee802154_disassociation_notif_frame { > > > > struct ieee802154_hdr mhr; > > > > struct ieee802154_mac_cmd_pl mac_pl; > > > > diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c > > > > index a08d75dd56ad..1670a71327a7 100644 > > > > --- a/net/ieee802154/core.c > > > > +++ b/net/ieee802154/core.c > > > > @@ -200,11 +200,18 @@ EXPORT_SYMBOL(wpan_phy_free); > > > > > > > > static void cfg802154_free_peer_structures(struct wpan_dev *wpan_dev) > > > > { > > > > + struct ieee802154_pan_device *child, *tmp; > > > > + > > > > mutex_lock(&wpan_dev->association_lock); > > > > > > > > kfree(wpan_dev->parent); > > > > wpan_dev->parent = NULL; > > > > > > > > + list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) { > > > > + list_del(&child->node); > > > > + kfree(child); > > > > + } > > > > + > > > > mutex_unlock(&wpan_dev->association_lock); > > > > } > > > > > > > > diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c > > > > index 9e1f1973c294..e99c64054dcb 100644 > > > > --- a/net/ieee802154/pan.c > > > > +++ b/net/ieee802154/pan.c > > > > @@ -73,3 +73,33 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev, > > > > return NULL; > > > > } > > > > EXPORT_SYMBOL_GPL(cfg802154_device_is_child); > > > > + > > > > +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev) > > > > +{ > > > > + struct ieee802154_pan_device *child; > > > > + __le16 addr; > > > > + > > > > + lockdep_assert_held(&wpan_dev->association_lock); > > > > + > > > > + do { > > > > + get_random_bytes(&addr, 2); > > > > + if (addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST) || > > > > + addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC)) > > > > + continue; > > > > + > > > > + if (wpan_dev->short_addr == addr) > > > > + continue; > > > > + > > > > + if (wpan_dev->parent && wpan_dev->parent->short_addr == addr) > > > > + continue; > > > > + > > > > + list_for_each_entry(child, &wpan_dev->children, node) > > > > + if (child->short_addr == addr) > > > > + continue; > > > > + > > > > + break; > > > > + } while (1); > > > > + > > > > > > I still believe that this random 2 bytes and check if it's already > > > being used is wrong here. We need something to use the next free > > > available number according to the data we are storing here. > > > > This issue I still have in mind is when you have this typology: > > > > device A -------> device B --------> device C <-------- device D > > (leaf) (coord) (PAN coord) (leaf) > > > > B associates with C > > A associates with B > > D associates with C > > > > If B and C run Linux's stack, they will always have the same short > > address. Yes this can be handled (realignment procedure). But any time > > this happens, you'll have a load of predictable realignments when A and > > D get in range with B or C. > > > > I see that it can be "more" predictable, but what happens when there > is the same short address case with the random number generator? It > sounds to me like there needs to be a kind of duplicate address > detection going on and then choose another one, if 802.15.4 even > handles this case... Yes it may happen, and yes it is handled by the spec (I did not implement it yet). When such a situation occurs (two devices using the same short address in a given PAN), the third-party device which detects the faulty situation must notify its coordinator and the coordinator (IIRC) must allocate a new short address as part of a realignment procedure. > I am also thinking that there is only one number left and the random > generator runs multiple times to find the last one aka "it's random > you can never be sure", when it always returns the same address. I'll try to summarize the two issues and solutions we have: - incremental short address: if two coordinators distribute short addresses in a PAN, at the time we perform a realignment procedure, if Coord A has allocated 1 short address (0) and Coord B in the same PAN has allocated 10 short addresses (from 0 to 9), you know that the device that needs a new address will be given 1, which will lead to a realignment, then 2, then 3... and produce a *lot* of noise. However despite being long on big network, we can assume the time to find the relevant address is bounded. - random short address: no conflict should happen following a realignment procedure (assuming regular-sized networks, probability is very low, close to 1/65000) however in case we have a huge network, the time taken to find a free slot is unbounded. At this stage I believe the former issue is more likely to happen than the second, but the second is a bit critical as it can lead to DoS situations. One easy way to mitigate this is to limit the number of devices on the network (as you said in another mail, we can refuse devices arbitrarily), which is the first denial procedure I've implemented, knowing that it should be improved as well, as that can be done later without much additional constraints. > However, that's only my thoughts about it and hopefully can be > improved in future. Yes, I am not convinced this is the perfect choice but at least it's simple enough and will work like a charm on small networks. Thanks, Miquèl
Hi Alexander, aahringo@redhat.com wrote on Tue, 26 Sep 2023 21:37:23 -0400: > Hi, > > On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > Coordinators may have to handle association requests from peers which > > want to join the PAN. The logic involves: > > - Acknowledging the request (done by hardware) > > - If requested, a random short address that is free on this PAN should > > be chosen for the device. > > - Sending an association response with the short address allocated for > > the peer and expecting it to be ack'ed. > > > > If anything fails during this procedure, the peer is considered not > > associated. > > I thought a coordinator can also reject requests for _any_ reason and > it's very user specific whatever that reason is. Absolutely. > If we have such a case (that it is very user specific what to do > exactly) this should be able to be controlled by the user space to > have there a logic to tell the kernel to accept or reject the > association. Agreed (not implemented yet, though). > However, I am fine with this solution, but I think we might want to > change this behaviour in the future so that an application in the user > space has the logic to tell the kernel to accept or reject an > association. That would make sense? Definitely, yes. Thanks, Miquèl
Hi, On Wed, Sep 27, 2023 at 11:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Tue, 26 Sep 2023 21:37:23 -0400: > > > Hi, > > > > On Fri, Sep 22, 2023 at 11:51 AM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > > > Coordinators may have to handle association requests from peers which > > > want to join the PAN. The logic involves: > > > - Acknowledging the request (done by hardware) > > > - If requested, a random short address that is free on this PAN should > > > be chosen for the device. > > > - Sending an association response with the short address allocated for > > > the peer and expecting it to be ack'ed. > > > > > > If anything fails during this procedure, the peer is considered not > > > associated. > > > > I thought a coordinator can also reject requests for _any_ reason and > > it's very user specific whatever that reason is. > > Absolutely. > > > If we have such a case (that it is very user specific what to do > > exactly) this should be able to be controlled by the user space to > > have there a logic to tell the kernel to accept or reject the > > association. > > Agreed (not implemented yet, though). > > > However, I am fine with this solution, but I think we might want to > > change this behaviour in the future so that an application in the user > > space has the logic to tell the kernel to accept or reject an > > association. That would make sense? > > Definitely, yes. ok, thanks to have some agreement here for the future. - Alex
diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h index 9b036ab20079..c844ae63bc04 100644 --- a/include/net/cfg802154.h +++ b/include/net/cfg802154.h @@ -583,4 +583,11 @@ struct ieee802154_pan_device * cfg802154_device_is_child(struct wpan_dev *wpan_dev, struct ieee802154_addr *target); +/** + * cfg802154_get_free_short_addr - Get a free address among the known devices + * @wpan_dev: the wpan device + * @return: a random short address expectedly unused on our PAN + */ +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev); + #endif /* __NET_CFG802154_H */ diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h index 16194356cfe7..4de858f9929e 100644 --- a/include/net/ieee802154_netdev.h +++ b/include/net/ieee802154_netdev.h @@ -211,6 +211,12 @@ struct ieee802154_association_req_frame { struct ieee802154_assoc_req_pl assoc_req_pl; }; +struct ieee802154_association_resp_frame { + struct ieee802154_hdr mhr; + struct ieee802154_mac_cmd_pl mac_pl; + struct ieee802154_assoc_resp_pl assoc_resp_pl; +}; + struct ieee802154_disassociation_notif_frame { struct ieee802154_hdr mhr; struct ieee802154_mac_cmd_pl mac_pl; diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c index a08d75dd56ad..1670a71327a7 100644 --- a/net/ieee802154/core.c +++ b/net/ieee802154/core.c @@ -200,11 +200,18 @@ EXPORT_SYMBOL(wpan_phy_free); static void cfg802154_free_peer_structures(struct wpan_dev *wpan_dev) { + struct ieee802154_pan_device *child, *tmp; + mutex_lock(&wpan_dev->association_lock); kfree(wpan_dev->parent); wpan_dev->parent = NULL; + list_for_each_entry_safe(child, tmp, &wpan_dev->children, node) { + list_del(&child->node); + kfree(child); + } + mutex_unlock(&wpan_dev->association_lock); } diff --git a/net/ieee802154/pan.c b/net/ieee802154/pan.c index 9e1f1973c294..e99c64054dcb 100644 --- a/net/ieee802154/pan.c +++ b/net/ieee802154/pan.c @@ -73,3 +73,33 @@ cfg802154_device_is_child(struct wpan_dev *wpan_dev, return NULL; } EXPORT_SYMBOL_GPL(cfg802154_device_is_child); + +__le16 cfg802154_get_free_short_addr(struct wpan_dev *wpan_dev) +{ + struct ieee802154_pan_device *child; + __le16 addr; + + lockdep_assert_held(&wpan_dev->association_lock); + + do { + get_random_bytes(&addr, 2); + if (addr == cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST) || + addr == cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC)) + continue; + + if (wpan_dev->short_addr == addr) + continue; + + if (wpan_dev->parent && wpan_dev->parent->short_addr == addr) + continue; + + list_for_each_entry(child, &wpan_dev->children, node) + if (child->short_addr == addr) + continue; + + break; + } while (1); + + return addr; +} +EXPORT_SYMBOL_GPL(cfg802154_get_free_short_addr); diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h index 92252f86c69c..432bfa87249e 100644 --- a/net/mac802154/ieee802154_i.h +++ b/net/mac802154/ieee802154_i.h @@ -318,6 +318,8 @@ static inline bool mac802154_is_associating(struct ieee802154_local *local) int mac802154_send_disassociation_notif(struct ieee802154_sub_if_data *sdata, struct ieee802154_pan_device *target, u8 reason); +int mac802154_process_association_req(struct ieee802154_sub_if_data *sdata, + struct sk_buff *skb); /* interface handling */ int ieee802154_iface_init(void); diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c index d0e08613a36b..96040b63a4fc 100644 --- a/net/mac802154/rx.c +++ b/net/mac802154/rx.c @@ -102,6 +102,14 @@ void mac802154_rx_mac_cmd_worker(struct work_struct *work) mac802154_process_association_resp(mac_pkt->sdata, mac_pkt->skb); break; + case IEEE802154_CMD_ASSOCIATION_REQ: + dev_dbg(&mac_pkt->sdata->dev->dev, "processing ASSOC REQ\n"); + if (mac_pkt->sdata->wpan_dev.iftype != NL802154_IFTYPE_COORD) + break; + + mac802154_process_association_req(mac_pkt->sdata, mac_pkt->skb); + break; + default: break; } diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c index e2f2e1235ec6..d5f66c204bc5 100644 --- a/net/mac802154/scan.c +++ b/net/mac802154/scan.c @@ -697,3 +697,145 @@ int mac802154_send_disassociation_notif(struct ieee802154_sub_if_data *sdata, dev_dbg(&sdata->dev->dev, "DISASSOC ACK received from %8phC\n", &teaddr); return 0; } + +static int +mac802154_send_association_resp_locked(struct ieee802154_sub_if_data *sdata, + struct ieee802154_pan_device *target, + struct ieee802154_assoc_resp_pl *assoc_resp_pl) +{ + u64 teaddr = swab64((__force u64)target->extended_addr); + struct ieee802154_association_resp_frame frame = {}; + struct ieee802154_local *local = sdata->local; + struct wpan_dev *wpan_dev = &sdata->wpan_dev; + struct sk_buff *skb; + int ret; + + frame.mhr.fc.type = IEEE802154_FC_TYPE_MAC_CMD; + frame.mhr.fc.security_enabled = 0; + frame.mhr.fc.frame_pending = 0; + frame.mhr.fc.ack_request = 1; /* We always expect an ack here */ + frame.mhr.fc.intra_pan = 1; + frame.mhr.fc.dest_addr_mode = IEEE802154_EXTENDED_ADDRESSING; + frame.mhr.fc.version = IEEE802154_2003_STD; + frame.mhr.fc.source_addr_mode = IEEE802154_EXTENDED_ADDRESSING; + frame.mhr.source.mode = IEEE802154_ADDR_LONG; + frame.mhr.source.extended_addr = wpan_dev->extended_addr; + frame.mhr.dest.mode = IEEE802154_ADDR_LONG; + frame.mhr.dest.pan_id = wpan_dev->pan_id; + frame.mhr.dest.extended_addr = target->extended_addr; + frame.mhr.seq = atomic_inc_return(&wpan_dev->dsn) & 0xFF; + frame.mac_pl.cmd_id = IEEE802154_CMD_ASSOCIATION_RESP; + + skb = alloc_skb(IEEE802154_MAC_CMD_SKB_SZ + sizeof(*assoc_resp_pl), + GFP_KERNEL); + if (!skb) + return -ENOBUFS; + + skb->dev = sdata->dev; + + ret = ieee802154_mac_cmd_push(skb, &frame, assoc_resp_pl, + sizeof(*assoc_resp_pl)); + if (ret) { + kfree_skb(skb); + return ret; + } + + ret = ieee802154_mlme_tx_locked(local, sdata, skb); + if (ret) { + dev_warn(&sdata->dev->dev, + "No ASSOC RESP ACK received from %8phC\n", &teaddr); + if (ret > 0) + ret = (ret == IEEE802154_NO_ACK) ? -EREMOTEIO : -EIO; + return ret; + } + + return 0; +} + +int mac802154_process_association_req(struct ieee802154_sub_if_data *sdata, + struct sk_buff *skb) +{ + struct wpan_dev *wpan_dev = &sdata->wpan_dev; + struct ieee802154_addr *src = &mac_cb(skb)->source; + struct ieee802154_addr *dest = &mac_cb(skb)->dest; + struct ieee802154_assoc_resp_pl assoc_resp_pl = {}; + struct ieee802154_assoc_req_pl assoc_req_pl; + struct ieee802154_pan_device *child, *exchild; + struct ieee802154_addr tmp = {}; + u64 ceaddr; + int ret; + + if (skb->len != sizeof(assoc_req_pl)) + return -EINVAL; + + if (unlikely(src->mode != IEEE802154_EXTENDED_ADDRESSING)) + return -EINVAL; + + if (unlikely(dest->pan_id != wpan_dev->pan_id)) + return -ENODEV; + + if (dest->mode == IEEE802154_EXTENDED_ADDRESSING && + unlikely(dest->extended_addr != wpan_dev->extended_addr)) + return -ENODEV; + else if (dest->mode == IEEE802154_SHORT_ADDRESSING && + unlikely(dest->short_addr != wpan_dev->short_addr)) + return -ENODEV; + + mutex_lock(&wpan_dev->association_lock); + + memcpy(&assoc_req_pl, skb->data, sizeof(assoc_req_pl)); + if (assoc_req_pl.assoc_type) { + dev_err(&skb->dev->dev, "Fast associations not supported yet\n"); + ret = -EOPNOTSUPP; + goto unlock; + } + + child = kzalloc(sizeof(*child), GFP_KERNEL); + if (!child) { + ret = -ENOMEM; + goto unlock; + } + + child->extended_addr = src->extended_addr; + child->mode = IEEE802154_EXTENDED_ADDRESSING; + ceaddr = swab64((__force u64)child->extended_addr); + + assoc_resp_pl.status = IEEE802154_ASSOCIATION_SUCCESSFUL; + if (assoc_req_pl.alloc_addr) { + assoc_resp_pl.short_addr = cfg802154_get_free_short_addr(wpan_dev); + child->mode = IEEE802154_SHORT_ADDRESSING; + } else { + assoc_resp_pl.short_addr = cpu_to_le16(IEEE802154_ADDR_SHORT_UNSPEC); + } + child->short_addr = assoc_resp_pl.short_addr; + dev_dbg(&sdata->dev->dev, + "Accepting ASSOC REQ from child %8phC, providing short address 0x%04x\n", + &ceaddr, le16_to_cpu(child->short_addr)); + + ret = mac802154_send_association_resp_locked(sdata, child, &assoc_resp_pl); + if (ret) { + kfree(child); + goto unlock; + } + + dev_dbg(&sdata->dev->dev, + "Successful association with new child %8phC\n", &ceaddr); + + /* Ensure this child is not already associated (might happen due to + * retransmissions), in this case drop the ex structure. + */ + tmp.mode = child->mode; + tmp.extended_addr = child->extended_addr; + exchild = cfg802154_device_is_child(wpan_dev, &tmp); + if (exchild) { + dev_dbg(&sdata->dev->dev, + "Child %8phC was already known\n", &ceaddr); + list_del(&exchild->node); + } + + list_add(&child->node, &wpan_dev->children); + +unlock: + mutex_unlock(&wpan_dev->association_lock); + return ret; +}
Coordinators may have to handle association requests from peers which want to join the PAN. The logic involves: - Acknowledging the request (done by hardware) - If requested, a random short address that is free on this PAN should be chosen for the device. - Sending an association response with the short address allocated for the peer and expecting it to be ack'ed. If anything fails during this procedure, the peer is considered not associated. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/net/cfg802154.h | 7 ++ include/net/ieee802154_netdev.h | 6 ++ net/ieee802154/core.c | 7 ++ net/ieee802154/pan.c | 30 +++++++ net/mac802154/ieee802154_i.h | 2 + net/mac802154/rx.c | 8 ++ net/mac802154/scan.c | 142 ++++++++++++++++++++++++++++++++ 7 files changed, 202 insertions(+)