[PATCH 1/3] common: machine_id: support deriving app specific UUIDs

[PATCH 1/3] common: machine_id: support deriving app specific UUIDs

[Ahmad Fatoum]

libsystemd provides a sd_id128_get_machine_app_specific() function that
allows deriving an application specific UUID without directly leaking
the machine ID.

Let's provide an equivalent for barebox that will be used in a following
commit to generate a stable MAC instead of randomizing it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/machine_id.c  | 84 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/uuid.h |  8 +++++
 include/machine_id.h | 10 ++++++
 3 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/common/machine_id.c b/common/machine_id.c
index 8c273b934989..edc8ad0ac156 100644
--- a/common/machine_id.c
+++ b/common/machine_id.c
@@ -13,17 +13,96 @@
 
 #define MACHINE_ID_LENGTH 32
 
+static bool __machine_id_initialized;
 static void *__machine_id_hashable;
 static size_t __machine_id_hashable_length;
 
-
+/**
+ * machine_id_set_hashable - Provide per-board unique data
+ * @hashable: Buffer
+ * @len: size of buffer
+ *
+ * The data supplied to the last call of this function prior to
+ * late_initcall will be hashed and stored into global.machine_id,
+ * which can be later used for fixup into the kernel command line
+ * or for deriving application specific unique IDs via
+ * machine_id_get_app_specific().
+ */
 void machine_id_set_hashable(const void *hashable, size_t len)
 {
-
 	__machine_id_hashable = xmemdup(hashable, len);
 	__machine_id_hashable_length = len;
 }
 
+/**
+ * machine_id_get_app_specific - Generates an application-specific UUID
+ * @result: UUID output of the function
+ * @...: pairs of (const void *, size_t) arguments of data to factor
+ * into the UUID followed by a NULL sentinel value.
+ *
+ * Combines the machine ID with the application specific varargs data
+ * to arrive at an application-specific and board-specific UUID that is
+ * stable and unique.
+ *
+ * The function returns 0 if a UUID was successfully written into @result
+ * and a negative error code otherwise.
+ */
+int machine_id_get_app_specific(uuid_t *result, ...)
+{
+	static u8 hmac[SHA256_DIGEST_SIZE];
+	const void *data;
+	size_t size;
+	va_list args;
+	struct digest *d;
+	int ret;
+
+	if (!__machine_id_initialized)
+		return -ENODATA;
+
+	d = digest_alloc("hmac(sha256)");
+	if (!d)
+		return -ENOSYS;
+
+	ret = digest_set_key(d, __machine_id_hashable, __machine_id_hashable_length);
+	if (ret)
+		goto out;
+
+	ret = digest_init(d);
+	if (ret)
+		goto out;
+
+	ret = -ENODATA;
+
+	va_start(args, result);
+
+	while ((data = va_arg(args, const void *))) {
+		size = va_arg(args, size_t);
+
+		ret = digest_update(d, data, size);
+		if (ret)
+			break;
+	}
+
+	va_end(args);
+
+	if (ret)
+		goto out;
+
+	ret = digest_final(d, hmac);
+	if (ret)
+		goto out;
+
+	/* Take only the first half. */
+	memcpy(result, hmac, min(sizeof(hmac), sizeof(*result)));
+
+	uuid_make_v4(result);
+
+out:
+	digest_free(d);
+
+	return ret;
+}
+
 static int machine_id_set_globalvar(void)
 {
 	struct digest *digest = NULL;
@@ -61,6 +140,7 @@ static int machine_id_set_globalvar(void)
 	env_machine_id = basprintf("%.*s", MACHINE_ID_LENGTH, hex_machine_id);
 	globalvar_add_simple("machine_id", env_machine_id);
 	free(env_machine_id);
+	__machine_id_initialized = true;
 
 out:
 	digest_free(digest);
diff --git a/include/linux/uuid.h b/include/linux/uuid.h
index 6b1a3efa1e0b..1e4ffb343452 100644
--- a/include/linux/uuid.h
+++ b/include/linux/uuid.h
@@ -107,6 +107,14 @@ extern const u8 uuid_index[16];
 int guid_parse(const char *uuid, guid_t *u);
 int uuid_parse(const char *uuid, uuid_t *u);
 
+static inline void uuid_make_v4(uuid_t *u) {
+	/* Set UUID version to 4 --- truly random generation */
+	u->b[6] = (u->b[6] & 0x0F) | 0x40;
+
+	/* Set the UUID variant to DCE */
+	u->b[8] = (u->b[8] & 0x3F) | 0x80;
+}
+
 /* MEI UUID type, don't use anywhere else */
 #include <uapi/linux/uuid.h>
 
diff --git a/include/machine_id.h b/include/machine_id.h
index e30bbada1acd..0ed4052ec47c 100644
--- a/include/machine_id.h
+++ b/include/machine_id.h
@@ -3,9 +3,13 @@
 #ifndef __MACHINE_ID_H__
 #define __MACHINE_ID_H__
 
+#include <linux/types.h>
+#include <linux/uuid.h>
+
 #if IS_ENABLED(CONFIG_MACHINE_ID)
 
 void machine_id_set_hashable(const void *hashable, size_t len);
+int machine_id_get_app_specific(uuid_t *result, ...) __attribute__((__sentinel__));
 
 #else
 
@@ -13,6 +17,12 @@ static inline void machine_id_set_hashable(const void *hashable, size_t len)
 {
 }
 
+static inline int machine_id_get_app_specific(uuid_t *result, ...)
+	__attribute__((__sentinel__));
+{
+	return -ENOSYS;
+}
+
 #endif /* CONFIG_MACHINE_ID */
 
 #endif  /* __MACHINE_ID_H__ */
-- 
2.39.2

[PATCH 2/3] net: import Linux eth_addr_inc/dec/add helpers

[Ahmad Fatoum]

Linux has a number of helpers to do arithmetic on Ethernet addresses,
which are useful for generating sequential MAC addresses. Import them.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/net.h | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/include/net.h b/include/net.h
index 5e029c71f29e..a0ef8bee0404 100644
--- a/include/net.h
+++ b/include/net.h
@@ -449,6 +449,77 @@ static inline int is_valid_ether_addr(const u8 *addr)
 	return !is_multicast_ether_addr(addr) && !is_zero_ether_addr(addr);
 }
 
+/**
+ * ether_addr_to_u64 - Convert an Ethernet address into a u64 value.
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Return a u64 value of the address
+ */
+static inline u64 ether_addr_to_u64(const u8 *addr)
+{
+	u64 u = 0;
+	int i;
+
+	for (i = 0; i < ETH_ALEN; i++)
+		u = u << 8 | addr[i];
+
+	return u;
+}
+
+/**
+ * u64_to_ether_addr - Convert a u64 to an Ethernet address.
+ * @u: u64 to convert to an Ethernet MAC address
+ * @addr: Pointer to a six-byte array to contain the Ethernet address
+ */
+static inline void u64_to_ether_addr(u64 u, u8 *addr)
+{
+	int i;
+
+	for (i = ETH_ALEN - 1; i >= 0; i--) {
+		addr[i] = u & 0xff;
+		u = u >> 8;
+	}
+}
+
+/**
+ * eth_addr_dec - Decrement the given MAC address
+ *
+ * @addr: Pointer to a six-byte array containing Ethernet address to decrement
+ */
+static inline void eth_addr_dec(u8 *addr)
+{
+	u64 u = ether_addr_to_u64(addr);
+
+	u--;
+	u64_to_ether_addr(u, addr);
+}
+
+/**
+ * eth_addr_inc() - Increment the given MAC address.
+ * @addr: Pointer to a six-byte array containing Ethernet address to increment.
+ */
+static inline void eth_addr_inc(u8 *addr)
+{
+	u64 u = ether_addr_to_u64(addr);
+
+	u++;
+	u64_to_ether_addr(u, addr);
+}
+
+/**
+ * eth_addr_add() - Add (or subtract) an offset to/from the given MAC address.
+ *
+ * @offset: Offset to add.
+ * @addr: Pointer to a six-byte array containing Ethernet address to increment.
+ */
+static inline void eth_addr_add(u8 *addr, long offset)
+{
+	u64 u = ether_addr_to_u64(addr);
+
+	u += offset;
+	u64_to_ether_addr(u, addr);
+}
+
 typedef void rx_handler_f(void *ctx, char *packet, unsigned int len);
 
 struct eth_device *eth_get_byname(const char *name);
