[PATCH wpan/next v4 0/8] net: ieee802154: Improve filtering support

[PATCH wpan/next v4 0/8] net: ieee802154: Improve filtering support

[Miquel Raynal]

Hello,

A fourth version of this series, where we try to improve filtering
support to ease scan integration. Will then come a short series about
the coordinator interfaces and then the proper scan series.

Thanks,
Miquèl

Changes in v4:
* Added a condition upon which the packets for a given interface would be
  dropped: in case AACK and/or address filtering was expected, but
  another interface has disabled it on the PHY.
* Changed the way Alexander's patch behaves regarding the handling of
  the different filtering levels. I added a third variable which shows
  the default filtering level for the interface. There is a second
  (per-interface) field giving the expected filtering level for this
  interface and finally we keep the per-PHY actual filtering level
  information. With this we can safely go back to the right level after
  a scan and also we can detect any wrong situation where ACKs would not
  be sent while expected and drop the frames if in this situation.
* Moved all the additional filtering logic out of the core and put it
  into hwsim's in-driver receive path, so that it can act like any other
  transceiver depending on the filtering level requested.
* Dropped the addition of the support for the ieee802154 promiscuous
  filtering mode which is anyway not usable yet.
* Dropped the "net:" prefixes in many patches to fit what Alexander
  does.

Changes in v3:
* Full rework of the way the promiscuous mode is handled, with new
  filtering levels, one per-phy and the actual one on the device.
* Dropped all the manual acking, everything is happenging on hardware.
* Better handling of the Acks in atusb to report the trac status.



Alexander Aring (2):
  mac802154: move receive parameters above start
  mac802154: set filter at drv_start()

Miquel Raynal (6):
  mac802154: Introduce filtering levels
  ieee802154: hwsim: Record the address filter values
  ieee802154: hwsim: Implement address filtering
  mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM
  mac802154: Avoid delivering frames received in a non satisfying
    filtering mode
  mac802154: Ensure proper scan-level filtering

 drivers/net/ieee802154/mac802154_hwsim.c | 150 +++++++++++-
 include/linux/ieee802154.h               |  24 ++
 include/net/cfg802154.h                  |   7 +-
 include/net/ieee802154_netdev.h          |   8 +
 include/net/mac802154.h                  |   4 -
 net/mac802154/cfg.c                      |   2 +-
 net/mac802154/driver-ops.h               | 281 ++++++++++++++---------
 net/mac802154/ieee802154_i.h             |  12 +
 net/mac802154/iface.c                    |  44 ++--
 net/mac802154/rx.c                       |  25 +-
 10 files changed, 409 insertions(+), 148 deletions(-)

-- 
2.34.1

[PATCH wpan/next v4 1/8] mac802154: Introduce filtering levels

[Miquel Raynal]

The 802154 specification details several filtering levels in which the
PHY and the MAC could be. The amount of filtering will vary if they are
in promiscuous mode or in scanning mode. Otherwise they are expected to
do some very basic checks, such as enforcing the frame validity. Either
the PHY is able to do so, and the MAC has nothing to do, or the PHY has
a lower filtering level than expected and the MAC should take over.

For now we just define these levels in an enumeration.

In a second time, we will add a per-PHY parameter showing the expected
filtering level as well as a per device current filtering level, and
will initialize all these fields.

In a third time, we will use them to apply more filtering by software
when the PHY is limited.

Indeed, if the drivers know they cannot reach the requested level of
filtering, they will overwrite the "current filtering" parameter so that
it reflects what they do. Then, in the core, the expected filtering
level will be used to decide whether some additional software processing
is needed or not.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/ieee802154.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
index f1f9412b6ac6..0303eb84d596 100644
--- a/include/linux/ieee802154.h
+++ b/include/linux/ieee802154.h
@@ -276,6 +276,30 @@ enum {
 	IEEE802154_SYSTEM_ERROR = 0xff,
 };
 
+/**
+ * enum ieee802154_filtering_level - Filtering levels applicable to a PHY
+ *
+ * @IEEE802154_FILTERING_NONE: No filtering at all, what is received is
+ *	forwarded to the softMAC
+ * @IEEE802154_FILTERING_1_FCS: First filtering level, frames with an invalid
+ *	FCS should be dropped
+ * @IEEE802154_FILTERING_2_PROMISCUOUS: Second filtering level, promiscuous
+ *	mode as described in the spec, identical in terms of filtering to the
+ *	level one on PHY side, but at the MAC level the frame should be
+ *	forwarded to the upper layer directly
+ * @IEEE802154_FILTERING_3_SCAN: Third filtering level, scan related, where
+ *	only beacons must be processed, all remaining traffic gets dropped
+ * @IEEE802154_FILTERING_4_FRAME_FIELDS: Fourth filtering level actually
+ *	enforcing the validity of the content of the frame with various checks
+ */
+enum ieee802154_filtering_level {
+	IEEE802154_FILTERING_NONE,
+	IEEE802154_FILTERING_1_FCS,
+	IEEE802154_FILTERING_2_PROMISCUOUS,
+	IEEE802154_FILTERING_3_SCAN,
+	IEEE802154_FILTERING_4_FRAME_FIELDS,
+};
+
 /* frame control handling */
 #define IEEE802154_FCTL_FTYPE		0x0003
 #define IEEE802154_FCTL_ACKREQ		0x0020
-- 
2.34.1

[PATCH wpan/next v4 2/8] mac802154: move receive parameters above start

[Miquel Raynal]

From: Alexander Aring <aahringo@redhat.com>

This patch moves all receive parameters above the drv_start()
functionality to make it accessibile in the drv_start() function.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/driver-ops.h | 210 ++++++++++++++++++-------------------
 1 file changed, 105 insertions(+), 105 deletions(-)

diff --git a/net/mac802154/driver-ops.h b/net/mac802154/driver-ops.h
index d23f0db98015..c9d54088a567 100644
--- a/net/mac802154/driver-ops.h
+++ b/net/mac802154/driver-ops.h
@@ -24,6 +24,111 @@ drv_xmit_sync(struct ieee802154_local *local, struct sk_buff *skb)
 	return local->ops->xmit_sync(&local->hw, skb);
 }
 
+static inline int drv_set_pan_id(struct ieee802154_local *local, __le16 pan_id)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.pan_id = pan_id;
+
+	trace_802154_drv_set_pan_id(local, pan_id);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_PANID_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_extended_addr(struct ieee802154_local *local, __le64 extended_addr)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.ieee_addr = extended_addr;
+
+	trace_802154_drv_set_extended_addr(local, extended_addr);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_IEEEADDR_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_short_addr(struct ieee802154_local *local, __le16 short_addr)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.short_addr = short_addr;
+
+	trace_802154_drv_set_short_addr(local, short_addr);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_SADDR_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_pan_coord(struct ieee802154_local *local, bool is_coord)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.pan_coord = is_coord;
+
+	trace_802154_drv_set_pan_coord(local, is_coord);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_PANC_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_promiscuous_mode(struct ieee802154_local *local, bool on)
+{
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_promiscuous_mode) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	trace_802154_drv_set_promiscuous_mode(local, on);
+	ret = local->ops->set_promiscuous_mode(&local->hw, on);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
 static inline int drv_start(struct ieee802154_local *local)
 {
 	int ret;
@@ -138,93 +243,6 @@ drv_set_cca_ed_level(struct ieee802154_local *local, s32 mbm)
 	return ret;
 }
 
