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

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.

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.

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.

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.

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.

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 .