Page MenuHomeFreeBSD

domain: make it safer to add domains post-domainfinalize
Needs ReviewPublic

Authored by kevans on Fri, Jun 26, 2:50 AM.

Details

Reviewers
jhb
imp
Summary

I can see two concerns for adding domains after domainfinalize:

1.) The slow/fast callouts have already been setup.
2.) Userland could create a socket while we're in the middle of initialization.

We can address #1 fairly easily by tracking whether the domain's been initialized for at least the default vnet. There are still some concerns about the callbacks being invoked while a vnet is in the process of being created/destroyed, but this is a pre-existing issue that the callbacks must coordinate anyways.

#2 we should address, but technically this has been an issue anyways because we don't assert on post-domainfinalize additions; we don't seem to hit it in practice.

Future work can fix that up to make sure we don't find partially constructed domains, but care must be taken to make sure that at least, e.g., the usages of pffindproto in ip_input.c can still find them.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 31969

Event Timeline

kevans requested review of this revision.Fri, Jun 26, 2:50 AM
kevans created this revision.
jhb added a subscriber: kib.Fri, Jun 26, 6:28 PM

Actually, why do we add the domain to the list via domain_add() before calling domain_init()? If we do domain_init() before domain_add() then doesn't that fix the issue? It would mean swapping the order of the SYSINIT's in DOMAIN_SET and VNET_DOMAIN_SET, but presumably none of the domain init routines really care that they are on the global list yet?

sys/kern/uipc_domain.c
194

Do you need a fence or the like here? There's no lock protecting this value. @kib might have some suggestions

In D25459#562323, @jhb wrote:

Actually, why do we add the domain to the list via domain_add() before calling domain_init()? If we do domain_init() before domain_add() then doesn't that fix the issue? It would mean swapping the order of the SYSINIT's in DOMAIN_SET and VNET_DOMAIN_SET, but presumably none of the domain init routines really care that they are on the global list yet?

Great question- I wondered myself when looking at it, but it was clearly deliberate so I didn't dig much into it.

kib added inline comments.Fri, Jun 26, 7:19 PM
sys/kern/uipc_domain.c
194

Formally this should be an atomic or with release semantic, and then reads of dom_flags need acquire for guaranteed observation of updates prior to the stage where DOMF_INITED was set (unless I misunderstood the algorithm).

jhb added a comment.Fri, Jun 26, 7:20 PM

I suspect it is less deliberate and it's probably worth looking into to see if we can reverse them. To be honest, what most other subsystems would do is use a single SYSINIT that initialized the object and then added it to the list (e.g. the module handler for device driver modules). If domains were per-VNET that would be trivial. As it is, you probably want to init on all VNETs first via the existing separate SYSINITs before the domain_add.

In D25459#562400, @jhb wrote:

I suspect it is less deliberate and it's probably worth looking into to see if we can reverse them. To be honest, what most other subsystems would do is use a single SYSINIT that initialized the object and then added it to the list (e.g. the module handler for device driver modules). If domains were per-VNET that would be trivial. As it is, you probably want to init on all VNETs first via the existing separate SYSINITs before the domain_add.

Sure- I'll investigate this angle more thoroughly.

I briefly looked at per-VNET domains and decided that was probably an incredibly slippery slope. It'd be trivial for socreate() to compensate, but to do it right I think you'd need per-VNET slow/fast callouts lest you face another synchronization nightmare.

OK, the problem with reversing them is that some pr_init will want to pffindproto() a protocol from the domain they're a part of (e.g. ip_init/ip6_init), and that's not necessarily trivial to resolve at first blush.