-static inline int drv_set_pan_id(struct ieee802154_local *local, __le16 pan_id)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.pan_id = pan_id;
-
-	trace_802154_drv_set_pan_id(local, pan_id);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_PANID_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
-static inline int
-drv_set_extended_addr(struct ieee802154_local *local, __le64 extended_addr)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.ieee_addr = extended_addr;
-
-	trace_802154_drv_set_extended_addr(local, extended_addr);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_IEEEADDR_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
-static inline int
-drv_set_short_addr(struct ieee802154_local *local, __le16 short_addr)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.short_addr = short_addr;
-
-	trace_802154_drv_set_short_addr(local, short_addr);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_SADDR_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
-static inline int
-drv_set_pan_coord(struct ieee802154_local *local, bool is_coord)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.pan_coord = is_coord;
-
-	trace_802154_drv_set_pan_coord(local, is_coord);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_PANC_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
 static inline int
 drv_set_csma_params(struct ieee802154_local *local, u8 min_be, u8 max_be,
 		    u8 max_csma_backoffs)
@@ -264,22 +282,4 @@ drv_set_max_frame_retries(struct ieee802154_local *local, s8 max_frame_retries)
 	return ret;
 }
 
-static inline int
-drv_set_promiscuous_mode(struct ieee802154_local *local, bool on)
-{
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_promiscuous_mode) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	trace_802154_drv_set_promiscuous_mode(local, on);
-	ret = local->ops->set_promiscuous_mode(&local->hw, on);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
 #endif /* __MAC802154_DRIVER_OPS */
-- 
2.34.1

[PATCH wpan/next v4 3/8] mac802154: set filter at drv_start()

[Miquel Raynal]

From: Alexander Aring <aahringo@redhat.com>

The current filtering level is set on the first interface up on a wpan
phy. If we support scan functionality we need to change the filtering
level on the fly on an operational phy and switching back again.

This patch will move the receive mode parameter e.g. address filter and
promiscuous mode to the drv_start() functionality to allow changing the
receive mode on an operational phy not on first ifup only. In future this
should be handled on driver layer because each hardware has it's own way
to enter a specific filtering level. However this should offer to switch
to mode IEEE802154_FILTERING_NONE and back to
IEEE802154_FILTERING_4_FRAME_FIELDS.

Only IEEE802154_FILTERING_4_FRAME_FIELDS and IEEE802154_FILTERING_NONE
are somewhat supported by current hardware. All other filtering levels
can be supported in future but will end in IEEE802154_FILTERING_NONE as
the receive part can kind of "emulate" those receive paths by doing
additional filtering routines.

There are in total three filtering levels in the code:
- the per-interface default level (should not be changed)
- the required per-interface level (mac commands may play with it)
- the actual per-PHY (hw) level that is currently in use

Signed-off-by: Alexander Aring <aahringo@redhat.com>
[<miquel.raynal@bootlin.com: Add the third filtering variable]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h      |  7 +++-
 net/mac802154/cfg.c          |  2 +-
 net/mac802154/driver-ops.h   | 71 +++++++++++++++++++++++++++++++++++-
 net/mac802154/ieee802154_i.h | 12 ++++++
 net/mac802154/iface.c        | 44 ++++++++--------------
 net/mac802154/rx.c           | 12 +++++-
 6 files changed, 115 insertions(+), 33 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 428cece22205..e1481f9cf049 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -223,6 +223,11 @@ struct wpan_phy {
 	atomic_t hold_txs;
 	wait_queue_head_t sync_txq;
 
+	/* Current filtering level on reception.
+	 * Only allowed to be changed if phy is not operational.
+	 */
+	enum ieee802154_filtering_level filtering;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
@@ -374,8 +379,6 @@ struct wpan_dev {
 
 	bool lbt;
 
-	bool promiscuous_mode;
-
 	/* fallback for acknowledgment bit setting */
 	bool ackreq;
 };
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 93df24f75572..dc2d918fac68 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -67,7 +67,7 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy)
 		goto wake_up;
 
 	/* restart hardware */
-	ret = drv_start(local);
+	ret = drv_start(local, local->phy->filtering, &local->addr_filt);
 	if (ret)
 		return ret;
 
diff --git a/net/mac802154/driver-ops.h b/net/mac802154/driver-ops.h
index c9d54088a567..a7af3f0ddb3e 100644
--- a/net/mac802154/driver-ops.h
+++ b/net/mac802154/driver-ops.h
@@ -129,12 +129,81 @@ drv_set_promiscuous_mode(struct ieee802154_local *local, bool on)
 	return ret;
 }
 
