fix(dracut.sh): abort on `instmods -c ...` failure #2202

sshedi · 2023-02-14T16:40:28Z

Currently, instmods exit status is not validated and as a result initrd generation continues even when dracut-install fails.

For instance when add-drivers is called with a non existent driver:

...
dracut: Executing: /usr/bin/dracut -H -f --add-drivers "xen-blkfront xen-acpi-processor xen-evtchn xen-gntalloc xen-gntdev xen-privcmd xen-pciback xenfs hv_utils hv_vmbus hv_storvsc hv_netvsc hv_sock hv_balloon cn dm-mod" /boot/initramfs-6.1.10-1.ph5-secure.img 6.1.10-1.ph5-secure
...
dracut-install: Failed to find module 'xen_blkfront'
dracut: FAILED:  /usr/lib/dracut/dracut-install -o -D /var/tmp/dracut.r2Cc7L/initramfs --kerneldir /lib/modules/6.1.10-1.ph5-secure/ -m xen_blkfront xen-acpi-processor xen-evtchn xen-gntalloc xen-gntdev xen-privcmd xen-pciback xenfs hv_utils hv_vmbus hv_storvsc hv_netvsc hv_sock hv_balloon cn dm-mod
dracut: *** Installing kernel module dependencies ***
dracut: *** Installing kernel module dependencies done ***
dracut: *** Resolving executable dependencies ***
dracut: *** Resolving executable dependencies done ***
dracut: *** Hardlinking files ***
...

Signed-off-by: Shreenidhi Shedi [email protected]

This pull request changes...

Changes

Checklist

I have tested it locally
I have reviewed and updated any documentation if relevant
I am providing new code and test(s) for it

Fixes #

sshedi · 2023-02-15T06:34:01Z

Hi @haraldh , @danimo & @johannbg,

Can you please take these changes?

sshedi · 2023-02-17T10:56:08Z

Hi @haraldh gentle reminder.

aafeijoo-suse

First, please stop spamming reviewers. Usually people have other things to do in life and do not need a ping every 2 days.

dracut: Executing: /usr/bin/dracut -H -f --add-drivers "xen-blkfront ...
...
dracut-install: Failed to find module 'xen_blkfront'
dracut: FAILED:  /usr/lib/dracut/dracut-install -o -D /var/tmp/dracut.r2Cc7L/initramfs --kerneldir /lib/modules/6.1.10-1.ph5-secure/ -m ...

You log does not make sense, the -o option is never printed before -D... and with --add-drivers, dracut-install is not called with -o. What's your distro/dracut version?

dracut.sh

sshedi · 2023-02-17T14:06:38Z

First, please stop spamming reviewers. Usually people have other things to do in life and do not need a ping every 2 days.

Thanks for your advice but I don't think asking for review and inputs is spam, correct me if I'm wrong and I'm unaware of the development process here. How are we supposed reach out to maintainers otherwise? And I tagged the top 3 contributors and asked for their assistance and if they feel that they are getting spammed, my apologies. And everyone have some something to do in their life, and that includes me as well.

Currently, instmods exit status is not validated and as a result initrd generation continues even when dracut-install fails. For instance when add-drivers is called with a non existent driver: ``` dracut: Executing: /usr/bin/dracut -L 5 -H -f --add-drivers "xen-scsifront invalid xen-blkfront xen-acpi-processor xen-evtchn xen-gntalloc xen-gntdev xen-privcmd xen-pciback xenfs hv_utils hv_vmbus hv_storvsc hv_netvsc hv_sock hv_balloon cn dm-mod megaraid_sas" /boot/initrd.img-6.0.7-8.ph5 6.0.7-8.ph5 ... dracut: *** Including modules done *** dracut-install: Failed to find module 'invalid' dracut: FAILED: /usr/lib/dracut/dracut-install -D /var/tmp/dracut.9bhous/initramfs --kerneldir /lib/modules/6.0.7-8.ph5/ -m xen_scsifront invalid xen-blkfront xen-acpi-processor xen-evtchn xen-gntalloc xen-gntdev xen-privcmd xen-pciback xenfs hv_utils hv_vmbus hv_storvsc hv_netvsc hv_sock hv_balloon cn dm-mod megaraid_sas dracut: *** Installing kernel module dependencies *** ... ``` Signed-off-by: Shreenidhi Shedi <[email protected]>

sshedi · 2023-02-17T17:35:00Z

Initially I tested with dracut-048 and so the -o in the parameter list.

This is the log from dracut-059

dracut: Executing: /usr/bin/dracut -L 5 -H -f --add-drivers "xen-scsifront invalid xen-blkfront xen-acpi-processor xen-evtchn xen-gntalloc xen-gntdev xen-privcmd xen-pciback xenfs hv_utils hv_vmbus hv_storvsc hv_netvsc hv_sock hv_balloon cn dm-mod megaraid_sas" /boot/initrd.img-6.0.7-8.ph5 6.0.7-8.ph5
...
dracut: *** Including modules done ***
dracut-install: Failed to find module 'invalid'
dracut: FAILED:  /usr/lib/dracut/dracut-install -D /var/tmp/dracut.9bhous/initramfs --kerneldir /lib/modules/6.0.7-8.ph5/ -m xen_scsifront invalid xen-blkfront xen-acpi-processor xen-evtchn xen-gntalloc xen-gntdev xen-privcmd xen-pciback xenfs hv_utils hv_vmbus hv_storvsc hv_netvsc hv_sock hv_balloon cn dm-mod megaraid_sas
dracut: *** Installing kernel module dependencies ***
...

LaszloGombos · 2023-02-18T01:21:53Z

initrd generation continues even when dracut-install fails

I understand that -c would imply check, but nowhere in dracut is that failure fatal. Not even in the fips module where I would expect this is more important to check.

