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

Add wrapper for linux kernel module loading #930

Merged
merged 4 commits into from Sep 23, 2018
Merged

Add wrapper for linux kernel module loading #930

merged 4 commits into from Sep 23, 2018

Conversation

@bachp
Copy link
Contributor

@bachp bachp commented Jul 20, 2018

  • init_module and finit_module to load kernel modules
  • delete_module to unload kernel modules
src/kmod.rs Outdated
/// init_module(&mut contents, &CString::new("").unwrap()).unwrap();
/// ```
///
pub fn init_module(module_image: &[u8], param_values: &CStr) -> Result<i64> {

This comment has been minimized.

@bachp

bachp Jul 20, 2018
Author Contributor

Is there a more flexible way then passing the binary as &[u8]?

src/lib.rs Outdated
@@ -43,6 +43,8 @@ pub mod fcntl;
target_os = "openbsd"))]
pub mod ifaddrs;
#[cfg(any(target_os = "linux", target_os = "android"))]
pub mod kmod;

This comment has been minimized.

@bachp

bachp Jul 20, 2018
Author Contributor

Not sure if this is the right place to put this nor if this is the proper name. Inputs are welcome.

@bachp bachp force-pushed the bachp:kmod branch from 1259487 to 43210ab Jul 23, 2018
@bachp bachp force-pushed the bachp:kmod branch from 43210ab to 732dae7 Jul 25, 2018
Copy link
Member

@asomers asomers left a comment

I think it looks good overall, but it could use some tests. Is there a trivial kernel module that you could use for testing? It's ok to have tests that can only run as root; we already have a few.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 4, 2018

@asomers A simple hello world module as describe here could be used. But as the kernel ABI is not stable it needs to be compiled for the currently running version of Linux. I'm trying to get it compiled during the test using the cc crate, but with no luck so far.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 4, 2018

@asomers What is the best way to run tests as root? Should I run all of the build as root or is there a better way?

@asomers
Copy link
Member

@asomers asomers commented Aug 4, 2018

Do it just like the test_setgroups test. The test should quit early if it's not running as root. When you want to actually run that test by hand, run all of the tests as root.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 4, 2018

@asomers I added some tests

printk(KERN_INFO "Goodbye world 1.\n");
}

MODULE_LICENSE("GPL");

This comment has been minimized.

@asomers

asomers Aug 5, 2018
Member

Why GPL? The rest of Nix uses MIT.

This comment has been minimized.

@bachp

bachp Aug 5, 2018
Author Contributor

The module is taken from tldp.org where it is licensed GPL.

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

This also isn't specific enough, as the GPL has versions. I assume that means 2.0. Let's get a new one written that we can license MIT. Also, can we name it hello.c. That -1 is really killing me.

This comment has been minimized.

@bachp

bachp Aug 6, 2018
Author Contributor

A pure MIT licensed module would taint the kernel if loaded. The option I see would be: Dual MIT/GPL. I willl create a new module and license it accordingly.

use std::process::Command;

