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

linux: add fanotify(7) API bindings. #1699

Merged
merged 1 commit into from Apr 5, 2020

Conversation

cpu
Copy link
Contributor

@cpu cpu commented Mar 18, 2020

The fanotify API is a Linux-specific API for notification and interception of filesystem events. In some ways it is similar to inotify, but with different advantages/tradeoffs. It is particularly well suited to full filesystem/mount monitoring (vs operating per directory/file like inotify) and for allowing/denying access to files (inotify lacks this capability).

The fanotify API has been updated several times since it was enabled in Linux 2.6.37. Presently I've only included support for the original fanotify features, and the FAN_MARK_FILESYSTEM addition made in Linux 4.20. There are subsequent updates in 5.0 and 5.1 not covered in this initial commit.

This commit adds the relevant constants and types from uapi/linux/fanotify.h and two new functions (fanotify_init (man page) and fanotify_mark (man page)) to src/unix/linux_like/linux/mod.rs. While I believe this API is also present on Android I have presently limited my attention to Linux.

Although this commit focuses on Linux 4.20.x's fanotify API/constants I have skipped adding constants for FAN_ALL_CLASS_BITS, FAN_ALL_INIT_FLAGS, FAN_ALL_MARK_FLAGS, FAN_ALL_EVENTS, FAN_ALL_PERM_EVENTS and FAN_ALL_OUTGOING_EVENTS even though they are present in this kernel version's headers. These defines were deprecated in later releases with instructions to not use them in new programs or extend them with new values. It would be a shame for new Rust programs to use deprecated #defines!

Note to reviewers: I'm very new to Rust and the libc crate. I've tried my best to match project guidelines for contributing but friendly advice/pointers are welcome! I've also tested my work by building some WIP nix wrappers on top of the libc work and porting an example program from the fanotify man page to Rust: https://github.com/cpu/rfanotify. I've had success running this program on Linux 4.4.0, 5.0.0 and 5.5.8

Edit: I also understand this Crate is understaffed. This is a side project and doesn't need immediate attention

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@cpu
Copy link
Contributor Author

cpu commented Mar 18, 2020

rust-lang.libc — #20200318.6 failed

Looks like there's some test failures to sort out 😓

It seems like like none of the musl jobs passed. That makes sense to me, I only tested with GNU libc. I don't have any experience with musl but it seems like there is support since this commit in 2014. Would folks prefer I pursue supporting musl or would it be best to move the bindings from src/unix/linux_like/linux/mod.rs to src/unix/linux_like/linux/gnu/mod.rs?

I'm less certain what could be at fault with the other jobs and probably need some assistance to make progress there.

@JohnTitor
Copy link
Member

Hey, thanks for the PR! I haven't reviewed deeply yet but I think we should add linux/fanotify.h here:

libc/libc-test/build.rs

Lines 2224 to 2229 in 276eaa2

// Include linux headers at the end:
headers! {
cfg:
"asm/mman.h",
"linux/dccp.h",
"linux/errqueue.h",

@cpu
Copy link
Contributor Author

cpu commented Mar 18, 2020

I haven't reviewed deeply yet but I think we should add linux/fanotify.h here:

@JohnTitor Thanks for the quick pointer! I've applied this suggestion in 501762b.

src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
libc-test/build.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
src/unix/linux_like/linux/mod.rs Outdated Show resolved Hide resolved
@JohnTitor JohnTitor assigned JohnTitor and unassigned gnzlbg Mar 19, 2020
@cpu
Copy link
Contributor Author

cpu commented Mar 19, 2020

@JohnTitor Thanks for the detailed review! I believe I've addressed all of your review feedback and this branch is ready for a second pass when you have a chance.

@JohnTitor
Copy link
Member

Great! Now some targets pass CI. I'll take a look at the log after taking a bath but at a glance:

  • some targets complain re-definition of fanotify_event_metadata and fanotify_response, so we already have them somewhere
  • i686-unknown-linux-gnu complains that fanotify_event_metadata's align size is wrong, we should tweak it.

@cpu
Copy link
Contributor Author

cpu commented Mar 19, 2020

Great! Now some targets pass CI

Progress! 🎉 Thanks for your help :-)

i686-unknown-linux-gnu complains that fanotify_event_metadata's align size is wrong, we should tweak it.

