sys/luid: add luid_get_eui48() / luid_get_eui64()

[benpicco]

Contribution description

The luid_get() function is used to generate a hardware address for networking interfaces.
For this, the lowest to bits of the first byte are always set to a defined value.
This is problematic as the current luid_get() implementation will only increment the first byte, resulting in a different address on every third invocation only:

0: d2 ae 0c 1b 20 54 05 8f 
1: d2 ae 0c 1b 20 54 05 8f 
2: d2 ae 0c 1b 20 54 05 8f 
3: d6 ae 0c 1b 20 54 05 8f 
4: d6 ae 0c 1b 20 54 05 8f 
5: d6 ae 0c 1b 20 54 05 8f 
6: d6 ae 0c 1b 20 54 05 8f 
7: da ae 0c 1b 20 54 05 8f 
8: da ae 0c 1b 20 54 05 8f 
9: da ae 0c 1b 20 54 05 8f 

To fix this and not duplicate the EUI48/64 generation logic in every driver, add dedicated functions for that use-case.

0: 06 13 23 23 06 13 23 22 
1: 06 13 23 23 06 13 23 21 
2: 06 13 23 23 06 13 23 20 
3: 06 13 23 23 06 13 23 27 
4: 06 13 23 23 06 13 23 26 
5: 06 13 23 23 06 13 23 25 
6: 06 13 23 23 06 13 23 24 
7: 06 13 23 23 06 13 23 2b 
8: 06 13 23 23 06 13 23 2a 
9: 06 13 23 23 06 13 23 29 

@herjulf wanted to source the MAC address from an EUI64MAC chip if no CPU id is present.
With this PR this is possible by implementing a custom size_t luid_get_eui64_custom(eui64_t *addr, uint8_t idx) function.

This also adds a unit test to ensure that two calls to luid_get_eui64_custom() have different results and two calls to luid_custom() have the same result if the same generator is used.

Testing procedure

Run make -C tests/unittests tests-luid.

Also make sure that unique hardware addresses for network interfaces are still generated.
Drivers are not yet converted to the new API.

Issues/PRs references

came up in #12537

[maribu]

@herjulf wanted to source the MAC address from an EUI64MAC chip if no CPU id is present.
With this PR this is trivially possibly by implementing a custom void luid_base(void *buf, size_t len) function.

Hmm, I don't think this is the right way to do this. luid is intended to generate locally unique IDs. This makes it suitable for generating MAC addresses, but that is only one of many places it could be used.

The data luid_get() also depends on the order of invocation. E.g. if someone is having other modules that happen to be called in auto_init() prior to the network device driver in question, simply providing a custom luid_base() that returns the intended L2 address will not work.

There was some discussion about having a generic network device configuration module using the netdev_driver_t::get()/netdev_driver_t::set() API that would allow setting up common stuff like TX power, L2 addresses etc. in a driver independet way. There was some talk of having a submodule for e.g. parsing EEPROM configuration data and using this to configure the network devices. I think that this is a promising idea for such tasks. Until some infrastructure like that pops up, I would however prefer to use the foo_params_t and add an l2_addr value there, if a compile time specified default address is really needed.

[benpicco]

Hmm, I don't think this is the right way to do this. luid is intended to generate locally unique IDs. This makes it suitable for generating MAC addresses, but that is only one of many places it could be used.

It still does generate locally unique IDs. The first one will be equal to the EUI64MAC (or CPU ID if the former does not exist), the others will be derived from it.

If you don't care about the content and just want a unique but constant ID, just keep calling luid_get().

If you want the value verbatim, you can still use luid_custom(buf, len, 0) or just call luid_base() directly.

[maribu]

If you want the value verbatim, you can still use luid_custom(buf, len, 0) or just call luid_base() directly.

If one would want to access a compile time constant byte array in verbatim, one could just declare it as a global byte array and access it directly. I see no reason to have a fancy API in that use case.

Being able to override luid_base() still can be very valuable. E.g. a board that uses an MCU that has no CPU ID, but that e.g. has a dallas 1-wire temperature sensor (with a unique ID in it), this could make life much easier. But to me overwriting luid_base() to manually set the L2 address to a specific value is overly complex.

[maribu]

Back to the issue with callers of luid_get() manually clearing/setting some bits of generated ID: This naturally will result in IDs no longer being unique. Mixing the 8 bit counter into all bytes of the returned ID will solve the issue to some degree, but not in every case: Let's assume someone whats 3 locally unique 4 bit IDs.

Maybe a better solution would be to an additional function like this:

typedef enum {
    LUID_LEADING_ZEROS_AT_LSB, /**< the least significat byte will be filled with leading zeros */
    LUID_LEADING_ZEROS_AT_MSB, /**< the most significat byte will be filled with leading zeros */
    LUID_ALIGN_NUMOF, /**< Number of alignment options */
} luid_align_t;

/**
 * @brief Generate a locally unique number with the given number of bits
 * @param buf The generated ID is stored here
 * @param n_bits The bit-length of the ID to generate
 * @param alignment Should leading zeros be added to the first or last byte of the ID
 *
 * If @p n_bits is a multiple of 8, this will be equivalent to a call of `luid_get(buf, n_bits/8);`
 */
