Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

get module_compressed_suffix from .config #402

Closed

Conversation

masonlherndon
Copy link

@masonlherndon masonlherndon commented Feb 10, 2024

The old (current) method of parsing modules.dep fails to detect compression if none of the currently installed modules are compressed, causing newly built modules to also be uncompressed, even if the kernel supports compression.

Based on 07de929, which already added this new behavior to run_test.sh.

Script demonstrating the issue:

#!/bin/bash

echo //// Compression settings from /lib/modules/$(uname -r)/build/.config ////
grep -e "CONFIG_MODULE_COMPRESS" /lib/modules/$(uname -r)/build/.config
echo

install_tree=/lib/modules

#### Snippet taken from dkms.in:234 ####
# Figure out the correct module suffix for the kernel we are currently
# dealing with, which may or may not be the currently installed kernel.
set_module_suffix()
{
    # $1 = the kernel to base the module_suffix on
    kernel_test="${1:-$(uname -r)}"
    module_uncompressed_suffix=".ko"
    grep -q '\.gz:' $install_tree/$kernel_test/modules.dep 2>/dev/null && module_compressed_suffix=".gz"
    grep -q '\.xz:' $install_tree/$kernel_test/modules.dep 2>/dev/null && module_compressed_suffix=".xz"
    grep -q '\.zst:' $install_tree/$kernel_test/modules.dep 2>/dev/null && module_compressed_suffix=".zst"
    module_suffix="$module_uncompressed_suffix$module_compressed_suffix"
}

echo //// Full module file extension determined by the current method ////
set_module_suffix $(uname -r)
echo $module_suffix
echo

KERNEL_VER=$(uname -r)

#### Snippet taken from run_test.sh:230 ####
mod_compression_ext=
kernel_config="/lib/modules/${KERNEL_VER}/build/.config"
if [ -f "${kernel_config}" ]; then
    if grep -q "^CONFIG_MODULE_COMPRESS_NONE=y" "${kernel_config}" ; then
        mod_compression_ext=
    elif grep -q "^CONFIG_MODULE_COMPRESS_GZIP=y" "${kernel_config}" ; then
        mod_compression_ext=.gz
    elif grep -q "^CONFIG_MODULE_COMPRESS_XZ=y" "${kernel_config}" ; then
        mod_compression_ext=.xz
    elif grep -q "^CONFIG_MODULE_COMPRESS_ZSTD=y" "${kernel_config}" ; then
        mod_compression_ext=.zst
    fi
fi

module_uncompressed_suffix=".ko"

echo //// Full module file extension expected by the currently failing test ////
echo "$module_uncompressed_suffix$mod_compression_ext"

Output on my machine:

user@host:~$ ./example.sh
//// Compression settings from /lib/modules/6.5.0-17-generic/build/.config ////
# CONFIG_MODULE_COMPRESS_NONE is not set
# CONFIG_MODULE_COMPRESS_GZIP is not set
# CONFIG_MODULE_COMPRESS_XZ is not set
CONFIG_MODULE_COMPRESS_ZSTD=y

//// Full module file extension determined by the current method ////
.ko

//// Full module file extension expected by the currently failing test ////
.ko.zst

@masonlherndon
Copy link
Author

masonlherndon commented Feb 10, 2024

I only tested by building/installing/removing a driver and verifying that compression was properly detected and applied to the module.

I'm also not 1000% sure that the .config file is guaranteed to exist during every use-case where set_module_suffix() is called, which is maybe why we're reading modules.dep in the first place?? Anyways, I'm very open to suggestions. :)

The old method of parsing modules.dep fails to detect compression if
none of the currently installed modules are compressed, causing newly
built modules to also be uncompressed, even if the kernel supports
compression.

Based on 07de929, which already added
this new behavior to run_test.sh.
@masonlherndon masonlherndon force-pushed the detect-module-compression branch from 159f0b8 to 37515ed Compare February 10, 2024 05:55
@evelikov
Copy link
Collaborator

Generally I'm in favour. More so, we should probably remove this in the long run - see #319 for more.

I'm inclined to merge this, just a couple of questions. In you test/example above:

  • what distro are you using?
  • was the linux package installed?
  • does the linux package include "modules.dep", if not - what's the call chain/trigger for depmod?

And to answer your question:

I'm also not 1000% sure that the .config file is guaranteed to exist during every use-case where set_module_suffix() is called, which is maybe why we're reading modules.dep in the first place??

