kern linker: Use EUNSDEP for unsatisfied dependencies

[zlei]

Summary

An error from loading a dependency of linker file means there's something wrong
with dependency but not the linker file itself. A propagated error from unsatisfied
dependencies is misleading and it is hard to figure out what happens behind.

See also the mailing list report [1].

1. https://lists.freebsd.org/archives/freebsd-virtualization/2024-March/002040.html

MFC after: 2 weeks

Test Plan

Put all test modules in /boot/modules/ .

Test directly unsatisfied dependency, load vboxdrv.ko build for release/13.2 on CURRENT machine:

# kldload vboxdrv.ko
kldload: an error occurred while loading module vboxdrv.ko. Please check dmesg(8) for more details.
# dmesg | tail 
...
KLD vboxdrv.ko: depends on kernel - version mismatch

Test transitive dependency error,
foo.ko -> [bar.ko, kernel CURRENT]
bar.ko -> kernel 13.3

Load foo.ko on CURRENT

# kldload foo
kldload: an error occurred while loading module foo. Please check dmesg(8) for more details.
# dmesg | tail 
...
KLD bar.ko: depends on kernel - version mismatch
KLD foo.ko: depends on bar - transitive dependency error

[zlei]

sys/kern/kern_linker.c

449

EUNSDEP means the linker file is recognized but its dependency / dependencies is / are failed to be loaded.

[imp]

sys/kern/kern_linker.c

2396

If EUNSDEP can be returned here, it needs to live in errno.h and be documented for the kld system call.

[olce]

sys/kern/kern_linker.c

449

I would just put EUNSDEP in errno.h, assign it a global value (we are far for running out of them, and EUNSDEP might be useful in other contexts), and a *negative* one at that. Negative values are already used in some places as internal values that must not leak to userland. They make it much easier to notice such leaks.

2364–2391

I'd move this printing in linker_load_module(). We would be a little more verbose for user-triggered module loading failures, but this would help by providing more context for in-kernel loads.

2396

I think you've been abused by Phabricator. It seems it indicates that the changes above are part of sys_kldload(), but they aren't. EUNSDEP as it is in this patch is not meant to be propagated to userland. See also the error code translation in kern_kldload() above.

[imp]

sys/kern/kern_linker.c

2396

It is returned from this routine... not sure where they might go from there... but if it does leak out, it should be documented...

[olce]

sys/kern/kern_linker.c

2396

I agree with you in principle (of course). But I've checked, and the value returned by this routine doesn't leak (and judging by the other changes above, it's clearly @zlei's intention that it doesn't). In the end, the value returned here percolates up to linker_load_file() in this file, which is only called by linker_load_module(). This routine (linker_load_dependencies()) is indeed only called by the implementations of linker_load_file().

That said, amending the herald comments of the modified functions to reflect the introduction of EUNSDEP and the fact it shouldn't leak, and also perhaps what I've just explained, would speed up understanding.

[zlei]

Made a reverse call graph to help evaluating the impact of this change:

linker_load_dependencies() -> linker_load_module()
	LINKER_LOAD_FILE()
		linker_load_file()
			linker_load_module()
				linker_reference_module()
					loadimage()
						TASK_INIT(&fwload_task, 0, loadimage, (void *)&fwli)
				kern_kldload()
					sys_kldload() # syscall
					vfs_byname_kld() ### translate error to ENODEV
						sys_mount() # syscall
						vfs_domount()
							vfs_donmount()
								sys_nmount() # syscall

[zlei]

sys/kern/kern_linker.c

449

Good point for the negative EUNSDEP :)

2364–2391

For in-kernel loads, IIUC you mean loadimage() from sys/kern/subr_firmware.c, there is already verbose mode to print the error:

loadimage(void *arg, int npending __unused)
{
...
        error = linker_reference_module(fwli->imagename, NULL, &result);
        if (error != 0) {
                if (bootverbose || (fwli->flags & FIRMWARE_GET_NOWARN) == 0)
                        printf("%s: could not load firmware image, error %d\n",
                            fwli->imagename, error);
                try_binary_file(fwli->imagename, fwli->flags);
                mtx_lock(&firmware_mtx);
                goto done;
        }
...
}

So those printings are most useful for dependency error only.

[zlei]

sys/kern/kern_linker.c

2396

But I've checked, and the value returned by this routine doesn't leak (and judging by the other changes above, it's clearly @zlei's intention that it doesn't).

Yes, EUNSDEP is not going to be leaked. A translation to ENOEXEC should be sufficient.

Anyway, it can be translated within function sys_kldload(), in case other in-kernel modules have interested in it.

[olce]

sys/kern/kern_linker.c

2364–2391

In-kernel loads are not limited to loadimage(). You've modified the stack trace, so you must have noticed that kern_kldload() has other callers than sys_kldload(), namely:

• request_module() <- iwl_req_fw_callback()
• vfs_byname_kld()
• ngc_send()

None of these callers print anything in case of failure. In particular, in vfs_mount.c, no message percolates to userland through errmsg.

So I definitely think we should do something, and linker_load_module() a priori seems to be the best place.

[olce]

sys/kern/kern_linker.c

2396

Yes, EUNSDEP is not going to be leaked. A translation to ENOEXEC should be sufficient.

Yes. The only thing I don't like much is that this translation is performed in different places. I'd prefer to have a new wrapper doing that, such as linker_load_module_nounsdep(), and callers doing the translation call that one instead.

Anyway, it can be translated within function sys_kldload(), in case other in-kernel modules have interested in it.

Yes, it could, but I see no need to do that now. Even if you see one, arguably this should be put in a separate commit, since that change will have impact elsewhere.

[zlei]

Moved the errno into sys/sys/errno.h, see D44581 .

[zlei]

sys/kern/kern_linker.c

573

The only in-tree consumer is static void loadimage() in sys/kern/subr_firmware.c and nonzero error from linker_reference_module() is checked, so this translation is redundant.

Out-tree consumers should check the return value of linker_reference_module() so they shall not be harmed.

[zlei]

Stop translating errno from linker_reference_module() .

[imp]

The code looks good now. I'm satisfied my concern was addressed as well.
Minor nits on comments are all I see still.

sys/kern/kern_linker.c

481

this comment now doesn't quite match the code.

546

need to say something about EUNSDEP here too?

[zlei]

Move translating errno EUNSDEP from kern_kldload() to sys_kldload()

After carefully checking, I can conclude it is safe to move the translation to sys_kldload(), as consumers of kern_kldload() either have done the translation themself, or totally ignore the return value, e.g. request_module in sys/compat/linuxkpi/common/include/linux/kmod.h, [1] and [2]. For the latter the current change will not make it worse.

Thanks @olce for pointing them out !

1.iwl_req_fw_callback() in sys/contrib/dev/iwlwifi/iwl-drv.c

2. mlx4_request_modules() in sys/dev/mlx4/mlx4_core/mlx4_main.c

[zlei]

sys/kern/kern_linker.c

2364–2391

In-kernel loads are not limited to loadimage(). You've modified the stack trace, so you must have noticed that kern_kldload() has other callers than sys_kldload(), namely:

• request_module() <- iwl_req_fw_callback()
• vfs_byname_kld()
• ngc_send()

None of these callers print anything in case of failure. In particular, in vfs_mount.c, no message percolates to userland through errmsg.

So I definitely think we should do something, and linker_load_module() a priori seems to be the best place.

I think that deserves a dedicated CR.

[zlei]

Address @imp 's comments.

[zlei]

sys/kern/kern_linker.c

486

Is a blank line needed before the comments ?

490

I'm still struggling if EUNSDEP should be explained in detail. From the author's point of view, apparently , EUNSDEP indicates that the format of linker file is recognized. Will that make any confusion?

546

The above comments

The format is now recognized

should explain well. So no need verbiage.
Actually I should have done that in D42474.

[olce]

In D44552#1016580, @zlei wrote:

Move translating errno EUNSDEP from kern_kldload() to sys_kldload()

I really think doing this in this review is ill-advised. And as is stands, the current change has impacts so should be amended.

After carefully checking, I can conclude it is safe to move the translation to sys_kldload(), as consumers of kern_kldload() either have done the translation themself, or totally ignore the return value, e.g. request_module in sys/compat/linuxkpi/common/include/linux/kmod.h, [1] and [2]. For the latter the current change will not make it worse.

I agree with the second part (some ignore the return value, so don't really care), but not with the first. Unless I'm mistaken, there are almost direct return paths to userland in these two cases:

• vfs_byname_kld()
• ngc_send()

so EUNSDEP is going to be leaked.

Could you revert that modification? In other words, kern_kldload() still should translate the error itself, for now.

sys/kern/kern_linker.c

486

Strictly speaking, no. But you might add one if you find it clearer (blank lines can help reading if used to separate logically distinct blocks; but they are not used to this end nearly enough in the tree currently). IMO, here, I don't think there's a need to. YMMV.

490

The comment is correct. As well as for EEXIST, if you get EUNSDEP, that means that the format was recognized (and then the dependencies browsed and some found missing), and thus there's no need to try another loader for the object file. I don't think it needs more explanations, but up to you.

546

EUNSDEP is not covered by the comment above, nor by the preceding the format is now recognized, so I think @imp is right.

You could say that EUNSDEP also needs to percolate so that some function up in the stack (currently, linker_load_dependencies()) can print a different message on missing/unloadable dependencies. See also, my new comment below.

2364–2391

I think that deserves a dedicated CR.

As you like. Doing it here would be quite logical since printing this information really depends on what percolates in the calls resolving the dependencies.

As the code stands, at the point of switch (error), you can only really get ENXIO, EEXIST, ENOENT and what linker_load_file() returns.

[zlei]

In D44552#1016696, @olce wrote:

In D44552#1016580, @zlei wrote:

Move translating errno EUNSDEP from kern_kldload() to sys_kldload()

I really think doing this in this review is ill-advised. And as is stands, the current change has impacts so should be amended.

Sorry for that.

After carefully checking, I can conclude it is safe to move the translation to sys_kldload(), as consumers of kern_kldload() either have done the translation themself, or totally ignore the return value, e.g. request_module in sys/compat/linuxkpi/common/include/linux/kmod.h, [1] and [2]. For the latter the current change will not make it worse.

I agree with the second part (some ignore the return value, so don't really care), but not with the first. Unless I'm mistaken, there are almost direct return paths to userland in these two cases:

• vfs_byname_kld()

Rechecked this, CURRENT and stable/14 vfs_byname_kld() has translated error to ENODEV. As for stable/13 it will be leaked.

• ngc_send()

ngcontrol_protosw.pr_send == ngc_send so EUNSDEP will be leaked. Really sorry I did not read the code carefully.

so EUNSDEP is going to be leaked.

Could you revert that modification? In other words, kern_kldload() still should translate the error itself, for now.

Commit 3eed4803f943 (vfs mount: Consistently use ENODEV internally for an invalid fstype) let me rethink a good practice for error translation.
I would conclude, an error in domain A has the context, so when it goes cross to another domain B the context get lost, then probably it should be translated (to a proper error for domain B).

As for this case, when callers (vfs_byname_kld in stable/13, ngc_send ) of kern_kldload() get errors other than 0 or EEXIST, it means the required module is not available (for whatever reason), then a good error should be chosen for that.

I'll second that it is not the fault of callee, i.e. kern_kldload() to lead the leakage of kernel-only error, callers should always be care of that.

I'd like to fix vfs_byname_kld() and ngc_send() in later commits. Once done the leakage should not happen. But if you insist to revert, kern_kldload() can be punished to do the translation.

[olce]

In D44552#1017019, @zlei wrote:

Rechecked this, CURRENT and stable/14 vfs_byname_kld() has translated error to ENODEV. As for stable/13 it will be leaked.

Ah, I had forgotten the added translation to ENODEV in vfs_byname_kld(), but not in stable/13 indeed.

I would conclude, an error in domain A has the context, so when it goes cross to another domain B the context get lost, then probably it should be translated (to a proper error for domain B).

That seems a good principle in general but then there's the question of where the "crossing" happens. kern_*() interfaces were initially meant to be called from syscall glue code (adaptation for alternate syscall tables, e.g., 32-bit on 64-bit or Linux), which in particular performs copyin/copyout, and my current view is that they can also be used to make some subsystem functionality available to other subsystem with a kind of relatively formal interface. So, kern_*() functions should control relatively strictly their return codes and not, e.g., let new ones leak to callers, which also implies new return codes should land in the appropriate manual pages. In other words, this means that kern_*() and sys_*() should generally be treated the same, unless you have good reasons, and return the same codes (barring additional ones the syscall glue code can return). And, concerning the "crossing" part, if kern_*() is considered a kind of public interface to the rest of the kernel, crossing happens in fact in there and must be handled by it, and not the caller. Now, it is true that KPI restrictions are less stringent than syscall API restrictions, which I'd guess could be a common "reason" for divergence between sys_*() and kern_*().

From another, simpler point of view, if kern_kldload() doesn't do the translation itself, you're likely to have to duplicate it in callers. So why not just factorize it in kern_kldload()? Or do you have specific uses of EUNSDEP in mind for the existing callers? Or do you plan to add new callers that would benefit from this information soon?

I'd like to fix vfs_byname_kld() and ngc_send() in later commits. Once done the leakage should not happen. But if you insist to revert, kern_kldload() can be punished to do the translation.

If you don't have specific use cases in mind for EUNSDEP, see my questions just above, I think this is unnecessary. In any case, please first make kern_kldload() do the translation for this review, and if need be then build another review on top that moves the translation, this is much more logical to follow.

An alternate way out would be to just suppress any translation and publicly document EUNSDEP, if deemed interesting to userspace (I personally think getting that error instead of ENOEXEC is useful information). I don't think our API compatibility policy necessarily precludes adding new return codes to system calls, especially those that are not specified by POSIX. I'd personally find that acceptable. Then, we would probably have to double-check that with some old timers/authorities.

[zlei]

In D44552#1017135, @olce wrote:

In D44552#1017019, @zlei wrote:

Rechecked this, CURRENT and stable/14 vfs_byname_kld() has translated error to ENODEV. As for stable/13 it will be leaked.

Ah, I had forgotten the added translation to ENODEV in vfs_byname_kld(), but not in stable/13 indeed.

I would conclude, an error in domain A has the context, so when it goes cross to another domain B the context get lost, then probably it should be translated (to a proper error for domain B).

That seems a good principle in general but then there's the question of where the "crossing" happens. kern_*() interfaces were initially meant to be called from syscall glue code (adaptation for alternate syscall tables, e.g., 32-bit on 64-bit or Linux), which in particular performs copyin/copyout, and my current view is that they can also be used to make some subsystem functionality available to other subsystem with a kind of relatively formal interface. So, kern_*() functions should control relatively strictly their return codes and not, e.g., let new ones leak to callers, which also implies new return codes should land in the appropriate manual pages. In other words, this means that kern_*() and sys_*() should generally be treated the same, unless you have good reasons, and return the same codes (barring additional ones the syscall glue code can return). And, concerning the "crossing" part, if kern_*() is considered a kind of public interface to the rest of the kernel, crossing happens in fact in there and must be handled by it, and not the caller. Now, it is true that KPI restrictions are less stringent than syscall API restrictions, which I'd guess could be a common "reason" for divergence between sys_*() and kern_*().

kern_*() interfaces were initially meant to be called from syscall glue code ..

Emm, I was not aware of that. Well I think that depends on what is the contract and how stable will KBI/KPI be.

From https://docs.freebsd.org/en/articles/freebsd-releng/#releng-terms-kbi-freeze

KBI/KPI stability implies that the caller of a function across two different releases of software that implement the function results in the same end state. The caller, whether it is a process, thread, or function, expects the function to operate in a certain way, otherwise the KBI/KPI stability on the branch is broken.

For the int kern_kldload(), a new error code seems break KPI stability, let's see how will caller expect:

1. 0, success
2. EEXIST, linker file has already been loaded
3. other errors, for whatever reason the linker fails to load the file, different error code indicates different reason, but the caller should not assume functions relies on the module will be functional

So the new error ENODEV would fall into the other errors catalogue, and the impact should be minimal. Anyway that depends on how caller act on new error code.

From another, simpler point of view, if kern_kldload() doesn't do the translation itself, you're likely to have to duplicate it in callers. So why not just factorize it in kern_kldload()? Or do you have specific uses of EUNSDEP in mind for the existing callers? Or do you plan to add new callers that would benefit from this information soon?

No, currently no plan. I think this is a little over design that we do not hide information (that might be useful) from callee. Simply put, caller is smart and callee is (and should be) noodle.

I'd like to fix vfs_byname_kld() and ngc_send() in later commits. Once done the leakage should not happen. But if you insist to revert, kern_kldload() can be punished to do the translation.

If you don't have specific use cases in mind for EUNSDEP, see my questions just above, I think this is unnecessary. In any case, please first make kern_kldload() do the translation for this review, and if need be then build another review on top that moves the translation, this is much more logical to follow.

An alternate way out would be to just suppress any translation and publicly document EUNSDEP, if deemed interesting to userspace (I personally think getting that error instead of ENOEXEC is useful information). I don't think our API compatibility policy necessarily precludes adding new return codes to system calls, especially those that are not specified by POSIX. I'd personally find that acceptable. Then, we would probably have to double-check that with some old timers/authorities.

Agreed.

[olce]

Responding to your comment in D44581 here:

In D44581#1018346, @zlei wrote:

The initial version of EUNSDEP is defined as (ELAST + 1) in sys/kern/kern_linker.c. I have to admit it is more a hack but I did not find a good candidate from 'sys/sys/error.h`.

To proceed, I'm proposing:

1. Re-use error EXDEV for unsatisfied dependency, thus omit unneeded translations, but it should be documented in kldload(2). This is preferred.
2. Re-use error EXDEV, but translate it to ENOEXEC. This is less ideal but prevent KPI breakage.
3. Define EUNSDEP in sys/kern/kern_linker.c, document it is only used internally and should not be leaked (even to other subsystem). Obviously translation to ENOEXEC is needed. Not preferred.

Following my last message in D44581, I would just rule out the first two, and in fact also replace the third with an even more radical option: Just forego EUNSDEP entirely. With the first two options ruled out, the only remaining purpose of this change is its initial one, that is to say, to print error messages allowing to distinguish between loading errors (including required modules' load failures), which is what the new switch(error) block containing the printf's already accomplishes. This code (without EUNSDEP) is enough to output the chain of dependencies in the kernel log. Could you please also split it, with only a single printf mentioning the dependency error remaining (the only important point here is to print both the names of the top-level module to load and that of the dependency that failed), and move the rest to linker_load_module(), for reasons explained in an earlier comment (better reporting for in-kernel loads and top-level modules)?

As for

with a signature allowing to pass back a text describing the error.

I think that is perfect, but given linker_load_module() can be called recursively, the signature should be designed carefully. IMO a failure from kernel linkers should be rare thus one of the above simpler fixes should be adequate. It is not perfect but user can still get the real reason from dmesg or /var/log/message.

I would just give up on that in that revision, this would be a separate change anyway, way more useful to users than just returning EUNSDEP, but it requires some (hopefully short) design discussion. This way, we also stop delaying the current change, which is useful to users on its own, and that I personally would like to see in, e.g., 14.1-RELEASE.

[zlei]

In D44552#1018569, @olce wrote:

Responding to your comment in D44581 here:

In D44581#1018346, @zlei wrote:

The initial version of EUNSDEP is defined as (ELAST + 1) in sys/kern/kern_linker.c. I have to admit it is more a hack but I did not find a good candidate from 'sys/sys/error.h`.

To proceed, I'm proposing:

1. Re-use error EXDEV for unsatisfied dependency, thus omit unneeded translations, but it should be documented in kldload(2). This is preferred.
2. Re-use error EXDEV, but translate it to ENOEXEC. This is less ideal but prevent KPI breakage.
3. Define EUNSDEP in sys/kern/kern_linker.c, document it is only used internally and should not be leaked (even to other subsystem). Obviously translation to ENOEXEC is needed. Not preferred.

Following my last message in D44581, I would just rule out the first two, and in fact also replace the third with an even more radical option: Just forego EUNSDEP entirely. With the first two options ruled out, the only remaining purpose of this change is its initial one, that is to say, to print error messages allowing to distinguish between loading errors (including required modules' load failures), which is what the new switch(error) block containing the printf's already accomplishes. This code (without EUNSDEP) is enough to output the chain of dependencies in the kernel log. Could you please also split it, with only a single printf mentioning the dependency error remaining (the only important point here is to print both the names of the top-level module to load and that of the dependency that failed), and move the rest to linker_load_module(), for reasons explained in an earlier comment (better reporting for in-kernel loads and top-level modules)?

I think I see what you meant. I'll try that.

As for

with a signature allowing to pass back a text describing the error.

I think that is perfect, but given linker_load_module() can be called recursively, the signature should be designed carefully. IMO a failure from kernel linkers should be rare thus one of the above simpler fixes should be adequate. It is not perfect but user can still get the real reason from dmesg or /var/log/message.

I would just give up on that in that revision, this would be a separate change anyway, way more useful to users than just returning EUNSDEP, but it requires some (hopefully short) design discussion. This way, we also stop delaying the current change, which is useful to users on its own, and that I personally would like to see in, e.g., 14.1-RELEASE.

As propagation error from other subsystem is a common issue. Actually languages such as C++ / Java do that well. Those languages have type exception and exceptions can be wrapped up ( nested exceptions ). I think C can mimic but that is not elegance IMO.
+1 for 14.1-RELEASE .