-- 
2.39.2

[PATCH 3/3] net: add generic MAC address derivation from machine ID

[Ahmad Fatoum]

From: Ahmad Fatoum <ahmad@a3f.at>

Especially during development, devices often lack a MAC address. While a
MAC address can be easily added to the environment:

  nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"

It's easily lost when flashing complete new images, e.g. from CI.
Make the development experience neater by deriving a stable MAC address
if possible.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/net.h |  2 ++
 net/Kconfig   | 17 +++++++++++++++++
 net/eth.c     | 11 ++++++++++-
 net/net.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/include/net.h b/include/net.h
index a0ef8bee0404..de43c29f74ac 100644
--- a/include/net.h
+++ b/include/net.h
@@ -418,6 +418,8 @@ static inline int is_broadcast_ether_addr(const u8 *addr)
 #define ETH_ALEN	6	/* Octets in an Ethernet address */
 #define ETH_HLEN	14	/* Total octets in header.*/
 
+int generate_ether_addr(u8 *addr, int ethid);
+
 /**
  * random_ether_addr - Generate software assigned random Ethernet address
  * @addr: Pointer to a six-byte array containing the Ethernet address
diff --git a/net/Kconfig b/net/Kconfig
index 59f14c23cba2..04fcdcbe5bd5 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -6,6 +6,23 @@ menuconfig NET
 
 if NET
 
+config NET_ETHADDR_FROM_MACHINE_ID
+	bool
+	prompt "generate stable Ethernet address"
+	depends on MACHINE_ID && HAVE_DIGEST_SHA256 && HAVE_DIGEST_HMAC
+	help
+	  By default, barebox will generate random Ethernet addresses for
+	  interfaces that had no explicit Ethernet address set via
+	  either board code or NVMEM properties in device tree.
+
+	  Say y here, to randomize Ethernet addresses only if no machine ID
+	  is available. Should barebox have a machine ID, it will be used
+	  alonside the hostname to generate MAC addresses that are unlikely
+	  to change between subsequent runs of barebox.
+
+	  This is not recommended for use in production as it may leak
+	  information about the machine ID.
+
 config NET_NFS
 	bool
 	prompt "nfs support"
diff --git a/net/eth.c b/net/eth.c
index ccac5e2a6488..e74c00e74a9a 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -10,6 +10,7 @@
 #include <dhcp.h>
 #include <net.h>
 #include <dma.h>
+#include <machine_id.h>
 #include <of.h>
 #include <of_net.h>
 #include <linux/phy.h>
@@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
 {
 	char str[sizeof("xx:xx:xx:xx:xx:xx")];
 	struct eth_device *edev;
+	bool generated = false;
 	int ret;
 
 	list_for_each_entry(edev, &netdev_list, list) {
@@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
 
 		ret = of_get_mac_addr_nvmem(edev->parent->of_node,
 					    edev->ethaddr);
+		if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
+			ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
+			generated = true;
+		}
 		if (ret)
 			continue;
 
 		ethaddr_to_string(edev->ethaddr, str);
-		dev_info(&edev->dev, "Got preset MAC address from device tree: %s\n", str);
+		if (generated)
+			dev_notice(&edev->dev, "Generated MAC address from unique id: %s\n", str);
+		else
+			dev_info(&edev->dev, "Got preset MAC address from NVMEM: %s\n", str);
 		eth_set_ethaddr(edev, edev->ethaddr);
 	}
 
diff --git a/net/net.c b/net/net.c
index bf2117ff7ec2..e38179491d7a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -25,6 +25,7 @@
 #include <init.h>
 #include <globalvar.h>
 #include <magicvar.h>
+#include <machine_id.h>
 #include <linux/ctype.h>
 #include <linux/err.h>
 
@@ -365,6 +366,43 @@ IPaddr_t net_get_gateway(void)
 
 static LIST_HEAD(connection_list);
 
+/**
+ * generate_ether_addr - Generates stable software assigned Ethernet address
+ * @addr: Pointer to a six-byte array to contain the Ethernet address
+ * @ethid: index of the Ethernet interface
+ *
+ * Derives an Ethernet address (MAC) from the machine ID, that's stable
+ * per board that is not multicast and has the local assigned bit set.
+ *
+ * Return 0 if an address could be generated or a negative error code otherwise.
+ */
+int generate_ether_addr(u8 *ethaddr, int ethid)
+{
+	const char *hostname;
+	uuid_t id;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID))
+		return -ENOSYS;
+
+	hostname = barebox_get_hostname();
+	if (!hostname)
+		return -EINVAL;
+
+	ret = machine_id_get_app_specific(&id, ARRAY_AND_SIZE("barebox-macaddr:"),
+					  hostname, strlen(hostname), NULL);
+	if (ret)
+		return ret;
+
+	memcpy(ethaddr, &id.b, ETH_ALEN);
+	eth_addr_add(ethaddr, ethid);
+
+	ethaddr[0] &= 0xfe;	/* clear multicast bit */
+	ethaddr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
+
+	return 0;
+}
+
 static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
 				      rx_handler_f *handler, void *ctx)
 {
@@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
 
 	if (!is_valid_ether_addr(edev->ethaddr)) {
 		char str[sizeof("xx:xx:xx:xx:xx:xx")];
-		random_ether_addr(edev->ethaddr);
+
+		ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
+		if (ret)
+			random_ether_addr(edev->ethaddr);
+
 		ethaddr_to_string(edev->ethaddr, str);
-		dev_warn(&edev->dev, "No MAC address set. Using random address %s\n", str);
+
+		dev_warn(&edev->dev, "No MAC address set. Using %s %s\n",
+			 ret == 1 ? "address computed from unique ID" : "random address",
+			 str);
 		eth_set_ethaddr(edev, edev->ethaddr);
 	}
 
-- 
2.39.2

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Sascha Hauer]

On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
> From: Ahmad Fatoum <ahmad@a3f.at>
> 
> Especially during development, devices often lack a MAC address. While a
> MAC address can be easily added to the environment:
> 
>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
> 
> It's easily lost when flashing complete new images, e.g. from CI.
> Make the development experience neater by deriving a stable MAC address
> if possible.

I like this, because my Fritzbox network overview is full of duplicate
entries coming from boards with random MAC addresses.


> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)

This function should be renamed. When reviewing this patch I asked
myself why you limit generating a fixed MAC address to the DT case until
I realized that you actually don't. I was just confused by the function
name.

