MFC sys/netinet6: Implement RFC 7217 (private stable addresses)

[madpilot]

Summary

I'd like to MFC my RFC 7217 implementation to 15-stable.

As suggested by @zarychtam_plan-b.pwste.edu.pl I'm also including SLAAC patches ([1] and followup fix [2]) from @linnemannr_gmail.com, to allow a clean merge without conflicts.

I think this is the cleanest choice, otherwise I could merge my patches only fixing conflicts, but then there would be further conflicts if [1] and [2] are to be merged later.

I'm including everyone who approved the previous patches as reviewers, plus @linnemannr_gmail.com and @kp as author and commiter of the extra patches I'm planning to bring along.

Also, I plan to merge each commit as a single cherry pick and push them in one go. Looks to me like the best way, if merging them in a single commit is preferred please let me know.

Test Plan

I've tested the patch in bhyve VMs and everything looks fine.

I'm also leveraging this code daily on head.

[madpilot]

Forgot to add references:

[1] 9e792f7ef7298080c058fbc2d36a4e60e596dae9
[2] f9fc93690aef7a56f62a051de6231fe2af699728

[zlei]

ALso, I plan to merge each commit as a single cherry pick and push them in one go. Looks to me like the best way, if merging them in a single commit is preferred please let me know.

Given this is a new feature, I think it will be much more clean to merge all related commits into a single one when MFCing.

[madpilot]

In D54382#1242898, @zlei wrote:

Also, I plan to merge each commit as a single cherry pick and push them in one go. Looks to me like the best way, if merging them in a single commit is preferred please let me know.

Given this is a new feature, I think it will be much more clean to merge all related commits into a single one when MFCing.

Thanks for the feedback. If this is the cnsensus I can go this way, using the commit message from 31ec8b6407fdd5a87d70265762457c67ce618283 but adding the required tags and "cherry picked" lines. And a comment about the other related commits.

[zarychtam_plan-b.pwste.edu.pl]

From a testing perspective, I can confirm that the solution has been working well on stable/15 for quite some time on a laptop and several desktops. I recently cherry-picked it at work onto our build machine and started testing it on servers running stable/15 as well, although only a small subset of those systems use EUI-64–based IPv6 addressing.

I’m not convinced that squashing these commits would add much value. None of them introduces critical breakage - the system will never panic. The MFC workflow is well known and clear, and bisecting can always be properly done against not only the main branch.

[kp]

In D54382#1242817, @madpilot wrote:

Forgot to add references:

[1] 9e792f7ef7298080c058fbc2d36a4e60e596dae9

This is a behaviour change, which is why I initially didn't mark it as an MFC candidate.
That said, it's very limited in scope, and only allows users to do new things, I don't expect this to break any existing setups, so there should be relatively little risk to including it.

[2] f9fc93690aef7a56f62a051de6231fe2af699728

This is a fix to the above, so either take both or neither.

[zarychtam_plan-b.pwste.edu.pl]

I had some time to test the below{1} merge on stable/15 following the traditional, still recommended upgrade path with intermediate reboot (install kernel, reboot, install world, reboot).
I hereby report that the stable/15 system with IPv6 EUI-64–based primary address and IPv6 static address configured as alias0 encountered no problems after the first and second reboot. Moreover, with stableddress enabled in the loader.conf[2], the system did not encounter any issues either and after the first reboot system was already following RFC 7217 requirements. Thus, it was proven that even if stableaddr will become the default at some point, the ABI will not be broken, and old ifconfig(8) will be able to take care of interface addressing.

! In D54382#1242898, @zlei wrote:
Given this is a new feature, I think it will be much more clean to merge all related commits into a single one when MFCing.

When I was testing the initial implementation, I believed that ABI breakage must happen, but it was a wrong assumption. The above tests have proven that squashing commits

• sys/netinet6: Implement RFC 7217 (cherry picked from commit 31ec8b6407fdd5a87d70265762457c67ce618283) and
• sys/netinet6: Fix ABI breakage introduced with RFC 7217 support (cherry picked from commit 0ec13430c583830cc4d29640787e2d154b140e31)

could be valuable and appreciated.

[1]:

sys/netinet6: Fix SLAAC for interfaces with no /64 LL address (cherry picked from commit 9e792f7ef7298080c058fbc2d36a4e60e596dae9)
sys/netinet6: Implement RFC 7217 (cherry picked from commit 31ec8b6407fdd5a87d70265762457c67ce618283)
sys/netinet6: Fix ABI breakage introduced with RFC 7217 support (cherry picked from commit 0ec13430c583830cc4d29640787e2d154b140e31)
sys/netinet6: Use atomic(9) for dad_failures counter (cherry picked from commit 0c94dc9da7c238603e43c55ca6da64c417fc2204)
sys/netinet6: fix memory corruption in in6_ifadd (cherry picked from commit f9fc93690aef7a56f62a051de6231fe2af699728)

