Porting VMMAP to RawPOSIX [REVIEW for comments] #87

Yaxuan-w · 2024-11-04T20:54:17Z

Description

Issue # 45

Type of change

Porting @pranav-bhatt and @ruchjoshi-nyu implementation of VMMAP to RawPOSIX. Request review for porting location and if more comments are needed.

I also added few comments to make things more clear. That would be great if @ruchjoshi-nyu could review their correctness.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Checklist:

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been added to a pull request and/or merged in other modules (native-client, lind-glibc, lind-project)

rennergade · 2024-11-04T23:29:44Z

src/safeposix/vmmap_constants.rs

@@ -0,0 +1,32 @@
+pub const PROT_READ: i32 = 0x1; /* Page can be read.  */


a lot of these are constants that will be used elsewhere, we probably want to figure out how to structure constants in general for RawPOSIX going forward

rennergade · 2024-11-04T23:30:48Z

src/safeposix/vmmap.rs

+    FileDescriptor(u64), // stores file descriptor addr
+}
+
+/// In the old native client based vmmap, we relied on the fd, shmid


I'm not sure where this comment pertains to.

rennergade · 2024-11-04T23:31:52Z

src/safeposix/vmmap.rs

+    pub removed: bool,    /* flag set in fn Update(); */
+    pub file_offset: i64, /* offset into desc */
+    pub file_size: i64,   /* backing store size */
+    pub cage_id: u64,


I think a note here for why we store the cageid here would be good. I believe its because we need it for when we need to check file protections for a memory backing.

I suppose this is fine for now, but in the future, it may be that there are multiple cage_ids able to simultaneously access a part of memory. Perhaps add a comment indicating this may need to be rethought later, unless there is a simple structural change which would make sense in this case...

This is the cage_id to refer to for file backings. That probably should be noted like that.

If we wanted multiple cages to access the same memory, we'd probably have to keep the cage id per file backed memory, though doing something like that would probably require rethinking the whole vmmap.

rennergade · 2024-11-04T23:32:36Z

src/safeposix/vmmap.rs

+        };
+    }
+
+    fn max_prot(&self) -> i32 {


I believe this is the function to find the max protection from a file backed memory region, and is currently incomplete? @pranav-bhatt @ruchjoshi-nyu can you confirm?

Yes, I believe it is incomplete

change this function name to get_max_prot()

@ruchjoshi-nyu change args to (self, cageid: u64, fd, u64)

@Yaxuan-w can you change the contents of this function to call into fdtables with the cageid and fd, and return the file protection of that fd.

Args and function definition changed

rennergade · 2024-11-04T23:33:04Z

src/safeposix/vmmap.rs

+    // memory that has no memory backing
+    // things that are backed by fd -> represented by -1
+
+    // Leave todo


or maybe this is? I'm not sure what the difference is here?

rennergade · 2024-11-04T23:34:15Z

src/safeposix/vmmap.rs

+    fn debug() {}
+}
+
+impl VmmapOps for Vmmap {


all of the functions here need comments explaining generally what they do, what the arguments are, and what they return

rennergade

It looks like a great start. We need more comments on the individual functions and a bit more clarity.

@ruchjoshi-nyu Can you help with this? Should be a priority.

rennergade · 2024-11-04T23:35:48Z

@qianxichen233 Can you give this review as well? mostly pointing out what needs clarification for now

qianxichen233 · 2024-11-05T08:54:16Z

src/safeposix/vmmap.rs

+
+    fn find_map_space(&self, num_pages: u32, pages_per_map: u32) -> Option<Interval<u32>> {
+        let start = self.first_entry();
+        let end = self.last_entry();


Looks like this is using the existing entry in vmmap to determine the current avaliable memory range. How should the memory initialization supposed to happen then? for example, if I want to tell vmmap that 0-2^20 pages are valid page and you should find space within the range, do I need to insert two empty entries (i.e. 0 num_pages entry) with page_num set to 0 and 2^20 respectively?

@pranav-bhatt this is a question were wondering about

ok it seems like we need to change this to initialize in some way.

qianxichen233 · 2024-11-05T08:55:33Z

src/safeposix/vmmap.rs

+        None
+    }
+
+    fn find_map_space(&self, num_pages: u32, pages_per_map: u32) -> Option<Interval<u32>> {


what is pages_per_map doing here

It's used to align the start and end of potential memory gaps to specific boundaries, to make sure that the found space is a multiple of pages_per_map and that it is large enough to accommodate the rounded-up num_pages

I wonder in which situation we want to find a map space that is multiple of pages_per_map(>1) pages, instead of just multiple of 1 page. Is this something also in nacl? @rennergade

pages_per_map probably should be a constant for the min page multiple size we can use for maps in wasm

qianxichen233 · 2024-11-05T09:00:16Z

src/safeposix/vmmap.rs

+
+                let gap_size = aligned_end_page - aligned_start_page;
+                if gap_size >= rounded_num_pages {
+                    return Some(ie(aligned_end_page - rounded_num_pages, aligned_end_page));


why return the page from the end instead of the start? For sorting performance?

may need to change this to return from start, need to figure out how to configure intervals

qianxichen233 · 2024-11-05T09:07:37Z

src/safeposix/vmmap.rs

+            // pages x to y, y included
+            ie(
+                vmmap_entry_ref.page_num,
+                vmmap_entry_ref.page_num + vmmap_entry_ref.npages,


passing an vmmap entry with npages equals to 0 would cause panic here. If npages is not supposed to be 0, then there should be a check at vmmap entry creation or here to prevent from this.

ruchjoshi-nyu · 2024-11-05T16:33:35Z

Thanks for the comments, I'll get on it

Yaxuan-w · 2024-11-11T15:39:40Z

src/safeposix/vmmap.rs

+        };
+    }
+
+    // Placeholder method to get maximum protection (currrently incomplete)


That would be great to have a more detailed comments here, like what kind of feature it suppose to achieve, what currently finished, and what's waiting to do

Yaxuan-w · 2024-11-11T15:41:10Z

src/safeposix/vmmap.rs

+        None
+    }
+
+    // Method to find space above a hint


Need a more detailed description of this function

Yaxuan-w · 2024-11-11T15:42:00Z

src/safeposix/vmmap.rs

+        None
+    }
+
+    // Method to find map space with a hint, rounding page numbers up to the nearest multiple of pages_per_map


Need a more descriptive comment

rennergade · 2024-11-11T22:03:22Z

src/safeposix/vmmap.rs

+        page_num: u32,
+        npages: u32,
+        prot: i32,
+        maxprot: i32,


I think we can just set this on create since it shouldnt be able to be modified after

rennergade · 2024-11-11T22:04:21Z

src/safeposix/vmmap.rs

+            page_num,
+            npages,
+            prot,
+            maxprot,


so instead of maxprot being a parameter here it should be like

if backing == file { maxprot = getmaxprot()

rennergade · 2024-11-11T23:03:40Z

src/interface/mod.rs

@@ -1,7 +1,3 @@
-// Author: Nicholas Renner


can we move all the removing commented out code in these files to a different pr?

src/safeposix/vmmap.rs

rennergade · 2024-11-12T23:25:05Z

src/safeposix/vmmap.rs

+        if start == None || end == None {
+            return None;
+        } else {
+            let start_unwrapped = start.unwrap().0.start();


need to walk through the logic here, should be from end of entry to start of next, but also need to configure 0 and 4GB as endpoints

Updated arguments -> self, cageid, fd

Yaxuan-w added 2 commits November 4, 2024 12:59

porting vmmap

f81ca4b

porting to dispatcher

ff4db4f

Yaxuan-w requested review from JustinCappos, rennergade, ChinmayShringi, yashaswi2000, yzhang71, qianxichen233, robinyuan1002 and ruchjoshi-nyu November 4, 2024 20:54

rennergade reviewed Nov 4, 2024

View reviewed changes

rennergade requested changes Nov 4, 2024

View reviewed changes

qianxichen233 reviewed Nov 5, 2024

View reviewed changes

Added more comments to vmmap.rs

a2c3e1f

Yaxuan-w commented Nov 11, 2024

View reviewed changes

rennergade reviewed Nov 11, 2024

View reviewed changes

src/safeposix/vmmap.rs Show resolved Hide resolved

rennergade reviewed Nov 11, 2024

View reviewed changes

src/safeposix/vmmap.rs Outdated Show resolved Hide resolved

rennergade reviewed Nov 12, 2024

View reviewed changes

ruchjoshi-nyu added 2 commits November 13, 2024 10:26

Changed max_prot function definition to get_max_prot

66737e6

Updated arguments -> self, cageid, fd

Updated vmmap.rs as per comments

429d91f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting VMMAP to RawPOSIX [REVIEW for comments] #87

Porting VMMAP to RawPOSIX [REVIEW for comments] #87

Yaxuan-w commented Nov 4, 2024

rennergade Nov 4, 2024

rennergade Nov 4, 2024

rennergade Nov 4, 2024

JustinCappos Nov 5, 2024

rennergade Nov 7, 2024

rennergade Nov 4, 2024

ruchjoshi-nyu Nov 6, 2024

rennergade Nov 11, 2024

rennergade Nov 11, 2024

rennergade Nov 11, 2024

ruchjoshi-nyu Nov 13, 2024 •

edited

Loading

rennergade Nov 4, 2024

rennergade Nov 4, 2024

rennergade left a comment

rennergade commented Nov 4, 2024

qianxichen233 Nov 5, 2024

rennergade Nov 11, 2024

rennergade Nov 12, 2024

qianxichen233 Nov 5, 2024

ruchjoshi-nyu Nov 11, 2024

qianxichen233 Nov 11, 2024

rennergade Nov 12, 2024

qianxichen233 Nov 5, 2024

rennergade Nov 12, 2024

qianxichen233 Nov 5, 2024

ruchjoshi-nyu commented Nov 5, 2024

Yaxuan-w Nov 11, 2024

Yaxuan-w Nov 11, 2024

Yaxuan-w Nov 11, 2024

rennergade Nov 11, 2024

rennergade Nov 11, 2024

rennergade Nov 11, 2024

rennergade Nov 13, 2024

rennergade Nov 12, 2024

		@@ -0,0 +1,32 @@
		pub const PROT_READ: i32 = 0x1; /* Page can be read. */

Porting VMMAP to RawPOSIX [REVIEW for comments] #87

Are you sure you want to change the base?

Porting VMMAP to RawPOSIX [REVIEW for comments] #87

Conversation

Yaxuan-w commented Nov 4, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

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

ruchjoshi-nyu Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rennergade left a comment

Choose a reason for hiding this comment

rennergade commented Nov 4, 2024

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

ruchjoshi-nyu commented Nov 5, 2024

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

ruchjoshi-nyu Nov 13, 2024 •

edited

Loading