>  {
>  	char str[sizeof("xx:xx:xx:xx:xx:xx")];
>  	struct eth_device *edev;
> +	bool generated = false;
>  	int ret;
>  
>  	list_for_each_entry(edev, &netdev_list, list) {
> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
>  
>  		ret = of_get_mac_addr_nvmem(edev->parent->of_node,
>  					    edev->ethaddr);
> +		if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {

This check doesn't seem to be needed, generate_ether_addr() already
returns -ENOSYS when this option is not enabled.

> +			ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> +			generated = true;
> +		}
>  		if (ret)
>  			continue;
>  
>  		ethaddr_to_string(edev->ethaddr, str);
> -		dev_info(&edev->dev, "Got preset MAC address from device tree: %s\n", str);
> +		if (generated)
> +			dev_notice(&edev->dev, "Generated MAC address from unique id: %s\n", str);
> +		else
> +			dev_info(&edev->dev, "Got preset MAC address from NVMEM: %s\n", str);
>  		eth_set_ethaddr(edev, edev->ethaddr);
>  	}
>  
> diff --git a/net/net.c b/net/net.c
> index bf2117ff7ec2..e38179491d7a 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -25,6 +25,7 @@
>  #include <init.h>
>  #include <globalvar.h>
>  #include <magicvar.h>
> +#include <machine_id.h>
>  #include <linux/ctype.h>
>  #include <linux/err.h>
>  
> @@ -365,6 +366,43 @@ IPaddr_t net_get_gateway(void)
>  
>  static LIST_HEAD(connection_list);
>  
> +/**
> + * generate_ether_addr - Generates stable software assigned Ethernet address
> + * @addr: Pointer to a six-byte array to contain the Ethernet address
> + * @ethid: index of the Ethernet interface
> + *
> + * Derives an Ethernet address (MAC) from the machine ID, that's stable
> + * per board that is not multicast and has the local assigned bit set.
> + *
> + * Return 0 if an address could be generated or a negative error code otherwise.
> + */
> +int generate_ether_addr(u8 *ethaddr, int ethid)
> +{
> +	const char *hostname;
> +	uuid_t id;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID))
> +		return -ENOSYS;
> +
> +	hostname = barebox_get_hostname();
> +	if (!hostname)
> +		return -EINVAL;
> +
> +	ret = machine_id_get_app_specific(&id, ARRAY_AND_SIZE("barebox-macaddr:"),
> +					  hostname, strlen(hostname), NULL);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(ethaddr, &id.b, ETH_ALEN);
> +	eth_addr_add(ethaddr, ethid);
> +
> +	ethaddr[0] &= 0xfe;	/* clear multicast bit */
> +	ethaddr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
> +
> +	return 0;
> +}
> +
>  static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
>  				      rx_handler_f *handler, void *ctx)
>  {
> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
>  
>  	if (!is_valid_ether_addr(edev->ethaddr)) {
>  		char str[sizeof("xx:xx:xx:xx:xx:xx")];
> -		random_ether_addr(edev->ethaddr);
> +
> +		ret = generate_ether_addr(edev->ethaddr, edev->dev.id);

For most network devices we won't get here because of_populate_ethaddr()
will already have done it for us. It's only used for devices that are
probed after postenvironment_initcall(), namely USB devices.

I think this deserves a cleanup before we merge this. The original
reason to introduce a postenvironment_initcall() for getting the MAC
address from nvmem was:

> We do this in a very late initcall, because we don't want to enforce a
> probe a probe order between nvmem providers and network devices. We
> can't do it at randomization time, because we need to fixup Ethernet mac
> addresses, even when barebox itself doesn't ifup the netdev.

I think we should have one centralized function that retrieves the MAC
address for an ethernet device. That should be called when we actually
need that MAC address, so once in net_new() and once at of_fixup time.

Right now the behaviour is inconsistent with regard to random MAC
addresses. Currently we propagate the random MAC address to the Linux
device tree when we use ethernet in barebox, but we don't propagate it
when we do not use ethernet in barebox. We should decide what the
desired behaviour is and do it consistently regardless if we use
ethernet in barebox or not. This could be cleaned up along the way.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Ahmad Fatoum]

Hi Sascha,

On 12.09.23 12:49, Sascha Hauer wrote:
> On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
>> From: Ahmad Fatoum <ahmad@a3f.at>
>>
>> Especially during development, devices often lack a MAC address. While a
>> MAC address can be easily added to the environment:
>>
>>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
>>
>> It's easily lost when flashing complete new images, e.g. from CI.
>> Make the development experience neater by deriving a stable MAC address
>> if possible.
> 
> I like this, because my Fritzbox network overview is full of duplicate
> entries coming from boards with random MAC addresses.

Glad you like it. If you are ok with the first two patches, can you take
those until I respin this?

> 
> 
>> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
> 
> This function should be renamed. When reviewing this patch I asked
> myself why you limit generating a fixed MAC address to the DT case until
> I realized that you actually don't. I was just confused by the function
> name.

Ah, didn't notice that. Will rename.

> 
>>  {
>>  	char str[sizeof("xx:xx:xx:xx:xx:xx")];
>>  	struct eth_device *edev;
>> +	bool generated = false;
>>  	int ret;
>>  
>>  	list_for_each_entry(edev, &netdev_list, list) {
>> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
>>  
>>  		ret = of_get_mac_addr_nvmem(edev->parent->of_node,
>>  					    edev->ethaddr);
>> +		if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
> 
> This check doesn't seem to be needed, generate_ether_addr() already
> returns -ENOSYS when this option is not enabled.

Until we have LTO, I'd like to keep this in, so the function need not be called.
In this instance, I find it is a less ugly alternative to have #ifdef and static inline..

>> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
>>  
>>  	if (!is_valid_ether_addr(edev->ethaddr)) {
>>  		char str[sizeof("xx:xx:xx:xx:xx:xx")];
>> -		random_ether_addr(edev->ethaddr);
>> +
>> +		ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> 
> For most network devices we won't get here because of_populate_ethaddr()
> will already have done it for us. It's only used for devices that are
> probed after postenvironment_initcall(), namely USB devices.

Or those that don't have CONFIG_NET_ETHADDR_FROM_MACHINE_ID enabled
or don't have a machine ID set.

> I think this deserves a cleanup before we merge this. The original
> reason to introduce a postenvironment_initcall() for getting the MAC
> address from nvmem was:
> 
>> We do this in a very late initcall, because we don't want to enforce a
>> probe a probe order between nvmem providers and network devices. We
>> can't do it at randomization time, because we need to fixup Ethernet mac
>> addresses, even when barebox itself doesn't ifup the netdev.
> 
> I think we should have one centralized function that retrieves the MAC
> address for an ethernet device. That should be called when we actually
> need that MAC address, so once in net_new() and once at of_fixup time.
> 
> Right now the behaviour is inconsistent with regard to random MAC
> addresses. Currently we propagate the random MAC address to the Linux
> device tree when we use ethernet in barebox, but we don't propagate it
> when we do not use ethernet in barebox. We should decide what the
> desired behaviour is and do it consistently regardless if we use
> ethernet in barebox or not. This could be cleaned up along the way.

Randomized MAC addresses are a necessary evil when barebox needs to do
networking and doesn't have a stable address. If we have a stable address,
we shouldn't randomize.

Fixing up a randomized MAC address is a lesser evil than having different
MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
we shouldn't generate a random one just to fix it up.

If we have a stable address, we should tell Linux about it.

That's the logic of the current code and it's sensible to me. Why change it?

Cheers,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Thorsten Scherer]

Hi Ahmad,

On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
> From: Ahmad Fatoum <ahmad@a3f.at>
> 
> Especially during development, devices often lack a MAC address. While a
> MAC address can be easily added to the environment:
> 
>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
> 
> It's easily lost when flashing complete new images, e.g. from CI.
> Make the development experience neater by deriving a stable MAC address
> if possible.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  include/net.h |  2 ++
>  net/Kconfig   | 17 +++++++++++++++++
>  net/eth.c     | 11 ++++++++++-
>  net/net.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net.h b/include/net.h
> index a0ef8bee0404..de43c29f74ac 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -418,6 +418,8 @@ static inline int is_broadcast_ether_addr(const u8 *addr)
>  #define ETH_ALEN	6	/* Octets in an Ethernet address */
>  #define ETH_HLEN	14	/* Total octets in header.*/
>  
> +int generate_ether_addr(u8 *addr, int ethid);
> +
>  /**
>   * random_ether_addr - Generate software assigned random Ethernet address
>   * @addr: Pointer to a six-byte array containing the Ethernet address
> diff --git a/net/Kconfig b/net/Kconfig
> index 59f14c23cba2..04fcdcbe5bd5 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -6,6 +6,23 @@ menuconfig NET
>  
>  if NET
>  
> +config NET_ETHADDR_FROM_MACHINE_ID
> +	bool
> +	prompt "generate stable Ethernet address"
> +	depends on MACHINE_ID && HAVE_DIGEST_SHA256 && HAVE_DIGEST_HMAC
> +	help
> +	  By default, barebox will generate random Ethernet addresses for
> +	  interfaces that had no explicit Ethernet address set via
> +	  either board code or NVMEM properties in device tree.
> +
> +	  Say y here, to randomize Ethernet addresses only if no machine ID
> +	  is available. Should barebox have a machine ID, it will be used
> +	  alonside the hostname to generate MAC addresses that are unlikely

s/alonside/alongside/

> +	  to change between subsequent runs of barebox.
> +
> +	  This is not recommended for use in production as it may leak
> +	  information about the machine ID.
> +
>  config NET_NFS
>  	bool
>  	prompt "nfs support"
> diff --git a/net/eth.c b/net/eth.c
> index ccac5e2a6488..e74c00e74a9a 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -10,6 +10,7 @@
>  #include <dhcp.h>
>  #include <net.h>
>  #include <dma.h>
> +#include <machine_id.h>
>  #include <of.h>
>  #include <of_net.h>
>  #include <linux/phy.h>
> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
>  {
>  	char str[sizeof("xx:xx:xx:xx:xx:xx")];
>  	struct eth_device *edev;
> +	bool generated = false;
>  	int ret;
>  
>  	list_for_each_entry(edev, &netdev_list, list) {
> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
>  
>  		ret = of_get_mac_addr_nvmem(edev->parent->of_node,
>  					    edev->ethaddr);
> +		if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
> +			ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> +			generated = true;
> +		}
>  		if (ret)
>  			continue;
>  
>  		ethaddr_to_string(edev->ethaddr, str);
> -		dev_info(&edev->dev, "Got preset MAC address from device tree: %s\n", str);
> +		if (generated)
> +			dev_notice(&edev->dev, "Generated MAC address from unique id: %s\n", str);
> +		else
> +			dev_info(&edev->dev, "Got preset MAC address from NVMEM: %s\n", str);
>  		eth_set_ethaddr(edev, edev->ethaddr);
>  	}
>  
> diff --git a/net/net.c b/net/net.c
> index bf2117ff7ec2..e38179491d7a 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -25,6 +25,7 @@
>  #include <init.h>
>  #include <globalvar.h>
>  #include <magicvar.h>
> +#include <machine_id.h>
>  #include <linux/ctype.h>
>  #include <linux/err.h>
>  
> @@ -365,6 +366,43 @@ IPaddr_t net_get_gateway(void)
>  
>  static LIST_HEAD(connection_list);
>  
> +/**
> + * generate_ether_addr - Generates stable software assigned Ethernet address
> + * @addr: Pointer to a six-byte array to contain the Ethernet address
> + * @ethid: index of the Ethernet interface
> + *
> + * Derives an Ethernet address (MAC) from the machine ID, that's stable
> + * per board that is not multicast and has the local assigned bit set.
> + *
> + * Return 0 if an address could be generated or a negative error code otherwise.
> + */
> +int generate_ether_addr(u8 *ethaddr, int ethid)
> +{
> +	const char *hostname;
> +	uuid_t id;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID))
> +		return -ENOSYS;
> +
> +	hostname = barebox_get_hostname();
> +	if (!hostname)
> +		return -EINVAL;
> +
> +	ret = machine_id_get_app_specific(&id, ARRAY_AND_SIZE("barebox-macaddr:"),
> +					  hostname, strlen(hostname), NULL);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(ethaddr, &id.b, ETH_ALEN);
> +	eth_addr_add(ethaddr, ethid);
> +
> +	ethaddr[0] &= 0xfe;	/* clear multicast bit */
> +	ethaddr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
> +
> +	return 0;
> +}
> +
>  static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
>  				      rx_handler_f *handler, void *ctx)
>  {
> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
>  
>  	if (!is_valid_ether_addr(edev->ethaddr)) {
>  		char str[sizeof("xx:xx:xx:xx:xx:xx")];
> -		random_ether_addr(edev->ethaddr);
> +
> +		ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> +		if (ret)
> +			random_ether_addr(edev->ethaddr);
> +
>  		ethaddr_to_string(edev->ethaddr, str);
> -		dev_warn(&edev->dev, "No MAC address set. Using random address %s\n", str);
> +
> +		dev_warn(&edev->dev, "No MAC address set. Using %s %s\n",
> +			 ret == 1 ? "address computed from unique ID" : "random address",
> +			 str);
>  		eth_set_ethaddr(edev, edev->ethaddr);
>  	}
>  
> -- 
> 2.39.2
> 
> 

Regards
Thorsten

Re: [PATCH 1/3] common: machine_id: support deriving app specific UUIDs

[Sascha Hauer]

On Mon, Sep 11, 2023 at 05:59:25PM +0200, Ahmad Fatoum wrote:
> libsystemd provides a sd_id128_get_machine_app_specific() function that
> allows deriving an application specific UUID without directly leaking
> the machine ID.
> 
> Let's provide an equivalent for barebox that will be used in a following
> commit to generate a stable MAC instead of randomizing it.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/machine_id.c  | 84 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/uuid.h |  8 +++++
>  include/machine_id.h | 10 ++++++
>  3 files changed, 100 insertions(+), 2 deletions(-)

Applied 1/2 and 2/2, thanks

Sascha

> 
> diff --git a/common/machine_id.c b/common/machine_id.c
> index 8c273b934989..edc8ad0ac156 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -13,17 +13,96 @@
>  
>  #define MACHINE_ID_LENGTH 32
>  
> +static bool __machine_id_initialized;
>  static void *__machine_id_hashable;
>  static size_t __machine_id_hashable_length;
>  
> -
> +/**
> + * machine_id_set_hashable - Provide per-board unique data
> + * @hashable: Buffer
> + * @len: size of buffer
> + *
> + * The data supplied to the last call of this function prior to
> + * late_initcall will be hashed and stored into global.machine_id,
> + * which can be later used for fixup into the kernel command line
> + * or for deriving application specific unique IDs via
> + * machine_id_get_app_specific().
> + */
>  void machine_id_set_hashable(const void *hashable, size_t len)
>  {
> -
>  	__machine_id_hashable = xmemdup(hashable, len);
>  	__machine_id_hashable_length = len;
>  }
>  
> +/**
> + * machine_id_get_app_specific - Generates an application-specific UUID
> + * @result: UUID output of the function
> + * @...: pairs of (const void *, size_t) arguments of data to factor
> + * into the UUID followed by a NULL sentinel value.
> + *
> + * Combines the machine ID with the application specific varargs data
> + * to arrive at an application-specific and board-specific UUID that is
> + * stable and unique.
> + *
> + * The function returns 0 if a UUID was successfully written into @result
> + * and a negative error code otherwise.
> + */
> +int machine_id_get_app_specific(uuid_t *result, ...)
> +{
> +	static u8 hmac[SHA256_DIGEST_SIZE];
> +	const void *data;
> +	size_t size;
> +	va_list args;
> +	struct digest *d;
> +	int ret;
> +
> +	if (!__machine_id_initialized)
> +		return -ENODATA;
> +
> +	d = digest_alloc("hmac(sha256)");
> +	if (!d)
> +		return -ENOSYS;
> +
> +	ret = digest_set_key(d, __machine_id_hashable, __machine_id_hashable_length);
> +	if (ret)
> +		goto out;
> +
> +	ret = digest_init(d);
> +	if (ret)
> +		goto out;
> +
> +	ret = -ENODATA;
> +
> +	va_start(args, result);
> +
> +	while ((data = va_arg(args, const void *))) {
> +		size = va_arg(args, size_t);
> +
> +		ret = digest_update(d, data, size);
> +		if (ret)
> +			break;
> +	}
> +
> +	va_end(args);
> +
> +	if (ret)
> +		goto out;
> +
> +	ret = digest_final(d, hmac);
> +	if (ret)
> +		goto out;
> +
> +	/* Take only the first half. */
> +	memcpy(result, hmac, min(sizeof(hmac), sizeof(*result)));
> +
> +	uuid_make_v4(result);
> +
> +out:
> +	digest_free(d);
> +
> +	return ret;
> +}
> +
>  static int machine_id_set_globalvar(void)
>  {
>  	struct digest *digest = NULL;
> @@ -61,6 +140,7 @@ static int machine_id_set_globalvar(void)
>  	env_machine_id = basprintf("%.*s", MACHINE_ID_LENGTH, hex_machine_id);
>  	globalvar_add_simple("machine_id", env_machine_id);
>  	free(env_machine_id);
> +	__machine_id_initialized = true;
>  
>  out:
>  	digest_free(digest);
> diff --git a/include/linux/uuid.h b/include/linux/uuid.h
> index 6b1a3efa1e0b..1e4ffb343452 100644
> --- a/include/linux/uuid.h
> +++ b/include/linux/uuid.h
> @@ -107,6 +107,14 @@ extern const u8 uuid_index[16];
>  int guid_parse(const char *uuid, guid_t *u);
>  int uuid_parse(const char *uuid, uuid_t *u);
>  
> +static inline void uuid_make_v4(uuid_t *u) {
> +	/* Set UUID version to 4 --- truly random generation */
> +	u->b[6] = (u->b[6] & 0x0F) | 0x40;
> +
> +	/* Set the UUID variant to DCE */
> +	u->b[8] = (u->b[8] & 0x3F) | 0x80;
> +}
> +
>  /* MEI UUID type, don't use anywhere else */
>  #include <uapi/linux/uuid.h>
>  
> diff --git a/include/machine_id.h b/include/machine_id.h
> index e30bbada1acd..0ed4052ec47c 100644
> --- a/include/machine_id.h
> +++ b/include/machine_id.h
> @@ -3,9 +3,13 @@
>  #ifndef __MACHINE_ID_H__
>  #define __MACHINE_ID_H__
>  
> +#include <linux/types.h>
> +#include <linux/uuid.h>
> +
>  #if IS_ENABLED(CONFIG_MACHINE_ID)
>  
>  void machine_id_set_hashable(const void *hashable, size_t len);
> +int machine_id_get_app_specific(uuid_t *result, ...) __attribute__((__sentinel__));
>  
>  #else
>  
> @@ -13,6 +17,12 @@ static inline void machine_id_set_hashable(const void *hashable, size_t len)
>  {
>  }
>  
> +static inline int machine_id_get_app_specific(uuid_t *result, ...)
> +	__attribute__((__sentinel__));
> +{
> +	return -ENOSYS;
> +}
> +
>  #endif /* CONFIG_MACHINE_ID */
>  
>  #endif  /* __MACHINE_ID_H__ */
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] common: machine_id: support deriving app specific UUIDs

[Sascha Hauer]

Hi Ahmad,

Reverted this one as a general please-test-your-static-inline-stubs
reminder ;)

