Details

Reviewers

bz
emaste
gnn
shurd
rgrimes

Group Reviewers

iflib

Commits

rS346326: iflib: Use new ether_gen_addr, restricting addresses to that subset
rS346324: net: adjust randomized address bits

Summary

Give 'random' devices a 16-bit allocation out of the FreeBSD Foundation OUI range. Change the name ether_fakeaddr to ether_randomaddr now that we're dealing real MAC addresses with a real OUI rather than random locally-administered addresses.

While here, provide a function for other components generating random addresses to do so by providing a custom mask to ether_randomaddr_lowmask -- given that most things allocating this range will be either deterministic or completely random, this seems fitting.

Move iflib generated-MACs specifically into the 16-bit random allocation here. I'm not against the thought of adding an iflib-specific allocation, but it's clear that the current scheme is against the current guidelines for the OUI.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 23120

Event Timeline

kevans created this revision.Mar 15 2019, 2:25 AM

Herald added a reviewer: shurd. · View Herald TranscriptMar 15 2019, 2:25 AM

Herald added a reviewer: iflib. · View Herald Transcript

Herald added subscribers: ae, imp. · View Herald Transcript

Harbormaster completed remote builds in B23100: Diff 55088.Mar 15 2019, 2:25 AM

OUI spaces aren't for randomly assigning address bits that change from boot to boot. They are for more permanent allocations according to the standards that I read a few years ago. The local administrative space is for things like random assignment of MAC addresses.

I think this is a really bad idea and we should not do it.

In D19587#419396, @imp wrote:

OUI spaces aren't for randomly assigning address bits that change from boot to boot. They are for more permanent allocations according to the standards that I read a few years ago. The local administrative space is for things like random assignment of MAC addresses.

(a) the fact that we are still not preserving the "randomly generated" addresses across boot is rather a management problem we still have and not a reason not to use OUI space.

