Add process_vm_readv and process_vm_writev #568

geofft · 2017-04-03T14:22:01Z

No description provided.

geofft · 2017-04-03T14:25:46Z

Opened too soon - I guess I should add both a platform conditional and some tests. Let me do that.

geofft · 2017-04-04T15:40:38Z

One design-ish question: these syscalls take a flags argument, but currently no flags are defined (see the manpage). I chose to just not expose that argument in the wrapper, but should I create an empty bitflags for API future-compatibility? The system call has been around since Linux 3.2 (January 2012) and I don't think anyone has proposed adding flags since then.

kamalmarhubi · 2017-04-05T13:53:04Z

For some reason I thought I did this back when rust-lang/libc#283 was filed for https://github.com/jvns/ruby-stacktrace.

Anyway, thanks for doing this!

Re the flags argument. I could see this going either way: we can have a breaking change in the future should that change. Alternatively, we can have the empty flags arg.

I prefer the empty flags arg since the ergonomics aren't that bad, and these aren't super frequently used functions. The alternative may at some point down the line force a major version bump on all users just to correctly handle these infrequently used functions.

geofft · 2017-04-05T18:51:02Z

Good point about a major-version bump for an obscure function. I've added a flags type. I wasn't really sure what to name it since no constants exist yet, but my current best guess is CMA_*, for "Cross-Memory Attach", the name of the feature these two system calls implement. But it could be PV_ or PVRW_ or PRWF_ (after preadv2 and pwritev2's RWF_) or something. I guess the cost of getting it wrong is pretty low: if a flag ever shows up, nix's type would be a bit unusually named, but that seems fine.

Adding an empty bitflags! is a bit ugly; the macro uses + repetition instead of *, so I added a variant that is always compiled out, which satisfies the macro. I wonder if the upstream crate would be interested in a fix to that.

kamalmarhubi · 2017-04-06T14:45:11Z

@alexcrichton thoughts on this for bitflags?

Adding an empty bitflags! is a bit ugly; the macro uses + repetition instead of *, so I added a variant that is always compiled out, which satisfies the macro. I wonder if the upstream crate would be interested in a fix to that.

kamalmarhubi · 2017-04-06T14:46:13Z

@geofft re the name of the flag type, I think we can go with more or less anything since we can avoid breaking changes by adding a type alias if / when something actually exists. What do you think?

alexcrichton · 2017-04-06T15:02:34Z

Oh yes we'd certainly be interested in a fix!

geofft · 2017-04-07T16:17:48Z

@kamalmarhubi Oh good call, that sounds forwards-compatible.

nix is on bitflags 0.7.0, and upstream is on 0.8.0 which has a few changes. Can we merge this as is and I'll send in a patch to bitflags + a cleanup for nix at some point in the future?

homu · 2017-04-15T21:13:22Z

☔ The latest upstream changes (presumably #536) made this pull request unmergeable. Please resolve the merge conflicts.

kamalmarhubi · 2017-04-15T22:49:18Z

@geofft sorry for the delay. I'd absolutely be ok to merge this and fix up the bitflag later. Nice trick with #[cfg(any())] too!

kamalmarhubi · 2017-04-15T22:49:29Z

(This needs a rebase though!)

geofft · 2017-04-16T21:23:15Z

Tests are failing on ARM only with "qemu: Unsupported syscall: 376" (or 270 on aarch64). These are the right syscall numbers. Is there a way to mark this as a Travis-specific false positive? I would expect the code to work on actual ARM (although I suppose I should dig out a device).

If we can't ignore this from Travis easily, I'm happy to cfg it away until Travis gets better.

Susurrus · 2017-04-17T00:36:02Z

We do need a system to handle when Qemu on Travis isn't able to test features. We need a way to disable them only when testing on CI (I think ideally we could detect the CI environment variable to disable it). I'd also like to have any such issues filed upstream so that we can have a comment by ever disabled test with a URL for the tracking issue so we will be able to easily turn the test on once the feature is added.

This adds `--cfg=travis` to builds under CI, allowing tests to be conditionally ignored by marking them with #[cfg_attr(travis, ignore)] refs nix-rust#568

kamalmarhubi · 2017-04-17T03:55:43Z

If #588 makes sense to at least a couple of maintainers, it'll be a way to ignore these tests in Travis. (I'm assuming they pass locally on a reals system.)

588: ci: Allow skipping tests when running under CI r=asomers This adds `--cfg=travis` to builds under CI, allowing tests to be conditionally ignored by marking them with #[cfg_attr(travis, ignore)] refs #568

kamalmarhubi · 2017-04-17T16:38:10Z

@geofft can you try rebasing and using the attribute mentioned in #588?

Susurrus · 2017-04-17T17:55:12Z

@kamalmarhubi What do you think about whenever we remove a test from running on Travis that we have a link to an upstream issue if one exists?

geofft · 2017-04-18T01:56:33Z

OK, I tried both #[cfg_attr(and(travis, target_arch...))] and #[cfg(not(and(travis, target_arch...)))] and it's still running the tests (and I've definitely rebased on top of the commit that sets $RUSTFLAGS)... what am I doing wrong?

Susurrus · 2017-04-18T02:45:08Z

@geofft I'm working on this in #590 and found the same issue. If you want to follow things, we'll work on it there.

geofft · 2017-04-20T03:01:46Z

Yeah, I saw the discussion on #590 - I'm trying a few things to see if they work while we wait for cross to get upgraded, and I'll comment there if they do.

homu · 2017-05-17T04:23:42Z

☔ The latest upstream changes (presumably ba5c837) made this pull request unmergeable. Please resolve the merge conflicts.

Susurrus · 2017-06-11T20:45:45Z

@geofft #590 is now merged. Please update this PR if you'd like to get it merged.

Susurrus · 2017-07-12T22:40:23Z

@geofft Are you still interested in merging this PR?

geofft · 2017-07-18T03:05:26Z

@Susurrus - rebased, sorry! The libc crate doesn't define these functions on Android, so I've left them as normal-Linux-only for now,. (I'm guessing this is because Bionic only gained wrappers for these syscalls in Android 6.0 / API level 23, which is way newer than rustc's target of Android 2.3 / API level 9.)

I did a #[cfg_attr(not(any(target_arch = "x86", target_arch = "x86_64")), ignore)] on the test since it fails for all ARM and MIPS and PowerPC variants, let me know if that's not right.

Susurrus

Mostly minor changes, primarily remove the Flags argument and rework the test to not fork as that will deadlock tests every so often.

Susurrus · 2017-07-18T03:52:11Z

src/sys/uio.rs

@@ -56,6 +59,38 @@ pub fn pread(fd: RawFd, buf: &mut [u8], offset: off_t) -> Result<usize>{
    Errno::result(res).map(|r| r as usize)
 }

+// process_vm_{read,write}v currently doesn't use flags, but we define this


Flags should always be zero in my current version of Linux. I don't think forward compatibility here is a concern because as soon as they're flags we have issues with backwards compatibility on the systems themselves. Let's just leave all this extra boilerplate off for now for the sake of a cleaner API. By the time new flags are added it could be years.

See the earlier discussion on the PR - I'm pretty indifferent here, @kamalmarhubi expressed a preference to just add the flags argument now. I agree that probably there will never be flags.

I guess the other option is that we just break names from libc slightly (which we do in other places) and add a four-parameter process_vm_readv_flags(pid, local_iov, remote_iov, flags) if we ever need to, which avoids the API break.

Let's remove flags. We can assess a breaking change or adding *_flags when/if this moment comes to pass in the future.

Susurrus · 2017-07-18T03:53:30Z

src/sys/uio.rs

+);
+
+#[cfg(any(target_os = "linux"))]
+pub fn process_vm_writev(pid: Pid, local_iov: &[IoVec<&[u8]>], remote_iov: &mut [IoVec<&mut [u8]>], flags: CmaFlags) -> Result<usize> {


These needs docs. See some of the recently closed PRs for an idea of what we're looking for. I think these are Linux-specific functions, so you should link to a manpage on man7.org in the doc comment. Please run cargo doc --no-deps --open and read through the generated docs to make sure they look nice as part of your testing.

Susurrus · 2017-07-18T03:53:36Z

src/sys/uio.rs

+}
+
+#[cfg(any(target_os = "linux"))]
+pub fn process_vm_readv(pid: Pid, local_iov: &mut [IoVec<&mut [u8]>], remote_iov: &[IoVec<&[u8]>], flags: CmaFlags) -> Result<usize> {


Also needs docs.

Susurrus · 2017-07-18T03:53:42Z

src/sys/uio.rs

+    Errno::result(res).map(|r| r as usize)
+}
+
+#[cfg(any(target_os = "linux"))]


Remove the any(

Susurrus · 2017-07-18T03:53:51Z

src/sys/uio.rs

+    }
+);
+
+#[cfg(any(target_os = "linux"))]


Remove the any(

Susurrus · 2017-07-18T03:54:47Z

src/sys/uio.rs

+pub fn process_vm_readv(pid: Pid, local_iov: &mut [IoVec<&mut [u8]>], remote_iov: &[IoVec<&[u8]>], flags: CmaFlags) -> Result<usize> {
+    let res = unsafe {
+        libc::process_vm_readv(pid.into(), local_iov.as_ptr() as *const libc::iovec, local_iov.len() as libc::c_ulong,
+                                           remote_iov.as_ptr() as *const libc::iovec, remote_iov.len() as libc::c_ulong, flags.bits())


This indentation is wrong, it should be aligned to pid.

Thanks, I'll add a linebreak before local_iov. (Should I be using rustfmt or something?)

I wouldn't recommend rustfmt as it'll probably change everything in the file and its style changes are also not stable. I used to use it for my own libs, but I don't anymore because of it. I think continuing to do this manually is fine.

Susurrus · 2017-07-18T03:55:01Z

test/sys/test_uio.rs

@@ -190,3 +190,51 @@ fn test_preadv() {
    let all = buffers.concat();
    assert_eq!(all, expected);
 }
+
+#[test]
+#[cfg(any(target_os = "linux", target_os = "android"))]


Remove the android here.

Susurrus · 2017-07-18T03:55:29Z

test/sys/test_uio.rs

+    use std::{str, slice};
+
+    let (r, w) = pipe().unwrap();
+    match fork() {


Forking is bad within the Rust test harness as it can cause deadlocks. Any other way you can implement this test? Also some comments in here explaining things for those unfamiliar with the code would be nice.

The child calls pause(), which means it never continues execution past that point, so it doesn't unwind anything and shouldn't cause any deadlocks in execution. If you're having trouble with deadlocks during child creation then yeah that's a problem but also that seems surprising... I guess another thread might be holding the malloc lock or something? Blah.

I can probably use process_vm_readv(getpid()...) with a static string.

I don't fully understand the problem, @asomers is our resident expert, so I'll let him chime in.

All the fork-related deadlocks that I've seen so far happen as the child returns from the test function. So if the child truly never returns, then you should be ok. You'll still have to grab the FORK_MTX however, or you could cause the wait tests to fail.

Susurrus · 2017-07-18T03:56:39Z

@asomers Can you chime in here about the use of fork as well?

geofft · 2017-07-19T20:21:28Z

Ugh, I had a moment of worry that using IoVec to refer to a foreign process's memory is UB (since IoVec wraps a &[u8] or &mut [u8]), and @ubsan is telling me on #rust that it probably is UB to construct a pointer to an address that isn't valid in the current process. Let me redo this to take a &[(usize, usize)] or something, one sec.

From git grep iovec in a man-pages checkout, I don't think anything other than these two functions uses struct iovec to refer to foreign memory, so I'm inclined not to make a type for it, but happy to do so if it seems useful.

geofft · 2017-07-19T21:37:53Z

@Susurrus all review comments addressed I think! There's a new RemoteIoVec type (with docs :P) because I needed #[repr(C)] on it. I'm still using fork in the test (since it seems better to test against a process other than getpid()), but I'm holding FORK_MTX.

asomers · 2017-07-23T22:23:44Z

test/sys/test_uio.rs

+        },
+        Ok(Child) => {
+            let _ = close(r);
+            let s = String::from("test");


Technically, you shouldn't use String::from in the child, because that requires allocating memory, and memory allocation isn't async signal safe. The format! macro probably does allocation too. See http://man7.org/linux/man-pages/man7/signal-safety.7.html .

@asomers Let me know if this looks better. (I'm not actually sure if it's fully safe, in the sense that I'm not sure there's a guarantee that the writes to memory actually happen, but at least it's not async-signal-unsafe.)

Susurrus · 2017-07-25T05:37:59Z

@kamalmarhubi This also touches on #305, which you filed. Could you comment here on the proposal here?

Susurrus · 2017-11-08T08:10:34Z

@geofft Just wanted to ping you on this and see if we could get this wrapped up soon.

geofft · 2017-11-08T15:12:45Z

@Susurrus Woops, I totally forgot that this hadn't landed (also I started a new job in August). What needs to be done here - just getting rid of memory allocation in the test's child process? I can update the PR to fix that, hopefully tonight.

Susurrus · 2017-11-08T16:05:11Z

@geofft I believe all my comments have been addressed, it's just @asomers who has some comments left and I think it's just that allocation one.

And congrats on the new job, I did the same thing in August, which is why I'm just now coming back to clean things up a bit!

asomers · 2017-11-11T20:08:16Z

LGTM

bors r+

568: Add process_vm_readv and process_vm_writev r=asomers a=geofft

bors · 2017-11-11T20:36:49Z

Build succeeded

geofft force-pushed the process-vm branch from b564344 to 122d0be Compare April 4, 2017 15:37

geofft force-pushed the process-vm branch 2 times, most recently from 7632f2a to 5dee86a Compare April 4, 2017 16:01

geofft force-pushed the process-vm branch from 5dee86a to 030b770 Compare April 5, 2017 18:47

geofft force-pushed the process-vm branch from 030b770 to 2f2ff2d Compare April 15, 2017 23:52

kamalmarhubi mentioned this pull request Apr 17, 2017

ci: Allow skipping tests when running under CI #588

Merged

geofft force-pushed the process-vm branch 2 times, most recently from 5c8d49f to 3a55a7a Compare April 18, 2017 01:38

geofft force-pushed the process-vm branch 2 times, most recently from cc3897b to 25e71ad Compare April 20, 2017 01:11

geofft force-pushed the process-vm branch from 9f09844 to 3a55a7a Compare April 20, 2017 15:05

geofft force-pushed the process-vm branch 3 times, most recently from dcccf7b to 9d1bfbc Compare July 18, 2017 00:10

Susurrus suggested changes Jul 18, 2017

View reviewed changes

geofft force-pushed the process-vm branch 2 times, most recently from 487cd87 to c84158f Compare July 19, 2017 21:37

geofft force-pushed the process-vm branch 2 times, most recently from 1289d24 to 37fa931 Compare July 21, 2017 16:20

asomers reviewed Jul 23, 2017

View reviewed changes

Susurrus approved these changes Nov 8, 2017

View reviewed changes

Add process_vm_readv and process_vm_writev

3f5de94

geofft force-pushed the process-vm branch from 37fa931 to 3f5de94 Compare November 9, 2017 02:35

bors bot added a commit that referenced this pull request Nov 11, 2017

Merge #568

41c9e1b

568: Add process_vm_readv and process_vm_writev r=asomers a=geofft

bors bot merged commit 3f5de94 into nix-rust:master Nov 11, 2017

geofft mentioned this pull request Jun 10, 2018

sysctl-register fixes fishinabarrel/linux-kernel-module-rust#46

Merged

Add process_vm_readv and process_vm_writev #568

Add process_vm_readv and process_vm_writev #568

Conversation

geofft commented Apr 3, 2017

geofft commented Apr 3, 2017

geofft commented Apr 4, 2017

kamalmarhubi commented Apr 5, 2017

geofft commented Apr 5, 2017

kamalmarhubi commented Apr 6, 2017 • edited Loading

kamalmarhubi commented Apr 6, 2017 • edited Loading

alexcrichton commented Apr 6, 2017

geofft commented Apr 7, 2017

homu commented Apr 15, 2017

kamalmarhubi commented Apr 15, 2017

kamalmarhubi commented Apr 15, 2017

geofft commented Apr 16, 2017

Susurrus commented Apr 17, 2017

kamalmarhubi commented Apr 17, 2017

kamalmarhubi commented Apr 17, 2017

Susurrus commented Apr 17, 2017

geofft commented Apr 18, 2017 • edited Loading

Susurrus commented Apr 18, 2017

geofft commented Apr 20, 2017

homu commented May 17, 2017

Susurrus commented Jun 11, 2017

Susurrus commented Jul 12, 2017

geofft commented Jul 18, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 18, 2017

geofft commented Jul 19, 2017

geofft commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 25, 2017

Susurrus commented Nov 8, 2017

geofft commented Nov 8, 2017

Susurrus commented Nov 8, 2017

asomers commented Nov 11, 2017

bors bot commented Nov 11, 2017

Build succeeded

kamalmarhubi commented Apr 6, 2017 •

edited

Loading

kamalmarhubi commented Apr 6, 2017 •

edited

Loading

geofft commented Apr 18, 2017 •

edited

Loading