[2]:

net.inet6.ip6.use_stableaddr="1"
net.inet6.ip6.stableaddr_netifsource="2"

[madpilot]

Updating this review to only inlcude my commits.

This should simplify the process and allow me to merge the individual commits in one go.

[glebius]

JFYI: this patch will need some trivial adjustments after I land https://reviews.freebsd.org/D54725

[madpilot]

In D54382#1253065, @glebius wrote:

JFYI: this patch will need some trivial adjustments after I land https://reviews.freebsd.org/D54725

Thanks for the heads up!

I'll be updating and testing again after you MFC that.

[madpilot]

In D54382#1253065, @glebius wrote:

JFYI: this patch will need some trivial adjustments after I land https://reviews.freebsd.org/D54725

Actually wouldn't merging this change make merging your change to stable/15 easier?

Not being a src committer I was waiting for an approval to do this merge.

[glebius]

I don't plan to merge my changes.

[pouria]

Fix some style(9) nits.

sys/netinet6/in6_ifattach.c

158–159

178

181–182

250

253

268–281

314

375

383–387

[pouria]

Fix manual style/warning nits

sbin/ifconfig/ifconfig.8

1011–1012 ↗

(On Diff #170219)

mandoc: STYLE: input text line longer than 80 bytes:

1016–1018 ↗

(On Diff #170219)

mandoc: STYLE: input text line longer than 80 bytes:
+ use Xr for loader.conf(5)

1022 ↗

(On Diff #170219)

mandoc: STYLE: input text line longer than 80 bytes:

1027 ↗

(On Diff #170219)

mandoc: STYLE: no blank before trailing delimiter...

1028 ↗

(On Diff #170219)

mandoc: WARNING: missing -width in -tag list, using 6n: Bl -tag

1035–1036 ↗

(On Diff #170219)

mandoc: WARNING: skipping paragraph macro: Pp before It

[ziaee]

sbin/ifconfig/ifconfig.8

1011–1012 ↗

(On Diff #170219)

You don't have to, but it's very nice to wrap at 72 so it's easier on people who use 80 character consoles with line numbering.

For manuals, I like to try to wrap early to keep greppable phrases together.

[pouria]

sbin/ifconfig/ifconfig.8

1011–1012 ↗

(On Diff #170219)

Thanks!
I don't like mandoc to complain about anything because I don't understand its syntax.
it’s like it’s speaking another language :)

[zarychtam_plan-b.pwste.edu.pl]

Please take into consideration that the changes discussed in this review are already present in the main branch (16.0-CURRENT). Escalating demands here is pointless. If the man page requires fixes, the right approach would be to fix it in the main branch and then MFC those changes to stable/15 (if this review ever lands in stable/15).

The same, in my opinion, applies to the rest of the code - the more that can be done via conflict-free cherry-picking, the better for the FreeBSD project.

By the way, I am still testing this in our stable/15 branch at work on servers running in production, as well as on my own laptop and PC. So far, so good.

[pouria]

Please fixs those on CURRENT. For this MFC, LGTM.

[madpilot]

Updated the review to include recent commits with style fixes.

[glebius]

Can you please rebase? There is no longer ND_IFINFO().

sys/netinet6/in6_ifattach.c

192–193

Please use uint8_t.

[madpilot]

Thanks for your comments,

In D54382#1263614, @glebius wrote:

Can you please rebase? There is no longer ND_IFINFO().

This is a request for an MFC against stable/15 The code already committed in head was modified to remove ND_IFINFO(), but thise changes are not in stable/15, so ND_IFINFO is still there. Or maybe I'm misunderstanding your request.

sys/netinet6/in6_ifattach.c

192–193

Being an MFC request I think I should first change this in head and then cherry-pick such a commit.

I can do that, in case you want me to, can I use this request as an implicit approval for that very commit?

[glebius]

No objection for MFC. Neither encouragement, though :)

sys/netinet6/in6_ifattach.c

192–193

I'm sorry, I didn't notice the review is about MFC.

[bms]

Heads up warning: these code paths are very likely to be touched by an ILNPv6 merge (RFC 6740 and up). @rgrimes and I need to have a chat first.

[rgrimes]

In D54382#1264959, @bms wrote:

Heads up warning: these code paths are very likely to be touched by an ILNPv6 merge (RFC 6740 and up). @rgrimes and I need to have a chat first.

Given this is an MFC from head to 15 it has precedence over any external ILNPv6 work at this point in time. I'll already have to deal with these changes as I migrate the ILNPv6 code from its current 14.3 base to -HEAD