Improvement for MAC address uniqueness of if_epair(4)

[pizzamig]

Summary

As reported in 2 PRs (PR176671 and PR184149) it can happen that epair devices can have the same MAC address.

My proposal, following what hrs@ propose in PR 184149 is to use the same if_index for both interfaces.
Moreover, I add some bits of the hostid to mitigate the problem reported by Olivier in PR176671, adopting the same approach used by if_bridge(4), as suggested by Olivier itself.
Following this approach, all element of the MAC address are used to improve uniqueness.

Test Plan

Already tested on CURRENT on my system to solve duplication issues.

[bz]

Apart from the style issue, I think this can be fine.

[eugen_grosbein.net]

Why do you bother calling getcredhostid (which involves mutex lock/inlock) and arc4random second time while creating second interface?
Can't you just use already computed values from scb->oifp->if_hw_addr ?

[wollman]

I'd like to see a few more bits of uniqueness here; I don't trust that two bytes of hostid is going to be sufficient. Nonetheless, this is a good thing. (We probably won't use it -- we embed the host's canonical IPv4 address into the MAC -- but it's important that the default out-of-the-box config be usable!)

Needs style(9) fixes as others have noted.

EDIT: I just checked a bunch of servers that I'm responsible for. 17 of them have a hostid of 0x80000000, 8 have a hostid of 0, and 11 more have different non-unique host IDs.

[luca.pizzamiglio_gmail.com]

Fix style, re-using computer values from scb->oifp->if_hw_addr

[pizzamig]

I'm not 100% happy with hostid as well, I've just followed what already implemented in if_bridge(4).
I can just use arc4rand for those 2 bytes, that should help to mitigate possible collisions and simplify the code.
What do you think?

[eugen_grosbein.net]

Using hostid should be fine as it adds protection against inter-jail MAC collisions. Just reuse it when possible instead of calling second time for it.

[eugen_grosbein.net]

sys/net/if_epair.c

732 ↗

(On Diff #42226)

This is unneeded since you do not use hostid inside this block but reuse data from first interface.

[pizzamig]

Further cleaning, remove dead code

[allanjude]

It might not make sense here, since it would reduce the available space for randomization, but just as a note, the FreeBSD Project has its own OUI space: 58:9C:FC:??:??:??

sys/net/if_epair.c

852 ↗

(On Diff #42231)

Does the if_index usually have a value greater than 255 for this to be anything other than 0?

[eugen_grosbein.net]

Well, I have many hosts having over housand of ngXXX interfaces, so yes, two bytes are needed at least.

[wollman]

In D15329#323193, @eugen_grosbein.net wrote:

Well, I have many hosts having over housand of ngXXX interfaces, so yes, two bytes are needed at least.

It's probably worth a little bit of thought as to what is the more common case, a zillion epair interfaces on one host (read: half a zillion vnet jails) or a much smaller number of epairs on a larger number of hosts. It sounds like you are well placed to assign your own addresses, whatever the default may be.

[wollman]

sys/net/if_epair.c

737 ↗

(On Diff #42231)

Why not just bcopy() the entire address and just change the last octet? It would be clearer.

[eugen_grosbein.net]

sys/net/if_epair.c

737 ↗

(On Diff #42231)

Why not just bcopy() the entire address and just change the last octet? It would be clearer.

Nice catch, but memcpy() might be better to get preferences of compiler's built-in.

[eugen_grosbein.net]

In D15329#323385, @wollman wrote:

It's probably worth a little bit of thought as to what is the more common case, a zillion epair interfaces on one host (read: half a zillion vnet jails) or a much smaller number of epairs on a larger number of hosts. It sounds like you are well placed to assign your own addresses, whatever the default may be.

Well, this is not network hot path and we now have jenkins_hash32() kernel function in all supported branches, so we could just allocate additional uint32_t key[3] array on stack of this function, copy there all 64 bits of (long)hostid plus (uint32_t)if_index and make use of jenkins_hash32() to get them mixed to 32-bit hash value. Then use it as-is for eaddr[1]-eaddr[4] similar to current manner or switch to using 58:9C:FC for upper half of MAC and use lower 24 bits of the hash for the rest of MAC resetting lowest bit to 0 for first interface and to 1 for second one.

[wollman]

In D15329#323510, @eugen_grosbein.net wrote:

In D15329#323385, @wollman wrote:

It's probably worth a little bit of thought as to what is the more common case, a zillion epair interfaces on one host (read: half a zillion vnet jails) or a much smaller number of epairs on a larger number of hosts. It sounds like you are well placed to assign your own addresses, whatever the default may be.

Well, this is not network hot path and we now have jenkins_hash32() kernel function in all supported branches, so we could just allocate additional uint32_t key[3] array on stack of this function, copy there all 64 bits of (long)hostid plus (uint32_t)if_index and make use of jenkins_hash32() to get them mixed to 32-bit hash value. Then use it as-is for eaddr[1]-eaddr[4] similar to current manner or switch to using 58:9C:FC for upper half of MAC and use lower 24 bits of the hash for the rest of MAC resetting lowest bit to 0 for first interface and to 1 for second one.

That would satisfy my objection, for sure. (Still need to set the LAA bit even if we use the official FreeBSD OUI, FWIW.)

[pizzamig]

In D15329#323510, @eugen_grosbein.net wrote:

In D15329#323385, @wollman wrote:

It's probably worth a little bit of thought as to what is the more common case, a zillion epair interfaces on one host (read: half a zillion vnet jails) or a much smaller number of epairs on a larger number of hosts. It sounds like you are well placed to assign your own addresses, whatever the default may be.

Well, this is not network hot path and we now have jenkins_hash32() kernel function in all supported branches, so we could just allocate additional uint32_t key[3] array on stack of this function, copy there all 64 bits of (long)hostid plus (uint32_t)if_index and make use of jenkins_hash32() to get them mixed to 32-bit hash value. Then use it as-is for eaddr[1]-eaddr[4] similar to current manner or switch to using 58:9C:FC for upper half of MAC and use lower 24 bits of the hash for the rest of MAC resetting lowest bit to 0 for first interface and to 1 for second one.

I like this approach. That's what I'll do (in pseudo code):

eaddr[0] = 0x02
eaddr[5] = 0x0a
eaddr[1-4] = hash( hostid and ifp->if_index )

Unfortunately, hostid is of type unsigned long and to correctly support 32 and 64 bit architectures, the key would be arranged in this way:

key[0] = (uint32_t)ifp->if_index;
key[1] = (uint32_t)(hostid & 0xffffffff);
if (sizeof(unsigned long) == 64)
    key[2] = (uint32_t)((hostid >> 32) & 0xfffffffff);
hash = jenkins_hash32(key, sizeof(unsigned long) == 64 ? 3 : 2, 0);

or alternatively:

key[0] = (uint32_t)ifp->if_index;
key[1] = (uint32_t)hostid & 0xffffffff;
if (sizeof(unsigned long) == 64)
    key[2] = (uint32_t)(hostid >> 32) & 0xfffffffff;
else
    key[2] = 0;
hash = jenkins_hash32(key, 3,  0);

I'm currently rebuilding everything to test the hash approach and then I'll update the patch. Do you see any other issue?

PS: theoretically, the eaddr can be re-arranged:

eaddr[0] = 0xa2; // 0xb2 for the other interface
eaddr[1-4] = hash(...)
eaddr[5] = random_value; // arc4rand maybe?

adding further 8 bit to ensure the etheraddr uniqueness, but I'm not sure about it.

[eugen_grosbein.net]

sizeof counts in octets (bytes), not bits
and you do not need complexity of "if (sizeof...)" but simply cast hostid to uint64_t unconditionally first.

[pizzamig]

Adopt the proposed approach based on jenkins_hash32

epairb etheraddr is now copied from epaira

[eugen_grosbein.net]

sys/net/if_epair.c

724 ↗

(On Diff #42312)

eaddr declaration was last for a purpose, move it after hash please so it stays last

739 ↗

(On Diff #42312)

"scp->ifp" can be shortened to just "ifp" here

857 ↗

(On Diff #42312)

style problem (extra spaces)

[pizzamig]

Style cleanups

[eugen_grosbein.net]

Looks just fine.

[pizzamig]

I've tested the patch on my laptop and I've found no issues.
I don't have commit bit for src, only for port, so it would be nice if one of you can commit it, now that the patch is approved.
In this way it would be possible to close PR176671 and PR184149.
thanks for all your feedback!