(b) bhyve, despite having a block from FreeBSD allocated, is still using the NetAPP OUI in two places, in exactly the same way this will use it for other interfaces. Same problem as (a) there (though there's actually things in place to pass them in on startup, and so does ifconfig ether exist).

(c) the locally administered space is not for random stuff, it is for a locally *administered* space, some local authority who assigns and manages; I have learnt this the hard way by sys-admins telling me off "no DHCPv4 for you", this LA MAC address was not assigned by us, before. They were actually keeping track of each locally administered address.

(d) What you are thinking of is an "unmanaged, I don't care network" where it doesn't even matter if you have conflicts with official OUI space or not. Been there; made that thinking mistake myself.

(e) Using the FreeBSD OUI, like other companies do, gives us a unique space, allows our users to rely on same EUI-48 if they want even in spaces where locally administered is not a thing and I'd hope that now that we centralise the "computation", we'd actually also grow a management part, as writing a rc snippet to save and restore them isn't that hard, we could even have notifications. That way FreeBSD "cloud" deployments or even just lots of FreeBSD machines running these interfaces and bridging them out (also for jail+vnet for example, which is not so much of a difference from say bhyve or the Fusion running on your OSX) can finally stop the admin headache.

(f) for that we should have long time ago carved out some space; if we carve it out anyway, we can as well use it.

(g) the other standards usages of OUIs (I believe NVME also uses the FreeBSD OUI somewhere; maybe that was in bhyve as well to identify the controller vendor), for config spaces, and others, for fixed, special purpose Ethernet MAC address values, exist and are equally valid; some may conflict, others don't, some could possibly even overlap, I guess, without causing any conflicts.

Sorry Kyle, that the full discussion on this is now done on this review. Thanks for starting to consolidate all the "random" uses.
Let's take this slowly and try to get it righter and with rough consensus into something which will (a) sort the various individual implementations, and (b) solve problems for our users.

sys/net/ieee_oui.h
74	I'll leave the comment here; I know I said I prefer "random" to "fake". Now given my longer reply to @imp I think that name is actually also still misleading and I think that's where Warner's comment comes from. Yes, they are currently "pseudo-randomly generated". I think that's however not the full use case for this range; looking at the range of interfaces: bridge, vxlan, epair, ng_eiface, ... I think we are talking about a range for virtual interfaces here; as I say, I'd really love to grow a way to also preserve the addresses once pseudo-randomly generated. Can we name this better?
sys/net/if_ethersubr.c
1406	Can we have this grow an "ifp" pointer argument, even if marked __unused for now; that way we will get interface name, possibly vnet context, and ifIndex later should we want to grow a notification and management part on top of this to preserve the EUI-48 addresses?
sys/net/iflib.c
1276–1277	Is there any good reason iflib still has its own function and doesn't call into the common ether_ function?
1276–1277	Unrelated: I was wondering how that would not immediately lead to conflicts. The answer is it's device_get_nameunit() and not just the unit number. However we could ponder if we would want to try to use something more deterministic, yet not leading to conflicts easily across multiple machines, in ether_*?

kevans added inline comments.Mar 15 2019, 11:46 AM

sys/net/ieee_oui.h
74	The problem is I intended to also use this space for the relatively small selection of real NICs in (at the very least) ARM-land that do the same thing in places. Some of them can pull from FDT, some of them generate from some EEPROM to match U-Boot, and some of them do this same logic or fallback to it in the event of the above not being possible.
sys/net/if_ethersubr.c
1406	Sure, that sounds like a good plan to me.
sys/net/iflib.c
1276–1277	That reason is entirely that I think wanting to have a deterministic MAC is a good idea if we have enough to go on to derive one. If we could come to an agreement on the best approach to do so in the common functions, I think it'd be a sensible change to make.

In D19587#419445, @bz wrote:

In D19587#419396, @imp wrote:

OUI spaces aren't for randomly assigning address bits that change from boot to boot. They are for more permanent allocations according to the standards that I read a few years ago. The local administrative space is for things like random assignment of MAC addresses.

(a) the fact that we are still not preserving the "randomly generated" addresses across boot is rather a management problem we still have and not a reason not to use OUI space.

(b) bhyve, despite having a block from FreeBSD allocated, is still using the NetAPP OUI in two places, in exactly the same way this will use it for other interfaces. Same problem as (a) there (though there's actually things in place to pass them in on startup, and so does ifconfig ether exist).

(c) the locally administered space is not for random stuff, it is for a locally *administered* space, some local authority who assigns and manages; I have learnt this the hard way by sys-admins telling me off "no DHCPv4 for you", this LA MAC address was not assigned by us, before. They were actually keeping track of each locally administered address.

(d) What you are thinking of is an "unmanaged, I don't care network" where it doesn't even matter if you have conflicts with official OUI space or not. Been there; made that thinking mistake myself.

(e) Using the FreeBSD OUI, like other companies do, gives us a unique space, allows our users to rely on same EUI-48 if they want even in spaces where locally administered is not a thing and I'd hope that now that we centralise the "computation", we'd actually also grow a management part, as writing a rc snippet to save and restore them isn't that hard, we could even have notifications. That way FreeBSD "cloud" deployments or even just lots of FreeBSD machines running these interfaces and bridging them out (also for jail+vnet for example, which is not so much of a difference from say bhyve or the Fusion running on your OSX) can finally stop the admin headache.

(f) for that we should have long time ago carved out some space; if we carve it out anyway, we can as well use it.

(g) the other standards usages of OUIs (I believe NVME also uses the FreeBSD OUI somewhere; maybe that was in bhyve as well to identify the controller vendor), for config spaces, and others, for fixed, special purpose Ethernet MAC address values, exist and are equally valid; some may conflict, others don't, some could possibly even overlap, I guess, without causing any conflicts.

I've gone back and found the docs that I read years ago. I think that bz is more correct than I was about these things, so will drop my objection. I do see that this notion was contemplated, even in the docs I had read before, but I'd missed it.

imp added inline comments.Mar 15 2019, 4:54 PM

sys/net/ieee_oui.h
74	We need a good word for "kernel assigned" since that's what we have here. They aren't really random (which is a huge hassle, btw, if they are really random, so I agree with bz there). I agree that random and fake aren't good words here. I'd suggest ether_generated_addr() and to use GENERATED here. Though the question becomes do we want two different ones: one that's generated to be unique on the system, and one that's persistent in some way, perhaps generated randomly the first time. If we wanted to do that, and flag the difference in code today, we could have ether_pgen_addr and ether_tgen_addr. p for persistent / permanant and t for temporary / transient. Though 'd' for deterministic might not be bad either for the former.

jhb added a subscriber: jhb.Mar 15 2019, 5:12 PM

jhb added inline comments.

sys/net/if_ethersubr.c
1434–1456	I think what would give more of what bz@ wants would be to pass in the 'ifp' and then use the interface name (not just unit number) as an input to a hash like SHA1 and then use the low bytes of that hash as the input to OUI_FREEBSD. That would give you "random" but deterministic names across boot (so long as the interface name is deterministic). If the kernel is aware of the hostid UUID you could also mix that into the hash input to give values that are deterministic on a given host but different on different hosts.

rgrimes added a subscriber: rgrimes.Mar 15 2019, 5:30 PM

kevans added inline comments.Mar 15 2019, 5:38 PM

sys/net/ieee_oui.h
74	Generated seems to tick all of the boxes that I care about.
sys/net/if_ethersubr.c
1434–1456	That sounds like a reasonable solution for all use-cases. I'll incorporate that into the next version and switch iflib over to using it, since that should also suit iflib's needs.

bz added a subscriber: network.Mar 15 2019, 5:42 PM

bz added inline comments.

sys/net/if_ethersubr.c
1434–1456	@jhb: interface names are not unique on systems. We do have ways (sigh) still to have two interfaces with the same name (and only one visible); it's considered a bug but even if not, interface name + jail name could do it and that's kind of what the host uuid is supposed to be... I keep wondering how a proper "restore" will work though but that's a next thing I guess.

jhb added inline comments.Mar 15 2019, 6:48 PM

sys/net/if_ethersubr.c
1434–1456	We can add more things to the input of the hash, sure, but a hash with an input derived from the ifp will give you "deterministic" values. The inputs to the hash are the keys that uniquely identify a given interface to avoid conflicts. The other thing to consider is that if people are doing really crazy things on purpose, they can manually assign MAC addresses as an override. We just need to work for common cases out of the box. With this approach there is no save/restore. Instead it makes the generation deterministic.

kevans added inline comments.Mar 15 2019, 7:01 PM

sys/net/if_ethersubr.c
1434–1456	Any strong opinions on MD5 vs. SHA1 given that we only need to come up with three bytes?

jhb added inline comments.Mar 15 2019, 7:25 PM

sys/net/if_ethersubr.c
1434–1456	This isn't a critical path (this happens during device attach, right?), so I don't think the overhead of SHA1 vs MD5 is a concern. I would probably stick with SHA1.

rgrimes added inline comments.Mar 15 2019, 7:42 PM

sys/net/if_ethersubr.c
1434–1456	I agree that sha1 is probably the way to go, we may well wish to remove md5 code in the future and this would be one less place for that effort. md5.s collission surface is 18 bits, below the desired 24 bits wanted, see: https://en.wikipedia.org/wiki/Secure_Hash_Algorithms

Here's a second stab at it. Highlights reel:

Changed terminology from random -> generated
Threw in some commentary about expectations
arc4random -> SHA1 w/ hostuuid and if_xname (to start with...)
Converted iflib to using the same mechanism since we're leaning towards "there's nothing wrong with these always being deterministic"

Harbormaster completed remote builds in B23120: Diff 55120.Mar 15 2019, 8:18 PM

Any further input on this latest iteration?

jhb added inline comments.Mar 28 2019, 8:46 PM

sys/net/if_ethersubr.c
1409	I would still not say "random", maybe something like: Allocate an address from the FreeBSD Foundation OUI. This uses a cryptographic hash on the containing jail's UUID and the interface name to attempt to provide a unique but stable address. Pseudo-interfaces which require a MAC address should use this function to allocate non-locally-administered addresses.
1416	Do we really need the masked version? I don't see any callers of that in this patchset, only ether_gen_addr? Maybe we should just call this ether_gen_addr and hardcode the mask for now. It can always be changed in the future if a new consumer shows up, but until then I think it just adds noise.

Stripped iflib.c of some headers it no longer needs
Pulled out the _masked version of ether_gen_addr; the envisioned other consumed will not be using it, so there is no need.
Revised comment to match suggested wording by jhb.

Harbormaster completed remote builds in B23360: Diff 55562.Mar 28 2019, 9:27 PM

kevans added inline comments.Mar 28 2019, 9:28 PM

sys/net/if_ethersubr.c
1409	Whoops, I meant to update that comment. I've stolen your wording verbatim, because it seems sensible to me.
1416	I had intention for another consumer shortly, but then realized that it also didn't deserve its own non-GENERATED category.

bz requested changes to this revision.Mar 31 2019, 10:09 PM

bz added inline comments.

sys/net/if_ethersubr.c
1445	Can you assume that this is always valid? I wonder if this should be something like: if (td->td_ucred != NULL) getcredhostuuid(td->td_ucred, uuid, sizeof(uuid));

This revision now requires changes to proceed.Mar 31 2019, 10:09 PM

jhb added inline comments.Apr 1 2019, 5:45 PM

sys/net/if_ethersubr.c
1445	td_ucred will never be NULL. There is the issue that the UUID might not be set during early boot, but that will still give deterministic results regardless of how the admin configures it. (That is, if you create interfaces during boot-time probe they will use a UUID of all zeroes, but if you use cloned_interfaces in rc.conf or the like you will always use the UUID), so I think the code is fine as-is.

Any other comments?

Would iflib like any kind of UPDATING or relnote mention for the generated 'stable' MAC address likely changing (since we're restricting the range now to a subset of the FF OUI), or is it fine without explicit mention?

In D19587#427247, @kevans wrote:

Would iflib like any kind of UPDATING or relnote mention for the generated 'stable' MAC address likely changing (since we're restricting the range now to a subset of the FF OUI), or is it fine without explicit mention?

RE@ would like a release notes entry, this change is important enough to be mentioned as it cleans up some prior headaches.

bz added inline comments.Apr 13 2019, 2:03 PM