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

rennergade · 2024-11-21T04:17:38Z

src/safeposix/vmmap.rs

+    // this is effectively whatever mode the file was opened with
+    // we need this because we shouldnt be able to change filed backed mappings 
+    // to have protections exceeding that of the file
+    fn get_max_prot(&self, cage_id: u64, virtual_fd: u64 -> i32 {


I fixed this function, we need to change the rest to use it correctly.

Added comments

Feat exec exit implementation

mmap/munmap implementation

rennergade · 2024-12-05T22:41:34Z

Ported to monorepo

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

lind and others added 8 commits November 8, 2024 19:27

fix: cargo build errors

198cb56

feat: update vmmaap exec_syscall

8fd990d

fix: updated vmmap init

7bb319f

feat: add implementation for exit and exec

5fdc169

fix: rm clear memory call

f1bc365

feat: rever comment

3f95f23

refactor: revert comment

f02d111

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 Outdated 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 and others added 6 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

merge main

cf69c60

mmap/munmap implementation

c240392

update comment

d119e86

fix get_max_prot

7104fea

rennergade reviewed Nov 21, 2024

View reviewed changes

ruchjoshi-nyu and others added 4 commits November 22, 2024 16:47

Added comments

6d600da

feat: add test cases for vmmap

c191b56

Merge remote-tracking branch 'origin/vmmap-alice' into mmap-qianxi

6d492e9

resolved some comments

c90e7b9

ChinmayShringi mentioned this pull request Nov 25, 2024

Porting VMMAP to RawPOSIX Lind-Project/lind-wasm#56

Open

6 tasks

rennergade and others added 5 commits November 25, 2024 21:41

Merge pull request #97 from Lind-Project/vmmap-with-added-comments

700ea92

Added comments

Merge pull request #90 from Lind-Project/feat-exec-exit-implementation

5f77427

Feat exec exit implementation

resolved some comments

7dee68e

merged from alice-mmap

e1d98dc

Merge pull request #96 from Lind-Project/mmap-qianxi

df5419c

mmap/munmap implementation

rennergade closed this Dec 5, 2024

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

Porting VMMAP to RawPOSIX [REVIEW for comments] #87

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

Choose a reason for hiding this comment

rennergade commented Dec 5, 2024

ruchjoshi-nyu Nov 13, 2024 •

edited

Loading