svcj: correctly handle kernels without INET or INET6
ClosedPublic
Actions

Authored by ivy on Apr 23 2025, 10:08 AM.

Details

Reviewers

kevans
des
netchild

Group Reviewers

Jails
rc

Commits

rG06c41801afac: svcj: correctly handle kernels without INET or INET6

Summary

if either INET or INET6 is not enabled in the kernel, then the jail(8)
options ip4=<new|inherit> resp. ip6=<new|inherit> are not available.
detect this case and don't try to provide those options, otherwise
svcjs will not start.

do this automatically (without a warning) so that net_basic, which
includes both netv4 and netv6, continues to work as expected.

if _svcj_ipaddrs is explicitly configured with an address for an IP
version not supported by the kernel, issue a warning but continue to
start the service. this can result in the service being started with
fewer addresses than expected, but never more.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 63665
Build 60549: arc lint + arc unit

Event Timeline

ivy created this revision.Apr 23 2025, 10:08 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 23 2025, 10:08 AM

ivy requested review of this revision.Apr 23 2025, 10:08 AM

Harbormaster completed remote builds in B63663: Diff 154154.Apr 23 2025, 10:08 AM

Wouldn't it be better to check at point of use rather than at point of initialization?

libexec/rc/rc.subr
1231–1250	What if inet is enabled but inet6 is not, but there are ip6 addrs? If I'm not mistaken, in that case you have set this it to ip6=new above, and this branch is wrongly taken. To me it looks it's better to simply set a "new" flag, and set both to new after this check and before the "translate service jail options" part.

This revision now requires changes to proceed.Apr 23 2025, 10:31 AM

ivy added inline comments.Apr 23 2025, 10:39 AM

libexec/rc/rc.subr
1231–1250	yes, you're right. i'll rethinking this and see if i can address des comment at the same time.

i think this is a more reasonable way to do it, and seems to work in testing.
i'd like to add a proper set of tests for this, but since the test jails run
with path=/, i'm not sure there's a reasonable way to test this kind of thing.

Harbormaster completed remote builds in B63665: Diff 154156.Apr 23 2025, 11:21 AM

In D49976#1139747, @des wrote:

Wouldn't it be better to check at point of use rather than at point of initialization?

I see your point. On the other hand I think the part which sets "the other one" may be more easy at the current place than at the place of use. I also like the current way of having the case statement "nice" (not convoluted with checks but simply listing what it entails).

i've tested the current patch with all combinations of:

INET+INET6, INET only, INET6 only
IPv4 addresses in _ipaddrs, IPv6 addresses in _ipaddrs, both

and it appears to work in all cases.

i did find a problem where, in the case where INET6 is not enabled in the kernel, adding 127.0.0.1 to _ipaddrs does not work ("jail: IPv4 addresses clash") but i believe this is a pre-existing problem.

@netchild are you okay with the current diff?

Just from reading I do not see an obvious issue now. Revision accepted (fixing the minor nit doesn't need another review IMO).