Page MenuHomeFreeBSD

Improvement for MAC address uniqueness of if_epair(4)
ClosedPublic

Authored by pizzamig on May 6 2018, 9:01 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pizzamig created this revision.May 6 2018, 9:01 PM
pizzamig edited the summary of this revision. (Show Details)May 6 2018, 9:09 PM
pizzamig edited the test plan for this revision. (Show Details)
pizzamig added reviewers: network, hrs.
bz added a subscriber: bz.May 6 2018, 9:40 PM

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

wollman added a subscriber: wollman.May 6 2018, 9:49 PM

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 requested changes to this revision.EditedMay 6 2018, 9:56 PM

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.

This revision now requires changes to proceed.May 6 2018, 9:56 PM
luca.pizzamiglio_gmail.com updated this revision to Diff 42226.

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

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?

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.

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 updated this revision to Diff 42231.May 7 2018, 2:09 PM

Further cleaning, remove dead code

pizzamig marked an inline comment as done.May 7 2018, 2:09 PM

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?

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

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 added inline comments.May 8 2018, 2:11 AM
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.

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.

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 added a comment.EditedMay 9 2018, 1:55 AM

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.)

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 added a comment.EditedMay 9 2018, 10:45 AM

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 updated this revision to Diff 42312.May 9 2018, 2:11 PM

Adopt the proposed approach based on jenkins_hash32

epairb etheraddr is now copied from epaira

pizzamig marked 2 inline comments as done.May 9 2018, 2:12 PM
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 updated this revision to Diff 42402.May 11 2018, 9:19 AM

Style cleanups

pizzamig marked 3 inline comments as done.May 11 2018, 9:20 AM

Looks just fine.

wollman accepted this revision.May 12 2018, 12:56 AM
This revision is now accepted and ready to land.May 12 2018, 12:56 AM

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!

This revision was automatically updated to reflect the committed changes.