Page MenuHomeFreeBSD

Import dhcpcd(8) into FreeBSD base.
Needs ReviewPublic

Authored by woodsb02 on Oct 13 2019, 6:07 PM.

Details

Summary

Import dhcpcd(8) into FreeBSD base.

This introduces DHCPv6 functionality to FreeBSD.
This exists in parallel with existing tools dhclient(8) and rtsol(8),
which continue to be the default.

To use dhcpcd(8) for dynamic address allocation of IPv4 and IPv6,
make the following changes to /etc/rc.conf:

  • set dhcpcd_enable="YES"
  • remove "DHCP" from the ifconfig_<iface> line
  • remove rtsold_enable
  • remove ifconfig_<iface>_ipv6="inet6 accept_rtadv"
Test Plan

dhcpcd_enable="YES" enables both DHCPv4 and DHCPv6 on all interfaces.

In addition to the normal review cycle, given I am only a ports committer (I don’t have a src commit bit), I will need this to be endorsed and approved by a src committer.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 47195
Build 44082: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

As far as I can tell the objections that were raised over the last few years have all been addressed, and it doesn't seem like much progress has been made on the proposed alternatives. It seems reasonable to me to import dhcpcd initially controlled by a build knob (i.e., WITH_DHCPCD) to allow for further development and experimentation.

pauamma added inline comments.
share/man/man5/rc.conf.5
27

Bump.

504–505
516–518

While here.

564

Ethernet only, not Wifi or other LAN interfaces?

share/man/man7/hier.7
31

Bump.

As far as I can tell the objections that were raised over the last few years have all been addressed, and it doesn't seem like much progress has been made on the proposed alternatives. It seems reasonable to me to import dhcpcd initially controlled by a build knob (i.e., WITH_DHCPCD) to allow for further development and experimentation.

it, then, also make sense to introduce WITHOUT_DHCLIENT, if dhcpcd to be used instead of dhclient.

In D22012#480886, @imp wrote:

I didn't see you address Brooks' objection in the posted thread. given the extreme level of exposure here, it needs to be answered satisfactorily

I believe all objections have long since been addressed in dhcpcd-9, the latest release being dhcpcd-9.4.0
The level of privilege separation and capsicum integration now far surpases how it's done in dhclient.

Given that we h ave a number of options in ports, why? What would dhcpd in base provide us that one of the alternatives in ports cannot?

What would dhcpd in base provide us that one of the alternatives in ports cannot?

A good out-of-the box experience, and the ability to have a documented "here's how to configure dhcpv6 on a fresh install" rather than "you could install a dhcpv6 client from the ports tree"

Given that we h ave a number of options in ports, why? What would dhcpd in base provide us that one of the alternatives in ports cannot?

Note: this is not dhcpd -- a DHCP server daemon. This is dhcpcd -- a DHCP client daemon.

Note: this is not dhcpd -- a DHCP server daemon. This is dhcpcd -- a DHCP client daemon.

Thanks Ravi, that is a good point that's not immediately obvious from the name.

What's the fate of rtsol(d) with this?

libexec/rc/network.subr
229 ↗(On Diff #97323)

We've had plenty of these sleep / no sleeps lately and @cperciva probably not happy with another one (even though wireless is not on his agenda). Why can dhcpcd not get the proper interface status and on sun when wlan interfaces as associated? [sorry lately asked myself that also for the current in-tree situation]. [just mentioning @cy as he probably wants to think about this as well]

Just one note, OpenBSD has recently implemented a new DHCP client called dhcpleased, which has a very small code size and should be checked before we integrated this patch.

libexec/rc/network.subr
229 ↗(On Diff #97323)

Indeed, I have a VM where 80% of the boot process is sleeps for IPv6 DAD. Please, no more sleeps in rc.d scripts!

(To make it even crazier, the only network interface on that VM is lo0...)

In D22012#818780, @gbe wrote:

Just one note, OpenBSD has recently implemented a new DHCP client called dhcpleased, which has a very small code size and should be checked before we integrated this patch.

I don't believe dhcpleased supports DHCPv6 or prefix delegation, which was the main driver for the switch to dhcpcd.

As far as I can tell the objections that were raised over the last few years have all been addressed, and it doesn't seem like much progress has been made on the proposed alternatives. It seems reasonable to me to import dhcpcd initially controlled by a build knob (i.e., WITH_DHCPCD) to allow for further development and experimentation.

Hi @emaste, I hadn't considered the WITH_DHCPCD build knob, since dhcpcd would initially be disabled by default in /etc/rc.d/dhcp_client.
However, I take the point that adding this will allow people on embedded systems to save space/time in the build, and also allow the interface for the rc scrips and network.subr to be excluded from the initial commit (if the goal is to break that out to make it easier to review/commit in smaller chunks).

With that in mind, I'll reduce the contents of this review to only include:

Included Path Reason for inclusion in initial commit
/contrib/dhcpcd/main code for vendor branch
/etc/mtree/required to create the installation folders
/libexec/rc/rc.d/dhcpcdallow people to manually enable in rc.conf
/sbin/dhcpcd/Makefiles referencing /contrib/dhcpcd/
/share/man/man7/hier.7required due to the inclusion of /etc/mtree/ above
/targets/psuedo/userlandrequired for Makefile build targets

For now I will exclude:

Excluded Path Reason for exclusion from initial commit
/libexec/rc/ (other than rc.d/dhcpcd)defer this until after initial import to allow more focused review of the rc.conf and network.subr structure and variable naming
/rescue/delay until dhcpcd enabled by default
/sbin/devd/delay until /libexec/rc lands
/share/examples/delay until /libexec/rc lands
/share/man/man5/rc.conf.5delay until /libexec/rc lands
/share/security/lomac-policy.contextsdelay until dhcpcd enabled by default

Does that feel about right?

A few more questions I have:

  1. Do you agree with dhcpcd being in the runtime package, or do you think it should be its own package?
  2. Is there a need/way to make mtree only add the folders if the build knob is enabled?
  3. Before all of this, would come a commit to add the new vendor branch for dhcpcd - assume that doesn't get a phabricator review of its own (not sure you can submit reviews for vendor branches)?
  4. Does this need another mention on freebsd-net@ mailing list before it lands?

https://lists.freebsd.org/archives/freebsd-net/2022-August/002234.html

I posted on the freebsd-net mailing list to raise the proposal once again with the broader community

libexec/rc/network.subr
229 ↗(On Diff #97323)

dhcpcd can and will react to proper interface status. It should not require this change here or break wpa_suppliant.

229 ↗(On Diff #97323)

Indeed, I have a VM where 80% of the boot process is sleeps for IPv6 DAD. Please, no more sleeps in rc.d scripts!

(To make it even crazier, the only network interface on that VM is lo0...)

See D5469 for a solution to that.

Can you quickly explain how this works along with RS/RA handling for DHCPv6? What does dhcpc do in this case and what does it not do?

This is mainly triggered by the fact that network.subr for dhcpif has an unconditional call for IPv6 duplicating the IPv4 logic but that's not how it works in IPv6 land so I am confused...

In D22012#819198, @bz wrote:

Can you quickly explain how this works along with RS/RA handling for DHCPv6? What does dhcpc do in this case and what does it not do?

This is mainly triggered by the fact that network.subr for dhcpif has an unconditional call for IPv6 duplicating the IPv4 logic but that's not how it works in IPv6 land so I am confused...

I don't understand the question. Could you explain your query futher please?

Remove any changes to rc scripts other than adding the new
/etc/rc.d/dhcpcd

woodsb02 edited the test plan for this revision. (Show Details)

Please note there's a bug in dhcpcd and/or FreeBSD: https://github.com/NetworkConfiguration/dhcpcd/pull/121. Could somebody more experienced with networking take a look, please? In short, without the mentioned PR, dhcpcd removes IPv6 routes from bridge0 interface while re0 is being configured. Thank you!

As nd6.c states /* flush all the prefix advertised by routers */ and route that dhcpcd removes is definetely not advertized by any router but set as part of ifconfig re0 inet6 ...., maybe the bug is on FreeBSD side?

I did a little research and it turns out that SIOCSPFXFLUSH_IN6 is only supported on FreeBSD and derivatives like DragonflyBSD so the check if it's defined is practically check if it's running on FreeBSD. As NetBSD removed it and OpenBSD doesn't have it, what will happen if we remove the SIOCSPFXFLUSH_IN6 call from dhcpcd?

I did a little research and it turns out that SIOCSPFXFLUSH_IN6 is only supported on FreeBSD and derivatives like DragonflyBSD so the check if it's defined is practically check if it's running on FreeBSD. As NetBSD removed it and OpenBSD doesn't have it, what will happen if we remove the SIOCSPFXFLUSH_IN6 call from dhcpcd?

NetBSD and OpenBSD no longer process Router Advertisements in the kernel so the ioctl no longer applies to them.
If the ioctl is removed from dhcpcd then the kernel will expire stuff might have learned from RA's once dhcpcd takes over and disables the kernel.
The only correct solution is to fix the ioctl, which is what I've done in D36306

The only correct solution is to fix the ioctl, which is what I've done in D36306

Variation of SIOCSPFXFLUSH_IN6 fix has landed ( 8036234c72c9 ).

woodsb02 marked 7 inline comments as done.

Implement review comments:

  • Add dhcpcd as a separate package (not in "runtime" package)
  • Allow dhcpcd to be compiled without IPv4 or IPv6 support
  • Remove duplicate system directory configuration
  • Bump manpage dates
  • Simplify rc.conf(5) language describing master mode

Close comments which have been implemented

Fix build Makefile by adding .include <src.opts.mk>

Thanks for the good work Ben.

libexec/rc/rc.d/dhcpcd
6

I'm wondering how we tackle the rc script when both dhcpcd exists in base and in ports (and both are installed).
I know for unbound, we made it "local_unbound" for the in-base version, and "unbound" for the port version.
I'm asking this as what happens when dhcpcd is installed from ports and is present in the base system with the same rc identifier, probably will be started twice with one dhcpcd_enable line?

14

Ditto

Implement review comments:

  • Add dhcpcd as a separate package (not in "runtime" package)
  • Allow dhcpcd to be compiled without IPv4 or IPv6 support
  • Remove duplicate system directory configuration
  • Bump manpage dates
  • Simplify rc.conf(5) language describing master mode

how to specifically disable ipv6 ( or ipv4 ) when dhcpcd is in /usr/src?

Some or all of this needs pushing upstream instead of/in addition to local changes. Since the upstream maintainer is subscribed, I haven't made the distinction.

contrib/dhcpcd/README.md
4–6

I believe Wikipedia has been redirecting http to https for 6 years or so. Let's save a round trip.

8

Per https://books.google.com/ngrams/graph?content=layman_INF%2Clayperson_INF&year_start=1800&year_end=2019&corpus=28&smoothing=3, "laym{ae]n" steeply declined in frequency of use starting around 1960 and is now very close to the gender-agnostic forms layperson(s) and laypeople. So I'd future-proof this.

96

After checking it works.

contrib/dhcpcd/hooks/dhcpcd-run-hooks.8
71
72

Spurious line?

79
88

Make clearer to non-native speakers it's "initialize now", not "initialization should already be complete".

104
107–108

Is "REBOOT" correct? I can't connect the dots between rebooting and getting a leased address for the first time.

147

For consistency, either remove .Nm here or add it to all list items above.

184

A SSID *is* a name.

195

What does that mean?

202

Same question as above.

210
216–220

Commafy list to make it clearer that "lexical order" applies to /libexec/dhcpcd-hooks/* only.

227

Since the https version works.

contrib/dhcpcd/src/dhcpcd.8
174
186

Or "The ... option"

324
454–457

Unclear which configuration "and configuration" refers to.

793
829
885
contrib/dhcpcd/src/dhcpcd.conf.5
1004

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

That would help me greatly bringing any changes requested here upstream.

As far as I can tell the objections that were raised over the last few years have all been addressed, and it doesn't seem like much progress has been made on the proposed alternatives. It seems reasonable to me to import dhcpcd initially controlled by a build knob (i.e., WITH_DHCPCD) to allow for further development and experimentation.

This seems reasonable. Just add DHCPCD to DEFAULT_NO and we should be good to go :)

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

That would help me greatly bringing any changes requested here upstream.

and hopefully it will be easier to review here too :)
Win win

Fix build WITHOUT_INET=yes: move SRCS+=privsep-bpf.c inside .if ${MK_INET_SUPPORT}

how to specifically disable ipv6 ( or ipv4 ) when dhcpcd is in /usr/src?

As per the rest of the FreeBSD src branch - add one of the following to your /etc/src.conf file before running make buildworld:

  • WITHOUT_INET=yes
  • WITHOUT_INET6=yes

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

No changes have been made in this review from the copy in the vendor/dhcpcd branch.
The only difference if that rather than being a direct copy/paste, I have used the mechanism built into dhcpcd to load the src into contrib/dhcpcd - this is described in contrib/dhcpcd/README.FREEBSD - extract below for convenience.

The source is imported via a Makefile target rather than by hand.
There is no README.DELETED for this import as it's all automated.

Use "git diff ../vendor/dhcpcd contrib/dhcpcd" to see local modifications.

The program, hook scripts and configuration file are installed by 'sbin/dhcpcd'.

Upgrade notes
-------------

1. Configure
2. Import
3. Copy config.h to sbin/dhcpcd
4. Tailor Makefile in sbin/dhcpcd to import

$ ./configure
$ make import-src DESTDIR=/usr/src/contrib/dhcpcd
$ cp config.h /usr/src/sbin/dhcpcd
$ vi /usr/src/sbin/dhcpcd/Makefile

The following files are only in contrib/dhcpcd:

  • hooks/30-hostname
  • hooks/50-ypbind
  • hooks/dhcpcd-run-hooks
  • hooks/dhcpcd-run-hooks.8
  • src/dhcpcd-embedded.c
  • src/dhcpcd-embedded.h
  • src/dhcpcd.8
  • src/dhcpcd.conf.5
  • README.FREEBSD

The following files are only in vendor/dhcpcd (not brought into contrib/dhcpcd by the "make import-src"):

  • .git/*
  • compat/crypt/md5.c
  • compat/crypt/md5.h
  • compat/crypt/sha256.c
  • compat/crypt/sha256.h
  • compat/arc4random.c
  • compat/arc4random.h
  • compat/arc4random_uniform.c
  • compat/arc4random_uniform.h
  • compat/dprintf.c
  • compat/dprintf.h
  • compat/endian.h
  • compat/queue.h
  • compat/reallocarray.c
  • compat/reallocarray.h
  • compat/setproctitle.c
  • compat/setproctitle.h
  • compat/strlcpy.c
  • compat/strlcpy.h
  • hooks/30-hostname.in
  • hooks/50-dhcpcd-compat
  • hooks/50-yp.conf
  • hooks/50-ypbind.in
  • hooks/Makefile
  • hooks/dhcpcd-run-hooks.8.in
  • hooks/dhcpcd-run-hooks.in
  • src/dev/*
  • src/GNUmakefile
  • src/Makefile
  • src/dev.c
  • src/dhcpcd-definitions-small.conf
  • src/dhcpcd-definitions.conf
  • src/dhcpcd-embedded.c.in
  • src/dhcpcd-embedded.h.in
  • src/dhcpcd.8.in
  • src/dhcpcd.conf.5.in
  • src/genembedc
  • src/genembedh
  • src/if-linux-wext.c
  • src/if-linux.c
  • src/if-sun.c
  • src/privsep-linux.c
  • src/privsep-sun.c
  • tests/*
  • .gitignore
  • BUILDING.md
  • Makefile
  • Makefile.inc
  • config-null.mk
  • configure
  • iconfig.mk
In D22012#826694, @imp wrote:

I'd suggest updating this review to be a diff against the copy of dhcpcd now in vendor/ merged into contrib/, so that changes if any to existing dhcpcd files are clear

That would help me greatly bringing any changes requested here upstream.

and hopefully it will be easier to review here too :)
Win win

Given I have not made any changes to the contents of contrib/dhcpcd (other than adding the new README.FreeBSD file), suggest reviewers could exclude contrib/dhcpcd (focus on other changes in the review)?

Some or all of this needs pushing upstream instead of/in addition to local changes. Since the upstream maintainer is subscribed, I haven't made the distinction.

Thanks for the detailed review Pau!

I am trying to keep the contrib/dhcpcd code exactly as per upstream, so rather than fix them here, I have submitted a pull request to upstream on your behalf:
https://github.com/NetworkConfiguration/dhcpcd/pull/123

The only comments of yours I didnt include were:

  • Your question about what was meant by IFF_UP - will leave it to @roy_marples.name to clarify wording
  • Your question about whether REBOOT was correct - will leave it to @roy_marples.name to clarify wording
  • Removing the .Nm from "dhcpcd" at the beginning of the ENVIRONMENT section of the manpage - as I suspect it is either referring to the name of the binary when executed, or is for consistency showing the bold name at the beginning of a new section. @roy_marples.name?

I've addressed the IFF_UP and REBOOT comments with hopefully suitable answers but not marked them as done as I think the asker needs to be satisified and close themselves?

contrib/dhcpcd/hooks/dhcpcd-run-hooks.8
107–108

REBOOT is when a previously aquired lease is acked by the server but not yet on the interface.
Such as when the host initially starts up or when the carrier goes down or comes back up again.

If there is no previouisly acquired lease, or it has expired then the BOUND event is raised instead.

Realistically there is no difference between the two and mainly exists as to be compatible with dhclient-script(8)

195

For example the link could be down or could be in some other transitive state.
I'm deliberately being vague because it's highly dependent on the interface state, what the OS supports and the state of various protocols running on it.

I think I'm trying to say that even if the interface is marked as up, dhcpcd might mark it as not up.

202

Same answer as above, but note that if_down is seperate from if_up - it's not possible both would be true, but it is possible for both to be false.