[PATCH 0/3] Handle DEL/ADD_MAC_ADDR events

[PATCH 0/3] Handle DEL/ADD_MAC_ADDR events

[James Prestwood]

So this did work all along, I was just looking at it the wrong way.
 - Scan randomization now works with hwsim
 - Radios can be moved to another namespace and still work correctly
   with hwsim

James Prestwood (3):
  hwsim: add ADD/DEL_MAC_ADDR events
  hwsim: move frame processing into a separate function
  hwsim: handle ADD/DEL_MAC_ADDR events

 tools/hwsim.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 158 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] hwsim: add ADD/DEL_MAC_ADDR events

[James Prestwood]

These events are important to support both moving radios to other
namespaces, and to allow scan address randomization to work.
---
 tools/hwsim.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/hwsim.c b/tools/hwsim.c
index 47352ad4..ecee0a0a 100644
--- a/tools/hwsim.c
+++ b/tools/hwsim.c
@@ -62,6 +62,8 @@ enum {
 	HWSIM_CMD_NEW_RADIO,
 	HWSIM_CMD_DEL_RADIO,
 	HWSIM_CMD_GET_RADIO,
+	HWSIM_CMD_ADD_MAC_ADDR,
+	HWSIM_CMD_DEL_MAC_ADDR,
 	__HWSIM_CMD_MAX,
 };
 #define HWSIM_CMD_MAX (__HWSIM_CMD_MAX - 1)
-- 
2.25.1

[PATCH 2/3] hwsim: move frame processing into a separate function

[James Prestwood]

The ADD/DEL_MAC_ADDR events come through the unicast handler so
move the frame processing to a separate function so these other
events can be handled.
---
 tools/hwsim.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/tools/hwsim.c b/tools/hwsim.c
index ecee0a0a..2fee9b19 100644
--- a/tools/hwsim.c
+++ b/tools/hwsim.c
@@ -1572,7 +1572,9 @@ static void process_frame(struct hwsim_frame *frame)
 	hwsim_frame_unref(frame);
 }
 
-static void unicast_handler(struct l_genl_msg *msg, void *user_data)
+
+
+static void hwsim_frame_event(struct l_genl_msg *msg)
 {
 	struct hwsim_frame *frame;
 	const struct mmpdu_header *mpdu;
@@ -1581,9 +1583,6 @@ static void unicast_handler(struct l_genl_msg *msg, void *user_data)
 	const void *data;
 	const uint8_t *transmitter = NULL, *freq = NULL, *flags = NULL;
 
-	if (l_genl_msg_get_command(msg) != HWSIM_CMD_FRAME)
-		return;
-
 	if (!l_genl_attr_init(&attr, msg))
 		return;
 
@@ -1683,6 +1682,24 @@ static void unicast_handler(struct l_genl_msg *msg, void *user_data)
 	process_frame(frame);
 }
 
+static void hwsim_unicast_handler(struct l_genl_msg *msg, void *user_data)
+{
+	uint8_t cmd;
+
+	if (l_genl_msg_get_error(msg) < 0)
+		return;
+
+	cmd = l_genl_msg_get_command(msg);
+
+	switch (cmd) {
+	case HWSIM_CMD_FRAME:
+		hwsim_frame_event(msg);
+		break;
+	default:
+		break;
+	}
+}
+
 static void radio_manager_create_callback(struct l_genl_msg *msg,
 						void *user_data)
 {
@@ -2939,8 +2956,9 @@ static void hwsim_ready(void)
 		}
 
 		if (!l_genl_add_unicast_watch(genl, "MAC80211_HWSIM",
-						unicast_handler, NULL, NULL)) {
-			l_error("Failed to set unicast handler");
+						hwsim_unicast_handler,
+						NULL, NULL)) {
+			l_error("Failed to set hwsim unicast handler");
 			goto error;
 		}
 
-- 
2.25.1

[PATCH 3/3] hwsim: handle ADD/DEL_MAC_ADDR events

[James Prestwood]

Handling these events notifies hwsim of address changes for interface
creation/removal outside the initial namespace as well as address
changes due to scanning address randomization.

Interfaces that hwsim already knows about are still handled via
nl80211. But any interfaces not known when ADD/DEL_MAC_ADDR events
come will be treated specially.

For ADD, a dummy interface object will be created and added to the
queue. This lets the frame processing match the destination address
correctly. This can happen both for scan randomization and interface
creation outside of the initial namespace.

For the DEL event we handle similarly and don't touch any interfaces
found via nl80211 (i.e. have a 'name') but need to also be careful
with the dummy interfaces that were created outside the initial
namespace. We want to keep these around but scanning MAC changes can
also delete them. This is why a reference count was added so scanning
doesn't cause a removal.

For example, the following sequence:

ADD_MAC_ADDR (interface creation)
ADD_MAC_ADDR (scanning started)
DEL_MAC_ADDR (scanning done)
---
 tools/hwsim.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/tools/hwsim.c b/tools/hwsim.c
index 2fee9b19..5cb28624 100644
--- a/tools/hwsim.c
+++ b/tools/hwsim.c
@@ -440,6 +440,7 @@ struct interface_info_rec {
 	uint8_t addr[ETH_ALEN];
 	char *name;
 	uint32_t iftype;
+	int ref;
 };
 
 static struct l_queue *radio_info;
@@ -515,6 +516,14 @@ static bool interface_info_match_id(const void *a, const void *b)
 	return rec->id == id;
 }
 
+static bool interface_info_match_addr(const void *a, const void *b)
+{
+	const struct interface_info_rec *rec = a;
+	const uint8_t *addr = b;
+
+	return memcmp(rec->addr, addr, ETH_ALEN) == 0;
+}
+
 static const char *radio_get_path(const struct radio_info_rec *rec)
 {
 	static char path[15];
@@ -1682,6 +1691,123 @@ static void hwsim_frame_event(struct l_genl_msg *msg)
 	process_frame(frame);
 }
 