fn compile_kernel_module() -> String {
#[allow(unused_variables)]

This comment has been minimized.

@asomers

asomers Aug 5, 2018
Member

The #[allow(unused_variables)] syntax is holdover from an older version of rustc. Nowadays you can just do let _, = ... and it won't complain.

This comment has been minimized.

@bachp

bachp Aug 5, 2018
Author Contributor

I tried it and it doesn't work. I'm not sure what's the scope of _ but it seems it doesn't stick around till the end of the block.

This comment has been minimized.

@asomers

asomers Aug 5, 2018
Member

Sorry, my last comment had a typo. You need to use let _m = ..., not let _ = .... There is a difference between _m and _.

This comment has been minimized.

@bachp

bachp Aug 5, 2018
Author Contributor

Good to know. I didn't know they behave differently. I update the code.

@asomers
Copy link
Member

@asomers asomers commented Aug 5, 2018

On my Debian system the test_finit_and_delete_module test fails with this error:

make -C /lib/modules/4.9.0-7-amd64/build M=/home/somers/src/rust/nix/test/test_kmod/hello_mod modules
make[1]: *** /lib/modules/4.9.0-7-amd64/build: No such file or directory.  Stop.
Makefile:4: recipe for target 'all' failed
make: *** [all] Error 2
FAILED

failures:

---- test_kmod::test_finit_and_delete_module stdout ----
        thread 'test_kmod::test_finit_and_delete_module' panicked at 'assertion failed: status.success()', test/test_kmod/mod.rs:12:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 5, 2018

@asomers You most probably don't have the kernel sources installed on your system. Building a kernel module requires the kernel source. I'm not sure what packages provides these on Debian.

@asomers
Copy link
Member

@asomers asomers commented Aug 5, 2018

I installed the linux-source package, and now I have sources in /usr/src, but I still get the same error. The fact that the error message mentions a "build" directory suggests that maybe there's a missing step?

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 5, 2018

I tried on a debian system and the required package is linux-headers-*.

You can install it via:

apt install linux-headers-$(uname -r)
@asomers
Copy link
Member

@asomers asomers commented Aug 5, 2018

Ok, installing linux-headers did the trick. But now "cargo test" is trying to create some temporary files in nix's test directory. Could you fix it so those are created in TMPDIR instead?

running 1 test
test test_kmod::test_finit_and_delete_module ... make -C /lib/modules/4.9.0-7-amd64/build M=/home/somers/src/rust/nix/test/test_kmod/hello_mod modules
make[1]: Entering directory '/usr/src/linux-headers-4.9.0-7-amd64'
arch/x86/Makefile:140: CONFIG_X86_X32 enabled but no binutils support
mkdir: cannot create directory ‘/home/somers/src/rust/nix/test/test_kmod/hello_mod/.tmp_versions’: Permission denied
  CC [M]  /home/somers/src/rust/nix/test/test_kmod/hello_mod/hello-1.o
/home/somers/src/rust/nix/test/test_kmod/hello_mod/hello-1.c:1:0: error: code model kernel does not support PIC mode
 /*
 
Assembler messages:
Fatal error: can't create /home/somers/src/rust/nix/test/test_kmod/hello_mod/.tmp_hello-1.o: Permission denied
/usr/src/linux-headers-4.9.0-7-common/scripts/Makefile.build:307: recipe for target '/home/somers/src/rust/nix/test/test_kmod/hello_mod/hello-1.o' failed
make[4]: *** [/home/somers/src/rust/nix/test/test_kmod/hello_mod/hello-1.o] Error 1
/usr/src/linux-headers-4.9.0-7-common/Makefile:1526: recipe for target '_module_/home/somers/src/rust/nix/test/test_kmod/hello_mod' failed
make[3]: *** [_module_/home/somers/src/rust/nix/test/test_kmod/hello_mod] Error 2
Makefile:152: recipe for target 'sub-make' failed
make[2]: *** [sub-make] Error 2
Makefile:8: recipe for target 'all' failed
make[1]: *** [all] Error 2
make[1]: Leaving directory '/usr/src/linux-headers-4.9.0-7-amd64'
Makefile:4: recipe for target 'all' failed
make: *** [all] Error 2
Copy link
Contributor

@Susurrus Susurrus left a comment

I'd really like to see the kernel module we use allow for parameters to be passed. I don't see those tested as part of this, and I think it's important that it get tested, ideally having 2 parameters or more.

printk(KERN_INFO "Goodbye world 1.\n");
}

MODULE_LICENSE("GPL");

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

This also isn't specific enough, as the GPL has versions. I assume that means 2.0. Let's get a new one written that we can license MIT. Also, can we name it hello.c. That -1 is really killing me.

@@ -0,0 +1,7 @@
obj-m += hello-1.o

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Also make this hello.o

mod sys;
mod test_fcntl;
#[cfg(any(target_os = "linux", target_os = "android"))]

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Please alphabetize the OSes

src/lib.rs Outdated
@@ -43,6 +43,8 @@ pub mod fcntl;
target_os = "openbsd"))]
pub mod ifaddrs;
#[cfg(any(target_os = "linux", target_os = "android"))]
pub mod kmod;
#[cfg(any(target_os = "linux", target_os = "android"))]

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Please alphabetize here.

src/lib.rs Outdated
@@ -43,6 +43,8 @@ pub mod fcntl;
target_os = "openbsd"))]
pub mod ifaddrs;
#[cfg(any(target_os = "linux", target_os = "android"))]

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Please alphabetize this.

src/kmod.rs Outdated

/// Loads a kernel module from a given file descriptor.
///
/// Example usage:

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Same as the above, use # Example instead.

src/kmod.rs Outdated
libc_bitflags!(
/// Flags used by `delete_module`.
pub struct OFlags: libc::c_int {
O_NONBLOCK;

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Please add doccomments for these items.

src/kmod.rs Outdated

libc_bitflags!(
/// Flags used by `delete_module`.
pub struct OFlags: libc::c_int {

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

Can we rename this to OutputFlags or something less short-hand?

src/kmod.rs Outdated

/// Unloads the kernel module with the given name.
///
/// Example usage:

This comment has been minimized.

@Susurrus

Susurrus Aug 6, 2018
Contributor

# Example

src/kmod.rs Show resolved Hide resolved
@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 6, 2018

@Susurrus Kernel module rewritten, it is now dual licensed MIT or GPL-2.0+ and includes parameters. These are also used in tests.

@asomers I didn't find a way how to build external kernel modules with a separate build directory.

@asomers
Copy link
Member

@asomers asomers commented Aug 6, 2018

Did you try simply cd'ing to ${TMPDIR:-/tmp} before running make?

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 7, 2018

@asomers I don't think switching the workdir before doing make would work as the workdir needs to be in the same directory. One option would be to copy everything to a TMP location, do the build and then delete it again after the test. But it seems hacky.

@asomers
Copy link
Member

@asomers asomers commented Aug 8, 2018

@bachp could you try that then? No test should be creating files in the source directory. I can't even run these tests, because my source directory is mounted over NFS with rootsquash on.

@bachp bachp mentioned this pull request Aug 8, 2018
@bachp bachp force-pushed the bachp:kmod branch 2 times, most recently from 8847d62 to 9eede2c Aug 9, 2018
@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 9, 2018

@asomers I bumped the minimum required rust version to 1.24.1 as this is the version packaged in Debian. So it should allow them to still package rust tools depending on nix.

I think the freebsd jobs should be updated to this version too.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 9, 2018

@Susurrus I incorporated your feedback into the documentation and added some additional information and links.

@asomers
Copy link
Member

@asomers asomers commented Aug 10, 2018

Why 1.24.1? If Nix only depends on 1.22.0 features, then the minimum rustc version should be 1.22.0. That shouldn't be a problem for building Debian packages. Nor should Nix depend in any way on Debian.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 10, 2018

@asomers Rust 1.22 produces some compile errors that are not present in 1.20 and are no longer present in 1.24.1. See: https://travis-ci.org/nix-rust/nix/builds/413801114 for details on the error.

@asomers
Copy link
Member

@asomers asomers commented Aug 10, 2018

That looks like a bug in Nix. ptr may be a mutable reference. But since it's passed by value to encode_into, it itself does not need to be declared mutable. Try removing the mut and rebuilding on 1.22.0.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 10, 2018

But why does this warning only pop up with 1.22 and not with 1.24.1 or 1.28?

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 14, 2018

I updated the MR to build with 1.22.1 and changed the offending code. But travis seems to have failed for some other reason now. Seems unrelated to the change.

@asomers
Copy link
Member

@asomers asomers commented Aug 14, 2018

The Travis failures were spurious. Restarting the jobs fixed them. I'm upgrading buildbot now.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 14, 2018

Anything else that needs to be fixed?


### Changed
- Bump minimum tested Rust version to 1.22.1.

This comment has been minimized.

@Susurrus

Susurrus Aug 15, 2018
Contributor

"tested" -> "required"

This comment has been minimized.

@bachp

bachp Aug 15, 2018
Author Contributor

This commit/change will be removed once #900 is merged

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 15, 2018

There are a bunch of changes unrelated to adding kmod, like the tempfile update and the various style changes. I'd prefer to merge #900 and then rebase and squash this merge into 2 commits:

  1. updating the Rust requirements to 1.22.1
  2. Adding kmod

We'll give #900 a couple of days.

@bachp
Copy link
Contributor Author

@bachp bachp commented Aug 15, 2018

@Susurrus I think the 1.22.1 update should go into #900 as it is related to tempfile.

@bachp bachp force-pushed the bachp:kmod branch 2 times, most recently from e664162 to b6eb166 Sep 2, 2018
@bachp
Copy link
Contributor Author

@bachp bachp commented Sep 2, 2018

Rebased after #900 got merged

Cargo.toml Outdated Show resolved Hide resolved
@bachp bachp force-pushed the bachp:kmod branch from b6eb166 to 31fc839 Sep 4, 2018
@bachp
Copy link
Contributor Author

@bachp bachp commented Sep 4, 2018

I rebased again to fix the changelog conflict.

Copy link
Contributor

@Susurrus Susurrus left a comment

This looks pretty good to me, so I think it's time to refactor the commits and then we can merge. Please refactor this PR into 4 commits in the following order:

  1. Update rand crate
  2. Remove allow(unused_variables)
  3. Factor out skipping tests if not root into a macro
  4. Adding the kmod module
bachp added 4 commits Aug 9, 2018
This avoids having both 0.4 and 0.5 (required by tempfile)
This macro can be used in tests to skip the test if it requires root to sucssfully run.
- init_module and finit_module to load kernel modules
- delete_module to unload kernel modules

Signed-off-by: Pascal Bach <pascal.bach@nextrem.ch>
@bachp bachp force-pushed the bachp:kmod branch from 31fc839 to a7fea44 Sep 5, 2018
@bachp
Copy link
Contributor Author

@bachp bachp commented Sep 5, 2018

@Susurrus PR reworked into 4 logical commits.

@bachp
Copy link
Contributor Author

@bachp bachp commented Sep 13, 2018

Anything still missing?

Copy link
Member

@asomers asomers left a comment

LGTM. @Susurrus do you have any more concerns?

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Sep 23, 2018

bors r+

bors bot added a commit that referenced this pull request Sep 23, 2018
Merge #930
930: Add wrapper for linux kernel module loading r=Susurrus a=bachp

- init_module and finit_module to load kernel modules
- delete_module to unload kernel modules

Co-authored-by: Pascal Bach <pascal.bach@nextrem.ch>
@bors bors bot merged commit a7fea44 into nix-rust:master Sep 23, 2018
4 checks passed
4 checks passed
bors Build succeeded
Details
buildbot/nix-rust/nix amd64_fbsd11 Build done.
Details
buildbot/nix-rust/nix i386_fbsd11 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bachp bachp deleted the bachp:kmod branch Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants