Add support for vectorized read with libc::readv #4084

shamb0 · 2024-12-10T16:24:24Z

Add POSIX readv() System Call Support to MIRI

This PR implements emulation support for libc::readv(), a POSIX vectored I/O operation that allows reading data into multiple buffers with a single system call. This enhancement addresses issue #4048.

RalfJung

I'll assume this should not be reviewed yet since it is marked as a draft. I just noticed this one small thing. ;)

I'd recommend to make a first PR with just readv. Smaller PRs are always easier to handle. Please write @rustbot ready and remove the "draft" status once you want a review.

RalfJung · 2024-12-12T16:59:48Z

src/shims/unix/foreign_items.rs

+                let [fd, iov, iovcnt] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
+                let fd = this.read_scalar(fd)?.to_i32()?;
+                let iovcnt = this.read_scalar(iovcnt)?.to_i32()?;
+                this.readv(fd, iov, iovcnt as _, None, dest)?;


Please don't use as casts. Why are you casting here anyway?

Hi @RalfJung,

Thank you for pointing this out—you're absolutely right that the casting is unnecessary. I appreciate your feedback.

As you noted, the PR is still in draft status, and there are some cleanups I plan to address. I shared it at this stage primarily to seek input, particularly regarding the issue we previously discussed here: Debugging Scalar Size Mismatch Error in Rust Miri.

Thank you again for reviewing and sharing your thoughts! :)

RalfJung · 2024-12-17T06:42:39Z

As you noted, the PR is still in draft status, and there are some cleanups I plan to address. I shared it at this stage primarily to seek input, particularly regarding the issue we previously discussed here: Debugging Scalar Size Mismatch Error in Rust Miri.

Ah, that was not clear at all. Please note things like that in the PR description.

Anyway judging from Zulip it seems you made progress there. I'm going to wait until you ask for further input on this PR. :)

shamb0 · 2024-12-17T10:03:52Z

Hi @RalfJung,

I've completed the initial implementation of readv() support and would appreciate your review. The changes focus on three main areas:

Core Implementation
- Added readv() emulation in src/shims/unix/fd.rs
- Designed to comply with POSIX vectored I/O specifications
Safety Enhancement
- Introduced new deref_pointer_and_offset_vectored() function in src/helpers.rs
- Implements specialized array bounds checking for vectored I/O operations
Comprehensive Testing
- Created test suite in tests/pass-dep/libc/libc-fs.rs covering:
  - Large buffer operations with pattern verification (64KB total)
  - Partial reads and EOF handling
  - Error condition validation
- All tests pass with MIRIFLAGS=-Zmiri-disable-isolation

Looking forward to your feedback, particularly on the new array bounds checking approach and test coverage strategy. :)

shamb0 · 2024-12-17T10:04:25Z

@rustbot ready

RalfJung · 2024-12-18T07:21:26Z

src/helpers.rs