After some fiddling I got the ci/run-docker.sh script working locally. I was able to fix this target/test failure in my local repro's by adding a #[repr(align(8))] attribute in bdd80b6. If I'm on the wrong track here (or am ignoring a compat. issue with other targets) I can revert the commit.

@cpu
Copy link
Contributor Author

cpu commented Mar 19, 2020

some targets complain re-definition of fanotify_event_metadata and fanotify_response, so we already have them somewhere

I noticed all of the tasks that failed with this error seem to be using musl and was able to repro locally. That led me to this musl project FAQ entry on this particular compiler error: "Q: Why am I getting “error: redefinition of struct ethhdr/tcphdr/etc”?"

I tried the first possible solution listed and removed the linux/fanotify.h include I added in 501762b (I left sys/fanotify.h).

Re-running the x86_64-unknown-linux-musl target's tests locally showed a different failure at this point:

cargo:warning=/checkout/target/x86_64-unknown-linux-musl/debug/build/libc-test-a1a47ab82627b1bc/out/main.c: In function '__test_fn_fanotify_mark':
cargo:warning=/checkout/target/x86_64-unknown-linux-musl/debug/build/libc-test-a1a47ab82627b1bc/out/main.c:31770:24: error: returning 'int (*)(int,  unsigned int,  long long unsigned int,  int,  const char *)' from a function with incompatible return type 'int (*)(int,  unsigned int,  uint64_t,  int,  const char *)' {aka 'int (*)(int,  unsigned int,  long unsigned int,  int,  const char *)'} [-Werror=incompatible-pointer-types]
cargo:warning=31770 |                 return fanotify_mark;
cargo:warning=      |                        ^~~~~~~~~~~~~

The long long unsigned int vs uint64_t argument mismatch seems like it answers the question you had about whether the fanotify_mark's mask argument needs to be ::c_ulonglong vs u64 :-)

When I reverted 42ac577 and f082b99 the target's tests passed locally 🎉 I'll push the relevant commits to this branch and see how a full CI run goes. 🤞

Edit: pushed 7aea581, 3d2f23b and 4938233. Hope I'm on the right track here. It's also possible I've been barking up the wrong tree 😆

@cpu
Copy link
Contributor Author

cpu commented Mar 19, 2020

It's also possible I've been barking up the wrong tree 😆

Seems like this might have been the case. Other targets that were passing before (e.g. x86_64-unknown-linux-gnu) failed. I'm going to put this branch down for the day and come back tomorrow with fresh eyes before I make more of a mess :-)

@JohnTitor
Copy link
Member

Thanks!

The long long unsigned int vs uint64_t argument mismatch seems like it answers the question you had about whether the fanotify_mark's mask argument needs to be ::c_ulonglong vs u64 :-)

That's fair but some other targets still complain that, hmm it's odd, I'll investigate.

@cpu
Copy link
Contributor Author

cpu commented Mar 20, 2020

That's fair but some other targets still complain that, hmm it's odd, I'll investigate.

It felt odd to me too. I poked at it a little bit more this morning and using u64 seems to work fine for everything except the musl targets. Using ::c_ulonglong works for musl but breaks on some other targets (e.g. 64bit glibc) (Which feels strange to me since I thought c_ulonglong was a type def for u64). I haven't found a solution that worked for all targets but I admit to being a neophyte when it comes to C types and portability.

@JohnTitor
Copy link
Member

Using ::c_ulonglong works for musl but breaks on some other targets (e.g. 64bit glibc) (Which feels strange to me since I thought c_ulonglong was a type def for u64).

So, I think we can switch u64 with c_ulonglong by adding #[cfg(target_env = "musl")] or #[cfg(not(target_env = "musl"))].

@cpu
Copy link
Contributor Author

cpu commented Mar 21, 2020

So, I think we can switch u64 with c_ulonglong by adding #[cfg(target_env = "musl")] or #[cfg(not(target_env = "musl"))].

Cool, 👍 I implemented that suggestion in 2eab0f8.

Almost there! It looks like CI is green across the board now with the exception of two tasks:

  1. DockerTierLinux2 sparc64-unknown-linux-gnu

Not sure what's up with this one. I'll try to repro locally and see what I can figure out. I can't see any failed tests in the test output on Azure 🤔

  1. StyleAndDocs - Check style

