lkrg-org / lkrg

Linux Kernel Runtime Guard
https://lkrg.org
Other
410 stars 72 forks source link

Added instructions on how to install lkrg using DKMS #200

Closed krishjainx closed 2 years ago

krishjainx commented 2 years ago

Description

Added instructions on how to install lkrg using DKMS

How Has This Been Tested?

Tested on Fedora 36 switching between kernels

krishjainx commented 2 years ago

modprobe p_lkrg and rmmod p_lkrg work as expected. I believe we should let the users know that the systemd service file is not installed automatically when using dkms. I am not aware of a way to do that, maybe there is?

In any case, it should be noted that the user should manually install the systemd service file so that that works

solardiz commented 2 years ago

@morfikov Please take a look at this PR, and comment on @Krish-sysadmin's comment above. Thank you!

krishjainx commented 2 years ago

@morfikov @solardiz I made changes to the README. Please have a look. If it is ok, then I'll merge the two commits into one

krishjainx commented 2 years ago

introduce double spaces between sentences for consistency with the rest of the text

I don't see double spaces in the rest so where is this?

solardiz commented 2 years ago

I don't see double spaces in the rest so where is this?

Everywhere else between sentences within a paragraph in README, e.g. in this one before "We" if you need a specific example:

LKRG is a kernel module (not a kernel patch), so it can be built for and loaded
on top of a wide range of mainline and distros' kernels, without needing to
patch those.  We currently support kernel versions ranging from as far back as
RHEL7's (and its many clones/revisions) and Ubuntu 16.04's to latest mainline
and distros' kernels.  We've tested this revision of LKRG with Linux kernels
up to and including 5.18-rc*.

This style is becoming increasingly archaic, though, so perhaps we'll need to switch away from it - but separately, not in this PR. Maybe along with a switch to Markdown.

morfikov commented 2 years ago

Please correct the word "downloaded" to "download", introduce double spaces between sentences for consistency with the rest of the text, and squash the two commits into one. Other than that, this looks ready unless @morfikov says otherwise. Thanks!

It looks OK to me.

krishjainx commented 2 years ago

Thank you @morfikov @solardiz . I fixed it. By double spaces I thought you meant double-spaces between the lines itself. Thanks for clarifying

krishjainx commented 2 years ago

If it looks OK, I'll merge the commits

krishjainx commented 2 years ago

Done @solardiz

solardiz commented 2 years ago

it should be noted that the user should manually install the systemd service file

We forgot to address this. And it's not only systemd, but configuring LKRG to be loaded on bootup in any way in general. The other PR that's about to be merged introduces OpenRC and also instructions for "other" init systems, so we'll need to have whatever we write here fit in with that.

krishjainx commented 2 years ago

Ok, so we need to wait for that to be completed?

solardiz commented 2 years ago

I've just merged that other PR. As expected, this caused a conflict here (sorry!), so you need to rebase and also write something about configuring LKRG to be loaded on bootup.

krishjainx commented 2 years ago

I figured something out. Is it good now?

krishjainx commented 2 years ago

@solardiz Ok, I made the required changes. Can you have a look? If it is OK i'll merge the commits

krishjainx commented 2 years ago

@solardiz Hope it's perfect now

solardiz commented 2 years ago

Hope it's perfect now

It was good enough, so merged. Thanks!

One thing GitHub UI didn't show is trailing whitespace. I'll plan on fixing that with a separate commit now.

solardiz commented 2 years ago

Also, many lines were over 79. Hard to see this in GitHub UI. All of this is now fixed with a subsequent commit.

More importantly, perhaps we should add something on how to uninstall LKRG when using DKMS? And how to upgrade it - is our advice to uninstall first appropriate in that case as well? @morfikov

krishjainx commented 2 years ago

@solardiz Thank you. Yup, uninstall instructions should be added in my opinion. For upgrading, I'm not sure that's the best idea with DKMS. Same as with the normal Upgrade instructions it requires manual work.

krishjainx commented 2 years ago

And how to upgrade it - is our advice to uninstall first appropriate in that case as well?

I believe so

morfikov commented 2 years ago

More importantly, perhaps we should add something on how to uninstall LKRG when using DKMS? And how to upgrade it - is our advice to uninstall first appropriate in that case as well? @morfikov

Removing is easy as executing the dkms remove ... command. Upgrading is to remove and install the module again (newer version).

solardiz commented 2 years ago

I wonder if we can/should simplify the instructions from separate use of tar and dkms add to running dkms add on the tarball or using dkms ldtarball. Are our release tarballs compatible with that?

Removing is easy as executing the dkms remove ... command.

OK, I'm adding this:

If you installed using DKMS, you'd uninstall with:

        sudo dkms remove -m lkrg -v 0.9.3

I hope I got this right (untested).

morfikov commented 2 years ago

Removing is easy as executing the dkms remove ... command.

OK, I'm adding this:

If you installed using DKMS, you'd uninstall with:

        sudo dkms remove -m lkrg -v 0.9.3

I hope I got this right (untested).

It should be something like this:

# dkms remove -m lkrg/0.9.3 -k 5.7.7-amd64

You can also use the following to remove the module for all kernels.

# dkms remove -m lkrg/0.9.3 --all

solardiz commented 2 years ago

Are you saying the command I mentioned isn't correct as-is, or do you merely suggest optional alternatives? Which one command do you recommend for inclusion in the documentation?

solardiz commented 2 years ago

I'd rather not include something like -k 5.7.7-amd64 (kernel version and distro specific), so if anything like this needs to be included, it'd have to be --all. But is this mandatory? Will my command not work as-is, I guess for the current kernel?

morfikov commented 2 years ago

Are you saying the command I mentioned isn't correct as-is, or do you merely suggest optional alternatives?

I checked man dkms and the command you provided appears to be valid. I was using only the two commands I posted in the previous post.

Which one command do you recommend for inclusion in the documentation?

All of them, :)

morfikov commented 2 years ago

Also be consistent with the command pattern.

-m lkrg -v 0.9.3 is the same as -m lkrg/0.9.3 , but I prefer the second one.