+    /// where we need to access multiple independent memory regions, such as when processing an array
+    /// of iovec structures. Unlike simple pointer arithmetic bounds checking, this implementation
+    /// understands and validates array-based access patterns.
+    fn deref_pointer_and_offset_vectored(


This is a very specific operation for a particular Linux/POSIX API. I don't think we need a global helper for this, so please move it into the only file where it is actually used.

It's also completely unclear to me why you need a complicated new check here. readv just iterates the array and does a read call for each element, right?

Thank you for your review comment regarding the placement and necessity of the deref_pointer_and_offset_vectored function. I'd like to explain the reasoning behind this implementation and agree with your suggestion about its placement.

The implementation of readv() requires handling a base pointer to an array of iovec structures (iov_ptr: &OpTy<'tcx>) along with a count (iovcnt: i32). When reconstructing array elements from this base pointer, I encountered limitations with the existing deref_pointer_and_offset() API, specifically triggering assertions that weren't suitable for our array-based access pattern.

You can find the complete MIRI backtrace demonstrating these limitations here: https://gist.github.com/shamb0/01491a8367c78404769b3e210ec860e0

While the boundary checking logic may appear complex, it's necessary for safely handling array reconstruction in this context. However, I agree that this functionality is specific to the readv implementation and doesn't need to be a global helper. I've moved the function to src/shims/unix/fd.rs where it's being used, making its purpose and context clearer.

RalfJung · 2024-12-18T07:21:43Z

src/shims/unix/fd.rs

+        fd_num: i32,
+        iov_ptr: &OpTy<'tcx>,
+        iovcnt: i32,
+        offset: Option<i128>,


This seems to be always None, so please remove the argument.

I’ve removed the offset parameter from the readv function signature as requested.

Initially, I had included it to align with the parameter structure of the existing read and write functions, given their similarity to file operations. Let me know if there’s anything else you’d like adjusted!"

RalfJung · 2024-12-18T07:22:27Z

src/shims/unix/fd.rs

+        // Early returns for empty or invalid cases
+        if iovcnt == 0 {


Is this justified by the docs? Also, what does the "or invalid" mean here?

I've updated the documentation to clarify the behavior of readv when iovcnt is 0. The updated documentation now explicitly covers:

The function's return value semantics based on the POSIX specification

The meaning of various error conditions and their corresponding return codes

Special cases like empty buffer lists (iovcnt = 0)

RalfJung · 2024-12-18T07:22:45Z

src/shims/unix/fd.rs

+            trace!("readv: FD not found");
+            return this.set_last_error_and_return(LibcError("EBADF"), dest);
+        };
+        trace!("readv: FD mapped to {fd:?}");


Please remove the debug tracing from the PR for review.

RalfJung · 2024-12-18T07:25:18Z

src/shims/unix/fd.rs

+        // We need temporary storage for each individual read operation's result
+        // Using an intermediate buffer helps handle error conditions cleanly
+        // We use i128 to safely handle both success (positive) and error (-1) cases
+        let read_dest = this.allocate(this.machine.layouts.i128, MiriMemoryKind::Machine.into())?;


I don't understand why you are allocating new storage here. What I would expect is that you call read once for each element of the vector, and directly put the results into the user-provided buffer. Yes that means we are using more than one syscall, but we don't really care about such details here -- the only thing that matters is the end-to-end behavior.

Ah it seems the docs say that the entire read must be atomic. Please fix the comment to explain why you are allocating a buffer here; currently you say something about error conditions but it's not clear what this means and it misses the main point.

I've cleaned up the code and removed the problematic logic that attempted to handle reads individually.

The new implementation will focus on ensuring atomicity by performing a single read operation into a contiguous buffer, as required by POSIX readv() semantics.

RalfJung · 2024-12-18T07:30:58Z

src/shims/unix/fd.rs

+        let mut current_offset = offset;
+
+        // Process each iovec structure
+        for i in 0..iovcnt {


You can't use a loop of reads, the read must be atomic. I thought that's why you are allocating a new buffer.

I've cleaned up the code and removed the problematic logic that attempted to handle reads individually.

The new implementation will focus on ensuring atomicity by performing a single read operation into a contiguous buffer, as required by POSIX readv() semantics.

RalfJung · 2024-12-18T07:32:01Z

src/shims/unix/fd.rs

+                this.read_scalar(&read_dest)?.to_i128()?
+            } else {
+                // Handle regular read case
+                fd.read(&fd, this.machine.communicate(), iov_base_ptr, iov_len, &read_dest, this)?;


This unfortunately does not work if read blocks. The code after read will run too early, before the read actually completed.

Implementing this properly for blocking read is very complicated and will require some non-trivial refactoring of the file description trait.

Thank you for highlighting the critical issue regarding blocking reads and concurrent access. This was an oversight in my initial implementation that could have led to race conditions.

I've refined the implementation, with a focus on atomic operations. The key improvement is the introduction of a new read_buffer interface in the FileDescription trait:

impl FileDescription for FileHandle { fn read_buffer<'tcx>( &self, self_ref: &FileDescriptionRef, communicate_allowed: bool, buf: &mut [u8], dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { // Implementation details } }

The revised strategy for readv() now follows these steps:

Allocates an intermediate byte buffer for the entire read operation

Performs a single atomic read operation into this buffer

Distributes the data from the intermediate buffer to the user's scattered iovec buffers

This approach ensures thread safety while maintaining POSIX compliance for atomic vectored I/O operations. I would appreciate your thoughts on this implementation and welcome any suggestions for further improvements.

RalfJung · 2024-12-18T07:33:06Z

You have CI failures. Please make sure CI is green before asking for review; that avoids reviewers having to spend time on things that CI can check automatically. If you don't understand why CI fails, ask for help.

shamb0 · 2024-12-20T07:32:39Z

You have CI failures. Please make sure CI is green before asking for review; that avoids reviewers having to spend time on things that CI can check automatically. If you don't understand why CI fails, ask for help.

You're absolutely correct about the importance of ensuring all CI checks pass before requesting a review. Moving forward, I will:

Wait for CI builds to successfully complete across all supported HOST_TARGET configurations.
Use rustbot ready only after confirming a green CI status.

- This enables vectorized reads. Signed-off-by: shamb0 <[email protected]>

shamb0 force-pushed the support-libc-readv-writev branch 2 times, most recently from 195ddfe to 144a986 Compare December 10, 2024 16:45

RalfJung reviewed Dec 12, 2024

View reviewed changes

shamb0 force-pushed the support-libc-readv-writev branch from 144a986 to 279090c Compare December 17, 2024 09:47

shamb0 marked this pull request as ready for review December 17, 2024 10:03

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Dec 17, 2024

shamb0 changed the title ~~Add support for vectorized read/write with libc::readv and libc::writev~~ Add support for vectorized read/ with libc::readv Dec 17, 2024

shamb0 changed the title ~~Add support for vectorized read/ with libc::readv~~ Add support for vectorized read with libc::readv Dec 17, 2024

RalfJung reviewed Dec 18, 2024

View reviewed changes

RalfJung added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Dec 18, 2024

shamb0 force-pushed the support-libc-readv-writev branch 3 times, most recently from b99627c to d1e7023 Compare December 20, 2024 07:19

shamb0 force-pushed the support-libc-readv-writev branch from d1e7023 to 102f366 Compare December 20, 2024 09:22

Add support for libc::readv

222bac2

- This enables vectorized reads. Signed-off-by: shamb0 <[email protected]>

shamb0 force-pushed the support-libc-readv-writev branch from 102f366 to 222bac2 Compare December 21, 2024 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for vectorized read with libc::readv #4084

Add support for vectorized read with libc::readv #4084

shamb0 commented Dec 10, 2024 •

edited

Loading

RalfJung left a comment

RalfJung Dec 12, 2024

shamb0 Dec 17, 2024

RalfJung commented Dec 17, 2024

shamb0 commented Dec 17, 2024

shamb0 commented Dec 17, 2024

RalfJung Dec 18, 2024

shamb0 Dec 19, 2024 •

edited

Loading

RalfJung Dec 18, 2024

shamb0 Dec 20, 2024

RalfJung Dec 18, 2024

shamb0 Dec 20, 2024

RalfJung Dec 18, 2024

shamb0 Dec 19, 2024

RalfJung Dec 18, 2024

RalfJung Dec 18, 2024

shamb0 Dec 19, 2024

RalfJung Dec 18, 2024

shamb0 Dec 19, 2024

RalfJung Dec 18, 2024 •

edited

Loading

shamb0 Dec 19, 2024 •

edited

Loading

RalfJung commented Dec 18, 2024

shamb0 commented Dec 20, 2024

Add support for vectorized read with libc::readv #4084

Are you sure you want to change the base?

Add support for vectorized read with libc::readv #4084

Conversation

shamb0 commented Dec 10, 2024 • edited Loading

Add POSIX readv() System Call Support to MIRI

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 17, 2024

shamb0 commented Dec 17, 2024

shamb0 commented Dec 17, 2024

Choose a reason for hiding this comment

shamb0 Dec 19, 2024 • edited Loading

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

RalfJung Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

shamb0 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Dec 18, 2024

shamb0 commented Dec 20, 2024

shamb0 commented Dec 10, 2024 •

edited

Loading

shamb0 Dec 19, 2024 •

edited

Loading

RalfJung Dec 18, 2024 •

edited

Loading

shamb0 Dec 19, 2024 •

edited

Loading