Sascha

On Mon, Sep 11, 2023 at 05:59:25PM +0200, Ahmad Fatoum wrote:
> libsystemd provides a sd_id128_get_machine_app_specific() function that
> allows deriving an application specific UUID without directly leaking
> the machine ID.
> 
> Let's provide an equivalent for barebox that will be used in a following
> commit to generate a stable MAC instead of randomizing it.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/machine_id.c  | 84 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/uuid.h |  8 +++++
>  include/machine_id.h | 10 ++++++
>  3 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/common/machine_id.c b/common/machine_id.c
> index 8c273b934989..edc8ad0ac156 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -13,17 +13,96 @@
>  
>  #define MACHINE_ID_LENGTH 32
>  
> +static bool __machine_id_initialized;
>  static void *__machine_id_hashable;
>  static size_t __machine_id_hashable_length;
>  
> -
> +/**
> + * machine_id_set_hashable - Provide per-board unique data
> + * @hashable: Buffer
> + * @len: size of buffer
> + *
> + * The data supplied to the last call of this function prior to
> + * late_initcall will be hashed and stored into global.machine_id,
> + * which can be later used for fixup into the kernel command line
> + * or for deriving application specific unique IDs via
> + * machine_id_get_app_specific().
> + */
>  void machine_id_set_hashable(const void *hashable, size_t len)
>  {
> -
>  	__machine_id_hashable = xmemdup(hashable, len);
>  	__machine_id_hashable_length = len;
>  }
>  
> +/**
> + * machine_id_get_app_specific - Generates an application-specific UUID
> + * @result: UUID output of the function
> + * @...: pairs of (const void *, size_t) arguments of data to factor
> + * into the UUID followed by a NULL sentinel value.
> + *
> + * Combines the machine ID with the application specific varargs data
> + * to arrive at an application-specific and board-specific UUID that is
> + * stable and unique.
> + *
> + * The function returns 0 if a UUID was successfully written into @result
> + * and a negative error code otherwise.
> + */
> +int machine_id_get_app_specific(uuid_t *result, ...)
> +{
> +	static u8 hmac[SHA256_DIGEST_SIZE];
> +	const void *data;
> +	size_t size;
> +	va_list args;
> +	struct digest *d;
> +	int ret;
> +
> +	if (!__machine_id_initialized)
> +		return -ENODATA;
> +
> +	d = digest_alloc("hmac(sha256)");
> +	if (!d)
> +		return -ENOSYS;
> +
> +	ret = digest_set_key(d, __machine_id_hashable, __machine_id_hashable_length);
> +	if (ret)
> +		goto out;
> +
> +	ret = digest_init(d);
> +	if (ret)
> +		goto out;
> +
> +	ret = -ENODATA;
> +
> +	va_start(args, result);
> +
> +	while ((data = va_arg(args, const void *))) {
> +		size = va_arg(args, size_t);
> +
> +		ret = digest_update(d, data, size);
> +		if (ret)
> +			break;
> +	}
> +
> +	va_end(args);
> +
> +	if (ret)
> +		goto out;
> +
> +	ret = digest_final(d, hmac);
> +	if (ret)
> +		goto out;
> +
> +	/* Take only the first half. */
> +	memcpy(result, hmac, min(sizeof(hmac), sizeof(*result)));
> +
> +	uuid_make_v4(result);
> +
> +out:
> +	digest_free(d);
> +
> +	return ret;
> +}
> +
>  static int machine_id_set_globalvar(void)
>  {
>  	struct digest *digest = NULL;
> @@ -61,6 +140,7 @@ static int machine_id_set_globalvar(void)
>  	env_machine_id = basprintf("%.*s", MACHINE_ID_LENGTH, hex_machine_id);
>  	globalvar_add_simple("machine_id", env_machine_id);
>  	free(env_machine_id);
> +	__machine_id_initialized = true;
>  
>  out:
>  	digest_free(digest);
> diff --git a/include/linux/uuid.h b/include/linux/uuid.h
> index 6b1a3efa1e0b..1e4ffb343452 100644
> --- a/include/linux/uuid.h
> +++ b/include/linux/uuid.h
> @@ -107,6 +107,14 @@ extern const u8 uuid_index[16];
>  int guid_parse(const char *uuid, guid_t *u);
>  int uuid_parse(const char *uuid, uuid_t *u);
>  
> +static inline void uuid_make_v4(uuid_t *u) {
> +	/* Set UUID version to 4 --- truly random generation */
> +	u->b[6] = (u->b[6] & 0x0F) | 0x40;
> +
> +	/* Set the UUID variant to DCE */
> +	u->b[8] = (u->b[8] & 0x3F) | 0x80;
> +}
> +
>  /* MEI UUID type, don't use anywhere else */
>  #include <uapi/linux/uuid.h>
>  
> diff --git a/include/machine_id.h b/include/machine_id.h
> index e30bbada1acd..0ed4052ec47c 100644
> --- a/include/machine_id.h
> +++ b/include/machine_id.h
> @@ -3,9 +3,13 @@
>  #ifndef __MACHINE_ID_H__
>  #define __MACHINE_ID_H__
>  
> +#include <linux/types.h>
> +#include <linux/uuid.h>
> +
>  #if IS_ENABLED(CONFIG_MACHINE_ID)
>  
>  void machine_id_set_hashable(const void *hashable, size_t len);
> +int machine_id_get_app_specific(uuid_t *result, ...) __attribute__((__sentinel__));
>  
>  #else
>  
> @@ -13,6 +17,12 @@ static inline void machine_id_set_hashable(const void *hashable, size_t len)
>  {
>  }
>  
> +static inline int machine_id_get_app_specific(uuid_t *result, ...)
> +	__attribute__((__sentinel__));
> +{
> +	return -ENOSYS;
> +}
> +
>  #endif /* CONFIG_MACHINE_ID */
>  
>  #endif  /* __MACHINE_ID_H__ */
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Ahmad Fatoum]