I used the cfg attribute directly in 2eab0f8 and it looks like I should prefer cfg_if or splitting things into the gnu and musl modules.

I gave a quick first shot at using cfg_if in the same context I was using cfg and it fails to build: https://gist.github.com/cpu/4280a148aad1619e9b71139b48e6fc8f so I think that's not the right approach.

@JohnTitor Q for you: If I move the pub fn fanotify_mark export with the u64 argument to src/unix/linux_like/linux/gnu/mod.rs and the pub fn fanotify_mark export with the ::c_ulonglong argument to src/unix/linux_like/linux/musl/mod.rs should I leave the common parts (pub fn fanotify_init, etc) in src/unix/linux_like/linux/mod.rs to avoid duplication, or should I remove the common parts from the linux mod and have everything self contained in the musl and gnu subcrates?

Thanks!

@cpu
Copy link
Contributor Author

cpu commented Mar 27, 2020

should I leave the common parts (pub fn fanotify_init, etc) in src/unix/linux_like/linux/mod.rs to avoid duplication

I decided to try this route in 0bef8ef, happy to adjust if required.

@cpu
Copy link
Contributor Author

cpu commented Mar 27, 2020

  1. DockerTierLinux2 sparc64-unknown-linux-gnu
    Not sure what's up with this one. I'll try to repro locally and see what I can figure out.

Both this and the style check are passing now (locally and in CI) 🎉

Unfortunately there's a handful of new task failures 😓 Three of them are "BuildChannelsLinux" tasks. There's also one "SemverOSX" failure that looks to be caused by a missing crate. Looking through the failures they all seem unrelated to my fanotify work and I'm not sure how to proceed/fix. Guidance welcome!

@cpu
Copy link
Contributor Author

cpu commented Apr 2, 2020

Unfortunately there's a handful of new task failures 😓 Three of them are "BuildChannelsLinux" tasks. There's also one "SemverOSX" failure that looks to be caused by a missing crate. Looking through the failures they all seem unrelated to my fanotify work and I'm not sure how to proceed/fix. Guidance welcome!

After merging master again the "SemverOSX" failure went away 🤷‍♂️

As for the "BuildChannelsLinux" failures I was able to figure out the problem and I was wrong about it being unrelated to the diffs in this branch. The root cause was bdd80b6 and my use of the #[repr(align(8))] attribute. This feature requires Rust 1.25+ and so failed the BuildChannelsLinux tasks using Rust 1.13.0, 1.19.0 and 1.24.0. (I found the background in #1042 to be useful context).

To continue supporting Rust versions < 1.25 I split the fanotify_event_metadata struct across src/unix/linux_like/linux/align.rs and src/unix/linux_like/linux/no_align.rs in 8b30245. The former keeps the nice #[repr(align(8))] attribute while the latter uses the older __align field style. I think this matches up with how the rest of the crate is operating today.

That did the trick! All green across the board now 🎆 🏁

@cpu
Copy link
Contributor Author

cpu commented Apr 2, 2020

@JohnTitor This branch is ready for another review pass when you have a chance. Thank you!

@JohnTitor
Copy link
Member

JohnTitor commented Apr 3, 2020

As for the "BuildChannelsLinux" failures I was able to figure out the problem and I was wrong about it being unrelated to the diffs in this branch. The root cause was bdd80b6 and my use of the #[repr(align(8))] attribute. This feature requires Rust 1.25+ and so failed the BuildChannelsLinux tasks using Rust 1.13.0, 1.19.0 and 1.24.0. (I found the background in #1042 to be useful context).

That's correct!

After merging master again the "SemverOSX" failure went away

The semver check depends on nightly rustc API and I fix it when noticing usually :)

Okay, finally I'd say "LGTM", the last work is to squash commits since they're verbose a bit to merge as-is. Thanks for separating commits, it made my reviews easy :)

@cpu cpu force-pushed the cpu-fanotify-linux-bindings branch from 8b30245 to 7b199bb Compare April 3, 2020 12:40
@cpu
Copy link
Contributor Author

cpu commented Apr 3, 2020

Okay, finally I'd say "LGTM", the last work is to squash commits since they're verbose a bit to merge as-is.

Great! I squashed all of my work down to one commit: 46b18b8

Thanks for separating commits, it made my reviews easy :)

