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

Use 'unbuild' instead of 'remove' in kernel prerm script #342

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

ian-abbott
Copy link
Contributor

When DKMS is invoked by the kernel_prerm.d_dkms script to remove modules from a kernel that is being removed, it uses the DKMS 'remove' command. If there are no other installations of the module, it will be removed completely from DKMS. That seems to be an undesirable side-effect and predates the introduction of the DKMS 'unbuild' command which merely undoes the 'install' and 'build' steps.

Change the script to use 'unbuild' instead of 'remove'. Keep the echoed message that says "dkms: removing: ..." because "dkms: unbuilding: ..." may be confusing to the user.

I have tested it by removing a kernel with a module installed by DKMS that was the only installed instance of the module. Without the change, the module was removed completely from DKMS. With the change, the module was removed from the kernel, but was not removed completely. On reinstalling the kernel, the module was rebuilt and reinstalled for the kernel by DKMS.

This change was suggested by @RalfGoebel for issue #37 and might be Debian-specific.

Link: #37 (comment)
Tested-by: Ian Abbott [email protected]

@evelikov
Copy link
Collaborator

Thanks for the MR o/

Bth I was fairly close to removing the script with #339 MR. Although decided to defer that.

Since I have your attention, can you share your POV of what the script should do - in the most naive, step-by-step way possible.

I've been gone through the git history, alongside the existing use-cases - although the lack of documentation across the board is fairly shocking.

@ian-abbott
Copy link
Contributor Author

ian-abbott commented Sep 25, 2023

The intention is to tidy up the DKMS state when a kernel is removed.

The Debian linux-header-${KVER} package installation and removal (the stuff required to build external modules) and linux-image-${KVER} package installation and removal (the actual kernel and module binaries) have no hierarchical relationship, but the DKMS build and install, and uninstall and unbuild commands do have a hierarchical relationship, i.e. install implies build and unbuild implies uninstall. The DKMS autoinstall will be invoked whenever either of the linux-header-${KVER} or linux-image-${KVER} packages are installed (although if a DKMS module needs to be built before being installed, that is doomed to fail unless the linux-header-${KVER} package is installed).

If the linux-image-${KVER} package depended on the linux-headers-${KVER} package, then the DKMS uninstall command could be invoked when the linux-image-${KVER} package is being removed, and the DKMS unbuild command could be invoked when the linux-headers-${KVER} package is being removed.

Since the linux-image-${KVER} and linux-headers-${KVER} packages have no hierarchical relationship, we do not want the DKMS unbuild command to be invoked when the linux-headers-${KVER} package is being removed because that would imply a DKMS uninstall, but the installed modules may still be required by a still installed linux-image-${KVER} package.

The compromise is to do nothing when the linux-headers-${KVER} package is being removed, but to unbuild modules when the linux-image-${KVER} package is being removed. If the same linux-image-${KVER} package is installed again later, this compromise is sub-optimal because it requires the autoinstallable DKMS modules to be rebuilt. However, unbuild is a better compromise than what the script currently does, which is to remove modules when the linux-image-${KVER} package is being removed, since that may require the DKMS modules to be manually added to the system again.

@evelikov
Copy link
Collaborator

Err, I was thinking of the wrong hook (dkms_common.postinst), although the under documentation sentiment still stands.

The compromise is to do nothing when the linux-headers-${KVER} package is being removed, but to unbuild modules when the linux-image-${KVER} package is being removed.

This seems reasonable IMHO.

If the same linux-image-${KVER} package is installed again later, this compromise is sub-optimal because it requires the autoinstallable DKMS modules to be rebuilt.

It's a little annoying, but normal IMHO. You need to tie the lifetime of the object to something - the headers package in this case.

However, unbuild is a better compromise than what the script currently does, which is to remove modules when the linux-image-${KVER} package is being removed, since that may require the DKMS modules to be manually added to the system again.

Indeed, reading your description reminded me that this is the exact reason why I introduced "unbuild" in the first place.

If I understood correctly, in a gist we have:

  • respective kernel header and image packages may lack hierarchical relationship
  • dkms actions on the other hand do
  • users may trigger dkms additive actions, via either or both packages - each with it's own caveats
  • dkms removal actions, are usually triggered by only one of the packages - kernel headers
  • using "dkms remove" in the hook, will require manual "dkms add" where "dkms unbuild" removes all meaningful files

Can I ask you to update the patch adding a couple 2-3 inline notes:

  • who/when the hook gets called - as linux-image, linux-headers, etc gets removed
  • why "install"-d modules are omitted