+static bool get_tx_rx_addrs(struct l_genl_msg *msg, const uint8_t **tx_out,
+				const uint8_t **rx_out)
+{
+	struct l_genl_attr attr;
+	uint16_t type, len;
+	const void *data;
+	const uint8_t *tx = NULL, *rx = NULL;
+
+	if (!l_genl_attr_init(&attr, msg))
+		return false;
+
+	while (l_genl_attr_next(&attr, &type, &len, &data)) {
+		switch (type) {
+		case HWSIM_ATTR_ADDR_TRANSMITTER:
+			if (len != ETH_ALEN)
+				return false;
+
+			tx = data;
+			break;
+		case HWSIM_ATTR_ADDR_RECEIVER:
+			if (len != ETH_ALEN)
+				return false;
+
+			rx = data;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!tx || !rx)
+		return false;
+
+	*tx_out = tx;
+	*rx_out = rx;
+
+	return true;
+}
+
+static void hwsim_add_mac_event(struct l_genl_msg *msg)
+{
+	const uint8_t *tx = NULL, *rx = NULL;
+	struct radio_info_rec *radio_rec;
+	struct interface_info_rec *interface_rec;
+
+	if (!get_tx_rx_addrs(msg, &tx, &rx))
+		return;
+
+	/* No radio matches the TX address, hwsim must not have created it */
+	radio_rec = l_queue_find(radio_info, radio_info_match_addr1, tx);
+	if (!radio_rec)
+		return;
+
+	interface_rec = l_queue_find(interface_info,
+					interface_info_match_addr, rx);
+	if (interface_rec) {
+		/* Existing interface, address changes handled via nl80211 */
+		if (interface_rec->name)
+			return;
+
+		/*
+		 * Transient/dummy interface we already know about. This likely
+		 * was created, then a scan changed the address temporarily.
+		 * Reflect this change and increment the ref so the following
+		 * DEL event doesn't destroy it
+		 */
+		__atomic_fetch_add(&interface_rec->ref, 1, __ATOMIC_SEQ_CST);
+		memcpy(interface_rec->addr, rx, ETH_ALEN);
+		return;
+	}
+
+	/*
+	 * Create a dummy interface entry for this address that only contains
+	 * the radio and address. This is either a transient entry due to scan
+	 * randomization or an interface created outside this namespace.
+	 */
+	interface_rec = l_new(struct interface_info_rec, 1);
+	interface_rec->radio_rec = radio_rec;
+	interface_rec->ref = 1;
+	memcpy(interface_rec->addr, rx, ETH_ALEN);
+
+	l_queue_push_tail(interface_info, interface_rec);
+}
+
+static void hwsim_del_mac_event(struct l_genl_msg *msg)
+{
+	const uint8_t *tx = NULL, *rx = NULL;
+	struct radio_info_rec *radio_rec;
+	struct interface_info_rec *interface_rec;
+
+	if (!get_tx_rx_addrs(msg, &tx, &rx))
+		return;
+
+	/* No radio matches the TX address, hwsim must not have created it */
+	radio_rec = l_queue_find(radio_info, radio_info_match_addr1, tx);
+	if (!radio_rec)
+		return;
+
+	interface_rec = l_queue_find(interface_info,
+						interface_info_match_addr, rx);
+	if (!interface_rec)
+		return;
+
+	/*
+	 * This change is handled via nl80211 so we don't want to touch this
+	 * interface here.
+	 */
+	if (interface_rec->name)
+		return;
+
+	if (__atomic_sub_fetch(&interface_rec->ref, 1, __ATOMIC_SEQ_CST))
+		return;
+
+	l_queue_remove(interface_info, interface_rec);
+	l_free(interface_rec);
+}
+
 static void hwsim_unicast_handler(struct l_genl_msg *msg, void *user_data)
 {
 	uint8_t cmd;
@@ -1695,6 +1821,12 @@ static void hwsim_unicast_handler(struct l_genl_msg *msg, void *user_data)
 	case HWSIM_CMD_FRAME:
 		hwsim_frame_event(msg);
 		break;
+	case HWSIM_CMD_ADD_MAC_ADDR:
+		hwsim_add_mac_event(msg);
+		break;
+	case HWSIM_CMD_DEL_MAC_ADDR:
+		hwsim_del_mac_event(msg);
+		break;
 	default:
 		break;
 	}
-- 
2.25.1

Re: [PATCH 3/3] hwsim: handle ADD/DEL_MAC_ADDR events

[James Prestwood]

On 6/28/23 1:20 PM, James Prestwood wrote:
> Handling these events notifies hwsim of address changes for interface
> creation/removal outside the initial namespace as well as address
> changes due to scanning address randomization.
> 
> Interfaces that hwsim already knows about are still handled via
> nl80211. But any interfaces not known when ADD/DEL_MAC_ADDR events
> come will be treated specially.
> 
> For ADD, a dummy interface object will be created and added to the
> queue. This lets the frame processing match the destination address
> correctly. This can happen both for scan randomization and interface
> creation outside of the initial namespace.
> 
> For the DEL event we handle similarly and don't touch any interfaces
> found via nl80211 (i.e. have a 'name') but need to also be careful
> with the dummy interfaces that were created outside the initial
> namespace. We want to keep these around but scanning MAC changes can
> also delete them. This is why a reference count was added so scanning
> doesn't cause a removal.
> 
> For example, the following sequence:
> 
> ADD_MAC_ADDR (interface creation)
> ADD_MAC_ADDR (scanning started)
> DEL_MAC_ADDR (scanning done)
> ---
>   tools/hwsim.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 132 insertions(+)
> 
> diff --git a/tools/hwsim.c b/tools/hwsim.c
> index 2fee9b19..5cb28624 100644
> --- a/tools/hwsim.c
> +++ b/tools/hwsim.c
> @@ -440,6 +440,7 @@ struct interface_info_rec {
>   	uint8_t addr[ETH_ALEN];
>   	char *name;
>   	uint32_t iftype;
> +	int ref;
>   };
>   
>   static struct l_queue *radio_info;
> @@ -515,6 +516,14 @@ static bool interface_info_match_id(const void *a, const void *b)
>   	return rec->id == id;
>   }
>   
> +static bool interface_info_match_addr(const void *a, const void *b)
> +{
> +	const struct interface_info_rec *rec = a;
> +	const uint8_t *addr = b;
> +
> +	return memcmp(rec->addr, addr, ETH_ALEN) == 0;
> +}
> +
>   static const char *radio_get_path(const struct radio_info_rec *rec)
>   {
>   	static char path[15];
> @@ -1682,6 +1691,123 @@ static void hwsim_frame_event(struct l_genl_msg *msg)
>   	process_frame(frame);
>   }
>   
> +static bool get_tx_rx_addrs(struct l_genl_msg *msg, const uint8_t **tx_out,
> +				const uint8_t **rx_out)
> +{
> +	struct l_genl_attr attr;
> +	uint16_t type, len;
> +	const void *data;
> +	const uint8_t *tx = NULL, *rx = NULL;
> +
> +	if (!l_genl_attr_init(&attr, msg))
> +		return false;
> +
> +	while (l_genl_attr_next(&attr, &type, &len, &data)) {
> +		switch (type) {
> +		case HWSIM_ATTR_ADDR_TRANSMITTER:
> +			if (len != ETH_ALEN)
> +				return false;
> +
> +			tx = data;
> +			break;
> +		case HWSIM_ATTR_ADDR_RECEIVER:
> +			if (len != ETH_ALEN)
> +				return false;
> +
> +			rx = data;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (!tx || !rx)
> +		return false;
> +
> +	*tx_out = tx;
> +	*rx_out = rx;
> +
> +	return true;
> +}
> +
> +static void hwsim_add_mac_event(struct l_genl_msg *msg)
> +{
> +	const uint8_t *tx = NULL, *rx = NULL;
> +	struct radio_info_rec *radio_rec;
> +	struct interface_info_rec *interface_rec;
> +
> +	if (!get_tx_rx_addrs(msg, &tx, &rx))
> +		return;
> +
> +	/* No radio matches the TX address, hwsim must not have created it */
> +	radio_rec = l_queue_find(radio_info, radio_info_match_addr1, tx);
> +	if (!radio_rec)
> +		return;
> +
> +	interface_rec = l_queue_find(interface_info,
> +					interface_info_match_addr, rx);
> +	if (interface_rec) {
> +		/* Existing interface, address changes handled via nl80211 */
> +		if (interface_rec->name)
> +			return;
> +
> +		/*
> +		 * Transient/dummy interface we already know about. This likely
> +		 * was created, then a scan changed the address temporarily.
> +		 * Reflect this change and increment the ref so the following
> +		 * DEL event doesn't destroy it
> +		 */
> +		__atomic_fetch_add(&interface_rec->ref, 1, __ATOMIC_SEQ_CST);
> +		memcpy(interface_rec->addr, rx, ETH_ALEN);

Just realized this memcpy is redundant since the interface_rec wouldn't 
match unless the address was already the same. This case only happens 
when randomization is off and is a consequence of the kernel having two 
separate addresses (one prefixed with 0x4, still not sure why they do 
this). So you get the following events (say the perm address is 
02:00:00:00:00:00):

ADD(tx=42:00:00:00:00:00, rx=02:00:00:00:00:00) (Interface Creation)
ADD(tx=42:00:00:00:00:00, rx=02:00:00:00:00:00) (Scanning started)
DEL(tx=42:00:00:00:00:00, rx=02:00:00:00:00:00) (Scanning ended)

When scan randomization is enabled it will create a completely separate 
dummy interface and the DEL event can freely delete that.

I can remove this in v2 if you have additional comments

> +		return;
> +	}
> +
> +	/*
> +	 * Create a dummy interface entry for this address that only contains
> +	 * the radio and address. This is either a transient entry due to scan
> +	 * randomization or an interface created outside this namespace.
> +	 */
> +	interface_rec = l_new(struct interface_info_rec, 1);
> +	interface_rec->radio_rec = radio_rec;
> +	interface_rec->ref = 1;
> +	memcpy(interface_rec->addr, rx, ETH_ALEN);
> +
> +	l_queue_push_tail(interface_info, interface_rec);
> +}
> +
> +static void hwsim_del_mac_event(struct l_genl_msg *msg)
> +{
> +	const uint8_t *tx = NULL, *rx = NULL;
> +	struct radio_info_rec *radio_rec;
> +	struct interface_info_rec *interface_rec;
> +
> +	if (!get_tx_rx_addrs(msg, &tx, &rx))
> +		return;
> +
> +	/* No radio matches the TX address, hwsim must not have created it */
> +	radio_rec = l_queue_find(radio_info, radio_info_match_addr1, tx);
> +	if (!radio_rec)
> +		return;
> +
> +	interface_rec = l_queue_find(interface_info,
> +						interface_info_match_addr, rx);
> +	if (!interface_rec)
> +		return;
> +
> +	/*
> +	 * This change is handled via nl80211 so we don't want to touch this
> +	 * interface here.
> +	 */
> +	if (interface_rec->name)
> +		return;
> +
> +	if (__atomic_sub_fetch(&interface_rec->ref, 1, __ATOMIC_SEQ_CST))
> +		return;
> +
> +	l_queue_remove(interface_info, interface_rec);
> +	l_free(interface_rec);
> +}
> +
>   static void hwsim_unicast_handler(struct l_genl_msg *msg, void *user_data)
>   {
>   	uint8_t cmd;
> @@ -1695,6 +1821,12 @@ static void hwsim_unicast_handler(struct l_genl_msg *msg, void *user_data)
>   	case HWSIM_CMD_FRAME:
>   		hwsim_frame_event(msg);
>   		break;
> +	case HWSIM_CMD_ADD_MAC_ADDR:
> +		hwsim_add_mac_event(msg);
> +		break;
> +	case HWSIM_CMD_DEL_MAC_ADDR:
> +		hwsim_del_mac_event(msg);
> +		break;
>   	default:
>   		break;
>   	}

Re: [PATCH 0/3] Handle DEL/ADD_MAC_ADDR events

[Denis Kenzior]

Hi James,

On 6/28/23 15:20, James Prestwood wrote:
> So this did work all along, I was just looking at it the wrong way.
>   - Scan randomization now works with hwsim
>   - Radios can be moved to another namespace and still work correctly
>     with hwsim
> 
> James Prestwood (3):
>    hwsim: add ADD/DEL_MAC_ADDR events
>    hwsim: move frame processing into a separate function
>    hwsim: handle ADD/DEL_MAC_ADDR events
> 
>   tools/hwsim.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 158 insertions(+), 6 deletions(-)
> 

Patch 1 & 2 applied, thanks.

Regards,
-Denis

Re: [PATCH 3/3] hwsim: handle ADD/DEL_MAC_ADDR events

[Denis Kenzior]

Hi James,

>> +        __atomic_fetch_add(&interface_rec->ref, 1, __ATOMIC_SEQ_CST);
>> +        memcpy(interface_rec->addr, rx, ETH_ALEN);
> 
> Just realized this memcpy is redundant since the interface_rec wouldn't match 
> unless the address was already the same. This case only happens when 
> randomization is off and is a consequence of the kernel having two separate 
> addresses (one prefixed with 0x4, still not sure why they do this). So you get 
> the following events (say the perm address is 02:00:00:00:00:00):
> 
> ADD(tx=42:00:00:00:00:00, rx=02:00:00:00:00:00) (Interface Creation)
> ADD(tx=42:00:00:00:00:00, rx=02:00:00:00:00:00) (Scanning started)
> DEL(tx=42:00:00:00:00:00, rx=02:00:00:00:00:00) (Scanning ended)
> 
> When scan randomization is enabled it will create a completely separate dummy 
> interface and the DEL event can freely delete that.
> 
> I can remove this in v2 if you have additional comments
> 

Ok, I amended the patch to remove the redundant memcpy and applied.  Please 
double check that I didn't screw anything up.

Regards,
-Denis