Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Stop handling dkms.conf as a bash/shell script #414

Open
evelikov opened this issue Apr 11, 2024 · 8 comments
Open

Stop handling dkms.conf as a bash/shell script #414

evelikov opened this issue Apr 11, 2024 · 8 comments

Comments

@evelikov
Copy link
Collaborator

Today dkms sources the dkms.conf, effectively treating it as a shell script instead of data/configuration file.

That in itself isn't great since:

  • it can have code flow relying on dkms internals or altering code flow
  • in some cases dkms needs to be run as root
  • dkms.conf recipes are not always thoroughly reviewed by maintainers/users

Combining the above, we end up with a substantial surface for misuse/abuse and co.

One rather benign example is #413 where dos<>unix line endings result in fatal breakage.

@xuzhen has already done a ton of great work here - see #265.

IMHO to unwrap/finish this completely we should:

  • standardize dkms.conf - flesh out a separate man page, version(?) and add some unit tests
  • select a set of linux distributions and briefly audit for dkms.conf shell abuse - off the top of my head zfs and Kernel match for BUILT_MODULE_NAME #392
    • evaluate if there's an alternative - reach out to respective parties
  • detect and warn when shell abuse is detected in dkms.conf
  • define transitionary period - reach out to affected distros/projects
  • once period is over, release dkms 4 making it a hard error
    • aside: should probably check and promote other warnings to fatal errors

@xuzhen @scaronni @anbe42 what do you think of this idea? If you know of any projects/packages where this will be a problem, can you share some details - name, distro/upstream URL, etc.

Thanks o/

@scaronni
Copy link
Collaborator

Sorry for the late reply, got busy at work.

It's fine for me but I think it's a bit risky. I believe ZFS will not be the only case where copious amounts of bash scripts end up in the DKMS configuration file. For those cases, we would need to constantly extend to meet their demand where it makes sense, or they will keep on rolling out their own DKMS along with the rest, like AMD was doing.

@evelikov
Copy link
Collaborator Author

evelikov commented May 8, 2024

No need to apologise - IRL get in the way.

I think it's beneficial to first get some rough numbers and if the number are favourable work from there. I've done a list for Arch below - @scaronni if you can do one (as time permits) for distros you care about that'll be appreciated.

Let me start with Arch proper, only. The AUR (user repos) have 400 of varying quality.

  • acpi_call - affected, fixable (MAKE[0]="make KVER=$kernelver)
  • bbswitch - similar to acpi_call
  • broadcom-wl - not affected , ships with dozen+ patches
  • deepin-anything - affected, fixable (MAKE[0]="make -C ${kernel_source_dir} KBUILD_EXTMOD=${dkms_tree}/${PACKAGE_NAME}/${PACKAGE_VERSION}/build modules" + ditto for clean)
  • ndiswrapper - affected, dead upstream 🤷
  • nvidia - affected, maybe fixable? (MAKE[0]="'make' -jnproc IGNORE_PREEMPT_RT_PRESENCE=1 NV_EXCLUDE_BUILD_MODULES='__EXCLUDE_MODULES' KERNEL_UNAME=${kernelver} module s")
  • nvidia-open - similar to nvidia above
  • openrazer - affected, fixable (MAKE="KERNELDIR=/lib/modules/${kernelver}/build make driver")
  • sysdig - not affected
  • v4l2loopback - affected, fixable. (sources .config, adds BUILD_EXCLUSIVE_KERNEL="REQUIRES CONFIG_VIDEO_XXX" based on based on { echo "$kernelver"; echo 5.18; } | sort -V -C;) Similar to the linked Intel/uvc?
  • vhba-module - not affected
  • virtualbox-host - not affected

From the above - there are two maybes (nvidia & nvidia-open) and the rest are easily fixable. Will chip through as time permits.

Aside: some packages seem somewhat borked (acpi-call, bbswitch, etc) aka PACKAGE_VERSION="#MODULE_VERSION#". Not sure if an Arch issue or upstream - will check as well.

@anbe42
Copy link
Contributor

anbe42 commented May 8, 2024

Aside: some packages seem somewhat borked (acpi-call, bbswitch, etc) aka PACKAGE_VERSION="#MODULE_VERSION#". Not sure if an Arch issue or upstream - will check as well.

That sounds like a Debian thing ;-) That placeholder is substituted by dh_dkms at package build time while generating/installing dkms.conf into the package.

@anbe42
Copy link
Contributor

anbe42 commented May 8, 2024

  • define transitionary period

my gut feeling says that must be rather two than one major release of all major distributions, the second phase with more annoying deprecation warnings

I expect that there are many third-party out-of-tree modules that are not part of any Linux distribution and that need (minor) adjustments for this proposed change ...

@evelikov
Copy link
Collaborator Author

evelikov commented Jun 4, 2024

my gut feeling says that must be rather two than one major release of all major distributions, the second phase with more annoying deprecation warnings

Not sure I follow... Are you saying that dkms transition should be defined by Linux distribution releases?
If so, I don't fully agree - each one has their own schedule which is outside of our control.

Although before we even remotely get there, let's try and address the known problematic cases. IRL got in the way, let me see if I can open a PR or two.

@scaronni
Copy link
Collaborator

Some out of tree kernel modules also add conditions based on which kernel you are building against, for example: https://github.com/intel/ipu6-drivers/blob/master/dkms.conf

This is very handy as you can have the same source tree usable for multiple kernel versions.

This is going to change further now for IPU6 drivers as in kernel 6.10 some more modules have been merged, and some even with differences (so the out of tree kernel module still takes precedence). I don't think it's a good idea to consider the dkms.conf configuration file as a static configuration at this point.

@adrelanos
Copy link
Contributor

Here's a snippet which has been invented by me:
/etc/dkms/framework.conf.d/30_security-misc.conf

ENOUGH_RAM="1950"
total_ram="$(free -m | sed  -n -e '/^Mem:/s/^[^0-9]*\([0-9]*\) .*/\1/p')"
if [ "$total_ram" -ge "$ENOUGH_RAM" ]; then
   true "INFO: Enough RAM available. Not lowering compilation cores."
else
   true "INFO: Not enough RAM available. Lowering compilation cores to 1."
   parallel_jobs=1
fi

Purpose: Avoid freezing a VM by too many parallel compilation jobs.

@evelikov
Copy link
Collaborator Author

This script reminds me of https://imgs.xkcd.com/comics/workflow.png and somewhat illustrates why I think this task is worth while.

In particular, the hardcoded enough_ram checked against the total memory. The memory required depends on the job itself - compare nvidia vs bbswitch for example. Where the build can OOM long even with a fraction of the total memory being used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants