Created attachment 1344531 [details]
kmodtool-kmodtool

Hello Nicolas,

Do you think the attached file would fix the various issues you mentioned?
This file is a work in progress to address these issues and to improve the functionality of this tool. It includes (with some minor correction) the signing feature for secureboot provided by Stanislas Leduc available here:

https://github.com/shannara/kmodtool-fedora/tree/f27-secureboot

and in response to this:

https://bugzilla.redhat.com/show_bug.cgi?id=1454824

It also includes the modifications concerning the different paths between Fedora and RHEL discussed here:

https://bugzilla.redhat.com/show_bug.cgi?id=1484293

and already pushed in the epel7 branch.

As you suggested it, I took example on https://git.centos.org/blob/rpms!kmod-redhat-ixgbe.git/c7/SPECS!ixgbe.spec and I hope I didn't miss something. I also read kmod-redhat-megaraid_sas and kmod-redhat-qed.

I choose to deal with the issues you mentioned by adding "%if 0%{?rhel}" to the output of kmodtool, instead of trying to deal with the difficulty to get safer distribution information  with bash (see https://access.redhat.com/discussions/1312334 for the difficulties).

I tried to address the 3 issues you mention in your message
- remove unneeded uname-r sub-package;
- enforce range dependency for kernels;
- register modules with weak-modules. Note: this really slows the 
  installation.

I included these features to kmodtool file:
- Reworked correction for RHEL/EPEL to akmods and kmodtool from #1484293
  and #1484295.
- Reworked and improved signing module for secureboot from Stanislas Leduc work.
  kmodtool file needs all the other files from his github repository and a
  correct setup in order to be functional with this feature.
- Compact module file if feature is present on the system (xz). Compacted 
  module seems to be compatible with registering module via weak-module. It is 
  also compatible with signing modules. 
- Try a fix for fixme "check if uname from Makefile is the same as ${kernel}"
  in print_customrpmtemplate function.
- As the kmod examples and the /usr/lib/rpm/redhat/kmodtool example file on 
  centos* uses weak-modules and depmod, depmod was added for RHEL/EPEL.
 
Lasts some questions, maybe naive.
Do we need to take care of these files for RHEL/EPEL?

/etc/depmod.d/%{kmod_name}.conf
/usr/share/doc/kmod-%{kmod_name}/greylist.txt

The examples I read deal with these files in the %files section, but they seem to be built in the %build section that kmodtool doesn't address. 

I've cleaned-up kmodtool file, and tried to address some "fixme", but it will be better to review these modifications.

I've made some tests on Qemu/VMs (centos6 and centos7) and had difficulties with the signing and zipping functionality because of the %ifarch macro not working on this virtualization platform.
Discussions in #1454824 showed that other virtualizer seems to work correctly (not tested). Otherwise, kmodtool and akmods seem to work correctly on centos6 and centos7 with the last patches applied from #1484293 and #1484295.

I had to disable globally debug_package in print_rpmtemplate_header function because centos* fails searching debugfiles.list (probably wrong path).
This is usually disabled in akmod-module.spec by packagers for rpmfusion, but I'm unsure this a good thing to do.

I've made the tests on VMs with rpmfusion wl-kmod. The module builds, installs, loads, unloads and "modinfo" correctly (zipped or not). I've not setup yet a secureboot VM to test signing with this particular kmodtool file. The one provided by Stanislas Leduc github repository (#1454824) was tested on real machine: they were conclusive (with akmod-nvidia).

Feel free to make any comments about this.

I can provide, if needed patches (one large or splitted ones), but as the clean-up add multiple minor corrections (space to tab indentation for example) I preferred to attach the whole file.

Hope kmodtool will give us shortly the ability to build with akmods third-party modules (zipped and/or signed or nothing) for RHEL/Fedora and derivatives.

Cordially,


-- 
NVieville

PS. Bug reports in relationship with my message:
fedora #1484293
fedora #1484295
fedora #1454824
rpmfusion #4627

Hello Nicolas,

In my previous post I forgot to mention that the following expression (lines 400 and 441 of the attached file) of the kmodtool script doesn't work on both centos* VMs:

local kernel_verrelarch=${kernel%%${kernels_known_variants}}

If the input is:

for kernel
    4.11.0-0.rc2.git2.2.fc26lpae

for kernels_known_variants (on one line - see /usr/share/kmodtool/kernel-variants file)
    @(smp?(-debug)|PAE?
    (-debug)|lpae|debug|kdump|xen|kirkwood|highbank|imx|omap|tegra)

the result should be:

4.11.0-0.rc2.git2.2.fc26

but it is:

4.11.0-0.rc2.git2.2.fc26lpae

So the lines 401, 441 and 447 of the same file containing this expression are wrong:

${kernel##${kernel_verrelarch}}

With same input as above, the result is an empty string. Should be "lpae".

I didn't investigate possible problems generated by this issue in the kmodtool script.

This doesn't happen on a bare metal Fedora machine (bash version differs between Fedora and Centos* but are all from 4.x branch, respectively 4.4 for Fedora, 4.2 for Centos7 and 4.1 for Centos6). Maybe this issue is another glitch of Qemu/KVM Vms, but I can't say it as I don't have any Centos installed as a primary system on any machine at my disposal.

Cordially,


-- 
NVieville

(In reply to nicolas.vieville from comment #2)
[...]
> the result should be:
> 
> 4.11.0-0.rc2.git2.2.fc26
> 
> but it is:
> 
> 4.11.0-0.rc2.git2.2.fc26lpae
4.11 kernel is quite old, why are you using it as a example ?
lpae is only used on armv7hl arch, you can try with PAE variant on i686.
I might get your question wrong, but I think the latter format is expected.

I think it would be easier to review the content from pagure or github than on a patch attached to bugzilla.

(In reply to Nicolas Chauvet (kwizart) from comment #3)

> 4.11 kernel is quite old, why are you using it as a example ?
> lpae is only used on armv7hl arch, you can try with PAE variant on i686.
> I might get your question wrong, but I think the latter format is expected.

Sorry for choosing an old kernel as an example, but I picked-up the first one in the past kernels that has as much part as possible in its name, in order to see if the couple of bash strings substitution (${kernel%%${kernels_known_variants}}
 and ${kernel##${kernel_verrelarch}}) were Ok on Fedora and why they don't work on Centos. That was only an example, and I used it for my tests more for the string and its form than for its real meaning.

> I think it would be easier to review the content from pagure or github than
> on a patch attached to bugzilla.

Do you mean for kmodtool? If yes, I'll try to provide you the new content on pagure or github ASAP.

Cordially,


-- 
NVieville

Hello Nicolas,

You'll find here a new branch of a fork of kmodtool in pagure:

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_bug1484041_and_more

It contains all the modifications proposed above splitted in differerent commits by type of enhancement, bug fix, fixme fix or new feature, from master branch to the enhancements_bug1484041_and_more branch.

Hope this will suit your request.

Cordially,


-- 
NVieville

Hello Nicolas,

Pagure fork for kmodtool has been updated:

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_bug1484041_and_more

This branch now contains all the features proposed above, and they are working on Fedora (tests passed on one bare metal fedora 26 with neagtivo17 akmod-nvidia) and centos 7 and 6 in Qemu/KVM vms.

The new feature now takes care of possible signmodules or zipmodules macros defined upstream by the akmod-xxx packager, provided he/she did all the needed verification.

Don't hesitate to make any comments about this WIP.

Do you want me to make a pull request to the official pagure kmodtool repository?

Cordially,


-- 
NVieville

Okay, I've finally got time to work on this

I've basically pushed a reworked serie for the RHEL feature.
(I think the kabi detection feature can be done better, but it's only available for RHEL, so let's defer this).

About the EFI and secureboot I've reworked and squashed the serie here:
https://pagure.io/kmodtool/commits/reworked_efi_secureboot
What need to be fixed is:
- The key generation scripts and perm are only relevant for akmods, so they need to be moved to this package (whereas the generic framework should be kept here).

For example, you need to consider that if(when) we will re-introduce pre-built kmod, the package could be signed with a key that will probably requires read acls not related to akmods (since akmods will not be used on the build infra).

- The sign-keypair.conf  should be all dropped (and eventually converted to RPM macro to set the pub/priv key if needed and if signing is done )
Please have a look on existing macros for parameters name (such as __gpg_sign_cmd)

- Keys must be generated into /etc/pki/akmods (from the akmods package).
But only on the first service run (not on package post installation).

and others issues...

(In reply to Nicolas Chauvet (kwizart) from comment #7)

Hello Nicolas,

> Okay, I've finally got time to work on this

Thank you very much for this. I've worked on the "signing for secure boot feature", but time is lacking actually, so let's say it's a work in progress. I'll try to push this new functionality quickly. I'll let you know.

> I've basically pushed a reworked serie for the RHEL feature.
> (I think the kabi detection feature can be done better, but it's only
> available for RHEL, so let's defer this).

While trying to remove the kernel_uname_r from kmod name of RHEL part of kmodtool, I hit an issue. The tests were made on both Centos7 and Centos6 with same results. For example, if we consider Centos7, last kernel updates pushed kernel-3.10.0-693.17.1.el7.x86_64 and kernel-3.10.0-693.21.1.el7.x86_64. Both kernels packages can be kept on the system at the same time (separate directories in /lib/modules). With kernel_uname_r removed from the name of the new kmodtool script, when akmods works, the kmod-foo package get build, but akmods refuse to install it because the package is already installed (kmod-foo for 3.10.0-693) and quit saying "nothing to do". If we remove  kernel_uname_r from the name of the kmod-foo name package, updating kernels will not ensure that akmods will install the corresponding kmod-foo packages.

For the moment, in my actual sources tree, I re-add the kernel_uname_r part of the kmod package name and the tests I made showed that kmod-foo packages were correctly installed on Centosx.

Maybe we should keep this part of the package name in order to make akmods working correctly as in Fedora part of the kmodtool script.

> About the EFI and secureboot I've reworked and squashed the serie here:
> 
> [...]
> 
> Please have a look on existing macros for parameters name (such as
> __gpg_sign_cmd)
> 
> - Keys must be generated into /etc/pki/akmods (from the akmods package).
> But only on the first service run (not on package post installation).

I agree that keys have to be placed in /etc/pki/akmods directory.
I'm not sure that keys have to be generated automatically on the first service run, as this action involves the administrator of the system to previously tweak the cacert.config file, then create the keys, then invoke mokutil to enroll the keys in UEFI and provide a password manually that will be used at reboot time when MOK Management will launch and enroll the new key.

I'm afraid this has to stay a manual action. That's the reason why Stanislas Leduc had wrote the README.secureboot file, and added the support script kmodgenca in order to facilitate the admin work.

Is there something more we could do in order to make this easier? What was your idea when you say to generate the keys only on the first service run.

Thank you very much again.

Cordially,


-- 
NVieville

(In reply to nicolas.vieville from comment #8)
> (In reply to Nicolas Chauvet (kwizart) from comment #7)
[...]
> While trying to remove the kernel_uname_r from kmod name of RHEL part of
> kmodtool, I hit an issue.
You are going backward for no reason.
It's just a matter to have the uname_r from a kmod sub-package to the release field of the main kmod.
At least, this seems an easyfix (before any real testing).

Then, it should be possible to force to co-install any kmod-foo. There is probably a RPM directive for this (look at Provides: installonlypkg(%name) from the kernel spec).

The pre-built kmod should only be generated once on a new RHEL "minor" release update. (and for the related kernel). But akmods should do the same. I think there is maybe some improvements expected with RHEL7.5 about this. For example a common directory across compatible kernel...

> I agree that keys have to be placed in /etc/pki/akmods directory.
> I'm not sure that keys have to be generated automatically on the first
The keys have to be generated on the first run for the same reason we don't want the openssh host key to be generated on the post installation of the ssh-server package.

If that was the case, the same ssh host private keys would be provided in all cloud/livemedia images. That would be a very dramatic security issue.

Of course the workaround is to clean the keys generated keys, but then it's harder to clean everything than to do the things right in the first step.
It also leave a room to setup the right keys on deployment. (such as kickstart post-installation).

You assume that the config file need to be tweaked before to be useful. I think the target is the opposite. There is no need to tweak anything to {at least} have the module signed.
If the system admin wants to customize the key, then it will be a matter to recreate the key pair with the custom fields and rebuild (and sign) the packages.

(In reply to Nicolas Chauvet (kwizart) from comment #9)
> (In reply to nicolas.vieville from comment #8)
> > (In reply to Nicolas Chauvet (kwizart) from comment #7)
> [...]
> > While trying to remove the kernel_uname_r from kmod name of RHEL part of
> > kmodtool, I hit an issue.
> You are going backward for no reason.
> It's just a matter to have the uname_r from a kmod sub-package to the
> release field of the main kmod.
> At least, this seems an easyfix (before any real testing).

As you suggested in your comment I tried to use the installonlypkg (kernel-module) feature, tried many combinations, tried to add Provides directives to help yum/dnf to catch the kernel_uname_r at install time, but none of them succeed. I'm sorry to say that maybe I misunderstood your suggestion, but for the moment any hint, suggestion, online manual or working example about this would be very useful. For all the attempt I made, Centos7 and centos6 refuse to co-install kmod-foo package build by akmods and exit with the message (generic example):

Examining /var/cache/akmods/foo/kmod-foo-version-release.el7.centos.x86_64.rpm: kmod-foo-version-release.el7.centos.x86_64
/var/cache/akmods/foo/kmod-foo-version-release.el7.centos.x86_64.rpm: does not update installed package.
Error: Nothing to do

I noticed that elrepo uses:

%package       -n kmod-${kmod_name}${dashvariant}

in order to be able to make a parallel install of a kmod-foo package for different "minor" release update of kernels (as kmodtool does for Fedora).

I would be very grateful if you could point me in the right direction to try to resolve this issue.

> > I agree that keys have to be placed in /etc/pki/akmods directory.
> > I'm not sure that keys have to be generated automatically on the first
> The keys have to be generated on the first run for the same reason we don't
> want the openssh host key to be generated on the post installation of the
> ssh-server package.

Understood and done (for the moment in my local folder - not pushed on pagure fork yet).

Cordially,


-- 
NVieville

It should have been more something like:
installonlypkg(kmod-${kmod_name}${dashvariant})

(In reply to Nicolas Chauvet (kwizart) from comment #11)
> It should have been more something like:
> installonlypkg(kmod-${kmod_name}${dashvariant})

Here is all the combinations I tried. None of them work.

installonlypkg(kernel-module)

installonlypkg(${kmodname}-kmod)
installonlypkg(${kmodname}-kmod-${kernel_uname_r})
installonlypkg(${kmodname}-kmod-%{?epoch:%{epoch}:}%{version}-%{release})
installonlypkg(${kmodname}-kmod-${kernel_uname_r}-%{?epoch:%{epoch}:}%{version}-%{release})
installonlypkg(${kmodname}-kmod-%{?epoch:%{epoch}:}%{version}-%{release}-${kernel_uname_r})

installonlypkg(kmod-${kmodname})
installonlypkg(kmod-${kmodname}-${kernel_uname_r})
installonlypkg(kmod-${kmodname}-%{?epoch:%{epoch}:}%{version}-%{release})
installonlypkg(kmod-${kmodname}-%{?epoch:%{epoch}:}%{version}-%{release}-${kernel_uname_r})
installonlypkg(kmod-${kmodname}-%{?epoch:%{epoch}:}%{version}-%{release}-${kernel_uname_r})

I also tried with all the combinations at the same time, but nothing to do.

Seems I don't understand how installonlypkg works, and I haven't found any documentation about its usage in a spec file.

Cordially,


-- 
NVieville

Hello Nicolas,

Concerning installonlypkg, I think I got it: this directive only allow to co-install packages with the same name but not the same version (avoid upgrade = remove and install). So for rhel, if kmodtool keeps the scheme kmod-${kmodname} to modify the spec file of akmod-${kmodname} package, there is a problem. I didn't already investigate how to deal with this case. Fedora case keeps using scheme kmod-${kmodname}-${kernel_uname_r}, so no problem while kernel upgrade here.

Concerning secure boot feature, the functionality was pushed on my fork of pagure kmodtool and akmods tree. They are available in there own branch here:

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/secure_boot_key_features

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/enhancements_and_secure_boot

I tried to keep all your suggestions and take care of your comments.

I also tried to take care of weak-modules in RHEL.

Any comment/review about this would be very appreciated. I didn't tried to push anything in the official pagure tree to avoid to add not enough tested updates/new features.

I wonder, if you could advise me concerning the RHEL problem (maybe with sub-package)?

Cordially,


-- 
NVieville

(In reply to nicolas.vieville from comment #13)
> Hello Nicolas,
Thx for the update.

I think the best way forward is to focus on landing the secureboot feature in f28+ , then look at the rhel case for later.

I will try to look into next week.

Hello,

As my pagure fork repository of kmodtool has been updated (a glitch detected by Stanislas Leduc on this bug report #1454824), could you please update, if needed, your personal local repo before trying the kmodtool and akmods tools with secure boot feature.

Cordially,


-- 
NVieville

Hello,

One word to say that with a local packages repository setup for fc27 and fc28 and containing kmodtool and akmods packages with secure boot feature, clients machines were upgraded from Fedora 27 on to Fedora 28 correctly, the kmod drivers were upgraded too and signed, letting the reboot working correctly.

Cordially,


-- 
NVieville

Commenting here are pagure doesn't allow me to comment inline.


* Improve and fix kernel_uname_r* variables
-> Choose between PR#3 and this patch (or eventually rebase)

* Fix minor glitches
-> merged

* Add signing and zipping modules methods for Fedora and RHEL
1/ I don't see the point to use dir and file in the loop where using mod would be appropriate. I see you have used the script from the kernel package (which is good) but the same question apply to their script.
2/ You need to pass the 4th parameter with the version of the kernel to pass the sign binary (or script) to use. But instead it would be more relevant to pass the path script to use as an optional parameter. (so it default to ./scripts/sign-file instead)
3/ It seems easier to use one "find" invocation than "find" + 2 grep, try something like:
find /tmp -name "*.ko"  -o -name "*.ko.xz"
4/ Can you remind why we output files into /var/run/rpm-kmod-${kmodname}-modules ? Is this secure ?  cleaned after build ? Cannot be tempered ?
5/ Why to copy %global __akmods_kernel_uname_r ${kernel_uname_r} and not use kernel_uname_r instead ?
6/ why using akmod_ prefix for theses rpm macros ? (same as previous comment). we are in kmodtool, so akmods might not be used at all.
So I would consider the same macro as the kernel with the hope that this can be merged in rpm and used for both kernel and external kmod. (so __modsign_install_post and __modzip_install_post
Also you need to wrap the macro with a enable macro (same as %{?__debug_package:) so it's possible to enable sign/zip separately on a per kmod compilation basis.


* Check if uname from Makefile is the same as kernel variable
7/ I understand this check was an old TODO in kmodtool, but I don't get the point of it. It seems to me that you are doing lot to validate some parameters that are expected to be good (as passed by akmods or even end-users) but the validation code looks heavy. Are we fixing real word issue here ?

*  Add installonlypkg(kernel-module)
8/ Why the provide kernel-module was changed from  = to  >=  ? It seems to be that if a rhel kernel-3.10-696 is removed that the related kmod-foo-3.10.696 should be removed also (I don't know if this parameter would control this).

* Take care of weak-modules generated for other kernels with depmod 
9/ I don't get the idea behind this. Why do you need to handle depmod of kernel modules handled by weak updates at all ?

* Last clean-up - remove space at end of line
merged thx

10/ Standardize kmod package name and check globally if building on RHEL
Instead of duplicating yet another variable I would use something like:
%package       -n kmod-${kmodname}%{?_with_kabi:-${kernel_uname_r}}
But using %{?_with_rhel_kabi:${kernel_uname_r_short}}%{!?_with_rhel_kabi:...} for summary is harder to read. (So there you need to have a variable that would work everywhere instead.

I still wonder if the idea is very appropriate as we used to produced both kmod-foo and kmod-foo-${kernel_uname_r} on fedora because the way we enforced newer kmod to be installed on updated kernel. But we might change that with boolean dependencies and others rich dependencies if pre-built kmod can be re-introduced on Fedora at some point.

Created attachment 1523651 [details]
Improve and fix kernel_uname_r* variables patch

> * Improve and fix kernel_uname_r* variables
> -> Choose between PR#3 and this patch (or eventually rebase)

Kwizart,

Attached a patch to improve and fix generation of kernel_uname_r* variables used in the kmodtool script to feet the needs for RHEL and Fedora. It is a rework of the PR#3 and the original I've proposed.

I'll try to address your comments in 1454824#c63 1607289#c2 and 1484041#c17 as soon as possible, to the extent of my abilities, my possibilities and my understanding of the needs. Sorry if that does not really meet your expectations.

Any comments are welcome.

Cordially,


-- 
NVieville

(In reply to Nicolas Chauvet (kwizart) from comment #17)
Hello Nicolas,

First of all, I would try to explain what I was trying to do when I 
first post message about secure boot and third-party modules.
My goal was to have a working solution to keep secure boot active on 
recent machines with fedora and "special" devices.
I never thought of reviewing and re-founding the packages akmods and 
kmodtool. I just tried adding the features needed to reach the initial 
goal. As my skill in shell scripts and rpm macros is limited, my goal
was limited. I'm not a developer. I'm fine with the direction you want 
to go, improve these tools, but please, I wonder if you might be more 
understanding and be more explicit in the way forward for the novice 
that I am.

> Commenting here are pagure doesn't allow me to comment inline.
> 
> 
> * Improve and fix kernel_uname_r* variables
> -> Choose between PR#3 and this patch (or eventually rebase)

Already merged.

> * Fix minor glitches
> -> merged
> 
> * Add signing and zipping modules methods for Fedora and RHEL
> 1/ I don't see the point to use dir and file in the loop where using 
> mod would be appropriate. I see you have used the script from the 
> kernel package (which is good) but the same question apply to their 
> script.

I agree with you. Corrected.

> 2/ You need to pass the 4th parameter with the version of the kernel 
> to pass the sign binary (or script) to use. But instead it would be 
> more relevant to pass the path script to use as an optional parameter.
> (so it default to ./scripts/sign-file instead)

Done.

> 3/ It seems easier to use one "find" invocation than "find" + 2 grep, 
> try something like:
> find /tmp -name "*.ko"  -o -name "*.ko.xz"

Done.

> 4/ Can you remind why we output files into 
> /var/run/rpm-kmod-${kmodname}-modules ?

No I can't. That's the way it was made in the original file from RHEL 
(always available in 
https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/master/f/kmodtool),
and I can't say why this directory was chosen.

> Is this secure ? 

I can't say. But the information gathered in this file is only a list of
module files already present on the system.

> cleaned after build ? 

Yes, in %postun scriptlet.
And according to https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard,
if /run directory is provided as a temporary filesystem (tmpfs), it is
erased while the boot process. /var/run directory is a symbolic link to 
/run directory in fedora.
/tmp directory would be a candidate too, but it is size restricted. 
I think the original choice (/var/run) was the good one.

> Cannot be tempered ?

I've got no idea how. Please don't hesitate to show me how.
According to the official documentation for packaging 
(https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_saving_state_between_scriptlets),
one should save temporary file between scriptlet in 
%{_localstatedir}/lib/rpm-state/ directory. The only example I found 
about that is the eclipse package 7 years ago. Today packages using this
directory depend on systemd or gconf. Using this directory needs that 
kmodtool package own one sub-directory of it and be responsible for its
cleaning. In a certain manner /var/run directory is auto-cleaned each 
time the system boots, even if we forget to clean it up in our script.
Do we have to go this way?

> 5/ Why to copy %global __akmods_kernel_uname_r ${kernel_uname_r} and 
> not use kernel_uname_r instead ?

The rpm macro %{__akmods_kernel_uname_r} is used in macros.akmods file 
(now macros.kmodtool file, see below), and I thought it would be the 
right way to ensure that the correct kernel target would be used.
This macro has been renamed __kmodtool_kernel_uname_r and was kept.

> 6/ why using akmod_ prefix for theses rpm macros ? (same as previous 
> comment). we are in kmodtool, so akmods might not be used at all.

Corrected. File has been transferred to kmodtool package, was renamed 
macros.kmodtool and all macros defined in it have been renamed to get 
kmodtool_ prefix.

> So I would consider the same macro as the kernel with the hope that 
> this can be merged in rpm and used for both kernel and external kmod. 
> (so __modsign_install_post and __modzip_install_post

As you can see it here:
https://src.fedoraproject.org/rpms/kernel/blob/master/f/kernel.spec#_1576
the macro responsible of signing modules is defined in the kernel.spec
file. It signs and zips modules. It is not purely a rpm macro and uses
shell script instructions and rpm macros ones.
That was the first source of inspiration.

> Also you need to wrap the macro with a enable macro (same as 
> %{?__debug_package:) so it's possible to enable sign/zip separately on 
> a per kmod compilation basis.

Done.

> * Check if uname from Makefile is the same as kernel variable
> 7/ I understand this check was an old TODO in kmodtool, but I don't 
> get the point of it. It seems to me that you are doing lot to validate
> some parameters that are expected to be good (as passed by akmods or 
> even end-users) but the validation code looks heavy. Are we fixing 
> real word issue here ?

I can't say. I'm not the author of this old TODO. I just tried to remove
it doing what it was suggested. I don't know if we are fixing a real
world issue, and if it is the case, it's probably a corner case with 
"homemade" kernels.

> *  Add installonlypkg(kernel-module)
> 8/ Why the provide kernel-module was changed from  = to  >=  ? It 
> seems to be that if a rhel kernel-3.10-696 is removed that the related
> kmod-foo-3.10.696 should be removed also (I don't know if this 
> parameter would control this).

Corrected (revert to "=").

> * Take care of weak-modules generated for other kernels with depmod 
> 9/ I don't get the idea behind this. Why do you need to handle depmod 
> of kernel modules handled by weak updates at all ?

My bad. Re-reading weak-modules shell script show that module that serve
as a weak-update built against another kernel will launch depmod for it.
Commit removed.

> * Last clean-up - remove space at end of line
> merged thx
> 
> 10/ Standardize kmod package name and check globally if building on RHEL
> Instead of duplicating yet another variable I would use something like:
> %package       -n kmod-${kmodname}%{?_with_kabi:-${kernel_uname_r}}

Done. Uses with_rhel_kabi macro now and %if %{with rhel_kabi} 
construction is possible and %{?with_rhel_kabi:some_setting} as well, 
but I keep using one variable defined at the beginning to avoid possible
errors when adding new features and omitting to add  
%{!?with_rhel_kabi:-${kernel_uname_r}} each time kmod-${kmodname} will 
be use. I can revert this to your suggestion and then each line 
containing %{package_kmod_name} (15 lines actually) will contain 
kmod-${kmodname}%{?with_rhel_kabi:-${kernel_uname_r_short}}%{!?with_rhel_kabi:-${kernel_uname_r}}
as a replacement.

> But using %{?_with_rhel_kabi:${kernel_uname_r_short}}%{!?_with_rhel_kabi:...} 
> for summary is harder to read. (So there you need to have a variable 
> that would work everywhere instead.

Done, and used to build %{package_kmod_name} too. I thought it should be
useful to document this construction in the first use of it with a 
comment (added too).

> I still wonder if the idea is very appropriate as we used to produced 
> both kmod-foo and kmod-foo-${kernel_uname_r} on fedora because the way
> we enforced newer kmod to be installed on updated kernel. But we might
> change that with boolean dependencies and others rich dependencies if 
> pre-built kmod can be re-introduced on Fedora at some point.

This would probably simplify kmodtool usage, but the way I worked on it
tried to keep it compatible with Fedora and RHEL versions. I wonder if 
you could show me the path to boolean dependencies and other rich
dependencies.
The tests I made on CentOS 6 and 7 (both using weak-modules) showed that
it was necessary to add some string related to the kernel "uname -r"
after the kmod-foo name (actually ${kernel_uname_r_short}), because 
akmods was failing with kernels 3.10.0-862.14.4.el7.x86_64 and 
3.10.0-957.10.1.el7.x86_64. It wasn't able to install one of the 
kmod-foo package (no weak-modules possible between these two kernels)
arguing that the build package was already installed.

If you look at the beginning of the weak-modules shell script in 
CentOS 7, the comment says:

"This is an updated version of the script which doesn't support multiple
installation of the same out-of-tree module (stored in the 'extra' 
subdirectory) for multiple kernels. This assumption is supposed to be 
verified at the rpm level of the packages delivering these modules.  
There are some checks for this assumption, however we really don't solve
this situation. This limitation allows for a much simpler version of the
script. Previous version tried to work in this case but was incorrect in
some cases."

This comment is not present in the old (2010) weak-modules shell script
provided in CentOS 6.
  
So, I'm afraid that the only way to avoid "incorrect cases" is to add
some kernel string to the kmod-foo package name.

To conclude, the branch of my kmodtool pagure repository fork 
"enhancements_and_secure_boot" has been re-based to reflect all these 
modifications.

The new kmodtool package has been tested and worked as expected on VMs 
(CentOS 6 and 7, Rawhide) and real UEFI Secure Boot HP Z440 machines 
with Fedora 29. Some more testing are probably needed.

Cordially,


-- 
NVieville

Hi there, what is the status of this ? Is current kmodtool works with el7(or later) ?
Seems Leigh reported kmodtool current kmodtool wasn't working as appropriate with el8 (needed to revert to an older version).

Can anyone checks ?

I can't run build or srpm from fedora-30 or 31


[leigh@leigh nvidia-kmod]$ rfpkg switch-branch el8
Branch 'el8' set up to track remote branch 'el8' from 'origin'.
[leigh@leigh nvidia-kmod]$ rfpkg srpm
error: line 31: %package       -n kmod-nvidia
: package kmod-nvidia already exists
error: query of specfile /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec failed, can't parse

Could not execute srpm: Could not get n-v-r-e from /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec
[leigh@leigh nvidia-kmod]$ rfpkg switch-branch el8
Switched to branch 'el8'
[leigh@leigh nvidia-kmod]$ rfpkg build 
error: line 31: %package       -n kmod-nvidia
: package kmod-nvidia already exists
error: query of specfile /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec failed, can't parse

Could not execute build: Could not get n-v-r-e from /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec
[leigh@leigh nvidia-kmod]$ rfpkg switch-branch el7
Branch 'el7' set up to track remote branch 'el7' from 'origin'.
[leigh@leigh nvidia-kmod]$ rfpkg srpm
error: line 31: %package       -n kmod-nvidia
: package kmod-nvidia already exists
error: query of specfile /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec failed, can't parse

Could not execute srpm: Could not get n-v-r-e from /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec
[leigh@leigh nvidia-kmod]$ rfpkg build 
error: line 31: %package       -n kmod-nvidia
: package kmod-nvidia already exists
error: query of specfile /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec failed, can't parse

Could not execute build: Could not get n-v-r-e from /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod.spec


The current kmod builds for el7 and 8 on the rpmfusion builder required the --skip-nvr-check, Removing kmodtool from the host machine fixes that.
I wont be supporting el7 or 8 kmods till this issue is fixed so I don't need to gut the host machine to build.


[leigh@leigh nvidia-kmod]$ rpm -q kmodtool
kmodtool-1-34.fc30.noarch
[leigh@leigh nvidia-kmod]$ sudo rpm -e kmodtool-1-34.fc30.noarch
[sudo] password for leigh: 
error: Failed dependencies:
	kmodtool >= 1-9 is needed by (installed) akmods-0.5.6-20.fc30.noarch
	kmodtool is needed by (installed) akmod-nvidia-3:435.21-1.fc32.x86_64
	/usr/bin/kmodtool is needed by (installed) akmod-nvidia-3:435.21-1.fc32.x86_64
[leigh@leigh nvidia-kmod]$ sudo rpm -e kmodtool-1-34.fc30.noarch --nodeps
[leigh@leigh nvidia-kmod]$ rfpkg srpm


Wrote: /home/leigh/development/fedora-git/nvidia-kmod/nvidia-kmod-430.40-1.el8.src.rpm

The resulting modules are non functional after a minor kernel update

https://bugzilla.rpmfusion.org/show_bug.cgi?id=5381

I've confirmed the issue with current log on epel7 recently. The problem is that the secure boot branch is far from been reviewable so there is a need to have a selection of fixes to introduce at least epel8 minimal support.

Specially from the secure boot branch there is no point to use any akmods group from the kmodtool macro file perspective. akmods isn't needed when building pre-compiled kmod-foo "by hands".

Also the _rpmmacrosdir isn't defined in el7/el6, so it has to be defined only for theses.

FEDORA-EPEL-2019-ed60ae7842 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-ed60ae7842

(In reply to Nicolas Chauvet (kwizart) from comment #22)
> Specially from the secure boot branch there is no point to use any akmods
> group from the kmodtool macro file perspective. akmods isn't needed when
> building pre-compiled kmod-foo "by hands".
> 
> Also the _rpmmacrosdir isn't defined in el7/el6, so it has to be defined
> only for theses.

These two issues were addressed in the secure boot branch of the kmodtool. 
As usual, the fork repository has been rebased on upstream and the git 
history rewritten. The new rebased fork repositories for kmodtool and
akmods with secure boot feature have not been tested widely, so use them
with caution.

Hope this will fit the needs and go in the right direction.

Cordially,


-- 
NVieville

(In reply to nicolas.vieville from comment #24)
> (In reply to Nicolas Chauvet (kwizart) from comment #22)
> > Specially from the secure boot branch there is no point to use any akmods
> > group from the kmodtool macro file perspective. akmods isn't needed when
> > building pre-compiled kmod-foo "by hands".
> > 
> > Also the _rpmmacrosdir isn't defined in el7/el6, so it has to be defined
> > only for theses.
> 
> These two issues were addressed in the secure boot branch of the kmodtool. 
@Nicolas,

Please understand that there is a need to split urgent patches and unfinished features into separate commits (or even branches).
Currently there are several features all mixed into this single branch, this is going to be more difficult to review or even look into.

Please concentrate to have the el support landed completely.
First test and fix the current layout and as a separate commits, land the kabi support.

Also as a method, please advertise in this bug report each time you update the branch with:
- the full URL of the current progress branch in pagure or elsewhere
- the changelog of fixed items

kmodtool-1-37.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-ed60ae7842

(In reply to Nicolas Chauvet (kwizart) from comment #25)
> (In reply to nicolas.vieville from comment #24)
> > (In reply to Nicolas Chauvet (kwizart) from comment #22)
> > > Specially from the secure boot branch there is no point to use any akmods
> > > group from the kmodtool macro file perspective. akmods isn't needed when
> > > building pre-compiled kmod-foo "by hands".
> > > 
> > > Also the _rpmmacrosdir isn't defined in el7/el6, so it has to be defined
> > > only for theses.
> > 
> > These two issues were addressed in the secure boot branch of the kmodtool. 
> @Nicolas,
> 
> Please understand that there is a need to split urgent patches and
> unfinished features into separate commits (or even branches).
> Currently there are several features all mixed into this single branch, this
> is going to be more difficult to review or even look into.

Ok. Understood.

> Please concentrate to have the el support landed completely.
> First test and fix the current layout and as a separate commits, land the
> kabi support.
> 
> Also as a method, please advertise in this bug report each time you update
> the branch with:
> - the full URL of the current progress branch in pagure or elsewhere
> - the changelog of fixed items

For kmodtool, you will find a new branch on the fork repository, that includes
all the fixes made for RHEL at the time I worked on them, extracted from the 
enhancements_and_secure_boot branch, but no new features as signing or zipping
modules for secure boot. This new branch is based on master branch from
upstream (official) repository for Fedora.

https://src.fedoraproject.org/fork/nvieville/rpms/kmodtool/commits/fedora_rhel_from_master

The changelogs are:
- Fix indentation
- Add installonlypkg(kernel-module) to allow kmod packages co-install (RHEL)
- Convert if statement from "[!] $variable" to "[!] -n $variable"
- Remove unused variables and code
- Force remove temporary file
- Remove grep pipe after find command
- Standardize globally kmod package names macros and kernel_uname_r
  variables in order to ease the build of kmod packages for non-default
  distribution (Fedora), for example for RHEL
- Try to fix: check if uname from Makefile is the same as kernel variable
- Update SPEC file and changelog

As I don't know which bug report to post to, below the same thing but for
akmods taken from the secure_boot_key_features branch of the fork repository.

https://src.fedoraproject.org/fork/nvieville/rpms/akmods/commits/fedora_rhel_from_master

The changelogs are:
- Remove trailing spaces and clean-up
- Use %%{name} when possible
- Convert if statement from "[!] $variable" to "[!] -n $variable"
- Fix kernel list build when parsing command line options
- Ensure to build for grub default kernel
- Improve detection of already installed (weak-)modules in akmods (RHEL)
- akmods uses logrotate and clean-up /var/cache/akmods sub-directories of
  old logs and rpm files from no more installed kmod packages
  (rhbz #1542658).
- Update SPEC file and changelog

Hope this will fit the needs and will open the way to signing modules
for secure boot feature.
One more precision: I haven't tested specifically all those commits, but as they
are all already included in the fork branches for secure boot, I suppose they will
work correctly. Review is needed. I tried to do as quickly as possible.

As usual, comments are welcome.

Cordially,


-- 
NVieville

kmodtool-1-37.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

Let's close this bug as there is too much history to continue here.