kern_linker: Handle module-loading failures in preloaded .ko files

[cem]

Summary

The runtime kernel loader, linker_load_file, unloads kernel files that
failed to load all of their modules. For consistency, treat preloaded
(loader.conf loaded) kernel files in the same way.

Test Plan

• I created a kernel object with one module that always fails MOD_LOAD.
• Before this patch, I observe:
  • module_register_init runs via .ko SYSINIT
  • It logs module_register_init: MOD_LOAD (...) error 6
  • kldstat shows kernel object still present (not unloaded)
  • kldstat -v shows no modules for the kernel object
  • Manual kldunload succeeds.
  • Manual kldload of the same module fails
  • After this, kldstat does not show the kernel object at all (desired behavior)
• After this patch,
  • The preloaded kernel object is unloaded correctly after module_register_init fails.
  • Manual kldload fails in the same way.

[kib]

I do not like the teared completion of the linker file state in your patch, and that the inconsistent state is visible after kld_sx is dropped.

You might transpose the change, by counting modules and failed modules in the linker files instead. Then finalizing action in the linker_preload() may handle the remnants, detected by the counters being equal.

sys/kern/kern_linker.c

1688 ↗

(On Diff #21192)

SI_SUB_LAST is after the userspace is started. As result, userspace may observe linker files which lack references.

[cem]

In D8200#170190, @kib wrote:

I do not like the teared completion of the linker file state in your patch, and that the inconsistent state is visible after kld_sx is dropped.

This is exactly the same in the runtime linker path. See linker_file_sysinit(), where kld_sx is dropped while sysinits run. Meanwhile the half-loaded kobj is visible.

(Note that this is a case where a linker file without LINKER_FILE_LINKED set would be visible to userspace — the case imagined but not proved in D8167.)

You might transpose the change, by counting modules and failed modules in the linker files instead. Then finalizing action in the linker_preload() may handle the remnants, detected by the counters being equal.

Modules may need to be visible in this half-loaded state for other preloaded linker files' SYSINITs to run. (We can't necessarily keep everything hidden until after preload_finish runs.)

Otherwise, that would be fine, but seems functionally identical. I don't see any reason to prefer that approach. Is there one? See for example the identical test in linker_load_file:

432                         modules = !TAILQ_EMPTY(&lf->modules);
433                         linker_file_register_sysctls(lf);
434                         linker_file_sysinit(lf);
435                         lf->flags |= LINKER_FILE_LINKED;
436
437                         /*
438                          * If all of the modules in this file failed
439                          * to load, unload the file and return an
440                          * error of ENOEXEC.
441                          */
442                         if (modules && TAILQ_EMPTY(&lf->modules)) {
443                                 linker_file_unload(lf, LINKER_UNLOAD_FORCE);
444                                 return (ENOEXEC);
445                         }

The LINKER_FILE_MODULES flag just takes the place of the boolean modules variable in linker_load_file for preloaded kobjs.

[cem]

sys/kern/kern_linker.c

1688 ↗

(On Diff #21192)

When is userspace started? SI_SUB_KTHREAD_INIT?

[kib]

sys/kern/kern_linker.c

1688 ↗

(On Diff #21192)

Yes, that it.

Also, another non-pleasant point is when drivers start and might try to find preloaded firmware modules, unloading them after some events. This can happen even before the userspace enabled.

[cem]

sys/kern/kern_linker.c

1688 ↗

(On Diff #21192)

Also, another non-pleasant point is when drivers start and might try to find preloaded firmware modules, unloading them after some events.

Hm, what problem exists there?

For example, firmware_get -> loadimage -> linker_reference_module uses refs, not userrefs.

[cem]

Run preload_finish before userspace starts.