I think that in the early days it was not guaranteed - with distro packaging varying greatly. These days, it's a hard-requirement for dkms and all the distros in our CI have it.

@evelikov
Copy link
Collaborator

On second thought, I don't think we can do this yet.

Namely:
Some distributions deliberately have a miss-match between the config and actual files shipped. See the run_tests.sh snippet for example

dkms/run_test.sh

Lines 246 to 266 in c1c2986

case "${os_id}" in
centos | fedora | rhel | ovm | almalinux)
expected_dest_loc=extra
mod_compression_ext=.xz
;;
sles | suse | opensuse*)
expected_dest_loc=updates
mod_compression_ext=.zst
;;
arch)
expected_dest_loc=updates/dkms
;;
debian* | ubuntu | linuxmint)
expected_dest_loc=updates/dkms
;;
alpine)
expected_dest_loc=kernel/extra
;;
gentoo)
expected_dest_loc=kernel/extra
mod_compression_ext=

So as-is this will cause a regression and there won't be an easy way for user/distro to reinstate the original behaviour. There's some ideas to have this as config toggle - see #328.

If you'd like to add a config for compression override type, then we can have this on top.

@masonlherndon
Copy link
Author

Generally I'm in favour. More so, we should probably remove this in the long run - see #319 for more.

I'm inclined to merge this, just a couple of questions. In you test/example above:

* what distro are you using?

* was the linux package installed?

* does the linux package include "modules.dep", if not - what's the call chain/trigger for `depmod`?

And to answer your question:

I'm also not 1000% sure that the .config file is guaranteed to exist during every use-case where set_module_suffix() is called, which is maybe why we're reading modules.dep in the first place??

I think that in the early days it was not guaranteed - with distro packaging varying greatly. These days, it's a hard-requirement for dkms and all the distros in our CI have it.

The output I showed was from a Ubuntu 22.04 machine. The relevant installed packages for this kernel version are linux-image-6.5.0-17-generic, linux-headers-6.5.0-17-generic, and linux-modules-6.5.0-17-generic, which all came from the meta-package linux-generic-hwe-22.04. I downloaded the .deb for linux-modules-6.5.0-17-generic to check if it came with a default modules.dep, however it seems to only have modules.builtin and modules.order. I don't know quite what you mean or how to find out what the call chain/trigger for depmod is.

@masonlherndon
Copy link
Author

masonlherndon commented Feb 10, 2024

On second thought, I don't think we can do this yet.

Namely: Some distributions deliberately have a miss-match between the config and actual files shipped. See the run_tests.sh snippet for example

dkms/run_test.sh

Lines 246 to 266 in c1c2986

case "${os_id}" in
centos | fedora | rhel | ovm | almalinux)
expected_dest_loc=extra
mod_compression_ext=.xz
;;
sles | suse | opensuse*)
expected_dest_loc=updates
mod_compression_ext=.zst
;;
arch)
expected_dest_loc=updates/dkms
;;
debian* | ubuntu | linuxmint)
expected_dest_loc=updates/dkms
;;
alpine)
expected_dest_loc=kernel/extra
;;
gentoo)
expected_dest_loc=kernel/extra
mod_compression_ext=

So as-is this will cause a regression and there won't be an easy way for user/distro to reinstate the original behaviour. There's some ideas to have this as config toggle - see #328.

If you'd like to add a config for compression override type, then we can have this on top.

If I'm understanding right, doesn't the use of the compressed_or_uncompressed() function make the .config method of setting module_compressed_suffix a non-issue though? When a distro ships a .ko for a kernel that also supports compression, then compressed_or_uncompressed() will just pick the .ko path since the .ko.<blah> path won't be found. And this exact case happens on my Ubuntu 22.04 machine, where all the distro-supplied modules are .ko's, but the kernel also supports ZSTD compression. I did not test uninstalling/removing .ko's after updating how module_compressed_suffix was set, but if compressed_or_uncompressed() is being used in all the correct places, then I don't think it would make a difference.

@masonlherndon
Copy link
Author

masonlherndon commented Feb 10, 2024

If you'd like to add a config for compression override type, then we can have this on top.

Are you suggesting like a --compression-type CLI option?

@evelikov
Copy link
Collaborator

... to check if it came with a default modules.dep, however it seems to only have modules.builtin and modules.order.

Nice - I believe most distros are doing this aka ship the required RO files.

I don't know quite what you mean or how to find out what the call chain/trigger for depmod is.