Hello Sascha,

On 12.09.23 14:34, Ahmad Fatoum wrote:
> On 12.09.23 12:49, Sascha Hauer wrote:
>> I think this deserves a cleanup before we merge this. The original
>> reason to introduce a postenvironment_initcall() for getting the MAC
>> address from nvmem was:
>>
>>> We do this in a very late initcall, because we don't want to enforce a
>>> probe a probe order between nvmem providers and network devices. We
>>> can't do it at randomization time, because we need to fixup Ethernet mac
>>> addresses, even when barebox itself doesn't ifup the netdev.
>>
>> I think we should have one centralized function that retrieves the MAC
>> address for an ethernet device. That should be called when we actually
>> need that MAC address, so once in net_new() and once at of_fixup time.
>>
>> Right now the behaviour is inconsistent with regard to random MAC
>> addresses. Currently we propagate the random MAC address to the Linux
>> device tree when we use ethernet in barebox, but we don't propagate it
>> when we do not use ethernet in barebox. We should decide what the
>> desired behaviour is and do it consistently regardless if we use
>> ethernet in barebox or not. This could be cleaned up along the way.
> 
> Randomized MAC addresses are a necessary evil when barebox needs to do
> networking and doesn't have a stable address. If we have a stable address,
> we shouldn't randomize.
> 
> Fixing up a randomized MAC address is a lesser evil than having different
> MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
> we shouldn't generate a random one just to fix it up.
> 
> If we have a stable address, we should tell Linux about it.
> 
> That's the logic of the current code and it's sensible to me. Why change it?