-static inline int drv_start(struct ieee802154_local *local)
+static inline int drv_start(struct ieee802154_local *local,
+			    enum ieee802154_filtering_level level,
+			    const struct ieee802154_hw_addr_filt *addr_filt)
 {
 	int ret;
 
 	might_sleep();
 
+	/* setup receive mode parameters e.g. address mode */
+	if (local->hw.flags & IEEE802154_HW_AFILT) {
+		ret = drv_set_pan_id(local, addr_filt->pan_id);
+		if (ret < 0)
+			return ret;
+
+		ret = drv_set_short_addr(local, addr_filt->short_addr);
+		if (ret < 0)
+			return ret;
+
+		ret = drv_set_extended_addr(local, addr_filt->ieee_addr);
+		if (ret < 0)
+			return ret;
+	}
+
+	switch (level) {
+	case IEEE802154_FILTERING_NONE:
+		fallthrough;
+	case IEEE802154_FILTERING_1_FCS:
+		fallthrough;
+	case IEEE802154_FILTERING_2_PROMISCUOUS:
+		/* TODO: Requires a different receive mode setup e.g.
+		 * at86rf233 hardware.
+		 */
+		fallthrough;
+	case IEEE802154_FILTERING_3_SCAN:
+		if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
+			ret = drv_set_promiscuous_mode(local, true);
+			if (ret < 0)
+				return ret;
+		} else {
+			return -EOPNOTSUPP;
+		}
+
+		/* In practice other filtering levels can be requested, but as
+		 * for now most hardware/drivers only support
+		 * IEEE802154_FILTERING_NONE, we fallback to this actual
+		 * filtering level in hardware and make our own additional
+		 * filtering in mac802154 receive path.
+		 *
+		 * TODO: Move this logic to the device drivers as hardware may
+		 * support more higher level filters. Hardware may also require
+		 * a different order how register are set, which could currently
+		 * be buggy, so all received parameters need to be moved to the
+		 * start() callback and let the driver go into the mode before
+		 * it will turn on receive handling.
+		 */
+		local->phy->filtering = IEEE802154_FILTERING_NONE;
+		break;
+	case IEEE802154_FILTERING_4_FRAME_FIELDS:
+		/* Do not error out if IEEE802154_HW_PROMISCUOUS because we
+		 * expect the hardware to operate at the level
+		 * IEEE802154_FILTERING_4_FRAME_FIELDS anyway.
+		 */
+		if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
+			ret = drv_set_promiscuous_mode(local, false);
+			if (ret < 0)
+				return ret;
+		}
+
+		local->phy->filtering = IEEE802154_FILTERING_4_FRAME_FIELDS;
+		break;
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
 	trace_802154_drv_start(local);
 	local->started = true;
 	smp_mb();
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 010365a6364e..509e0172fe82 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -26,6 +26,8 @@ struct ieee802154_local {
 	struct ieee802154_hw hw;
 	const struct ieee802154_ops *ops;
 
+	/* hardware address filter */
+	struct ieee802154_hw_addr_filt addr_filt;
 	/* ieee802154 phy */
 	struct wpan_phy *phy;
 
@@ -82,6 +84,16 @@ struct ieee802154_sub_if_data {
 	struct ieee802154_local *local;
 	struct net_device *dev;
 
+	/* Each interface starts and works in nominal state at a given filtering
+	 * level given by iface_default_filtering, which is set once for all at
+	 * the interface creation and should not evolve over time. For some MAC
+	 * operations however, the filtering level may change temporarily, as
+	 * reflected in the required_filtering field. The actual filtering at
+	 * the PHY level may be different and is shown in struct wpan_phy.
+	 */
+	enum ieee802154_filtering_level iface_default_filtering;
+	enum ieee802154_filtering_level required_filtering;
+
 	unsigned long state;
 	char name[IFNAMSIZ];
 
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 500ed1b81250..d9b50884d34e 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -147,25 +147,12 @@ static int ieee802154_setup_hw(struct ieee802154_sub_if_data *sdata)
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
 	int ret;
 
-	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
-		ret = drv_set_promiscuous_mode(local,
-					       wpan_dev->promiscuous_mode);
-		if (ret < 0)
-			return ret;
-	}
+	sdata->required_filtering = sdata->iface_default_filtering;
 
 	if (local->hw.flags & IEEE802154_HW_AFILT) {
-		ret = drv_set_pan_id(local, wpan_dev->pan_id);
-		if (ret < 0)
-			return ret;
-
-		ret = drv_set_extended_addr(local, wpan_dev->extended_addr);
-		if (ret < 0)
-			return ret;
-
-		ret = drv_set_short_addr(local, wpan_dev->short_addr);
-		if (ret < 0)
-			return ret;
+		local->addr_filt.pan_id = wpan_dev->pan_id;
+		local->addr_filt.ieee_addr = wpan_dev->extended_addr;
+		local->addr_filt.short_addr = wpan_dev->short_addr;
 	}
 
 	if (local->hw.flags & IEEE802154_HW_LBT) {
@@ -206,7 +193,8 @@ static int mac802154_slave_open(struct net_device *dev)
 		if (res)
 			goto err;
 
-		res = drv_start(local);
+		res = drv_start(local, sdata->required_filtering,
+				&local->addr_filt);
 		if (res)
 			goto err;
 	}
@@ -223,15 +211,16 @@ static int mac802154_slave_open(struct net_device *dev)
 
 static int
 ieee802154_check_mac_settings(struct ieee802154_local *local,
-			      struct wpan_dev *wpan_dev,
-			      struct wpan_dev *nwpan_dev)
+			      struct ieee802154_sub_if_data *sdata,
+			      struct ieee802154_sub_if_data *nsdata)
 {
+	struct wpan_dev *nwpan_dev = &nsdata->wpan_dev;
+	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+
 	ASSERT_RTNL();
 
-	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
-		if (wpan_dev->promiscuous_mode != nwpan_dev->promiscuous_mode)
-			return -EBUSY;
-	}
+	if (sdata->iface_default_filtering != nsdata->iface_default_filtering)
+		return -EBUSY;
 
 	if (local->hw.flags & IEEE802154_HW_AFILT) {
 		if (wpan_dev->pan_id != nwpan_dev->pan_id ||
@@ -285,8 +274,7 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
 			/* check all phy mac sublayer settings are the same.
 			 * We have only one phy, different values makes trouble.
 			 */
-			ret = ieee802154_check_mac_settings(local, wpan_dev,
-							    &nsdata->wpan_dev);
+			ret = ieee802154_check_mac_settings(local, sdata, nsdata);
 			if (ret < 0)
 				return ret;
 		}
@@ -586,7 +574,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 		sdata->dev->priv_destructor = mac802154_wpan_free;
 		sdata->dev->netdev_ops = &mac802154_wpan_ops;
 		sdata->dev->ml_priv = &mac802154_mlme_wpan;
-		wpan_dev->promiscuous_mode = false;
+		sdata->iface_default_filtering = IEEE802154_FILTERING_4_FRAME_FIELDS;
 		wpan_dev->header_ops = &ieee802154_header_ops;
 
 		mutex_init(&sdata->sec_mtx);
@@ -600,7 +588,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 	case NL802154_IFTYPE_MONITOR:
 		sdata->dev->needs_free_netdev = true;
 		sdata->dev->netdev_ops = &mac802154_monitor_ops;
-		wpan_dev->promiscuous_mode = true;
+		sdata->iface_default_filtering = IEEE802154_FILTERING_NONE;
 		break;
 	default:
 		BUG();
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index c0fd8d0c7f03..b68d62335f66 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -270,10 +270,20 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 
 	ieee802154_monitors_rx(local, skb);
 
+	/* TODO: Avoid delivering frames received at the level
+	 * IEEE802154_FILTERING_NONE on interfaces not expecting it because of
+	 * the missing auto ACK handling feature.
+	 */
+
+	/* TODO: Handle upcomming receive path where the PHY is at the
+	 * IEEE802154_FILTERING_NONE level during a scan.
+	 */
+
 	/* Check if transceiver doesn't validate the checksum.
 	 * If not we validate the checksum here.
 	 */
-	if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM) {
+	if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM ||
+	    local->phy->filtering == IEEE802154_FILTERING_NONE) {
 		crc = crc_ccitt(0, skb->data, skb->len);
 		if (crc) {
 			rcu_read_unlock();
-- 
2.34.1

[PATCH wpan/next v4 4/8] ieee802154: hwsim: Record the address filter values

[Miquel Raynal]

As a first step, introduce a basic implementation for the
->set_hw_addr_filt() hook. In a second step, the values recorded here
will be used to perform proper filtering during reception.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mac802154_hwsim.c | 36 ++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 38c217bd7c82..458be66b5195 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -47,6 +47,7 @@ static const struct genl_multicast_group hwsim_mcgrps[] = {
 struct hwsim_pib {
 	u8 page;
 	u8 channel;
+	struct ieee802154_hw_addr_filt filt;
 
 	struct rcu_head rcu;
 };
@@ -101,6 +102,38 @@ static int hwsim_hw_channel(struct ieee802154_hw *hw, u8 page, u8 channel)
 	pib->channel = channel;
 
 	pib_old = rtnl_dereference(phy->pib);
+
+	pib->filt.short_addr = pib_old->filt.short_addr;
+	pib->filt.pan_id = pib_old->filt.pan_id;
+	pib->filt.ieee_addr = pib_old->filt.ieee_addr;
+	pib->filt.pan_coord = pib_old->filt.pan_coord;
+
+	rcu_assign_pointer(phy->pib, pib);
+	kfree_rcu(pib_old, rcu);
+	return 0;
+}
+
+static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
+			      struct ieee802154_hw_addr_filt *filt,
+			      unsigned long changed)
+{
+	struct hwsim_phy *phy = hw->priv;
+	struct hwsim_pib *pib, *pib_old;
+
+	pib = kzalloc(sizeof(*pib), GFP_KERNEL);
+	if (!pib)
+		return -ENOMEM;
+
+	pib_old = rtnl_dereference(phy->pib);
+
+	pib->page = pib_old->page;
+	pib->channel = pib_old->channel;
+
+	pib->filt.short_addr = filt->short_addr;
+	pib->filt.pan_id = filt->pan_id;
+	pib->filt.ieee_addr = filt->ieee_addr;
+	pib->filt.pan_coord = filt->pan_coord;
+
 	rcu_assign_pointer(phy->pib, pib);
 	kfree_rcu(pib_old, rcu);
 	return 0;
@@ -172,6 +205,7 @@ static const struct ieee802154_ops hwsim_ops = {
 	.start = hwsim_hw_start,
 	.stop = hwsim_hw_stop,
 	.set_promiscuous_mode = hwsim_set_promiscuous_mode,
+	.set_hw_addr_filt = hwsim_hw_addr_filt,
 };
 
 static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
@@ -787,6 +821,8 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
 	}
 
 	pib->channel = 13;
+	pib->filt.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
+	pib->filt.pan_id = cpu_to_le16(IEEE802154_PANID_BROADCAST);
 	rcu_assign_pointer(phy->pib, pib);
 	phy->idx = idx;
 	INIT_LIST_HEAD(&phy->edges);
-- 
2.34.1

[PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Miquel Raynal]

We have access to the address filters being theoretically applied, we
also have access to the actual filtering level applied, so let's add a
proper frame validation sequence in hwsim.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
 include/net/ieee802154_netdev.h          |   8 ++
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 458be66b5195..84ee948f35bc 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -18,6 +18,7 @@
 #include <linux/netdevice.h>
 #include <linux/device.h>
 #include <linux/spinlock.h>
+#include <net/ieee802154_netdev.h>
 #include <net/mac802154.h>
 #include <net/cfg802154.h>
 #include <net/genetlink.h>
@@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
 	return 0;
 }
 