All the packager tools out there (that I've seen) have specific methods to trigger post-install hooks. Debian/Ubuntu are using the dkms_common.postinst.in (see Makefile for installed name), which triggers dkms which in itself calls depmod (can disable that via --no-depmod).

@evelikov
Copy link
Collaborator

It's a log story, so you might want to grab a drink:

compressed_or_uncompressed() is a bit of a pet peeve of mine since it creates a lot of guesswork and assumptions, that result in a large permutation once you go across few calls up/down the call stack. The stack is quite convoluted as-is, I'm not sure if we want the extra "fun" that ^^ brings.

In particular:

There are two(?) ways to load modules - kmod or busybox, which vary in a few ways.

  • kmod supports zstd, xz, etc
  • busybox does not support zstd
  • both are very configurable (aka you can build w/o xz support), yet there's no way to see check that at build time
  • newer kmod can pass the module compressed to the kernel
    • it (kernel really) has a funky ordering assumption of which compressions can work
    • does not fallback if module is compressed in a incompatible way
  • busybox does not support ^^

Taking the above into account, the CONFIG toggle indicates (at least) two things:

  • the in-kernel capability for module decompression
  • the compression be used with make modules_install (which we don't use yet but should)

Most if not all the above vary across kernel/kmod/busybox version and do not (yet) have a hard requirement for specific minimal versions.

So what happens when we take everything into consideration is ... unclear.

Abstracting ourselves from all of this - the key premise is that this will change the behaviour. Before we'll install uncompressed modules when compressed ones are expected or vice-versa.

@evelikov
Copy link
Collaborator

Are you suggesting like a --compression-type CLI option?

A little more - a "module_compression" field in dkms/framework.conf and a dkms cli equivalent. Don't forget the docs - manual, help screen and bash/zsh completion.

Ideally we'll also have 1-2 tests in the CI.

@masonlherndon
Copy link
Author

masonlherndon commented Feb 11, 2024

compressed_or_uncompressed() is a bit of a pet peeve of mine since it creates a lot of guesswork and assumptions, that result in a large permutation once you go across few calls up/down the call stack. The stack is quite convoluted as-is, I'm not sure if we want the extra "fun" that ^^ brings.

Yea I can see how promoting/relying more on compressed_or_uncompressed() might be harmful. However I don't know if this PR can be salvaged (as is) without leaning even further into compressed_or_uncompressed(). Implementing a new module_compression field in dkms/framework.conf and CLI does sound like a good idea, but is a little outside the scope of what I was going for with this change.

So, should I close this PR? Leave it open to maybe get merged in the distant future? I don't think I'm gonna have time to implement a new module_compression field anytime soon.

@evelikov
Copy link
Collaborator

Unwrapping/removing compressed_or_uncompressed() is definitely out of scope here.

IMHO module_compression override is a requirement, but if you don't have the time to add it just leave the PR open. We'll get to it eventually.

Thanks for your time bearing with this behemoth :-P

@scaronni
Copy link
Collaborator

I'm sorry but I've merged something different than getting the compression from the .config file. On Fedora/RHEL (& derivatives) the kernel modules are not compressed in the configuration file but rather later in the build process:

$ cat /lib/modules/6.11.10-300.fc41.x86_64/config | grep MODULE_COMPRESS
CONFIG_MODULE_COMPRESS_NONE=y
# CONFIG_MODULE_COMPRESS_GZIP is not set
# CONFIG_MODULE_COMPRESS_XZ is not set
# CONFIG_MODULE_COMPRESS_ZSTD is not set
$ find /lib/modules/6.11.10-300.fc41.x86_64/ -name "*xz" | head -3
/lib/modules/6.11.10-300.fc41.x86_64/kernel/arch/x86/crypto/aegis128-aesni.ko.xz
/lib/modules/6.11.10-300.fc41.x86_64/kernel/arch/x86/crypto/blowfish-x86_64.ko.xz
/lib/modules/6.11.10-300.fc41.x86_64/kernel/arch/x86/crypto/camellia-aesni-avx-x86_64.ko.xz

On top of that, the kernel modules are not part of the kernel package, the base kernel package (kernel-core, which is used for minimal VMs, etc.) does not contain any. So the only way to get the module compression algorithm is through checking what's in the kernel tree.

I've changed the code and added some comment so the next person looking at this does not step into the same issue: https://github.com/dell/dkms/blob/master/dkms.in#L261-L274

Thanks.

@scaronni scaronni closed this Nov 30, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants