fix(dkms): run ./configure on the correct kernel headers - #1787
Open
kaysond wants to merge 1 commit into
Open
Conversation
robertbaldyga
requested changes
Aug 4, 2026
Comment on lines
+233
to
+236
| # Records which kernel the generated header was probed against, so that a build | ||
| # aimed at a different one fails with a clear message instead of a wall of | ||
| # compiler errors - see modules/Makefile. | ||
| KERNEL_STAMP=$SCRIPTPATH/modules/.configured_kernel |
Member
There was a problem hiding this comment.
The problem with having a separate file for this is apparent a few lines below: when using an existing config file, we cannot perform the check. The fix is very simple - include the kernel path in the config.out instead of introducing a separate file.
It's also worth adding it as a define or comment to generated_defines.h, so that make does not need to know the path to the config (in case it was not config.out).
Comment on lines
36
to
+102
| @@ -46,13 +49,29 @@ else | |||
| sync distsync: | |||
| endif | |||
|
|
|||
| default: sync | |||
| default: sync check_configured_kernel | |||
| cd $(KERNEL_DIR) && $(MAKE) M=$(PWD) modules | |||
|
|
|||
| # generated_defines.h describes the kernel API ./configure probed. Building it | |||
| # against a different kernel silently produces code for the wrong API, which at | |||
| # best fails to compile - so refuse up front and say why. | |||
| check_configured_kernel: | |||
| @stamp=$(PWD)/$(KERNEL_STAMP); \ | |||
| [ -f "$$stamp" ] || exit 0; \ | |||
| configured=$$(cat "$$stamp"); \ | |||
| building=$$(readlink -f $(KERNEL_DIR) 2>/dev/null); \ | |||
| [ -n "$$building" ] || building=$(KERNEL_DIR); \ | |||
| [ "$$configured" = "$$building" ] || { \ | |||
| echo "ERROR: ./configure was run against $$configured, but the modules" >&2; \ | |||
| echo "are being built against $$building." >&2; \ | |||
| echo "Re-run './configure --kernel-dir $$building' first." >&2; \ | |||
| exit 1; \ | |||
| } | |||
|
|
|||
| clean: | |||
| cd $(KERNEL_DIR) && make M=$(PWD) clean | |||
| distclean: clean distsync | |||
| @rm -f $(PWD)/generated_defines.h | |||
| @rm -f $(PWD)/generated_defines.h $(PWD)/$(KERNEL_STAMP) | |||
|
|
|||
| install: install_files | |||
| @$(DEPMOD) | |||
| @@ -80,6 +99,6 @@ uninstall: | |||
|
|
|||
| reinstall: uninstall install | |||
|
|
|||
| .PHONY: all default clean distclean sync distsync install uninstall | |||
| .PHONY: all default check_configured_kernel clean distclean sync distsync install uninstall | |||
Member
There was a problem hiding this comment.
This is a nice safety mechnism. Not related to the DKMS fix directly though, so I'd suggest moving it to a separate commit.
| KERNEL_DIR="$OPT_KERNEL_DIR" | ||
| elif [ -n "$OPT_KERNEL_VERSION" ]; then | ||
| KERNEL_DIR="/lib/modules/$OPT_KERNEL_VERSION/build" | ||
| elif [ -z "$KERNEL_DIR" ] && [ -n "$KERNEL_VERSION" ]; then |
Member
There was a problem hiding this comment.
Having multiple ways to set KERNEL_DIR and KERNEL_VERSION is asking for problems. We can simply do else here and hard overwrite the variables to avoid confusion.
Member
There was a problem hiding this comment.
When dropping setting the KERNEL_DIR by env variable, we also need to update it in RPM building path (which is a single line change).
Comment on lines
+10
to
+90
| usage() { | ||
| cat <<-EOF | ||
| Usage: $0 [options] [config-file] | ||
|
|
||
| Probes the kernel the modules will be built against and generates | ||
| modules/generated_defines.h accordingly. | ||
|
|
||
| Options: | ||
| --kernel-dir DIR use the kernel headers in DIR | ||
| (default: \$KERNEL_DIR, else the running kernel's) | ||
| --kernel-version VER use the kernel headers of VER, i.e. | ||
| /lib/modules/VER/build; --kernel-dir takes precedence | ||
| -h, --help print this message | ||
|
|
||
| If config-file is given, the kernel is not probed at all and the | ||
| configuration stored in that file is applied as is. | ||
|
|
||
| Whatever kernel is selected here must be the same one 'make' is later | ||
| pointed at, otherwise the generated header describes a different kernel | ||
| API than the one the modules are compiled against. | ||
| EOF | ||
| } | ||
|
|
||
| # $1 - name of the option, $2 - number of arguments left after it | ||
| require_arg() { | ||
| if [ "$2" -lt 1 ]; then | ||
| echo >&2 "Error: '$1' requires an argument" | ||
| usage >&2 | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| POSITIONAL=() | ||
| while [ $# -gt 0 ]; do | ||
| case "$1" in | ||
| --kernel-dir) | ||
| require_arg "$1" $(($# - 1)) | ||
| OPT_KERNEL_DIR="$2" | ||
| shift 2 ;; | ||
| --kernel-dir=*) | ||
| OPT_KERNEL_DIR="${1#*=}" | ||
| shift ;; | ||
| --kernel-version) | ||
| require_arg "$1" $(($# - 1)) | ||
| OPT_KERNEL_VERSION="$2" | ||
| shift 2 ;; | ||
| --kernel-version=*) | ||
| OPT_KERNEL_VERSION="${1#*=}" | ||
| shift ;; | ||
| -h|--help) | ||
| usage | ||
| exit 0 ;; | ||
| --) | ||
| shift | ||
| POSITIONAL+=("$@") | ||
| break ;; | ||
| -*) | ||
| echo >&2 "Error: unknown option '$1'" | ||
| usage >&2 | ||
| exit 1 ;; | ||
| *) | ||
| POSITIONAL+=("$1") | ||
| shift ;; | ||
| esac | ||
| done | ||
| set -- "${POSITIONAL[@]}" | ||
|
|
||
| # Pick the kernel to configure for: the options win over the environment, and | ||
| # a directory wins over a version. With nothing given at all, fall back to the | ||
| # running kernel - the same default configure.d/conf_framework.sh and | ||
| # modules/Makefile use. | ||
| if [ -n "$OPT_KERNEL_DIR" ]; then | ||
| KERNEL_DIR="$OPT_KERNEL_DIR" | ||
| elif [ -n "$OPT_KERNEL_VERSION" ]; then | ||
| KERNEL_DIR="/lib/modules/$OPT_KERNEL_VERSION/build" | ||
| elif [ -z "$KERNEL_DIR" ] && [ -n "$KERNEL_VERSION" ]; then | ||
| KERNEL_DIR="/lib/modules/$KERNEL_VERSION/build" | ||
| fi | ||
| KERNEL_DIR="${KERNEL_DIR:-/lib/modules/$(uname -r)/build}" | ||
| export KERNEL_DIR | ||
|
|
Member
There was a problem hiding this comment.
This entire ./configure paremeters rework deserves a separate commit.
Comment on lines
+143
to
+145
| if [ ! -f "$KERNEL_CONFIG" ]; then | ||
| KERNEL_CONFIG="$KERNEL_DIR/include/config/auto.conf" | ||
| fi |
Member
There was a problem hiding this comment.
This part should be a yet another separate commit.
Member
|
The overall idea behind those changes is great, but as it goes with LLM generated code, there is a huge imbalance between paranoid defensiveness and corner case blindness :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1772
This was written by Claude Opus, so it's a little aggressive in preventing the issue from occurring, as coding agents tend to be defensive at paranoid levels. I figured I'd leave it all here in case any of it is desirable for the team.
Please feel free to leave feedback then I'll clean up/update the PR manually.