Page MenuHomeFreeBSD

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

Authored by cem on Oct 8 2016, 8:15 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem retitled this revision from to kern_linker: Handle module-loading failures in preloaded .ko files.
cem updated this object.
cem edited the test plan for this revision. (Show Details)
cem added reviewers: jhb, kib, imp.

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.

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.

sys/kern/kern_linker.c
1688 ↗(On Diff #21192)

When is userspace started? SI_SUB_KTHREAD_INIT?

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.

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 edited edge metadata.

Run preload_finish before userspace starts.

cem marked 2 inline comments as done.Oct 11 2016, 7:05 PM
kib edited edge metadata.
This revision is now accepted and ready to land.Oct 13 2016, 1:24 AM
This revision was automatically updated to reflect the committed changes.