luid_get_bits(void *buf, size_t n_bits, luid_align_t alignment);

In the given case one could use this like this:

uint8_t euid64[8];
luid_get_bits(euid64, 62, LUID_LEADING_ZEROS_AT_LSB);
euid64[0] <<= 2;

[benpicco]

Hm, I didn't want to change the luid API with this PR, but rather fix the existing implementation while keeping the API.

The API already says

The resulting LUID will repeat after 255 calls.

Which IMHO should be enough for most applications.
If this becomes a problem, we can fix it in a separate PR.

[maribu]

but rather fix the existing implementation

There is nothing nothing to fix in LUID. When I have e.g. four at86rf233 devices, four 64 bit locally unique IDs are requested and correctly delivered. The driver than manipulates the IDs by clearing some bits. As a result, the IDs are no longer IDs. The bug is that the driver manipulates the ID.

Also: People might actually rely on the value luid_get() returns. Changing the algorithm to something else could be a frustrating thing to do.

The problem the driver (incorrectly) tries to solve is to get a 62 bit locally unique ID. This is currently not possible with the API. Changing the algorithm to mitigate (rather than fix!) the issue does not really solve the general issue: It would still be impossible to generate e.g. 7 bit IDs.

[maribu]

NACK. Doesn't solve the general issue and breaks LUID for users relying on LUID to keep generate the same IDs for their hardware.

[maribu]

(If you would drop the change of the algorithm and only keep luid_base() overridable and change the test of LUID to only check if the first 256 calls to luid_get() do indeed generate unique IDs, I'd ACK.)

[benpicco]

I don't think there is a nice way to fix this and not change the generated MAC addresses in some way.
#12600 will also lead to different addresses being generated.

[maribu]

For the at86rf2xx I see no way to fix it without changing the L2 addresses. I haven't checked if the other 802.15.4 drivers also have the same bug, but at least non 802.15.4 transceivers should not be affected and could keep their current L2 addresses (when used with the same CPU ID).

[benpicco]

non 802.15.4 transceivers should not be affected

When I look at e.g. usbus/cdc/ecm/cdc_ecm.c it calls eui48_set_local() and eui48_clear_group() which does

addr->uint8[0] |= EUI48_LOCAL_FLAG;
addr->uint8[0] &= ~EUI48_GROUP_FLAG;

[miri64]

I think this is the more saner approach compared to changing all device drivers (and potentially doing something wrong because byte-order issues tend to put knots in peoples brains). I always found it weird that the entropy of luid is so small and only the first byte changes (though it's main use is generation of MAC addresses, where the first bytes are usually the manufacturer ID and the device ID in the later bytes is the one that changes often).

[maribu]

I always found it weird that the entropy of luid is so small and only the first byte changes

I think that this is a good design in context of the use case described by the API: Generating IDs that are

1. Guaranteed to be unique on the system they are generated
2. Are as likely as possible to be unique in the environment, if neighbors are using the same generation pattern

the first bytes are usually the manufacturer ID and the device ID in the later bytes is the one that changes often

I also believe this to be true most of the time.

With those two things in mind let's say a huge company is installing smart light bulbs. They would very likely buy a huge number of identical light bulbs in a single transaction. Those will likely contain MCUs produces by the same manufacturer on the same day. So if the MCU manufacturer has the id 0xaf, 0xfe and the MCU model has the id 0x13, 0x37, we could likely end up with three light bulbs having almost the same CPU ID, e.g.:

1. 0xaf, 0xfe, 0x13, 0x37, 0x42, 0x01
2. 0xaf, 0xfe, 0x13, 0x37, 0x42, 0x02
3. 0xaf, 0xfe, 0x13, 0x37, 0x42, 0x03

So if indeed that head of the CPU IDs is often identical, the trailer is the part containing most of the "uniqueness", and the first byte is the one containing the "least amount of uniqueness". If we only modify the first byte for subsequent calls of luid_get(), the chances of collisions with the LUIDs of other devices are the least.

Because of this, I think that only changing the first byte is a good design choice.

Also: I like the fact that LUIDs (with at least a few bytes in length) generated on the same node can often easily be linked to that node. E.g. if devices would have multiple 802.15.4 transceivers, I could (often) easily see which long addresses belong to the same device.

[maribu]

non 802.15.4 transceivers should not be affected

When I look at e.g. usbus/cdc/ecm/cdc_ecm.c it calls eui48_set_local() and eui48_clear_group() which does

addr->uint8[0] |= EUI48_LOCAL_FLAG;
addr->uint8[0] &= ~EUI48_GROUP_FLAG;

☹️

How about adding luid_get_eui48() and luid_get_eui64() which guarantee to return locally unique IDs with the two magic bits set to indicate unicast and no global uniqueness? That should also de-duplicate some code.

[herjulf]

Brain is slow... luid_get_ can in theory clash with any vendor registered OUI? The EUI48_LOCAL_FLAG to save us? The bit even vaild for EUI64.

No private or usable OUI's?

[schlatterbeck]

I'm following this discussion.
Our use-case is setting a globally-unique eui64 address (from a hardware module or an eeprom, see issues/12616)
Concerning luid_base: I'm with maribu here that luid_base is the wrong way to do it. My approach would be to provide a set_eui64_address (name to be discussed) lib function that optionally calls back into the board code. This generic implementation would be called by each driver that doesn't have an identifier in the card. In this way my use-case (a globally unique ID) could be implemented as could be other use-cases. But this would mean a change to almost any driver currently out there (but would factor the luid stuff out of the drivers which is good imo).
This generic set_eui64_address function could use luid_base as previously. But if we come up with a different way to do it, it could be changed at one point in the code in the future. So going the rough way of changing all the drivers now is imo a good idea.

[miri64]

The more I think about it I come to the conclusion that this should be a different module, as it is basically completely changing the implementation. As for luid "classic": I think for the use-case of using its result as MAC address it suffices that the LSB instead of the MSB is changing on re-calls. I will open a PR for that.

[miri64]

(this is a NACK in case this stays luid)

[maribu]

I think for the use-case of using its result as MAC address it suffices that the LSB instead of the MSB is changing on re-calls.

@miri64: Keep in mind that the LSB of the CPU ID is the one containing the most "uniqueness". The best way to go for me would be to simply append the counter to the CPU ID, rather than modifying byte of the CPU ID. That way it is guaranteed for LUIDs (with length >= CPU-ID-length + 1) to never collide, if the CPU IDs didn't collide.

[schlatterbeck]

On Mon, Nov 04, 2019 at 03:58:20AM -0800, Koen Zandberg wrote: > Instead of this `mac_idx` counter, maybe something along the lines of the driver name (used already in the thread name) and the position in the `xxx_params_t` array is sufficient. Still a bit of a hack though :)

Hmm, having a driver-specific info would be good if one interface has a paid-for unique mac address. In that case the driver info helps to distinguish for which interfaces a board-specific callback wants to set the MAC if there is more than one interface. See #12616 for my use-case. So maybe a hack for the use-case of assigning a random mac but very useful for a fixed unique mac. Ralf -- Dr. Ralf Schlatterbeck Tel: +43/2243/26465-16 Open Source Consulting www: http://www.runtux.com Reichergasse 131, A-3411 Weidling email: office@runtux.com

[maribu]

Hmm, having a driver-specific info would be good if one interface has a paid-for unique mac address. In that case the driver info helps to distinguish for which interfaces a board-specific callback wants to set the MAC if there is more than one interface.

See #12616 for my use-case.

So maybe a hack for the use-case of assigning a random mac but very useful for a fixed unique mac.

I agree that code wanting to assign specific L2 addresses to specific network devices have tremendous interest in knowing which device they are just assigning an address. However, this info should not be exposed to LUID in my opinion, as LUID is only about providing locally unique IDs. So to me, it should not be concerned with any details of the network stack (or other users of LUIDs) (*). This information is easily available in the network modules, so to my opinion the L2 address assignment should be done there (with calls to luid_get_eui64() and friends by default).

(*) with the exception of the format of the LUID. Having callers of LUID meddle with the generated IDs risks creating collisions even on the same boards (for less than 256 IDs), without carefully checking the implementation of LUID.

[schlatterbeck]

On Mon, Nov 04, 2019 at 05:54:05AM -0800, Marian Buschsieweke wrote: I agree that code wanting to assign specific L2 addresses to specific network devices have tremendous interest in knowing which device they are just assigning an address. However, this info should not be exposed to LUID in my opinion, [...]

Of course not, but factoring out the MAC-Adress generation of all boards into a non board-specific method that is called by all hardware drivers would ease integration of different methods for MAC-Adress generation later on (or allow override in a board-specific package). Maybe this can be done with this patch, too. Ralf -- Dr. Ralf Schlatterbeck Tel: +43/2243/26465-16 Open Source Consulting www: http://www.runtux.com Reichergasse 131, A-3411 Weidling email: office@runtux.com

[benpicco]

OK, now since the consensus seems to be that there should be a two-layer approach, this PR now only deals with adding luid_get_euiXX() convenience functions. I moved decision as to whether a luid-based address or a board-supplied one should be used to #12641.

[maribu]

ACK. Thanks for addressing this!

[maribu]

Ran all unit tests (including the one on LUID provided by this PR) on a Nucleo-F767ZI:

main(): This is RIOT! (Version: 2020.01-devel-388-gb33d9-ben)
.................................................................................................................................................................................................................................................................................
.................................................................................................................................................................................................................................................................................
.................................................................................................................................................................................................................................................................................
.............................................................................................................................
OK (944 tests)

[maribu]

@benpicco: I think you can feel free to squash.

[benpicco]

Ran all unit tests (including the one on LUID provided by this PR) on a Nucleo-F767ZI:

You could have saved yourself some time and only run make tests-luid

[maribu]

You could have saved yourself some time and only run make tests-luid

The F767ZI executes the additional tests faster than I can type tests-luid 😆

My comments and concerns were addressed. Let's go ahead with this.

[miri64]

(Let's run the tests, just to be sure).