+static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
+			     u8 lqi)
+{
+	struct ieee802154_hdr hdr;
+	struct hwsim_phy *phy = hw->priv;
+	struct hwsim_pib *pib;
+
+	rcu_read_lock();
+	pib = rcu_dereference(phy->pib);
+
+	if (!pskb_may_pull(skb, 3)) {
+		dev_dbg(hw->parent, "invalid frame\n");
+		goto drop;
+	}
+
+	memcpy(&hdr, skb->data, 3);
+
+	/* Level 4 filtering: Frame fields validity */
+	if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
+
+		/* a) Drop reserved frame types */
+		switch (mac_cb(skb)->type) {
+		case IEEE802154_FC_TYPE_BEACON:
+		case IEEE802154_FC_TYPE_DATA:
+		case IEEE802154_FC_TYPE_ACK:
+		case IEEE802154_FC_TYPE_MAC_CMD:
+			break;
+		default:
+			dev_dbg(hw->parent, "unrecognized frame type 0x%x\n",
+				mac_cb(skb)->type);
+			goto drop;
+		}
+
+		/* b) Drop reserved frame versions */
+		switch (hdr.fc.version) {
+		case IEEE802154_2003_STD:
+		case IEEE802154_2006_STD:
+		case IEEE802154_STD:
+			break;
+		default:
+			dev_dbg(hw->parent,
+				"unrecognized frame version 0x%x\n",
+				hdr.fc.version);
+			goto drop;
+		}
+
+		/* c) PAN ID constraints */
+		if ((mac_cb(skb)->dest.mode == IEEE802154_ADDR_LONG ||
+		     mac_cb(skb)->dest.mode == IEEE802154_ADDR_SHORT) &&
+		    mac_cb(skb)->dest.pan_id != pib->filt.pan_id &&
+		    mac_cb(skb)->dest.pan_id != cpu_to_le16(IEEE802154_PANID_BROADCAST)) {
+			dev_dbg(hw->parent,
+				"unrecognized PAN ID %04x\n",
+				le16_to_cpu(mac_cb(skb)->dest.pan_id));
+			goto drop;
+		}
+
+		/* d1) Short address constraints */
+		if (mac_cb(skb)->dest.mode == IEEE802154_ADDR_SHORT &&
+		    mac_cb(skb)->dest.short_addr != pib->filt.short_addr &&
+		    mac_cb(skb)->dest.short_addr != cpu_to_le16(IEEE802154_ADDR_BROADCAST)) {
+			dev_dbg(hw->parent,
+				"unrecognized short address %04x\n",
+				le16_to_cpu(mac_cb(skb)->dest.short_addr));
+			goto drop;
+		}
+
+		/* d2) Extended address constraints */
+		if (mac_cb(skb)->dest.mode == IEEE802154_ADDR_LONG &&
+		    mac_cb(skb)->dest.extended_addr != pib->filt.ieee_addr) {
+			dev_dbg(hw->parent,
+				"unrecognized long address 0x%016llx\n",
+				mac_cb(skb)->dest.extended_addr);
+			goto drop;
+		}
+
+		/* d4) Specific PAN coordinator case (no parent) */
+		if ((mac_cb(skb)->type == IEEE802154_FC_TYPE_DATA ||
+		     mac_cb(skb)->type == IEEE802154_FC_TYPE_MAC_CMD) &&
+		    mac_cb(skb)->dest.mode == IEEE802154_ADDR_NONE) {
+			dev_dbg(hw->parent,
+				"relaying is not supported\n");
+			goto drop;
+		}
+
+		/* e) Beacon frames follow specific PAN ID rules */
+		if (mac_cb(skb)->type == IEEE802154_FC_TYPE_BEACON &&
+		    pib->filt.pan_id != cpu_to_le16(IEEE802154_PANID_BROADCAST) &&
+		    mac_cb(skb)->dest.pan_id != pib->filt.pan_id) {
+			dev_dbg(hw->parent,
+				"invalid beacon PAN ID %04x\n",
+				le16_to_cpu(mac_cb(skb)->dest.pan_id));
+			goto drop;
+		}
+        }
+
+	rcu_read_unlock();
+
+	ieee802154_rx_irqsafe(hw, skb, lqi);
+
+	return;
+
+drop:
+	rcu_read_unlock();
+	kfree_skb(skb);
+}
+
 static int hwsim_hw_xmit(struct ieee802154_hw *hw, struct sk_buff *skb)
 {
 	struct hwsim_phy *current_phy = hw->priv;
@@ -166,8 +274,7 @@ static int hwsim_hw_xmit(struct ieee802154_hw *hw, struct sk_buff *skb)
 
 			einfo = rcu_dereference(e->info);
 			if (newskb)
-				ieee802154_rx_irqsafe(e->endpoint->hw, newskb,
-						      einfo->lqi);
+				hwsim_hw_receive(e->endpoint->hw, newskb, einfo->lqi);
 		}
 	}
 	rcu_read_unlock();
diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index d0d188c3294b..1b82bbafe8c7 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
 #endif
 };
 
+enum ieee802154_frame_version {
+	IEEE802154_2003_STD,
+	IEEE802154_2006_STD,
+	IEEE802154_STD,
+	IEEE802154_RESERVED_STD,
+	IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
+};
+
 struct ieee802154_hdr {
 	struct ieee802154_hdr_fc fc;
 	u8 seq;
-- 
2.34.1

[PATCH wpan/next v4 6/8] mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM

[Miquel Raynal]

This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to
reflect the fact that it would not validate the checksum (FCS). So this
was only useful while the only filtering level hwsim was capable of was
"NONE". Now that the driver has been improved we no longer need this
flag.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mac802154_hwsim.c | 3 ++-
 include/net/mac802154.h                  | 4 ----
 net/mac802154/rx.c                       | 7 ++-----
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 84ee948f35bc..d98f62e9a97d 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -288,6 +288,7 @@ static int hwsim_hw_start(struct ieee802154_hw *hw)
 	struct hwsim_phy *phy = hw->priv;
 
 	phy->suspended = false;
+
 	return 0;
 }
 
@@ -934,7 +935,7 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
 	phy->idx = idx;
 	INIT_LIST_HEAD(&phy->edges);
 
-	hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM;
+	hw->flags = IEEE802154_HW_PROMISCUOUS;
 	hw->parent = dev;
 
 	err = ieee802154_register_hw(hw);
diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index 357d25ef627a..4a3a9de9da73 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -111,9 +111,6 @@ struct ieee802154_hw {
  *	promiscuous mode setting.
  *
  * @IEEE802154_HW_RX_OMIT_CKSUM: Indicates that receiver omits FCS.
- *
- * @IEEE802154_HW_RX_DROP_BAD_CKSUM: Indicates that receiver will not filter
- *	frames with bad checksum.
  */
 enum ieee802154_hw_flags {
 	IEEE802154_HW_TX_OMIT_CKSUM	= BIT(0),
@@ -123,7 +120,6 @@ enum ieee802154_hw_flags {
 	IEEE802154_HW_AFILT		= BIT(4),
 	IEEE802154_HW_PROMISCUOUS	= BIT(5),
 	IEEE802154_HW_RX_OMIT_CKSUM	= BIT(6),
-	IEEE802154_HW_RX_DROP_BAD_CKSUM	= BIT(7),
 };
 
 /* Indicates that receiver omits FCS and xmitter will add FCS on it's own. */
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index b68d62335f66..8438bdcd5042 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -279,11 +279,8 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 	 * IEEE802154_FILTERING_NONE level during a scan.
 	 */
 
-	/* Check if transceiver doesn't validate the checksum.
-	 * If not we validate the checksum here.
-	 */
-	if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM ||
-	    local->phy->filtering == IEEE802154_FILTERING_NONE) {
+	/* Level 1 filtering: Check the FCS by software when relevant */
+	if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) {
 		crc = crc_ccitt(0, skb->data, skb->len);
 		if (crc) {
 			rcu_read_unlock();
-- 
2.34.1

[PATCH wpan/next v4 7/8] mac802154: Avoid delivering frames received in a non satisfying filtering mode

[Miquel Raynal]

We must avoid the situation where one interface disables address
filtering and AACK on the PHY while another interface expects to run
with AACK and address filtering enabled. Just ignore the frames on the
concerned interface if this happens.

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

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 8438bdcd5042..14bc646b9ab7 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -211,6 +211,13 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
 		if (!ieee802154_sdata_running(sdata))
 			continue;
 
+		/* Do not deliver packets received on interfaces expecting
+		 * AACK=1 if the address filters where disabled.
+		 */
+		if (local->hw.phy->filtering < IEEE802154_FILTERING_4_FRAME_FIELDS &&
+		    sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS)
+			continue;
+
 		ieee802154_subif_frame(sdata, skb, &hdr);
 		skb = NULL;
 		break;
@@ -270,11 +277,6 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 
 	ieee802154_monitors_rx(local, skb);
 
-	/* TODO: Avoid delivering frames received at the level
-	 * IEEE802154_FILTERING_NONE on interfaces not expecting it because of
-	 * the missing auto ACK handling feature.
-	 */
-
 	/* TODO: Handle upcomming receive path where the PHY is at the
 	 * IEEE802154_FILTERING_NONE level during a scan.
 	 */
-- 
2.34.1

[PATCH wpan/next v4 8/8] mac802154: Ensure proper scan-level filtering

[Miquel Raynal]

We now have a fine grained filtering information so let's ensure proper
filtering in scan mode, which means that only beacons are processed.

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

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 14bc646b9ab7..4d799b477a7f 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -34,6 +34,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		       struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 {
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+	struct wpan_phy *wpan_phy = sdata->local->hw.phy;
 	__le16 span, sshort;
 	int rc;
 
@@ -42,6 +43,17 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 	span = wpan_dev->pan_id;
 	sshort = wpan_dev->short_addr;
 
+	/* Level 3 filtering: Only beacons are accepted during scans */
+	if (sdata->required_filtering == IEEE802154_FILTERING_3_SCAN &&
+	    sdata->required_filtering > wpan_phy->filtering) {
+		if (mac_cb(skb)->type != IEEE802154_FC_TYPE_BEACON) {
+			dev_dbg(&sdata->dev->dev,
+				"drop !beacon frame (0x%x) during scan\n",
+				mac_cb(skb)->type);
+			goto fail;
+		}
+	}
+
 	switch (mac_cb(skb)->dest.mode) {
 	case IEEE802154_ADDR_NONE:
 		if (hdr->source.mode != IEEE802154_ADDR_NONE)
@@ -277,10 +289,6 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 
 	ieee802154_monitors_rx(local, skb);
 
-	/* TODO: Handle upcomming receive path where the PHY is at the
-	 * IEEE802154_FILTERING_NONE level during a scan.
-	 */
-
 	/* Level 1 filtering: Check the FCS by software when relevant */
 	if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) {
 		crc = crc_ccitt(0, skb->data, skb->len);
-- 
2.34.1

Re: [PATCH wpan/next v4 0/8] net: ieee802154: Improve filtering support

[Alexander Aring]

Hi,

On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hello,
>
> A fourth version of this series, where we try to improve filtering
> support to ease scan integration. Will then come a short series about
> the coordinator interfaces and then the proper scan series.
>

I think this patch series goes into the right direction. So I would
give it a go:

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

- Alex

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Alexander Aring]

Hi,

On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> We have access to the address filters being theoretically applied, we
> also have access to the actual filtering level applied, so let's add a
> proper frame validation sequence in hwsim.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
>  include/net/ieee802154_netdev.h          |   8 ++
>  2 files changed, 117 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> index 458be66b5195..84ee948f35bc 100644
> --- a/drivers/net/ieee802154/mac802154_hwsim.c
> +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> @@ -18,6 +18,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/device.h>
>  #include <linux/spinlock.h>
> +#include <net/ieee802154_netdev.h>
>  #include <net/mac802154.h>
>  #include <net/cfg802154.h>
>  #include <net/genetlink.h>
> @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
>         return 0;
>  }
>
> +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> +                            u8 lqi)
> +{
> +       struct ieee802154_hdr hdr;
> +       struct hwsim_phy *phy = hw->priv;
> +       struct hwsim_pib *pib;
> +
> +       rcu_read_lock();
> +       pib = rcu_dereference(phy->pib);
> +
> +       if (!pskb_may_pull(skb, 3)) {
> +               dev_dbg(hw->parent, "invalid frame\n");
> +               goto drop;
> +       }
> +
> +       memcpy(&hdr, skb->data, 3);
> +
> +       /* Level 4 filtering: Frame fields validity */
> +       if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
> +
> +               /* a) Drop reserved frame types */
> +               switch (mac_cb(skb)->type) {
> +               case IEEE802154_FC_TYPE_BEACON:
> +               case IEEE802154_FC_TYPE_DATA:
> +               case IEEE802154_FC_TYPE_ACK:
> +               case IEEE802154_FC_TYPE_MAC_CMD:
> +                       break;
> +               default:
> +                       dev_dbg(hw->parent, "unrecognized frame type 0x%x\n",
> +                               mac_cb(skb)->type);
> +                       goto drop;
> +               }
> +
> +               /* b) Drop reserved frame versions */
> +               switch (hdr.fc.version) {
> +               case IEEE802154_2003_STD:
> +               case IEEE802154_2006_STD:
> +               case IEEE802154_STD:
> +                       break;
> +               default:
> +                       dev_dbg(hw->parent,
> +                               "unrecognized frame version 0x%x\n",
> +                               hdr.fc.version);
> +                       goto drop;
> +               }
> +
> +               /* c) PAN ID constraints */
> +               if ((mac_cb(skb)->dest.mode == IEEE802154_ADDR_LONG ||
> +                    mac_cb(skb)->dest.mode == IEEE802154_ADDR_SHORT) &&
> +                   mac_cb(skb)->dest.pan_id != pib->filt.pan_id &&
> +                   mac_cb(skb)->dest.pan_id != cpu_to_le16(IEEE802154_PANID_BROADCAST)) {
> +                       dev_dbg(hw->parent,
> +                               "unrecognized PAN ID %04x\n",
> +                               le16_to_cpu(mac_cb(skb)->dest.pan_id));
> +                       goto drop;
> +               }
> +
> +               /* d1) Short address constraints */
> +               if (mac_cb(skb)->dest.mode == IEEE802154_ADDR_SHORT &&
> +                   mac_cb(skb)->dest.short_addr != pib->filt.short_addr &&
> +                   mac_cb(skb)->dest.short_addr != cpu_to_le16(IEEE802154_ADDR_BROADCAST)) {
> +                       dev_dbg(hw->parent,
> +                               "unrecognized short address %04x\n",
> +                               le16_to_cpu(mac_cb(skb)->dest.short_addr));
> +                       goto drop;
> +               }
> +
> +               /* d2) Extended address constraints */
> +               if (mac_cb(skb)->dest.mode == IEEE802154_ADDR_LONG &&
> +                   mac_cb(skb)->dest.extended_addr != pib->filt.ieee_addr) {
> +                       dev_dbg(hw->parent,
> +                               "unrecognized long address 0x%016llx\n",
> +                               mac_cb(skb)->dest.extended_addr);
> +                       goto drop;
> +               }
> +
> +               /* d4) Specific PAN coordinator case (no parent) */
> +               if ((mac_cb(skb)->type == IEEE802154_FC_TYPE_DATA ||
> +                    mac_cb(skb)->type == IEEE802154_FC_TYPE_MAC_CMD) &&
> +                   mac_cb(skb)->dest.mode == IEEE802154_ADDR_NONE) {
> +                       dev_dbg(hw->parent,
> +                               "relaying is not supported\n");
> +                       goto drop;
> +               }
> +
> +               /* e) Beacon frames follow specific PAN ID rules */
> +               if (mac_cb(skb)->type == IEEE802154_FC_TYPE_BEACON &&
> +                   pib->filt.pan_id != cpu_to_le16(IEEE802154_PANID_BROADCAST) &&
> +                   mac_cb(skb)->dest.pan_id != pib->filt.pan_id) {
> +                       dev_dbg(hw->parent,
> +                               "invalid beacon PAN ID %04x\n",
> +                               le16_to_cpu(mac_cb(skb)->dest.pan_id));
> +                       goto drop;
> +               }
> +        }
> +
> +       rcu_read_unlock();
> +
> +       ieee802154_rx_irqsafe(hw, skb, lqi);

what is about if hwsim goes into promiscuous mode, then this software
filtering should be skipped?

- Alex

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Alexander Aring]

Hi,

On Mon, Oct 10, 2022 at 9:04 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > We have access to the address filters being theoretically applied, we
> > also have access to the actual filtering level applied, so let's add a
> > proper frame validation sequence in hwsim.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
> >  include/net/ieee802154_netdev.h          |   8 ++
> >  2 files changed, 117 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > index 458be66b5195..84ee948f35bc 100644
> > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/device.h>
> >  #include <linux/spinlock.h>
> > +#include <net/ieee802154_netdev.h>
> >  #include <net/mac802154.h>
> >  #include <net/cfg802154.h>
> >  #include <net/genetlink.h>
> > @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
> >         return 0;
> >  }
> >
> > +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> > +                            u8 lqi)
> > +{
> > +       struct ieee802154_hdr hdr;
> > +       struct hwsim_phy *phy = hw->priv;
> > +       struct hwsim_pib *pib;
> > +
> > +       rcu_read_lock();
> > +       pib = rcu_dereference(phy->pib);
> > +
> > +       if (!pskb_may_pull(skb, 3)) {
> > +               dev_dbg(hw->parent, "invalid frame\n");
> > +               goto drop;
> > +       }
> > +
> > +       memcpy(&hdr, skb->data, 3);
> > +
> > +       /* Level 4 filtering: Frame fields validity */
> > +       if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {

I see, there is this big if handling. But it accesses the
hw->phy->filtering value. It should be part of the hwsim pib setting
set by the driver callback. It is a question here of mac802154 layer
setting vs driver layer setting. We should do what the mac802154 tells
the driver to do, this way we do what the mac802154 layer is set to.

However it's a minor thing and it's okay to do it so...

- Alex

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Alexander Aring]

Hi,

On Mon, Oct 10, 2022 at 9:13 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Mon, Oct 10, 2022 at 9:04 PM Alexander Aring <aahringo@redhat.com> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > We have access to the address filters being theoretically applied, we
> > > also have access to the actual filtering level applied, so let's add a
> > > proper frame validation sequence in hwsim.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
> > >  include/net/ieee802154_netdev.h          |   8 ++
> > >  2 files changed, 117 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > > index 458be66b5195..84ee948f35bc 100644
> > > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/netdevice.h>
> > >  #include <linux/device.h>
> > >  #include <linux/spinlock.h>
> > > +#include <net/ieee802154_netdev.h>
> > >  #include <net/mac802154.h>
> > >  #include <net/cfg802154.h>
> > >  #include <net/genetlink.h>
> > > @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
> > >         return 0;
> > >  }
> > >
> > > +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> > > +                            u8 lqi)
> > > +{
> > > +       struct ieee802154_hdr hdr;
> > > +       struct hwsim_phy *phy = hw->priv;
> > > +       struct hwsim_pib *pib;
> > > +
> > > +       rcu_read_lock();
> > > +       pib = rcu_dereference(phy->pib);
> > > +
> > > +       if (!pskb_may_pull(skb, 3)) {
> > > +               dev_dbg(hw->parent, "invalid frame\n");
> > > +               goto drop;
> > > +       }
> > > +
> > > +       memcpy(&hdr, skb->data, 3);
> > > +
> > > +       /* Level 4 filtering: Frame fields validity */
> > > +       if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
>
> I see, there is this big if handling. But it accesses the
> hw->phy->filtering value. It should be part of the hwsim pib setting
> set by the driver callback. It is a question here of mac802154 layer
> setting vs driver layer setting. We should do what the mac802154 tells
> the driver to do, this way we do what the mac802154 layer is set to.
>
> However it's a minor thing and it's okay to do it so...

* whereas we never let the driver know at any time of what different
filter levels exist _currently_ we have only the promiscuous mode
on/off switch which is do nothing or 4_FRAME_FIELDS.
It will work for now, changing anything in the mac802154 filtering
fields or something will end in probably breakage in this handling. In
my point of view as the current state is it should not do that, as
remember that hwsim will "simulate" hardware it should not be able to
access mac802154 fields (especially when doing receiving of frames) as
other hardware will only set register bits (as hwsim pib values is
there for)...

Still I think it's fine for now.

- Alex

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Stefan Schmidt]

Hello Miquel.

This patch has given me some checkpatch wawrnings and errors.

Commit d9abecc4a0fc ("ieee802154: hwsim: Implement address filtering")
----------------------------------------------------------------------
CHECK: Blank lines aren't necessary after an open brace '{'
#53: FILE: drivers/net/ieee802154/mac802154_hwsim.c:162:
+	if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
+

ERROR: code indent should use tabs where possible
#128: FILE: drivers/net/ieee802154/mac802154_hwsim.c:237:
+        }$

WARNING: please, no spaces at the start of a line
#128: FILE: drivers/net/ieee802154/mac802154_hwsim.c:237:
+        }$

total: 1 errors, 1 warnings, 1 checks, 143 lines checked

I fixed this up in palce for you tp proceed with applying this patches. 
Just so you are aware.

regards
Stefan Schmidt

Re: [PATCH wpan/next v4 8/8] mac802154: Ensure proper scan-level filtering

[Stefan Schmidt]

Hello Miquel.

On 07.10.22 10:53, Miquel Raynal wrote:
> We now have a fine grained filtering information so let's ensure proper
> filtering in scan mode, which means that only beacons are processed.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/mac802154/rx.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 14bc646b9ab7..4d799b477a7f 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -34,6 +34,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>   		       struct sk_buff *skb, const struct ieee802154_hdr *hdr)
>   {
>   	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
> +	struct wpan_phy *wpan_phy = sdata->local->hw.phy;
>   	__le16 span, sshort;
>   	int rc;
>   
> @@ -42,6 +43,17 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>   	span = wpan_dev->pan_id;
>   	sshort = wpan_dev->short_addr;
>   
> +	/* Level 3 filtering: Only beacons are accepted during scans */
> +	if (sdata->required_filtering == IEEE802154_FILTERING_3_SCAN &&
> +	    sdata->required_filtering > wpan_phy->filtering) {
> +		if (mac_cb(skb)->type != IEEE802154_FC_TYPE_BEACON) {
> +			dev_dbg(&sdata->dev->dev,
> +				"drop !beacon frame (0x%x) during scan\n",

This ! before the beacon looks like a typo. Please fix.

> +				mac_cb(skb)->type);
> +			goto fail;
> +		}
> +	}
> +
>   	switch (mac_cb(skb)->dest.mode) {
>   	case IEEE802154_ADDR_NONE:
>   		if (hdr->source.mode != IEEE802154_ADDR_NONE)
> @@ -277,10 +289,6 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
>   
>   	ieee802154_monitors_rx(local, skb);
>   
> -	/* TODO: Handle upcomming receive path where the PHY is at the
> -	 * IEEE802154_FILTERING_NONE level during a scan.
> -	 */
> -
>   	/* Level 1 filtering: Check the FCS by software when relevant */
>   	if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) {
>   		crc = crc_ccitt(0, skb->data, skb->len);

When trying to apply the patch it did not work:

Failed to apply patch:
error: patch failed: net/mac802154/rx.c:42
error: net/mac802154/rx.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: mac802154: Ensure proper scan-level filtering
Patch failed at 0001 mac802154: Ensure proper scan-level filtering

On top of what tree or branch is this? Maybe you based it on some not 
applied patches? Please rebase against wpan-next and re-submit. The rest 
of the patches got applied.

Thanks for the ongoing work on this.

regards
Stefan Schmidt

Re: [PATCH wpan/next v4 0/8] net: ieee802154: Improve filtering support

[Stefan Schmidt]

Hello Miquel.

On 11.10.22 03:01, Alexander Aring wrote:
> Hi,
> 
> On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> Hello,
>>
>> A fourth version of this series, where we try to improve filtering
>> support to ease scan integration. Will then come a short series about
>> the coordinator interfaces and then the proper scan series.
>>
> 
> I think this patch series goes into the right direction. So I would
> give it a go:
> 
> Acked-by: Alexander Aring <aahringo@redhat.com>


Besides patch 8 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/next v4 8/8] mac802154: Ensure proper scan-level filtering

[Miquel Raynal]

Hi Stefan,

stefan@datenfreihafen.org wrote on Wed, 12 Oct 2022 12:50:34 +0200:

> Hello Miquel.
> 
> On 07.10.22 10:53, Miquel Raynal wrote:
> > We now have a fine grained filtering information so let's ensure proper
> > filtering in scan mode, which means that only beacons are processed.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >   net/mac802154/rx.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > index 14bc646b9ab7..4d799b477a7f 100644
> > --- a/net/mac802154/rx.c
> > +++ b/net/mac802154/rx.c
> > @@ -34,6 +34,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> >   		       struct sk_buff *skb, const struct ieee802154_hdr *hdr)
> >   {
> >   	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
> > +	struct wpan_phy *wpan_phy = sdata->local->hw.phy;
> >   	__le16 span, sshort;
> >   	int rc;  
> >   > @@ -42,6 +43,17 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,  
> >   	span = wpan_dev->pan_id;
> >   	sshort = wpan_dev->short_addr;  
> >   > +	/* Level 3 filtering: Only beacons are accepted during scans */  
> > +	if (sdata->required_filtering == IEEE802154_FILTERING_3_SCAN &&
> > +	    sdata->required_filtering > wpan_phy->filtering) {
> > +		if (mac_cb(skb)->type != IEEE802154_FC_TYPE_BEACON) {
> > +			dev_dbg(&sdata->dev->dev,
> > +				"drop !beacon frame (0x%x) during scan\n",  
> 
> This ! before the beacon looks like a typo. Please fix.

Actually it's not, I meant "this is a non-beacon frame", but I might
have been too lazy to write it in plain english. But you're right, it
looks like a typo, so I'll rephrase this string.

> 
> > +				mac_cb(skb)->type);
> > +			goto fail;
> > +		}
> > +	}
> > +
> >   	switch (mac_cb(skb)->dest.mode) {
> >   	case IEEE802154_ADDR_NONE:
> >   		if (hdr->source.mode != IEEE802154_ADDR_NONE)
> > @@ -277,10 +289,6 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)  
> >   >   	ieee802154_monitors_rx(local, skb);
> >   > -	/* TODO: Handle upcomming receive path where the PHY is at the  
> > -	 * IEEE802154_FILTERING_NONE level during a scan.
> > -	 */
> > -
> >   	/* Level 1 filtering: Check the FCS by software when relevant */
> >   	if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) {
> >   		crc = crc_ccitt(0, skb->data, skb->len);  
> 
> When trying to apply the patch it did not work:
> 
> Failed to apply patch:
> error: patch failed: net/mac802154/rx.c:42
> error: net/mac802154/rx.c: patch does not apply
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> Applying: mac802154: Ensure proper scan-level filtering
> Patch failed at 0001 mac802154: Ensure proper scan-level filtering
> 
> On top of what tree or branch is this? Maybe you based it on some not applied patches? Please rebase against wpan-next and re-submit. The rest of the patches got applied.

This series was based on top of wpan/next, but I assumed it would have
been applied on top of this fix that was picked up a month ago:
https://lkml.kernel.org/stable/57b7d918-1da1-f490-4882-5ed25ea17503@datenfreihafen.org/
I will update the above dev_dbg string, but I suggest we wait for
6.1-rc1 to be out before applying it? Otherwise if I "fix" it for
immediate appliance on the current wpan-next branch, it will likely
conflict with linux-next.
 
> 
> Thanks for the ongoing work on this.

You're welcome, thank you both for the reviews and time spent on your
side as well!

Thanks,
Miquèl

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Mon, 10 Oct 2022 21:21:17 -0400:

> Hi,
> 
> On Mon, Oct 10, 2022 at 9:13 PM Alexander Aring <aahringo@redhat.com> wrote:
> >
> > Hi,
> >
> > On Mon, Oct 10, 2022 at 9:04 PM Alexander Aring <aahringo@redhat.com> wrote:  
> > >
> > > Hi,
> > >
> > > On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > We have access to the address filters being theoretically applied, we
> > > > also have access to the actual filtering level applied, so let's add a
> > > > proper frame validation sequence in hwsim.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
> > > >  include/net/ieee802154_netdev.h          |   8 ++
> > > >  2 files changed, 117 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > index 458be66b5195..84ee948f35bc 100644
> > > > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > > > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > @@ -18,6 +18,7 @@
> > > >  #include <linux/netdevice.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/spinlock.h>
> > > > +#include <net/ieee802154_netdev.h>
> > > >  #include <net/mac802154.h>
> > > >  #include <net/cfg802154.h>
> > > >  #include <net/genetlink.h>
> > > > @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
> > > >         return 0;
> > > >  }
> > > >
> > > > +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> > > > +                            u8 lqi)
> > > > +{
> > > > +       struct ieee802154_hdr hdr;
> > > > +       struct hwsim_phy *phy = hw->priv;
> > > > +       struct hwsim_pib *pib;
> > > > +
> > > > +       rcu_read_lock();
> > > > +       pib = rcu_dereference(phy->pib);
> > > > +
> > > > +       if (!pskb_may_pull(skb, 3)) {
> > > > +               dev_dbg(hw->parent, "invalid frame\n");
> > > > +               goto drop;
> > > > +       }
> > > > +
> > > > +       memcpy(&hdr, skb->data, 3);
> > > > +
> > > > +       /* Level 4 filtering: Frame fields validity */
> > > > +       if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {  
> >
> > I see, there is this big if handling. But it accesses the
> > hw->phy->filtering value. It should be part of the hwsim pib setting
> > set by the driver callback. It is a question here of mac802154 layer
> > setting vs driver layer setting. We should do what the mac802154 tells
> > the driver to do, this way we do what the mac802154 layer is set to.
> >
> > However it's a minor thing and it's okay to do it so...  
> 
> * whereas we never let the driver know at any time of what different
> filter levels exist _currently_ we have only the promiscuous mode
> on/off switch which is do nothing or 4_FRAME_FIELDS.
> It will work for now, changing anything in the mac802154 filtering
> fields or something will end in probably breakage in this handling. In
> my point of view as the current state is it should not do that, as
> remember that hwsim will "simulate" hardware it should not be able to
> access mac802154 fields (especially when doing receiving of frames) as
> other hardware will only set register bits (as hwsim pib values is
> there for)...
> 
> Still I think it's fine for now.

I see your point, indeed I could have added another PIB attribute
instead of accessing the PHY state.

I am fine doing it in a followup patch if this what you prefer. Shall I
do it?

Thanks,
Miquèl

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Miquel Raynal]

Hi Stefan,

stefan@datenfreihafen.org wrote on Wed, 12 Oct 2022 12:48:06 +0200:

> Hello Miquel.
> 
> This patch has given me some checkpatch wawrnings and errors.
> 
> Commit d9abecc4a0fc ("ieee802154: hwsim: Implement address filtering")
> ----------------------------------------------------------------------
> CHECK: Blank lines aren't necessary after an open brace '{'
> #53: FILE: drivers/net/ieee802154/mac802154_hwsim.c:162:
> +	if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
> +
> 
> ERROR: code indent should use tabs where possible
> #128: FILE: drivers/net/ieee802154/mac802154_hwsim.c:237:
> +        }$
> 
> WARNING: please, no spaces at the start of a line
> #128: FILE: drivers/net/ieee802154/mac802154_hwsim.c:237:
> +        }$
> 
> total: 1 errors, 1 warnings, 1 checks, 143 lines checked
> 
> I fixed this up in palce for you tp proceed with applying this patches. Just so you are aware.

