| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK-6q+hoJiLWyHNi90_7kbyGp9h_jV-bvRHYRQDVrEb1u_enEA@mail.gmail.com>
Date: Tue, 18 Oct 2022 19:57:19 -0400
From: Alexander Aring <aahringo@...hat.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Alexander Aring <alex.aring@...il.com>,
Stefan Schmidt <stefan@...enfreihafen.org>,
linux-wpan@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
David Girault <david.girault@...vo.com>,
Romuald Despres <romuald.despres@...vo.com>,
Frederic Blain <frederic.blain@...vo.com>,
Nicolas Schodet <nico@...fr.eu.org>,
Guilhem Imberton <guilhem.imberton@...vo.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next] mac802154: Allow the creation of coordinator interfaces
Hi,
On Tue, Oct 18, 2022 at 2:36 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> As a first strep in introducing proper PAN management and association,
> we need to be able to create coordinator interfaces which might act as
> coordinator or PAN coordinator.
>
> Hence, let's add the minimum support to allow the creation of these
> interfaces. This support will be improved later, in particular regarding
> the filtering.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
> ---
> net/mac802154/iface.c | 14 ++++++++------
> net/mac802154/main.c | 2 ++
> net/mac802154/rx.c | 11 +++++++----
> 3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index d9b50884d34e..682249f3369b 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -262,13 +262,13 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
> if (nsdata != sdata && ieee802154_sdata_running(nsdata)) {
> int ret;
>
> - /* TODO currently we don't support multiple node types
> - * we need to run skb_clone at rx path. Check if there
> - * exist really an use case if we need to support
> - * multiple node types at the same time.
> + /* TODO currently we don't support multiple node/coord
> + * types we need to run skb_clone at rx path. Check if
> + * there exist really an use case if we need to support
> + * multiple node/coord types at the same time.
> */
> - if (wpan_dev->iftype == NL802154_IFTYPE_NODE &&
> - nsdata->wpan_dev.iftype == NL802154_IFTYPE_NODE)
> + if (wpan_dev->iftype != NL802154_IFTYPE_MONITOR &&
> + nsdata->wpan_dev.iftype != NL802154_IFTYPE_MONITOR)
> return -EBUSY;
>
> /* check all phy mac sublayer settings are the same.
> @@ -565,6 +565,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
> wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
>
> switch (type) {
> + case NL802154_IFTYPE_COORD:
> case NL802154_IFTYPE_NODE:
> ieee802154_be64_to_le64(&wpan_dev->extended_addr,
> sdata->dev->dev_addr);
> @@ -624,6 +625,7 @@ ieee802154_if_add(struct ieee802154_local *local, const char *name,
> ieee802154_le64_to_be64(ndev->perm_addr,
> &local->hw.phy->perm_extended_addr);
> switch (type) {
> + case NL802154_IFTYPE_COORD:
> case NL802154_IFTYPE_NODE:
> ndev->type = ARPHRD_IEEE802154;
> if (ieee802154_is_valid_extended_unicast_addr(extended_addr)) {
> diff --git a/net/mac802154/main.c b/net/mac802154/main.c
> index 40fab08df24b..d03ecb747afc 100644
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -219,6 +219,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw)
>
> if (hw->flags & IEEE802154_HW_PROMISCUOUS)
> local->phy->supported.iftypes |= BIT(NL802154_IFTYPE_MONITOR);
> + else
> + local->phy->supported.iftypes &= ~BIT(NL802154_IFTYPE_COORD);
>
So this means if somebody in the driver sets iftype COORD is supported
but then IEEE802154_HW_PROMISCUOUS is not supported it will not
support COORD?
Why is IEEE802154_HW_PROMISCUOUS required for COORD iftype? I thought
IEEE802154_HW_PROMISCUOUS is required to do a scan?
> rc = wpan_phy_register(local->phy);
> if (rc < 0)
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 2ae23a2f4a09..aca348d7834b 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -208,6 +208,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> int ret;
> struct ieee802154_sub_if_data *sdata;
> struct ieee802154_hdr hdr;
> + struct sk_buff *skb2;
>
> ret = ieee802154_parse_frame_start(skb, &hdr);
> if (ret) {
> @@ -217,7 +218,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> }
>
> list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> - if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
> + if (sdata->wpan_dev.iftype == NL802154_IFTYPE_MONITOR)
> continue;
I guess this will work but I would like to see no logic about if it's
not MONITOR it's NODE or COORD, because introducing others requires
updating those again... however I think it's fine. I would like to see
a different receive path for coord_rx() and node_rx(), but yea
currently I guess they are mostly the same... in future I think they
are required as PACKTE_HOST, etc. will be changed regarding pan
coordinator or just coordinator (so far I understood).
>
> if (!ieee802154_sdata_running(sdata))
> @@ -230,9 +231,11 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
> sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS)
> continue;
>
> - ieee802154_subif_frame(sdata, skb, &hdr);
> - skb = NULL;
> - break;
> + skb2 = skb_clone(skb, GFP_ATOMIC);
> + if (skb2) {
> + skb2->dev = sdata->dev;
> + ieee802154_subif_frame(sdata, skb2, &hdr);
> + }
> }
>
> kfree_skb(skb);
If we do the clone above this kfree_skb() should be move to
ieee802154_rx() right after __ieee802154_rx_handle_packet(). This
patch also changes that we deliver one skb to multiple interfaces if
there are more than one and I was not aware that we currently do that.
:/
- Alex
Powered by blists - more mailing lists