Glad to hear it :-) Thanks for the reviews!

@JohnTitor
Copy link
Member

Ohh, you messed up the branch, try to rebase/squash again?

@cpu cpu force-pushed the cpu-fanotify-linux-bindings branch from 0e49ca1 to 01b4fc7 Compare April 4, 2020 20:00
@cpu
Copy link
Contributor Author

cpu commented Apr 4, 2020

Ohh, you messed up the branch, try to rebase/squash again?

Drat I see what you mean :-( Sorry about that! Should be fixed now w/ my force-push to 01b4fc7 5c7a82a. (edit: caught another rebase error, I lost the align/no align split, fixed now.)

The `fanotify` API[0] is a linux-specific API for notification and interception
of filesystem events. In some ways it is similar to `inotify`, but with
different advantages/tradeoffs. It is particularly well suited to full
filesystem/mount monitoring (vs per directory) and for allowing/denying access
to files (`inotify` lacks this capability).

The `fanotify` API has been updated several times since it was enabled in Linux
2.6.37. Presently I've only included support for the original `fanotify`
features, and the `FAN_MARK_FILESYSTEM` addition made in Linux 4.20. There are
subsequent updates in 5.0 and 5.1 not covered in this initial commit.

This commit adds the relevant constants and types from
`uapi/linux/fanotify.h`[1] and two new functions (`fanotify_init`[2] and
`fanotify_wrap`[3]) to `src/unix/linux_like/linux/mod.rs`. While I believe this
API is also present on Android I have presently limited my attention to Linux.

Although this commit focuses on Linux 4.20.x's `fanotify` API/constants I have
skipped adding constants for `FAN_ALL_CLASS_BITS`, `FAN_ALL_INIT_FLAGS`,
`FAN_ALL_MARK_FLAGS`, `FAN_ALL_EVENTS`, `FAN_ALL_PERM_EVENTS` and
`FAN_ALL_OUTGOING_EVENTS` even though they are present in this kernel version's
headers. These defines were deprecated[4] in later releases with instructions to
not use them in new programs or extend them with new values. It would be a shame
for new Rust programs to use deprecated #defines!

[0]: http://man7.org/linux/man-pages/man7/fanotify.7.html
[1]: https://github.com/torvalds/linux/blob/d54f4fba889b205e9cd8239182ca5d27d0ac3bc2/include/uapi/linux/fanotify.h
[2]: http://man7.org/linux/man-pages/man2/fanotify_init.2.html
[3]: http://man7.org/linux/man-pages/man2/fanotify_mark.2.html
[4]: torvalds/linux@23c9dee#diff-4c9ca62be6bf38cc08f7ea9daf16e379
@cpu cpu force-pushed the cpu-fanotify-linux-bindings branch from 01b4fc7 to 5c7a82a Compare April 4, 2020 20:07
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the work!

@JohnTitor JohnTitor merged commit 1d9e5fb into rust-lang:master Apr 5, 2020
@cpu cpu deleted the cpu-fanotify-linux-bindings branch April 5, 2020 12:52
bors added a commit that referenced this pull request Oct 27, 2023
feat: add new constants from fanotify linux api

Since it's introduction in Linux and in this repo (#1699), several new features have been introduced in the fanotify API.
In the original fanotify API, only a limited set of events was supported.  In particular, there was no support for create, delete, and move events.  The support for those events was added in Linux 5.1.

This PR adds missing constants from `fanotify.h` to support the newest features.
bors added a commit that referenced this pull request Oct 28, 2023
feat: add new constants from fanotify linux api

Since it's introduction in Linux and in this repo (#1699), several new features have been introduced in the fanotify API.
In the original fanotify API, only a limited set of events was supported.  In particular, there was no support for create, delete, and move events.  The support for those events was added in Linux 5.1.

This PR adds missing constants from `fanotify.h` to support the newest features.
bors added a commit that referenced this pull request Nov 1, 2023
feat: add new constants from fanotify linux api

Since it's introduction in Linux and in this repo (#1699), several new features have been introduced in the fanotify API.
In the original fanotify API, only a limited set of events was supported.  In particular, there was no support for create, delete, and move events.  The support for those events was added in Linux 5.1.

This PR adds missing constants from `fanotify.h` to support the newest features.
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.

None yet

4 participants