Introduce a new kernel-only errno EUNSDEP for unsatisfied dependency

[zlei]

Summary

The first consumers of this errno would be kernel linkers. While
initially, this errno was not intended to be leaked, but since the
function int linker_load_dependencies() is public and called by
implementations of LINKER_LOAD_FILE, it actually does. olce@ also
notes that it might be useful in other contexts, so let's make this
new errno public.

MFC after: 2 weeks

[zlei]

Hi @imp , does this need broader attention ?

[imp]

In D44581#1016533, @zlei wrote:

Hi @imp , does this need broader attention ?

No. I don't think so.

[glebius]

Is there any good reasons to keep in kernel only rather than making it a BSD extension, placing it a few lines above in the errno.h right under #ifndef _POSIX_SOURCE?It is a good error for kldload(1) to return to a user, as long as strerror() supports it.

[olce]

In D44581#1017637, @glebius wrote:

Is there any good reasons to keep in kernel only rather than making it a BSD extension, placing it a few lines above in the errno.h right under #ifndef _POSIX_SOURCE?It is a good error for kldload(1) to return to a user, as long as strerror() supports it.

I completely agree. See also D44552. I would just like to be sure that nobody objects to having kldload(2) return a new, different error than before in the case of a problem with dependencies.

So, whom should we ask? imp, jhb, kib, markj, some other people? Or are you confident yourself that we can proceed?

[glebius]

In D44581#1017639, @olce wrote:

So, whom should we ask? imp, jhb, kib, markj, some other people? Or are you confident yourself that we can proceed?

There is nothing wrong with adding them to the review to ask!

[kib]

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.

More, the hack you propose is for very special and local situation, where error can be propagated by other means, if ever needed.

[glebius]

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.
More, the hack you propose is for very special and local situation, where error can be propagated by other means, if ever needed.

Would you like if it get a positive value and goes under #ifndef _POSIX_SOURCE like other our errnos that are extensions?

[kib]

In D44581#1017679, @glebius wrote:

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.
More, the hack you propose is for very special and local situation, where error can be propagated by other means, if ever needed.

Would you like if it get a positive value and goes under #ifndef _POSIX_SOURCE like other our errnos that are extensions?

This is a no-starter at all. It would increase the sys_errlist[] size which is de-facto ABI break with all the consequeneces.

[olce]

In D44581#1017690, @kib wrote:

This is a no-starter at all. It would increase the sys_errlist[] size which is de-facto ABI break with all the consequeneces.

There are lots of unused slots in sys_errlist[] that, IIUC, you added exactly for the purpose of maintaining ABI compatibility (commit cd49e866fcf9b07d, "Bump sys_errlist size to keep ABI backward-compatible for some time."). So what would cause the ABI breakage?

sys_nerr being a constant, old binaries may be compiled with an older size which, as long as sys_errlist[] is enlarged and not shrunk, is always smaller than the actual one. There's still the problem of linkage, as explained in message "Using sys_errlist from executables is not ABI-stable". I'm not versed in linking matters, but I find it strange that there's no relocation type that can prevent an executable from having to copy the full array in a pre-determined (and fixed sized) memory area. Isn't it possible to just have sys_errlist treated like a simple pointer to the first element of the array?

So, could you please give us more details for our own education?

[kib]

There is a wart in ELF, called copy relocation. When a binary references data object defined in dso, static linker allocates the symbol in the binary and issues the copy relocation, to copy the content of the dso at this symbol to binary. The dso is resolved to reference the binary' object, not its own instance. This was done to emulate the semantic of the .a archives, as everything in ELF.

If the binary object is shorter then the dso object, there are two possibilities. Either the dynamic linker uses the length of the binary object, then the tail from dso is not copied, and then the dso code that relies on the presence of tail, is compromised. Or the dynamic linker uses the object size from dso, causing random memory corruption by overwriting some adjusted objects in the binary.

[olce]

In D44581#1017709, @kib wrote:

There is a wart in ELF, called copy relocation. When a binary references data object defined in dso, static linker allocates the symbol in the binary and issues the copy relocation, to copy the content of the dso at this symbol to binary. The dso is resolved to reference the binary' object, not its own instance. This was done to emulate the semantic of the .a archives, as everything in ELF.

I see. I've searched a bit and stumbled on https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected, interesting reading in complement (even if I'm not understanding all of it).

