if_epair(4): use ether_gen_addr(9) for stable MAC address

[ronald_klop.ws]

Summary

Before this change epair interfaces get a random MAC. This does
not help dhcp/dyndns when an epair gets destroyed/recreated
after restart of a jail.

$ sysctl net.link.epair.ether_gen_addr=0
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8b | grep ether; ifconfig epair8a destroy
        ether 02:cb:78:56:e4:0a
        ether 02:cb:78:56:e4:0b
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8b | grep ether; ifconfig epair8a destroy
        ether 02:8b:9b:6a:8f:0a
        ether 02:8b:9b:6a:8f:0b

$ sysctl net.link.epair.ether_gen_addr=1
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8b | grep ether; ifconfig epair8a destroy
        ether 58:9c:fc:10:2b:b4
        ether 58:9c:fc:00:39:10
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8b | grep ether; ifconfig epair8a destroy
        ether 58:9c:fc:10:2b:b4
        ether 58:9c:fc:00:39:10

A follow up commit will change the default to 1 in main.
When no issues appear within about 2 weeks I would like to
remove the "old" code and the sysctl.

MFC after: 2 weeks
Relnotes: yes

Test Plan

Running this on my computers for a while already.

[ronald_klop.ws]

fix commit message; lines starting with # were remove :-)

[ivy]

this seems to bring epair in line with other pseudointerface types, e.g. tap(4) and bridge(4). i'm not sure the existing behaviour of forcing the last octet to 0x0a/0x0b achieves anything other than reducing entropy.

[bz]

$ sysctl net.link.epair.ether_gen_addr=1
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8a destroy # (twice)
ether 58:9c:fc:10:2b:b4
ether 58:9c:fc:10:2b:b4

You cannot have the same MAC address on two interfaces unless you are looking for trouble. People are not just using epair for vnet jails and even then souring packets from one or the other side correctly seems quite important. That's where the 'a' and 'b' endings came from. You could simply do a +1 (and handling the wrap around) as well.

[zlei]

For hardware interfaces, the hardware address is a nature of the interface, regardless of its name or host uuid or jail name.

For clone created interfaces, I think a possible nature would be the sequence when they got created. So I'd recommend have host uuid, jail name and interface type ( ifc_name from cloner ) as entropy, and the sequence ( the unique unit from cloner ) to be the nature of an interface.

asprintf(&buf, M_TEMP, "%s-%s-%s-%d", 
    uuid, jailname, ifc_name, unit,);

Then we can get stable mac address on every restart of a jail.

What about this proposal ?

Well, the unit from cloner is reusable, probably a per-cloner increasing only counter is required to avoid reuse previously generate mac address.

[ronald_klop.ws]

In D51157#1167848, @bz wrote:

$ sysctl net.link.epair.ether_gen_addr=1
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8a destroy # (twice)
ether 58:9c:fc:10:2b:b4
ether 58:9c:fc:10:2b:b4

You cannot have the same MAC address on two interfaces unless you are looking for trouble. People are not just using epair for vnet jails and even then souring packets from one or the other side correctly seems quite important. That's where the 'a' and 'b' endings came from. You could simply do a +1 (and handling the wrap around) as well.

I abbreviated the output of the command a bit in the commit message. I'm afraid that gave some confusion. The commit message shows that epair8a gets the same address when destroyed and created again.
The two paired interfaces get different addresses as the code calls epair_generate_mac_byname() separate for both ends.

# ifconfig epair8 create
epair8a
# ifconfig epair8a ether
epair8a: flags=1008842<BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=8<VLAN_MTU>
	ether 58:9c:fc:10:2b:b4
# ifconfig epair8b ether
epair8b: flags=1008842<BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=8<VLAN_MTU>
	ether 58:9c:fc:00:39:10
# ifconfig epair8a destroy

I hope this clarifies my intention.

[ronald_klop.ws]

For clarity about unique MAC addresses, show both ends of the epair in the commit message.

[kp]

A follow up commit will change the default to 1 in main.

That's going to break a couple of pf tests. There are a few that make assumptions about the mac addresses of epair interfaces.
The tests can probably be adjusted, or we can keep the 'set the last byte to 0x0a/0x0b' bit while still generating deterministic mac addresses.

[ronald_klop.ws]