What made you to believe that this condition should be fatal instead of just a warning ? Perhaps some man page or some other documentation ? The English word "check" does not necessary imply fatal condition.

My concern here is compatibility with earlier dracut version. Bailing out early when e.g. a module name is miss-typed, could lead to initrd no longer generated (when previously it was) and next boot time failed boot.

sshedi · 2023-02-18T06:12:18Z

What made you to believe that this condition should be fatal instead of just a warning ?

Say this is my add_drivers config:

add_drivers+=" driver1 driver2 invalid-entry driver3 driver4 "

I think case, dracut-install add driver1, driver2 to initrd and when it hits invalid-driver, it fails and comes out completely. driver3, driver4 will not get added at all and if in case driver 3 & 4 are a must have in initrd, even though we have an initrd image generated it will be useless but users will think that initrd is generated and is fine.

Yes, what you have told is right too. This might result in an entirely different behavior compared to earlier versions but even with the existing approach there is a chance that dracut generates a faulty initrd which makes system unusable.

-c is enforced in dracut.sh and modules.d/01fips/module-setup.sh but this should be a fatal error IMO.

sshedi · 2023-02-18T06:17:46Z

Or we can use a new configuration parameter which can be set in /etc/dracut.conf.d/ where user can configure whether to consider this error as fatal or not. Something like:

cat /etc/dracut.conf.d/instmods.conf
instmods_check_errs_fatal=0/1

And by default we can keep it 0 to maintain current behavior and it's up to users to set it to 1.

LaszloGombos · 2023-02-18T06:50:25Z

dracut.sh

@@ -1954,19 +1954,28 @@ if [[ $no_kernel != yes ]]; then

    if [[ -n ${add_drivers// /} ]]; then
        # shellcheck disable=SC2086
-        hostonly='' instmods -c $add_drivers
+        if ! hostonly='' instmods -c $add_drivers; then
+            dfatal "instmods failed for add_drivers: $add_drivers"


Perhaps this should be a warning and we should remove the exit to maintain compatibility.

A new config parameter will have the same effect right? And users can modify to their needs.
If the logs are suppressed, users will never be aware that something is going wrong.

OTOH, agree with you on maintaining compatibility but it should be more flexible and configurable.

A new config parameter will have the same effect right?

No, it is not. As it is, this PR increases complexity of the code and reduces test coverage for the code as you did not added a test case.

Does this module has a test case? I can enhance it to cover this case, can you please point me where it's done currently?

I was not suggesting that the way to improve this PR is to add a test case. I simply was kind enough to answer your question.

My opinion is that this should be a warning and the new config parameter should not be part of this PR.

I suggest to give time for other reviewers as well to chime in.

Thanks for your time and patience. Let's wait.

Perhaps this should be a warning and we should remove the exit to maintain compatibility.

instmods() already prints an error message, so adding another warning would be worthless.

IMHO I would not merge this PR as is, as it will break the current behavior.

sshedi requested review from haraldh, danimo and johannbg as code owners February 14, 2023 16:40

aafeijoo-suse reviewed Feb 17, 2023

View reviewed changes

dracut.sh Outdated Show resolved Hide resolved

sshedi force-pushed the instmods-fix branch from 7628b7c to 471b355 Compare February 17, 2023 17:32

LaszloGombos reviewed Feb 18, 2023

View reviewed changes

LaszloGombos added the enhancement Issue adding new functionality label Feb 22, 2023

LaszloGombos removed the enhancement Issue adding new functionality label Mar 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dracut.sh): abort on `instmods -c ...` failure #2202

fix(dracut.sh): abort on `instmods -c ...` failure #2202

sshedi commented Feb 14, 2023

sshedi commented Feb 15, 2023

sshedi commented Feb 17, 2023

aafeijoo-suse left a comment

sshedi commented Feb 17, 2023 •

edited

Loading

sshedi commented Feb 17, 2023

LaszloGombos commented Feb 18, 2023 •

edited

Loading

sshedi commented Feb 18, 2023

sshedi commented Feb 18, 2023

LaszloGombos Feb 18, 2023

sshedi Feb 21, 2023

LaszloGombos Feb 21, 2023

sshedi Feb 21, 2023

LaszloGombos Feb 21, 2023

sshedi Feb 21, 2023

aafeijoo-suse Feb 27, 2023

fix(dracut.sh): abort on instmods -c ... failure #2202

Are you sure you want to change the base?

fix(dracut.sh): abort on instmods -c ... failure #2202

Conversation

sshedi commented Feb 14, 2023

Changes

Checklist

sshedi commented Feb 15, 2023

sshedi commented Feb 17, 2023

aafeijoo-suse left a comment

Choose a reason for hiding this comment

sshedi commented Feb 17, 2023 • edited Loading

sshedi commented Feb 17, 2023

LaszloGombos commented Feb 18, 2023 • edited Loading

sshedi commented Feb 18, 2023

sshedi commented Feb 18, 2023

LaszloGombos Feb 18, 2023

Choose a reason for hiding this comment

sshedi Feb 21, 2023

Choose a reason for hiding this comment

LaszloGombos Feb 21, 2023

Choose a reason for hiding this comment

sshedi Feb 21, 2023

Choose a reason for hiding this comment

LaszloGombos Feb 21, 2023

Choose a reason for hiding this comment

sshedi Feb 21, 2023

Choose a reason for hiding this comment

aafeijoo-suse Feb 27, 2023

Choose a reason for hiding this comment

fix(dracut.sh): abort on `instmods -c ...` failure #2202

fix(dracut.sh): abort on `instmods -c ...` failure #2202

sshedi commented Feb 17, 2023 •

edited

Loading

LaszloGombos commented Feb 18, 2023 •

edited

Loading