kern_linker: Hide incompletely loaded modules from userspace
AbandonedPublic
Actions

Authored by cem on Oct 6 2016, 7:49 PM.

Details

Reviewers

kib
emaste
jhb
imp

Summary

For consistency with linker_find_file_by_id, used by kldunload(), add
the option to linker_find_file_by_name (used by kldfind()) to ignore
incompletely linked modules.

It doesn't make sense to find a module before it can be unloaded.

Internal kernel consumers of linker_find_file_by_name continue to find
partially linked modules, as their goal is to avoid loading the same
module twice.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5486
Build 5706: arc lint + arc unit

Event Timeline

cem updated this revision to Diff 21126.Oct 6 2016, 7:49 PM

cem retitled this revision from to kern_linker: Hide incompletely loaded modules from userspace.

cem updated this object.

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

cem added reviewers: kib, emaste, jhb, imp.

How is that possible at all ? Did you observed this in real-life scenario ?

It seems that the flag is either set for the file, or the file is removed from the list, under the single kld_sx-protected region. Then, I do not see how could kldnext consumer, protected by the same lock, see a file without the flag.

In D8167#169639, @kib wrote:

How is that possible at all ? Did you observed this in real-life scenario ?

Don't know if it is possible. No, not observed.

It seems inconsistent with linker_find_file_by_id and sys_kldnext, which is also only used under the same sx lock.

It seems that the flag is either set for the file, or the file is removed from the list, under the single kld_sx-protected region. Then, I do not see how could kldnext consumer, protected by the same lock, see a file without the flag.

If this is true, I don't see why we need to even check for the flag in kldnext or linker_find_file_by_id.

In D8167#169675, @cem wrote:

If this is true, I don't see why we need to even check for the flag in kldnext or linker_find_file_by_id.

The flag should be removed where not needed. I suspect that it is result of the code first being locked by Giant which is dropped during sleeps.

I did not traced all paths in the code to confirm either validity of the patch in review, nor the removal of flags.

I'm still annoyed at the inconsistency but not interested in exhaustively looking at callers to verify no such path exists for linker_find_file_by_id.

cem mentioned this in D8200: kern_linker: Handle module-loading failures in preloaded .ko files.Oct 9 2016, 8:14 PM

Run-time loaded linker files are present on the global list but missing LINKER_FILE_LINKED during linker_file_sysinit() in linker_file_load(), where kld_sx is dropped.

In D8167#170196, @cem wrote:

Run-time loaded linker files are present on the global list but missing LINKER_FILE_LINKED during linker_file_sysinit() in linker_file_load(), where kld_sx is dropped.

In this case, I think that LINKER_FILE_LINKED might be reasonably set before sysinit sets are invoked. There is no possible failure in linker_file_sysinit(), so the state is de-facto finalized.

Then unset the flag if linker_file_unload/ENOEXEC is returned.

cem abandoned this revision.Dec 23 2016, 12:00 AM

Ok, please commit this as is. I do think that implementing my last note would make the change better, but anyway, just go ahead.

Revision Contents
Changeset List

Path

Size

sys/

kern/

kern_linker.c

19 lines

Diff 21126

View Options

kern_linker: Hide incompletely loaded modules from userspaceAbandonedPublicActions