Bug 295139 - kldload prints wrong error message to terminal upon attempt to load incorrect module version

[jim.chen.1827_gmail.com]

Summary

Addresses a failure in linker_load_module (sys/kern/kern_linker.c) to verify that an already-loaded module matches the version requirement, which causes the method to return the error (EEXIST). This is then propagated back up to kldload, which incorrectly prints that the module has already been loaded.

This patch adds a lookup to modlist_lookup2 to distinguish between the two cases at that point in the code:

1. A module is already loaded that is of the correct version, so the error (EEXIST) should be returned
2. An already-loaded module is of the incorrect version, so the error (ENOEXEC) is returned (changed from ENOENT)

[imp]

This looks good to my eye.
However, in the future, can you please use git-arc to upload patches, or generate the raw diffs with -U99999 please? There's no context to this review making it harder to verify that other paths make this same check.

[jim.chen.1827_gmail.com]

Changed ENOENT errors to ENOEXEC for more semantic correctness in the case where the file exists but is unusable due to version incompatibility.

Also updated patch to implement some suggestions from Mr. Maste:

1. Use EXTERROR to give additional context regarding the errors
2. Unbreak error/warning strings so future developers are able to "grep" for the source

[kib]

exterr_cat_filenames.h should be regenerated.

Overall, the messages are too long and too wordy, I tried to suggest more concise texts.

sys/kern/kern_linker.c

60

Move the definition before the include block, and move the sys/exterrvar.h into the include block.

461

"security level %d" should be enough in the error text, there is already 'permission denied' part from EPERM

515

"no modules loaded"

550

"module format error"

553

"kld file not found"

2271

"root not yet mounted"

2275

"module was already loaded"

2277

See above

2288

"kld file not found"

2301

2305

"incompatible module version already loaded"

2326

"incompatible module version"

Perhaps would be nice to report the version

2375

"module version %d already loaded"

[jim.chen.1827_gmail.com]

• Shortened error messages
• Added entry to exterr_cat_filenames.h
• Addressed potential memleak introduced in linker_load_module() line 2315

[jim.chen.1827_gmail.com]

Added missing diff context

[jim.chen.1827_gmail.com]

Added suggested edit to comment

[jim.chen.1827_gmail.com]

Rebased onto main

[kib]

sbin/kldload/kldload.c

185

No need to wrap the line

sys/kern/kern_linker.c

2269

The message is reversed, it should be 'verinfo is NULL', after all.

[jim.chen.1827_gmail.com]

Removed unnecessary line wrap

[jim.chen.1827_gmail.com]

sys/kern/kern_linker.c

2269

I'm not sure I understand. It seems to me that there will be a panic only if verinfo == NULL is false, which would make the error message "verinfo is not NULL" correct when a panic does occur.

If the message is ambiguous due to what it "is" at time of panic vs. what it "should be," I can edit it to "verinfo should be NULL" or "verinfo is unexpectedly not NULL"

It also occurs to me that since the message offers virtually no additional information compared to MPASS, just the information that "Assertion verinfo == NULL failed" could be an equivalent but less ambiguous choice here.

Which course of action would you suggest?

[emaste]

sys/kern/kern_linker.c

2269

Yes, this is one of my complaints about KASSERT. You could make the message verinfo must be NULL to avoid the ambiguity about is, but I'd just go with MPASS; panic: Assertion verinfo == NULL failed at kern_linker.c:2268 seems like a fine message.

[jim.chen.1827_gmail.com]

Changed potentially ambiguous KASSERTs into MPASS