Thanks

@ian-abbott
Copy link
Contributor Author

ian-abbott commented Sep 25, 2023

  • dkms removal actions, are usually triggered by only one of the packages - kernel headers

Actually, the kernel image package.

Can I ask you to update the patch adding a couple 2-3 inline notes:

  • who/when the hook gets called - as linux-image, linux-headers, etc gets removed
  • why "install"-d modules are omitted

I'm not sure about what you meant by 'why "install"-d modules are omitted', but I can write something about why the script needs to "unbuild" modules rather than "uninstall" them.

@ian-abbott ian-abbott force-pushed the do-not-remove-completely-in-prerm branch from 4900ce3 to c7761d4 Compare September 25, 2023 14:15
@evelikov
Copy link
Collaborator

Was referring to the [ "$status" = "installed" ] || continue line just above the dkms unbuild.

Which seemingly I misread 🤦 although it should be documented nevertheless.

@ian-abbott
Copy link
Contributor Author

ian-abbott commented Sep 25, 2023

Was referring to the [ "$status" = "installed" ] || continue line just above the dkms unbuild.

Which seemingly I misread 🤦 although it should be documented nevertheless.

I think the script is playing safe. Most of the files in "/lib/modules/${KVER}" should be under the control of the package manager (at least for kernels installed by the package manager), although some files are updated dynamically by depmod. DKMS will only have created files in "/lib/modules/${KVER}" that are outside the control of the package manager for modules whose status is "installed".

I suppose the script could be changed to also unbuild modules whose status is "built", but I guess that is deemed unnecessary from the point of view of the package manager as all the files in /var/lib/dkms are outside the control of the package manager apart from the directory itself. There was probably some manual intervention that resulted in the module being in the "built" state instead of the "installed" state.

@evelikov
Copy link
Collaborator

although some files are updated dynamically by depmod.

Are you talking about the mutable modules.XXX files? What do they have to do with the dkms status/build/installed check ? Sorry I'm missing the point here.

If the module is not installed, dkms will not run depmod. Not to mention that there's a dkms switch to avoid running depmod all together. In case you're interested I've added a depmod --outdir option, so one can control those files are generated.

DKMS will only have created files in "/lib/modules/${KVER}" that are outside the control of the package manager for modules whose status is "installed".

Correct. A bunch of files in /var/lib/dkms/dkms-module-name/kernel-$uname_r-$uname_m/ will remain, which aren't handled by the package manager.

I don't have an opinion if build or installed is used - it's up-to Debian savvy folks to decide, where I'm not :-\

All I'm asking is a brief inline comment why the current option is chosen. So that everyone else can read it tomorrow, in 6 months from now and reason about things.