IIUC, old executables using copy relocation cannot be fixed. What we could attempt, however, but I don't know if/how it's possible, is to "tag" sys_errlist[] in some way so that executables using it are forced to perform text relocation (in the above article's parlance) instead of copy relocation. There are other, less experimental possibilities, such as symbol versioning (could be coupled with the previous idea), or even just suppressing the public declaration of sys_errlist[] so that future executables don't use it (they still could provide their own declaration of course, but then it's their own problem).

If the binary object is shorter then the dso object, there are two possibilities. Either the dynamic linker uses the length of the binary object, then the tail from dso is not copied, and then the dso code that relies on the presence of tail, is compromised. Or the dynamic linker uses the object size from dso, causing random memory corruption by overwriting some adjusted objects in the binary.

In the case of sys_errlist[], given the linker uses the first strategy (use the length of the binary object as recorded in the executable), the problem of in-DSO access was fixed by commit rS255108, IIUC.

We could also have two separate objects, and change only the internal one (I would personally be reluctant to do that).

Anyway, there are reserved slots in the existing sys_errlist[] that we could use.

So it seems to me that there are ways to add EUNSDEP, and that the real contention point is rather whether we should make it happen in this case, which you object to.

The main point of D44552 is to add kernel logging for these cases. I think it could be mildly useful to also have kldload(8) distinguish the cases where ENOEXEC is returned, indicating e.g., some corruption of the module file itself, from EUNSDEP (but I agree that EUNSDEP would be returned if a dependent module itself is corrupted), and print different error messages. What is a bit unfortunate currently is that, when you get an error from kldload(8), you have to look at dmesg/syslog to learn the real reason. But maybe EUNSDEP as is stands is not a reason enough to change public return codes, since in reality it doesn't convey much more information on its own. A more elaborate mechanism to report a list of problematic modules would be ideal, but that's probably out-of-scope for D44552.

[kib]

Adding anything to errno is the wrong direction. If you want to get arbitrary error back from kldload(2), then perhaps kldload2(2) is due, with a signature allowing to pass back a text describing the error.
This would be similar to the interface of rtld itself, where dl*() functions do not try to extend errno but provide dlerror() with the description of the problem.

[zlei]

In D44581#1017690, @kib wrote:

In D44581#1017679, @glebius wrote:

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.
More, the hack you propose is for very special and local situation, where error can be propagated by other means, if ever needed.

Would you like if it get a positive value and goes under #ifndef _POSIX_SOURCE like other our errnos that are extensions?

This is a no-starter at all. It would increase the sys_errlist[] size which is de-facto ABI break with all the consequeneces.

The sys_errlist[] has been marked deprecated since BSD 4.4 Lite (1994). I'm curious for what reason this deprecated feature is still supported ?

[zlei]

In D44581#1017956, @kib wrote:

Adding anything to errno is the wrong direction. If you want to get arbitrary error back from kldload(2), then perhaps kldload2(2) is due, with a signature allowing to pass back a text describing the error.
This would be similar to the interface of rtld itself, where dl*() functions do not try to extend errno but provide dlerror() with the description of the problem.

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.

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.

What do you think ?

[olce]

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.

If "Errno" above means the standard macro errno, of course, but then ERESTART and EJUSTRETURN are also just hacks: They never reach userland (i.e., are never placed in errno).

On the contrary, if "errno" means the header containing (among others) the error values, then it is an appropriate place to put any error value, unless perhaps it is confined to a small portion of the system and code is added to test that it's easy to be absolutely confident that it doesn't escape. This is already the case for the other negative values above, which are protected by defined(_KERNEL) and other guards in the header. I'd even argue that, as soon the code portion using a new value is not tiny, it *should* be declared in errno.h (under the proper guards) and assigned a distinct negative value, so that it is both immediately visible that an internal value leaked *and* to ease diagnosing the leak's provenance.

Thinking further, there's the kernel ABI, where (some) negative values of errno could be used by external kernel modules, and kernel internal mechanisms happening to share the same (internal) error value which mustn't be part of the kernel ABI. Is that what you are referring to when singling out ERESTART and EJUSTRETURN?

In D44581#1017786, @olce wrote:

The main point of D44552 is to add kernel logging for these cases. I think it could be mildly useful to also have kldload(8) distinguish the cases where ENOEXEC is returned, indicating e.g., some corruption of the module file itself, from EUNSDEP (but I agree that EUNSDEP would be returned if a dependent module itself is corrupted), and print different error messages. What is a bit unfortunate currently is that, when you get an error from kldload(8), you have to look at dmesg/syslog to learn the real reason. But maybe EUNSDEP as is stands is not a reason enough to change public return codes, since in reality it doesn't convey much more information on its own. A more elaborate mechanism to report a list of problematic modules would be ideal, but that's probably out-of-scope for D44552.

A small note on this I wrote: Before writing it, I proposed officializing the "new" error code in D44552 as another way out of its current state (propagating some internal error to public APIs), since I thought for a while that distinguishing it could be useful, in the process ending the concern of where exactly to perform the error code translation. But I then mostly changed my mind on its usefulness, hence that quoted paragraph.

In D44581#1017956, @kib wrote:

Adding anything to errno is the wrong direction. If you want to get arbitrary error back from kldload(2), then perhaps kldload2(2) is due, with a signature allowing to pass back a text describing the error.
This would be similar to the interface of rtld itself, where dl*() functions do not try to extend errno but provide dlerror() with the description of the problem.

I tend to agree with that. Glancing at D44633, it doesn't seem that it has a special need for an additional return value, but I'll let @glebius handle that.

Going to return to D44552.

[olce]

@zlei: Responsed to your comment above in D44552. Thanks.

[kib]

In D44581#1018549, @olce wrote:

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.

If "Errno" above means the standard macro errno, of course, but then ERESTART and EJUSTRETURN are also just hacks: They never reach userland (i.e., are never placed in errno).

They are hacks, but not in the same sense as the proposed value. ERESTART and EJUSTRETURN directly affect the content of the user context restored by the syscall return to userspace, same as any other userspace errno values, unlike the EUNSDEP (and unlike ENOIOCTL/EDIRIOCTL/ERELOOKUP).