In D51157#1168571, @kp wrote:

A follow up commit will change the default to 1 in main.

That's going to break a couple of pf tests. There are a few that make assumptions about the mac addresses of epair interfaces.
The tests can probably be adjusted, or we can keep the 'set the last byte to 0x0a/0x0b' bit while still generating deterministic mac addresses.

Do you have a pointer to the tests for me? I hope we can work out a fix for the tests together. Personally I think using the info from 0x0a/0x0b in tests is a leaky abstraction of the epair interface.
Keeping the 0x0a/0x0b would reduce the entropy of the MAC to only 1 byte.
See net/ieee_oui.h: 58:9c:fc:AA:BB:CC
58:9c:fc = fixed prefix
AA = always 10 for OUI_FREEBSD_GENERATED_MASK (why do we spent 1 byte for this almost unused dividing of the range?)
BB & CC = random
If we use 0x0a/0x0b than CC isn't random anymore.

Anyway, I'm happy to take a look at the tests and help.

[kp]

In D51157#1168587, @ronald_klop.ws wrote:

Do you have a pointer to the tests for me?

https://cgit.freebsd.org/src/tree/tests/sys/netpfil/pf/icmp.py#n96
https://cgit.freebsd.org/src/tree/tests/sys/netpfil/pf/header.py#n58

Both of those are instances where we need to know the mac address of the other end of the epair interface. If it's not predictable from the one on our end that's going to be annoying.
I'm not immediately finding others, but applying this patch and running the pf tests may reveal more.

I hope we can work out a fix for the tests together. Person> ally I think using the info from 0x0a/0x0b in tests is a leaky abstraction of the epair interface.

Arguably, but they're tests so it's a lot easier to accept there.
I'm of the fairly firm opinion that test code should be as simple as possible, and if that means making assumptions that are only valid in the context of the test or duplicating code across tests that's fine. Not fine in production code, but fine in test code.

Keeping the 0x0a/0x0b would reduce the entropy of the MAC to only 1 byte.
See net/ieee_oui.h: 58:9c:fc:AA:BB:CC
58:9c:fc = fixed prefix
AA = always 10 for OUI_FREEBSD_GENERATED_MASK (why do we spent 1 byte for this almost unused dividing of the range?)
BB & CC = random
If we use 0x0a/0x0b than CC isn't random anymore.

Anyway, I'm happy to take a look at the tests and help.

We do make the assumption that the mac will end with :0a and :0b, but it'd be fairly straightforward to cope with :$(random)a and :$(random)b.
That'd buy us another half of a byte for entropy.

[ivy]

In D51157#1168606, @kp wrote:

We do make the assumption that the mac will end with :0a and :0b, but it'd be fairly straightforward to cope with :$(random)a and :$(random)b.
That'd buy us another half of a byte for entropy.

what about (as i believe someone else proposed) making the 'b' end address the 'a' end address plus 1? this avoids wasting any entropy (we always need two addresses anyway) and it should be possible to calculate this in the tests where needed.

to avoid wraparound, an 'a' side address ending in :ff:ff:ff can be masked to :ff:ff:fe.

edit: or simply always mask the last bit in the last byte, so the 'a' side address is even and the 'b' side address is odd, which also makes this easier to calculate in the tests.

[kp]

In D51157#1168607, @ivy wrote:

In D51157#1168606, @kp wrote:

We do make the assumption that the mac will end with :0a and :0b, but it'd be fairly straightforward to cope with :$(random)a and :$(random)b.
That'd buy us another half of a byte for entropy.

what about (as i believe someone else proposed) making the 'b' end address the 'a' end address plus 1? this avoids wasting any entropy (we always need two addresses anyway) and it should be possible to calculate this in the tests where needed.

to avoid wraparound, an 'a' side address ending in :ff:ff:ff can be masked to :ff:ff:fe.

edit: or simply always mask the last bit in the last byte, so the 'a' side address is even and the 'b' side address is odd, which also makes this easier to calculate in the tests.

That should all be fine too, yeah.
We'll have to tweak the tests a little, but that's not a big deal. We have to do that with any of these options anyway.

[ronald_klop.ws]

In D51157#1168608, @kp wrote:

In D51157#1168607, @ivy wrote:

In D51157#1168606, @kp wrote:

We do make the assumption that the mac will end with :0a and :0b, but it'd be fairly straightforward to cope with :$(random)a and :$(random)b.
That'd buy us another half of a byte for entropy.

what about (as i believe someone else proposed) making the 'b' end address the 'a' end address plus 1? this avoids wasting any entropy (we always need two addresses anyway) and it should be possible to calculate this in the tests where needed.

to avoid wraparound, an 'a' side address ending in :ff:ff:ff can be masked to :ff:ff:fe.

edit: or simply always mask the last bit in the last byte, so the 'a' side address is even and the 'b' side address is odd, which also makes this easier to calculate in the tests.

That should all be fine too, yeah.
We'll have to tweak the tests a little, but that's not a big deal. We have to do that with any of these options anyway.

These concerns are addressed in: D51205.

[ronald_klop.ws]

In D51157#1167906, @ronald_klop.ws wrote:

In D51157#1167848, @bz wrote:

$ sysctl net.link.epair.ether_gen_addr=1
$ ifconfig epair8 create > /dev/null; ifconfig epair8a | grep ether; ifconfig epair8a destroy # (twice)
ether 58:9c:fc:10:2b:b4
ether 58:9c:fc:10:2b:b4

You cannot have the same MAC address on two interfaces unless you are looking for trouble. People are not just using epair for vnet jails and even then souring packets from one or the other side correctly seems quite important. That's where the 'a' and 'b' endings came from. You could simply do a +1 (and handling the wrap around) as well.

I abbreviated the output of the command a bit in the commit message. I'm afraid that gave some confusion. The commit message shows that epair8a gets the same address when destroyed and created again.
The two paired interfaces get different addresses as the code calls epair_generate_mac_byname() separate for both ends.

# ifconfig epair8 create
epair8a
# ifconfig epair8a ether
epair8a: flags=1008842<BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=8<VLAN_MTU>
	ether 58:9c:fc:10:2b:b4
# ifconfig epair8b ether
epair8b: flags=1008842<BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=8<VLAN_MTU>
	ether 58:9c:fc:00:39:10
# ifconfig epair8a destroy

I hope this clarifies my intention.

@bz Does this reply take away your concerns or do you still require some changes for this to land?

[bz]

In D51157#1169776, @ronald_klop.ws wrote:

@bz Does this reply take away your concerns or do you still require some changes for this to land?

Yes but the description is still not fixed.
Use "Edit revision" on the upper right corner; just so in case someone pulls it in ad doesn't edit it before landing it'll be right.

If tests and kp are fine please go ahead then.

[kp]

In D51157#1173148, @bz wrote:

If tests and kp are fine please go ahead then.

My concerns have been addressed.

[bz]

I think this is unreasonable (within 2 weeks). I assume in FreeBSD 16 you can remove the sysctl. for 14 the default should never switch. Probably deserves an UPDATING entry from anyone who commits this as I anticipate that a lot of provisioning scripts might break.

@kp do you want to deal with it and go ahead?

[kp]

In D51157#1173358, @bz wrote:

I think this is unreasonable (within 2 weeks). I assume in FreeBSD 16 you can remove the sysctl. for 14 the default should never switch. Probably deserves an UPDATING entry from anyone who commits this as I anticipate that a lot of provisioning scripts might break.

@kp do you want to deal with it and go ahead?

I think I mostly agree with you. The current patch is fine, and can be landed and MFC'd as-is.

As for changing the default, we're basically not going to get useful feedback on that until after we actually do it, no matter how much noise anyone makes about this, so I think it's fine to do that soon as well. In fact, it's probably better to do it sooner rather than later. The default change needs to come with an UPDATING entry.

However, we shouldn't remove the sysctl until after 15 is branched (i.e. the sysctl should be there for all of 15.x, but can go away for 16.x).
(That also means that if we do see a lot of fallout from his we can still revert the default, even fairly late in the release process. Although ideally we'd avoid needing to do that.)

Ronald has a commit bit (although I'm not sure what type), so can commit this, with my approval if required.

[ronald_klop.ws]

Thanks for all the comments.
I have a ports commit bit I can use.
And will use the advice about UPDATING and what to MFC and what not.

Currently holidays are ongoing so will commit in the near future (couple of days hopefully).