Thanks (sorry I've err on the side of verbosity)

@ian-abbott
Copy link
Contributor Author

although some files are updated dynamically by depmod.

Are you talking about the mutable modules.XXX files? What do they have to do with the dkms status/build/installed check ? Sorry I'm missing the point here.

I was just giving examples of some files not under the control of the package manager. Other files not under direct control of the package manager include the modules installed by DKMS.

I don't have an opinion if build or installed is used - it's up-to Debian savvy folks to decide, where I'm not :-\

All I'm asking is a brief inline comment why the current option is chosen. So that everyone else can read it tomorrow, in 6 months from now and reason about things.

My updated commit message in c7761d4 is possibly a bit too verbose, but I tried to explain why I replaced 'remove' with 'unbuild' rather than 'uninstall'. My updated commit message does not attempt to explain why only "installed" DKMS modules are affected by the script. I can attempt to explain that in another updated commit message, but it would just be my best guess.

@ian-abbott ian-abbott force-pushed the do-not-remove-completely-in-prerm branch from c7761d4 to 5a0729d Compare September 25, 2023 18:09
@evelikov
Copy link
Collaborator

I think you misread my earlier request - please add a brief inline note.

@ian-abbott ian-abbott force-pushed the do-not-remove-completely-in-prerm branch from 5a0729d to 08e7d08 Compare October 11, 2023 12:00
@ian-abbott
Copy link
Contributor Author

I think you misread my earlier request - please add a brief inline note.

OK, I added a comment in the actual script.

@ian-abbott ian-abbott force-pushed the do-not-remove-completely-in-prerm branch from 08e7d08 to 21b980c Compare October 11, 2023 12:59
When DKMS is invoked by the kernel_prerm.d_dkms script to remove modules
from a kernel that is being removed, it uses the DKMS 'remove' command.
If there are no other installations of the module, it will be removed
completely from DKMS.  That seems to be an undesirable side-effect and
predates the introduction of the DKMS 'unbuild' command which merely
undoes the 'install' and 'build' steps.

Change the script to use 'unbuild' instead of 'remove'.  Keep the echoed
message that says "dkms: removing: ..." because "dkms: unbuilding: ..."
may be confusing to the user.

I have tested it by removing a kernel with a module installed by DKMS
that was the only installed instance of the module.  Without the change,
the module was removed completely from DKMS.  With the change, the
module was removed from the kernel, but was not removed completely.  On
reinstalling the kernel, the module was rebuilt and reinstalled for the
kernel by DKMS.

This change was suggested by @RalfGoebel for issue dell#37 and might be
Debian-specific.

For Debian and Ubuntu, the 'kernel_postinst.d_dkms' script is installed as
'/etc/kernel/postinst.d/dkms' and '/etc/kernel/header_postinst.d/dkms',
and the 'kernel_prerm.d_dkms' script is installed as
'/etc/kernel/prerm.d/dkms'.  The scripts are invoked in the following
circumstances, with the script parameter $1 set to the kernel version
from the kernel package (referred to as "${KVER}" below):

1. When a "linux-headers-${KVER}" package is installed (containing the
   stuff needed to build external modules), the
   '/etc/kernel/header_postinst.d/dkms' ('kernel_postinst.d_dkms')
   script is invoked to autoinstall DKMS modules for the kernel version,
   building the modules if necessary.

2. When a "linux-image-${KVER}" package is installed (containing the
   Linux kernel binary, and in the case of Debian, also the kernel
   module binaries), the '/etc/kernel/postinst.d/dkms
   ('kernel_postinst.d_dkms') script is invoked to autoinstall DKMS
   modules for the kernel version, building the modules if necessary and
   possible.  (Building the modules will fail unless the
   "linux-headers-${KVER}" package is installed.)

3. When a "linux-headers-${KVER}" package is being removed, none of the
   DKMS scripts are invoked.

4. When a "linux-image-${KVER}" package is being removed, the
   '/etc/kernel/prerm.d/dkms' ('kernel_prerm.d_dkms') script is invoked
   to perform some sort of uninstallation action on all DKMS modules
   that have been built for the kernel version.  Prior to this patch,
   the DKMS 'remove' command was used to uninstall the modules.  This
   patch changes that to the DKMS 'unbuild' command which is less
   destructive.

On first glance, it may seem better to use the DKMS 'uninstall' command
when removing a "linux-image-${KVER}" package, and to use the DKMS
'unbuild' command  when removing a "linux-headers-${KVER}" package.
However, the "linux-image-${KVER}" and "linux-headers-${KVER}" packages
do not depend on each other, so unbuilding the DKMS modules when the
"linux-headers-${KVER}" package is being removed would have the side
effect of uninstalling the DKMS modules for the kernel, even if the
"linux-image-${KVER}" package is still installed.  Therefore, the
compromise is to use the DKMS 'unbuild' command when removing a
"linux-image-${KVER}" package and to do nothing when removing a
"linux-headers-${KVER}" package.  This helps avoid littering the
'/var/lib/dkms' directory with stale builds.  It is sub-optimal if the
same version of the "linux-image-${KVER}" package is to be reinstalled
later because the DKMS modules would need to be built again, but that
should be fairly rare.  Normally, kernel packages are upgraded to a
different version, requiring the DKMS modules to be built anyway.

The script only affects DKMS modules with the "installed" status.  Only
DKMS modules with the "installed" status have files in
"/lib/modules/${KVER}" that need to be cleaned up.  It could be argued
that since the script is using the DKMS 'unbuild' command, it should
also unbuild DKMS modules with the "built" status.  There must have been
some external intervention outside of the kernel installation/removal
hook scripts to leave those modules at the "built" status, so the script
may be playing it safe by leaving those DKMS modules alone.

Link: dell#37 (comment)
Tested-by: Ian Abbott <[email protected]>
Signed-off-by: Ian Abbott <[email protected]>
@ian-abbott ian-abbott force-pushed the do-not-remove-completely-in-prerm branch from 21b980c to 0ddf438 Compare October 11, 2023 13:02
@evelikov evelikov merged commit 334294d into dell:master Oct 11, 2023
21 checks passed
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

Successfully merging this pull request may close these issues.

2 participants