We have had two boards trying to add something similar in the last couple of
months. I still need your input here on how to proceed.

Thanks,
Ahmad

> 
> Cheers,
> Ahmad
> 
>>
>> Sascha
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Sascha Hauer]

On Tue, Sep 12, 2023 at 02:34:28PM +0200, Ahmad Fatoum wrote:
> Hi Sascha,
> 
> On 12.09.23 12:49, Sascha Hauer wrote:
> > On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
> >> From: Ahmad Fatoum <ahmad@a3f.at>
> >>
> >> Especially during development, devices often lack a MAC address. While a
> >> MAC address can be easily added to the environment:
> >>
> >>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
> >>
> >> It's easily lost when flashing complete new images, e.g. from CI.
> >> Make the development experience neater by deriving a stable MAC address
> >> if possible.
> > 
> > I like this, because my Fritzbox network overview is full of duplicate
> > entries coming from boards with random MAC addresses.
> 
> Glad you like it. If you are ok with the first two patches, can you take
> those until I respin this?
> 
> > 
> > 
> >> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
> > 
> > This function should be renamed. When reviewing this patch I asked
> > myself why you limit generating a fixed MAC address to the DT case until
> > I realized that you actually don't. I was just confused by the function
> > name.
> 
> Ah, didn't notice that. Will rename.
> 
> > 
> >>  {
> >>  	char str[sizeof("xx:xx:xx:xx:xx:xx")];
> >>  	struct eth_device *edev;
> >> +	bool generated = false;
> >>  	int ret;
> >>  
> >>  	list_for_each_entry(edev, &netdev_list, list) {
> >> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
> >>  
> >>  		ret = of_get_mac_addr_nvmem(edev->parent->of_node,
> >>  					    edev->ethaddr);
> >> +		if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
> > 
> > This check doesn't seem to be needed, generate_ether_addr() already
> > returns -ENOSYS when this option is not enabled.
> 
> Until we have LTO, I'd like to keep this in, so the function need not be called.
> In this instance, I find it is a less ugly alternative to have #ifdef and static inline..
> 
> >> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
> >>  
> >>  	if (!is_valid_ether_addr(edev->ethaddr)) {
> >>  		char str[sizeof("xx:xx:xx:xx:xx:xx")];
> >> -		random_ether_addr(edev->ethaddr);
> >> +
> >> +		ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> > 
> > For most network devices we won't get here because of_populate_ethaddr()
> > will already have done it for us. It's only used for devices that are
> > probed after postenvironment_initcall(), namely USB devices.
> 
> Or those that don't have CONFIG_NET_ETHADDR_FROM_MACHINE_ID enabled
> or don't have a machine ID set.
> 
> > I think this deserves a cleanup before we merge this. The original
> > reason to introduce a postenvironment_initcall() for getting the MAC
> > address from nvmem was:
> > 
> >> We do this in a very late initcall, because we don't want to enforce a
> >> probe a probe order between nvmem providers and network devices. We
> >> can't do it at randomization time, because we need to fixup Ethernet mac
> >> addresses, even when barebox itself doesn't ifup the netdev.
> > 
> > I think we should have one centralized function that retrieves the MAC
> > address for an ethernet device. That should be called when we actually
> > need that MAC address, so once in net_new() and once at of_fixup time.
> > 
> > Right now the behaviour is inconsistent with regard to random MAC
> > addresses. Currently we propagate the random MAC address to the Linux
> > device tree when we use ethernet in barebox, but we don't propagate it
> > when we do not use ethernet in barebox. We should decide what the
> > desired behaviour is and do it consistently regardless if we use
> > ethernet in barebox or not. This could be cleaned up along the way.
> 
> Randomized MAC addresses are a necessary evil when barebox needs to do
> networking and doesn't have a stable address. If we have a stable address,
> we shouldn't randomize.
> 
> Fixing up a randomized MAC address is a lesser evil than having different
> MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
> we shouldn't generate a random one just to fix it up.
> 
> If we have a stable address, we should tell Linux about it.
> 
> That's the logic of the current code and it's sensible to me. Why change it?

Ok, makes sense. Let's go for it then.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Alexander Shiyan]

Hello.

Any news on this patch?

пт, 6 окт. 2023 г. в 14:59, Sascha Hauer <sha@pengutronix.de>:
>
> On Tue, Sep 12, 2023 at 02:34:28PM +0200, Ahmad Fatoum wrote:
> > Hi Sascha,
> >
> > On 12.09.23 12:49, Sascha Hauer wrote:
> > > On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
> > >> From: Ahmad Fatoum <ahmad@a3f.at>
> > >>
> > >> Especially during development, devices often lack a MAC address. While a
> > >> MAC address can be easily added to the environment:
> > >>
> > >>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
> > >>
> > >> It's easily lost when flashing complete new images, e.g. from CI.
> > >> Make the development experience neater by deriving a stable MAC address
> > >> if possible.
> > >
> > > I like this, because my Fritzbox network overview is full of duplicate
> > > entries coming from boards with random MAC addresses.
> >
> > Glad you like it. If you are ok with the first two patches, can you take
> > those until I respin this?
> >
> > >
> > >
> > >> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
> > >
> > > This function should be renamed. When reviewing this patch I asked
> > > myself why you limit generating a fixed MAC address to the DT case until
> > > I realized that you actually don't. I was just confused by the function
> > > name.
> >
> > Ah, didn't notice that. Will rename.
> >
> > >
> > >>  {
> > >>    char str[sizeof("xx:xx:xx:xx:xx:xx")];
> > >>    struct eth_device *edev;
> > >> +  bool generated = false;
> > >>    int ret;
> > >>
> > >>    list_for_each_entry(edev, &netdev_list, list) {
> > >> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
> > >>
> > >>            ret = of_get_mac_addr_nvmem(edev->parent->of_node,
> > >>                                        edev->ethaddr);
> > >> +          if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
> > >
> > > This check doesn't seem to be needed, generate_ether_addr() already
> > > returns -ENOSYS when this option is not enabled.
> >
> > Until we have LTO, I'd like to keep this in, so the function need not be called.
> > In this instance, I find it is a less ugly alternative to have #ifdef and static inline..
> >
> > >> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
> > >>
> > >>    if (!is_valid_ether_addr(edev->ethaddr)) {
> > >>            char str[sizeof("xx:xx:xx:xx:xx:xx")];
> > >> -          random_ether_addr(edev->ethaddr);
> > >> +
> > >> +          ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> > >
> > > For most network devices we won't get here because of_populate_ethaddr()
> > > will already have done it for us. It's only used for devices that are
> > > probed after postenvironment_initcall(), namely USB devices.
> >
> > Or those that don't have CONFIG_NET_ETHADDR_FROM_MACHINE_ID enabled
> > or don't have a machine ID set.
> >
> > > I think this deserves a cleanup before we merge this. The original
> > > reason to introduce a postenvironment_initcall() for getting the MAC
> > > address from nvmem was:
> > >
> > >> We do this in a very late initcall, because we don't want to enforce a
> > >> probe a probe order between nvmem providers and network devices. We
> > >> can't do it at randomization time, because we need to fixup Ethernet mac
> > >> addresses, even when barebox itself doesn't ifup the netdev.
> > >
> > > I think we should have one centralized function that retrieves the MAC
> > > address for an ethernet device. That should be called when we actually
> > > need that MAC address, so once in net_new() and once at of_fixup time.
> > >
> > > Right now the behaviour is inconsistent with regard to random MAC
> > > addresses. Currently we propagate the random MAC address to the Linux
> > > device tree when we use ethernet in barebox, but we don't propagate it
> > > when we do not use ethernet in barebox. We should decide what the
> > > desired behaviour is and do it consistently regardless if we use
> > > ethernet in barebox or not. This could be cleaned up along the way.
> >
> > Randomized MAC addresses are a necessary evil when barebox needs to do
> > networking and doesn't have a stable address. If we have a stable address,
> > we shouldn't randomize.
> >
> > Fixing up a randomized MAC address is a lesser evil than having different
> > MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
> > we shouldn't generate a random one just to fix it up.
> >
> > If we have a stable address, we should tell Linux about it.
> >
> > That's the logic of the current code and it's sensible to me. Why change it?
>
> Ok, makes sense. Let's go for it then.
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Sascha Hauer]

On Thu, Oct 12, 2023 at 05:59:45PM +0300, Alexander Shiyan wrote:
> Hello.
> 
> Any news on this patch?

It's Ahmad's turn to integrate the remaining review topic(s) and resend.

Sascha

> 
> пт, 6 окт. 2023 г. в 14:59, Sascha Hauer <sha@pengutronix.de>:
> >
> > On Tue, Sep 12, 2023 at 02:34:28PM +0200, Ahmad Fatoum wrote:
> > > Hi Sascha,
> > >
> > > On 12.09.23 12:49, Sascha Hauer wrote:
> > > > On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
> > > >> From: Ahmad Fatoum <ahmad@a3f.at>
> > > >>
> > > >> Especially during development, devices often lack a MAC address. While a
> > > >> MAC address can be easily added to the environment:
> > > >>
> > > >>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
> > > >>
> > > >> It's easily lost when flashing complete new images, e.g. from CI.
> > > >> Make the development experience neater by deriving a stable MAC address
> > > >> if possible.
> > > >
> > > > I like this, because my Fritzbox network overview is full of duplicate
> > > > entries coming from boards with random MAC addresses.
> > >
> > > Glad you like it. If you are ok with the first two patches, can you take
> > > those until I respin this?
> > >
> > > >
> > > >
> > > >> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
> > > >
> > > > This function should be renamed. When reviewing this patch I asked
> > > > myself why you limit generating a fixed MAC address to the DT case until
> > > > I realized that you actually don't. I was just confused by the function
> > > > name.
> > >
> > > Ah, didn't notice that. Will rename.
> > >
> > > >
> > > >>  {
> > > >>    char str[sizeof("xx:xx:xx:xx:xx:xx")];
> > > >>    struct eth_device *edev;
> > > >> +  bool generated = false;
> > > >>    int ret;
> > > >>
> > > >>    list_for_each_entry(edev, &netdev_list, list) {
> > > >> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
> > > >>
> > > >>            ret = of_get_mac_addr_nvmem(edev->parent->of_node,
> > > >>                                        edev->ethaddr);
> > > >> +          if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
> > > >
> > > > This check doesn't seem to be needed, generate_ether_addr() already
> > > > returns -ENOSYS when this option is not enabled.
> > >
> > > Until we have LTO, I'd like to keep this in, so the function need not be called.
> > > In this instance, I find it is a less ugly alternative to have #ifdef and static inline..
> > >
> > > >> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
> > > >>
> > > >>    if (!is_valid_ether_addr(edev->ethaddr)) {
> > > >>            char str[sizeof("xx:xx:xx:xx:xx:xx")];
> > > >> -          random_ether_addr(edev->ethaddr);
> > > >> +
> > > >> +          ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
> > > >
> > > > For most network devices we won't get here because of_populate_ethaddr()
> > > > will already have done it for us. It's only used for devices that are
> > > > probed after postenvironment_initcall(), namely USB devices.
> > >
> > > Or those that don't have CONFIG_NET_ETHADDR_FROM_MACHINE_ID enabled
> > > or don't have a machine ID set.
> > >
> > > > I think this deserves a cleanup before we merge this. The original
> > > > reason to introduce a postenvironment_initcall() for getting the MAC
> > > > address from nvmem was:
> > > >
> > > >> We do this in a very late initcall, because we don't want to enforce a
> > > >> probe a probe order between nvmem providers and network devices. We
> > > >> can't do it at randomization time, because we need to fixup Ethernet mac
> > > >> addresses, even when barebox itself doesn't ifup the netdev.
> > > >
> > > > I think we should have one centralized function that retrieves the MAC
> > > > address for an ethernet device. That should be called when we actually
> > > > need that MAC address, so once in net_new() and once at of_fixup time.
> > > >
> > > > Right now the behaviour is inconsistent with regard to random MAC
> > > > addresses. Currently we propagate the random MAC address to the Linux
> > > > device tree when we use ethernet in barebox, but we don't propagate it
> > > > when we do not use ethernet in barebox. We should decide what the
> > > > desired behaviour is and do it consistently regardless if we use
> > > > ethernet in barebox or not. This could be cleaned up along the way.
> > >
> > > Randomized MAC addresses are a necessary evil when barebox needs to do
> > > networking and doesn't have a stable address. If we have a stable address,
> > > we shouldn't randomize.
> > >
> > > Fixing up a randomized MAC address is a lesser evil than having different
> > > MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
> > > we shouldn't generate a random one just to fix it up.
> > >
> > > If we have a stable address, we should tell Linux about it.
> > >
> > > That's the logic of the current code and it's sensible to me. Why change it?
> >
> > Ok, makes sense. Let's go for it then.
> >
> > Sascha
> >
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] net: add generic MAC address derivation from machine ID

[Ahmad Fatoum]

Hello Alexander,

On 13.10.23 13:11, Sascha Hauer wrote:
> On Thu, Oct 12, 2023 at 05:59:45PM +0300, Alexander Shiyan wrote:
>> Hello.
>>
>> Any news on this patch?
> 
> It's Ahmad's turn to integrate the remaining review topic(s) and resend.

I don't mind if you send the v2. It'll take me at least a week until
I get around to respin this.

Thanks,
Ahmad

> 
> Sascha
> 
>>
>> пт, 6 окт. 2023 г. в 14:59, Sascha Hauer <sha@pengutronix.de>:
>>>
>>> On Tue, Sep 12, 2023 at 02:34:28PM +0200, Ahmad Fatoum wrote:
>>>> Hi Sascha,
>>>>
>>>> On 12.09.23 12:49, Sascha Hauer wrote:
>>>>> On Mon, Sep 11, 2023 at 05:59:27PM +0200, Ahmad Fatoum wrote:
>>>>>> From: Ahmad Fatoum <ahmad@a3f.at>
>>>>>>
>>>>>> Especially during development, devices often lack a MAC address. While a
>>>>>> MAC address can be easily added to the environment:
>>>>>>
>>>>>>   nv dev.eth0.ethaddr="aa:bb:cc:dd:ee:ff"
>>>>>>
>>>>>> It's easily lost when flashing complete new images, e.g. from CI.
>>>>>> Make the development experience neater by deriving a stable MAC address
>>>>>> if possible.
>>>>>
>>>>> I like this, because my Fritzbox network overview is full of duplicate
>>>>> entries coming from boards with random MAC addresses.
>>>>
>>>> Glad you like it. If you are ok with the first two patches, can you take
>>>> those until I respin this?
>>>>
>>>>>
>>>>>
>>>>>> @@ -558,6 +559,7 @@ static int of_populate_ethaddr(void)
>>>>>
>>>>> This function should be renamed. When reviewing this patch I asked
>>>>> myself why you limit generating a fixed MAC address to the DT case until
>>>>> I realized that you actually don't. I was just confused by the function
>>>>> name.
>>>>
>>>> Ah, didn't notice that. Will rename.
>>>>
>>>>>
>>>>>>  {
>>>>>>    char str[sizeof("xx:xx:xx:xx:xx:xx")];
>>>>>>    struct eth_device *edev;
>>>>>> +  bool generated = false;
>>>>>>    int ret;
>>>>>>
>>>>>>    list_for_each_entry(edev, &netdev_list, list) {
>>>>>> @@ -566,11 +568,18 @@ static int of_populate_ethaddr(void)
>>>>>>
>>>>>>            ret = of_get_mac_addr_nvmem(edev->parent->of_node,
>>>>>>                                        edev->ethaddr);
>>>>>> +          if (ret && IS_ENABLED(CONFIG_NET_ETHADDR_FROM_MACHINE_ID)) {
>>>>>
>>>>> This check doesn't seem to be needed, generate_ether_addr() already
>>>>> returns -ENOSYS when this option is not enabled.
>>>>
>>>> Until we have LTO, I'd like to keep this in, so the function need not be called.
>>>> In this instance, I find it is a less ugly alternative to have #ifdef and static inline..
>>>>
>>>>>> @@ -381,9 +419,16 @@ static struct net_connection *net_new(struct eth_device *edev, IPaddr_t dest,
>>>>>>
>>>>>>    if (!is_valid_ether_addr(edev->ethaddr)) {
>>>>>>            char str[sizeof("xx:xx:xx:xx:xx:xx")];
>>>>>> -          random_ether_addr(edev->ethaddr);
>>>>>> +
>>>>>> +          ret = generate_ether_addr(edev->ethaddr, edev->dev.id);
>>>>>
>>>>> For most network devices we won't get here because of_populate_ethaddr()
>>>>> will already have done it for us. It's only used for devices that are
>>>>> probed after postenvironment_initcall(), namely USB devices.
>>>>
>>>> Or those that don't have CONFIG_NET_ETHADDR_FROM_MACHINE_ID enabled
>>>> or don't have a machine ID set.
>>>>
>>>>> I think this deserves a cleanup before we merge this. The original
>>>>> reason to introduce a postenvironment_initcall() for getting the MAC
>>>>> address from nvmem was:
>>>>>
>>>>>> We do this in a very late initcall, because we don't want to enforce a
>>>>>> probe a probe order between nvmem providers and network devices. We
>>>>>> can't do it at randomization time, because we need to fixup Ethernet mac
>>>>>> addresses, even when barebox itself doesn't ifup the netdev.
>>>>>
>>>>> I think we should have one centralized function that retrieves the MAC
>>>>> address for an ethernet device. That should be called when we actually
>>>>> need that MAC address, so once in net_new() and once at of_fixup time.
>>>>>
>>>>> Right now the behaviour is inconsistent with regard to random MAC
>>>>> addresses. Currently we propagate the random MAC address to the Linux
>>>>> device tree when we use ethernet in barebox, but we don't propagate it
>>>>> when we do not use ethernet in barebox. We should decide what the
>>>>> desired behaviour is and do it consistently regardless if we use
>>>>> ethernet in barebox or not. This could be cleaned up along the way.
>>>>
>>>> Randomized MAC addresses are a necessary evil when barebox needs to do
>>>> networking and doesn't have a stable address. If we have a stable address,
>>>> we shouldn't randomize.
>>>>
>>>> Fixing up a randomized MAC address is a lesser evil than having different
>>>> MAC addresses for barebox and Linux. If we don't need a MAC address in barebox,
>>>> we shouldn't generate a random one just to fix it up.
>>>>
>>>> If we have a stable address, we should tell Linux about it.
>>>>
>>>> That's the logic of the current code and it's sensible to me. Why change it?
>>>
>>> Ok, makes sense. Let's go for it then.
>>>
>>> Sascha
>>>
>>> --
>>> Pengutronix e.K.                           |                             |
>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |