Differential D24383
ethersubr: Make the mac address generation more robust Authored by kp on Apr 12 2020, 5:09 PM. Tags None Referenced Files
Details
If we create two (vnet) jails and create a bridge interface in each we These very often conflicts, resulting in same mac address in both jails. Mitigate this problem by including the jail ID in the mac address.
Diff Detail
Event TimelineComment Actions Hmm... any objection to using pr_name, instead? My reasoning is perhaps a bit silly, so I won't insist, but I like the idea of specifically-named jails being able to get a stable MAC address for any given cloned interface. IIRC the name will default to the jid if not specified and names can't collide, so it should satisfy the uniqueness you need for multiple bridge0 on a given host. Comment Actions Er, specifically, pr_name for ether_gen_addr. I think the jid still makes sense for the derivation done in if_bridge, and I'm not as tied to those being name-consistent. =-) Comment Actions Yeah, that makes sense, and it'd fix the issue I ran into just as well. I'll try to update this patch tomorrow. Comment Actions (1) can we stop assigning locally administered addresses. We have general infrastructure in the FreeBSD OUI. Or is there a reason we cannot do that? Comment Actions We can, and if_bridge does so, if hostid == 0 (which is the case for jails not created with host=inherit). I'm not clear on why the bridge has these two approaches. We could simplify the code by removing that, and just always calling ether_gen_addr().
Yes, but if it's set (i.e. not zero) it's identical to the host unless overruled. They also have a hostuuid, which is also identical to the host. I assumed there was a theory or policy behind this, Comment Actions A quick check finds this: https://svnweb.freebsd.org/base/head/sys/net/if_bridge.c?revision=225380&view=markup Given we don't do this for any other "virtual cloner" interfaces I am not sure why bridge needs to be special (it is in some other ways in the IPv6 code). I think people who want stable link-layer addresses should use ifconfig ether in their configuration framework (and we should up our game in that we help this somehow but that's a different story). That said ether_gen_addr() already uses the hostuuid for this so that should be good by itself and we can save ourselves that part of complications in bridge? So that brings us back to the problem from (2) and that with same interface names you get same link-layer addresses which is a problem with vnets or even between multiple machines on the same broadcast domain if the host uuids are 0 (which I think they normally should not be but random at least?)?
The probably is in that a default probably is inherit rather than generating a new one. Something to discuss on jails@ with @jamie I'd assume. If we all agree on (1) that one method would be good enough rather than two, then I think we could split this problem into two as then the general problem is independent of bridge(4) and could be solved separately? Comment Actions
Comment Actions I think this LGTM, thanks! I suspect it would make sense to remove the LA MAC assignment from epair as well if we're removing it from bridge. Comment Actions Yes, that'd be good too. I did spot that one when I started looking at the bridge. It's on my list, probably in the next couple of days. Comment Actions Split off the bridge changes. This review now only deals with the changes to ether_gen_addr() Comment Actions I'd be happy with this. I'd still think there could be a comment saying "If each (vnet) jail would also have a unique hostuuid this would not be necessary." for the getjailname() call. Comment Actions Please consider to decrease abuse of kernel stack. We already had problems with kernel-land double faults due to kernel thread stack overflows. Could you please aggregate these big but fixed-sized arrays to single struct and allocate it using one malloc() call at heap instead? Comment Actions Kernel side asprintf() uses M_NOWAIT internally and may return -1 on memory allocation failure. It would need a check. Also, it would be good instead moving all large on-stack objects (SHA1_CTX ctx and arrays uuid, digest, name) to single struct and allocate/free memory for the struct as a whole. Comment Actions Are we really that concerned about stacks that will generally be < 512 bytes that are: 1.) infrequently called, and Comment Actions Fall back to arc4random() if we fail to allocate memory. We're very unlikely to succeed in attaching the interface, but we shouldn't Comment Actions No, we're not. I'm not going to make this trivial function even more complex for the sake of stack size in what are, as you say, very short code paths anyway. |