I'm sorry for these, I focused on getting the feature more than on the
presentation and I forgot to re-run checkpatch after all the copy-paste
handling towards hwsim. Next time I'll fix it myself, don't hesitate to
tell me when this happens ;-)

Thanks,
Miquèl

Re: [PATCH wpan/next v4 5/8] ieee802154: hwsim: Implement address filtering

[Alexander Aring]

Hi,

On Sat, Oct 15, 2022 at 4:59 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Mon, 10 Oct 2022 21:21:17 -0400:
>
> > Hi,
> >
> > On Mon, Oct 10, 2022 at 9:13 PM Alexander Aring <aahringo@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Oct 10, 2022 at 9:04 PM Alexander Aring <aahringo@redhat.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > We have access to the address filters being theoretically applied, we
> > > > > also have access to the actual filtering level applied, so let's add a
> > > > > proper frame validation sequence in hwsim.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > >  drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
> > > > >  include/net/ieee802154_netdev.h          |   8 ++
> > > > >  2 files changed, 117 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > > index 458be66b5195..84ee948f35bc 100644
> > > > > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > > > > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > > @@ -18,6 +18,7 @@
> > > > >  #include <linux/netdevice.h>
> > > > >  #include <linux/device.h>
> > > > >  #include <linux/spinlock.h>
> > > > > +#include <net/ieee802154_netdev.h>
> > > > >  #include <net/mac802154.h>
> > > > >  #include <net/cfg802154.h>
> > > > >  #include <net/genetlink.h>
> > > > > @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> > > > > +                            u8 lqi)
> > > > > +{
> > > > > +       struct ieee802154_hdr hdr;
> > > > > +       struct hwsim_phy *phy = hw->priv;
> > > > > +       struct hwsim_pib *pib;
> > > > > +
> > > > > +       rcu_read_lock();
> > > > > +       pib = rcu_dereference(phy->pib);
> > > > > +
> > > > > +       if (!pskb_may_pull(skb, 3)) {
> > > > > +               dev_dbg(hw->parent, "invalid frame\n");
> > > > > +               goto drop;
> > > > > +       }
> > > > > +
> > > > > +       memcpy(&hdr, skb->data, 3);
> > > > > +
> > > > > +       /* Level 4 filtering: Frame fields validity */
> > > > > +       if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
> > >
> > > I see, there is this big if handling. But it accesses the
> > > hw->phy->filtering value. It should be part of the hwsim pib setting
> > > set by the driver callback. It is a question here of mac802154 layer
> > > setting vs driver layer setting. We should do what the mac802154 tells
> > > the driver to do, this way we do what the mac802154 layer is set to.
> > >
> > > However it's a minor thing and it's okay to do it so...
> >
> > * whereas we never let the driver know at any time of what different
> > filter levels exist _currently_ we have only the promiscuous mode
> > on/off switch which is do nothing or 4_FRAME_FIELDS.
> > It will work for now, changing anything in the mac802154 filtering
> > fields or something will end in probably breakage in this handling. In
> > my point of view as the current state is it should not do that, as
> > remember that hwsim will "simulate" hardware it should not be able to
> > access mac802154 fields (especially when doing receiving of frames) as
> > other hardware will only set register bits (as hwsim pib values is
> > there for)...
> >
> > Still I think it's fine for now.
>
> I see your point, indeed I could have added another PIB attribute
> instead of accessing the PHY state.
>
> I am fine doing it in a followup patch if this what you prefer. Shall I
> do it?

okay, note that you did it right with the address filter patches by
copying them from the drivers-ops.

- Alex