ethersubr: Make the mac address generation more robust

[kp]

Summary

If we create two (vnet) jails and create a bridge interface in each we
end up with the same mac address on both bridge interfaces.
The bridge mac address is generated based on the host id if set, or
based on the host UUID and interface name.

These very often conflicts, resulting in same mac address in both jails.

Mitigate this problem by including the jail ID in the mac address.

[kevans]

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.

[kevans]

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

[kp]

Yeah, that makes sense, and it'd fix the issue I ran into just as well.

I'll try to update this patch tomorrow.

[bz]

(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?
(2) jails have a hostid as well. If they are the same for all of them then maybe we should fix the the jail code to make sure they actually are unique if not set?

[kp]

In D24383#536382, @bz wrote:

(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?

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

(2) jails have a hostid as well. If they are the same for all of them then maybe we should fix the the jail code to make sure they actually are unique if not set?

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,

[bz]

In D24383#536488, @kp wrote:

In D24383#536382, @bz wrote:

(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?

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

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

(2) jails have a hostid as well. If they are the same for all of them then maybe we should fix the the jail code to make sure they actually are unique if not set?

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,

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?

[kp]

• Use the jail name rather than its ID as a seed
• Remove one of the two ways in which if_bridge generated mac addresses

[kevans]

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.

[kp]

In D24383#536640, @kevans wrote:

I suspect it would make sense to remove the LA MAC assignment from epair as well if we're removing it from bridge.

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.

[kp]

In D24383#536640, @kevans wrote:

I suspect it would make sense to remove the LA MAC assignment from epair as well if we're removing it from bridge.

https://reviews.freebsd.org/D24416

[kp]

Split off the bridge changes. This review now only deals with the changes to ether_gen_addr()

[bz]

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.

[eugen_grosbein.net]

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?

[kp]

• Comment on why we add the jail name
• Dynamically allocate the string buffer

[eugen_grosbein.net]

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.

[kevans]

Are we really that concerned about stacks that will generally be < 512 bytes that are:

1.) infrequently called, and
2.) almost always called in short code-paths because it's a part of ifnet creation?

[kp]

Fall back to arc4random() if we fail to allocate memory.

We're very unlikely to succeed in attaching the interface, but we shouldn't
leave the mac blank.

[kp]

If we generate a random mac we should indicate it's a locally administered addresses

[kp]

In D24383#537847, @kevans wrote:

Are we really that concerned about stacks that will generally be < 512 bytes that are:

1.) infrequently called, and
2.) almost always called in short code-paths because it's a part of ifnet creation?

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.

[melifaro]

LGTM.
Btw, it's probably worth having a test to ensure we're not in that boat again?

sys/net/if_ethersubr.c

1422 ↗

(On Diff #70654)

Worth updating to reflect that jail name is also a part?

1436 ↗

(On Diff #70654)

Nit: maybe jail_name would be a bit more descriptive?

1442 ↗

(On Diff #70654)

Nit: probably it's worth considering using if_name() accessor.

[kp]

• Update comment
• name -> jailname
• Use accessor for ifp name

[kp]

In D24383#537879, @melifaro wrote:

LGTM.
Btw, it's probably worth having a test to ensure we're not in that boat again?

Yes. That's on my list for Sunday.