Only map with executable permissions when using code space (#1176)

This PR closes #7. This PR
* refactors `MmapStrategy` to include protection flags, and allows each
space to pass `Mmapstrategy` to the mmapper.
* removes exec permission from most spaces. Only code spaces will have
exec permission.
* adds a feature `exec_permission_on_all_spaces` for bindings that may
allocate code into normal spaces.

---------

Co-authored-by: Yi Lin <qinsoon@gmail.com>

Author: k-sareen

Cargo.toml

@@ -98,9 +98,12 @@ set_unlog_bits_vm_space = []
 # TODO: This is not properly implemented yet. We currently use an immortal space instead, and do not guarantee read-only semantics.
 ro_space = []
 # A code space with execution permission.
-# TODO: This is not properly implemented yet. We currently use an immortal space instead, and all our spaces have execution permission at the moment.
 code_space  = []
 
+# By default, we only allow execution permission for code spaces. With this feature, all the spaces have execution permission.
+# Use with care.
+exec_permission_on_all_spaces = []
+
 # Global valid object (VO) bit metadata.
 # The VO bit is set when an object is allocated, and cleared when the GC determines it is dead.
 # See `src/util/metadata/vo_bit/mod.rs`

docs/userguide/src/tutorial/code/mygc_semispace/global.rs

@@ -175,9 +175,9 @@ impl<VM: VMBinding> MyGC<VM> {
         let res = MyGC {
             hi: AtomicBool::new(false),
             // ANCHOR: copyspace_new
-            copyspace0: CopySpace::new(plan_args.get_space_args("copyspace0", true, VMRequest::discontiguous()), false),
+            copyspace0: CopySpace::new(plan_args.get_space_args("copyspace0", true, false, VMRequest::discontiguous()), false),
             // ANCHOR_END: copyspace_new
-            copyspace1: CopySpace::new(plan_args.get_space_args("copyspace1", true, VMRequest::discontiguous()), true),
+            copyspace1: CopySpace::new(plan_args.get_space_args("copyspace1", true, false, VMRequest::discontiguous()), true),
             common: CommonPlan::new(plan_args),
         };
 

src/mmtk.rs

@@ -203,10 +203,6 @@ impl<VM: VMBinding> MMTK<VM> {
             },
         );
 
-        if *options.transparent_hugepages {
-            MMAPPER.set_mmap_strategy(crate::util::memory::MmapStrategy::TransparentHugePages);
-        }
-
         MMTK {
             options,
             state,

src/plan/generational/copying/global.rs

@@ -209,11 +209,11 @@ impl<VM: VMBinding> GenCopy<VM> {
         };
 
         let copyspace0 = CopySpace::new(
-            plan_args.get_space_args("copyspace0", true, VMRequest::discontiguous()),
+            plan_args.get_space_args("copyspace0", true, false, VMRequest::discontiguous()),
             false,
         );
         let copyspace1 = CopySpace::new(
-            plan_args.get_space_args("copyspace1", true, VMRequest::discontiguous()),
+            plan_args.get_space_args("copyspace1", true, false, VMRequest::discontiguous()),
             true,
         );
 

src/plan/generational/global.rs

@@ -41,7 +41,7 @@ pub struct CommonGenPlan<VM: VMBinding> {
 impl<VM: VMBinding> CommonGenPlan<VM> {
     pub fn new(mut args: CreateSpecificPlanArgs<VM>) -> Self {
         let nursery = CopySpace::new(
-            args.get_space_args("nursery", true, VMRequest::discontiguous()),
+            args.get_space_args("nursery", true, false, VMRequest::discontiguous()),
             true,
         );
         let full_heap_gc_count = args

src/plan/generational/immix/global.rs

@@ -247,7 +247,7 @@ impl<VM: VMBinding> GenImmix<VM> {
                 crate::plan::generational::new_generational_global_metadata_specs::<VM>(),
         };
         let immix_space = ImmixSpace::new(
-            plan_args.get_space_args("immix_mature", true, VMRequest::discontiguous()),
+            plan_args.get_space_args("immix_mature", true, false, VMRequest::discontiguous()),
             ImmixSpaceArgs {
                 reset_log_bit_in_major_gc: false,
                 // We don't need to unlog objects at tracing. Instead, we unlog objects at copying.

src/plan/global.rs

@@ -396,11 +396,13 @@ impl<'a, VM: VMBinding> CreateSpecificPlanArgs<'a, VM> {
         &mut self,
         name: &'static str,
         zeroed: bool,
+        permission_exec: bool,
         vmrequest: VMRequest,
     ) -> PlanCreateSpaceArgs<VM> {
         PlanCreateSpaceArgs {
             name,
             zeroed,
+            permission_exec,
             vmrequest,
             global_side_metadata_specs: self.global_side_metadata_specs.clone(),
             vm_map: self.global_args.vm_map,
@@ -409,7 +411,7 @@ impl<'a, VM: VMBinding> CreateSpecificPlanArgs<'a, VM> {
             constraints: self.constraints,
             gc_trigger: self.global_args.gc_trigger.clone(),
             scheduler: self.global_args.scheduler.clone(),
-            options: &self.global_args.options,
+            options: self.global_args.options.clone(),
             global_state: self.global_args.state.clone(),
         }
     }
@@ -423,24 +425,28 @@ impl<VM: VMBinding> BasePlan<VM> {
             code_space: ImmortalSpace::new(args.get_space_args(
                 "code_space",
                 true,
+                true,
                 VMRequest::discontiguous(),
             )),
             #[cfg(feature = "code_space")]
             code_lo_space: ImmortalSpace::new(args.get_space_args(
                 "code_lo_space",
                 true,
+                true,
                 VMRequest::discontiguous(),
             )),
             #[cfg(feature = "ro_space")]
             ro_space: ImmortalSpace::new(args.get_space_args(
                 "ro_space",
                 true,
+                false,
                 VMRequest::discontiguous(),
             )),
             #[cfg(feature = "vm_space")]
             vm_space: VMSpace::new(args.get_space_args(
                 "vm_space",
                 false,
+                false, // it doesn't matter -- we are not mmapping for VM space.
                 VMRequest::discontiguous(),
             )),
 
@@ -585,15 +591,17 @@ impl<VM: VMBinding> CommonPlan<VM> {
             immortal: ImmortalSpace::new(args.get_space_args(
                 "immortal",
                 true,
+                false,
                 VMRequest::discontiguous(),
             )),
             los: LargeObjectSpace::new(
-                args.get_space_args("los", true, VMRequest::discontiguous()),
+                args.get_space_args("los", true, false, VMRequest::discontiguous()),
                 false,
             ),
             nonmoving: ImmortalSpace::new(args.get_space_args(
                 "nonmoving",
                 true,
+                false,
                 VMRequest::discontiguous(),
             )),
             base: BasePlan::new(args),

src/plan/immix/global.rs

@@ -150,7 +150,7 @@ impl<VM: VMBinding> Immix<VM> {
     ) -> Self {
         let immix = Immix {
             immix_space: ImmixSpace::new(
-                plan_args.get_space_args("immix", true, VMRequest::discontiguous()),
+                plan_args.get_space_args("immix", true, false, VMRequest::discontiguous()),
                 space_args,
             ),
             common: CommonPlan::new(plan_args),

src/plan/markcompact/global.rs

@@ -195,8 +195,12 @@ impl<VM: VMBinding> MarkCompact<VM> {
             global_side_metadata_specs,
         };
 
-        let mc_space =
-            MarkCompactSpace::new(plan_args.get_space_args("mc", true, VMRequest::discontiguous()));
+        let mc_space = MarkCompactSpace::new(plan_args.get_space_args(
+            "mc",
+            true,
+            false,
+            VMRequest::discontiguous(),
+        ));
 
         let res = MarkCompact {
             mc_space,

src/plan/marksweep/global.rs

@@ -109,6 +109,7 @@ impl<VM: VMBinding> MarkSweep<VM> {
             ms: MarkSweepSpace::new(plan_args.get_space_args(
                 "ms",
                 true,
+                false,
                 VMRequest::discontiguous(),
             )),
             common: CommonPlan::new(plan_args),

src/plan/nogc/global.rs

@@ -98,16 +98,19 @@ impl<VM: VMBinding> NoGC<VM> {
             nogc_space: NoGCImmortalSpace::new(plan_args.get_space_args(
                 "nogc_space",
                 cfg!(not(feature = "nogc_no_zeroing")),
+                false,
                 VMRequest::discontiguous(),
             )),
             immortal: ImmortalSpace::new(plan_args.get_space_args(
                 "immortal",
                 true,
+                false,
                 VMRequest::discontiguous(),
             )),
             los: ImmortalSpace::new(plan_args.get_space_args(
                 "los",
                 true,
+                false,
                 VMRequest::discontiguous(),
             )),
             base: BasePlan::new(plan_args),

src/plan/pageprotect/global.rs

@@ -105,7 +105,7 @@ impl<VM: VMBinding> PageProtect<VM> {
 
         let ret = PageProtect {
             space: LargeObjectSpace::new(
-                plan_args.get_space_args("pageprotect", true, VMRequest::discontiguous()),
+                plan_args.get_space_args("pageprotect", true, false, VMRequest::discontiguous()),
                 true,
             ),
             common: CommonPlan::new(plan_args),

src/plan/semispace/global.rs

@@ -143,11 +143,11 @@ impl<VM: VMBinding> SemiSpace<VM> {
         let res = SemiSpace {
             hi: AtomicBool::new(false),
             copyspace0: CopySpace::new(
-                plan_args.get_space_args("copyspace0", true, VMRequest::discontiguous()),
+                plan_args.get_space_args("copyspace0", true, false, VMRequest::discontiguous()),
                 false,
             ),
             copyspace1: CopySpace::new(
-                plan_args.get_space_args("copyspace1", true, VMRequest::discontiguous()),
+                plan_args.get_space_args("copyspace1", true, false, VMRequest::discontiguous()),
                 true,
             ),
             common: CommonPlan::new(plan_args),

src/policy/largeobjectspace.rs

@@ -212,7 +212,11 @@ impl<VM: VMBinding> LargeObjectSpace<VM> {
         } else {
             FreeListPageResource::new_contiguous(common.start, common.extent, vm_map)
         };
-        pr.protect_memory_on_release = protect_memory_on_release;
+        pr.protect_memory_on_release = if protect_memory_on_release {
+            Some(common.mmap_strategy().prot)
+        } else {
+            None
+        };
         LargeObjectSpace {
             pr,
             common,

src/policy/lockfreeimmortalspace.rs

@@ -232,11 +232,10 @@ impl<VM: VMBinding> LockFreeImmortalSpace<VM> {
         };
 
         // Eagerly memory map the entire heap (also zero all the memory)
-        let strategy = if *args.options.transparent_hugepages {
-            MmapStrategy::TransparentHugePages
-        } else {
-            MmapStrategy::Normal
-        };
+        let strategy = MmapStrategy::new(
+            *args.options.transparent_hugepages,
+            crate::util::memory::MmapProtection::ReadWrite,
+        );
         crate::util::memory::dzmmap_noreplace(start, aligned_total_bytes, strategy).unwrap();
         if space
             .metadata

src/policy/space.rs

@@ -28,7 +28,7 @@ use crate::util::heap::layout::Mmapper;
 use crate::util::heap::layout::VMMap;
 use crate::util::heap::space_descriptor::SpaceDescriptor;
 use crate::util::heap::HeapMeta;
-use crate::util::memory;
+use crate::util::memory::{self, HugePageSupport, MmapProtection, MmapStrategy};
 use crate::vm::VMBinding;
 
 use std::marker::PhantomData;
@@ -137,13 +137,13 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
                     );
                     let bytes = conversions::pages_to_bytes(res.pages);
 
-                    let map_sidemetadata = || {
+                    let mmap = || {
                         // Mmap the pages and the side metadata, and handle error. In case of any error,
                         // we will either call back to the VM for OOM, or simply panic.
                         if let Err(mmap_error) = self
                             .common()
                             .mmapper
-                            .ensure_mapped(res.start, res.pages)
+                            .ensure_mapped(res.start, res.pages, self.common().mmap_strategy())
                             .and(
                                 self.common()
                                     .metadata
@@ -160,7 +160,7 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
                     // The scope of the lock is important in terms of performance when we have many allocator threads.
                     if SFT_MAP.get_side_metadata().is_some() {
                         // If the SFT map uses side metadata, so we have to initialize side metadata first.
-                        map_sidemetadata();
+                        mmap();
                         // then grow space, which will use the side metadata we mapped above
                         grow_space();
                         // then we can drop the lock after grow_space()
@@ -170,7 +170,7 @@ pub trait Space<VM: VMBinding>: 'static + SFT + Sync + Downcast {
                         grow_space();
                         drop(lock);
                         // and map side metadata without holding the lock
-                        map_sidemetadata();
+                        mmap();
                     }
 
                     // TODO: Concurrent zeroing
@@ -421,11 +421,13 @@ pub struct CommonSpace<VM: VMBinding> {
     // the copy semantics for the space.
     pub copy: Option<CopySemantics>,
 
-    immortal: bool,
-    movable: bool,
+    pub immortal: bool,
+    pub movable: bool,
     pub contiguous: bool,
     pub zeroed: bool,
 
+    pub permission_exec: bool,
+
     pub start: Address,
     pub extent: usize,
 
@@ -443,6 +445,7 @@ pub struct CommonSpace<VM: VMBinding> {
 
     pub gc_trigger: Arc<GCTrigger<VM>>,
     pub global_state: Arc<GlobalState>,
+    pub options: Arc<Options>,
 
     p: PhantomData<VM>,
 }
@@ -459,6 +462,7 @@ pub struct PolicyCreateSpaceArgs<'a, VM: VMBinding> {
 pub struct PlanCreateSpaceArgs<'a, VM: VMBinding> {
     pub name: &'static str,
     pub zeroed: bool,
+    pub permission_exec: bool,
     pub vmrequest: VMRequest,
     pub global_side_metadata_specs: Vec<SideMetadataSpec>,
     pub vm_map: &'static dyn VMMap,
@@ -467,7 +471,7 @@ pub struct PlanCreateSpaceArgs<'a, VM: VMBinding> {
     pub constraints: &'a PlanConstraints,
     pub gc_trigger: Arc<GCTrigger<VM>>,
     pub scheduler: Arc<GCWorkScheduler<VM>>,
-    pub options: &'a Options,
+    pub options: Arc<Options>,
     pub global_state: Arc<GlobalState>,
 }
 
@@ -498,6 +502,7 @@ impl<VM: VMBinding> CommonSpace<VM> {
             immortal: args.immortal,
             movable: args.movable,
             contiguous: true,
+            permission_exec: args.plan_args.permission_exec,
             zeroed: args.plan_args.zeroed,
             start: unsafe { Address::zero() },
             extent: 0,
@@ -511,6 +516,7 @@ impl<VM: VMBinding> CommonSpace<VM> {
             },
             acquire_lock: Mutex::new(()),
             global_state: args.plan_args.global_state,
+            options: args.plan_args.options.clone(),
             p: PhantomData,
         };
 
@@ -619,6 +625,21 @@ impl<VM: VMBinding> CommonSpace<VM> {
     pub fn vm_map(&self) -> &'static dyn VMMap {
         self.vm_map
     }
+
+    pub fn mmap_strategy(&self) -> MmapStrategy {
+        MmapStrategy {
+            huge_page: if *self.options.transparent_hugepages {
+                HugePageSupport::TransparentHugePages
+            } else {
+                HugePageSupport::No
+            },
+            prot: if self.permission_exec || cfg!(feature = "exec_permission_on_all_spaces") {
+                MmapProtection::ReadWriteExec
+            } else {
+                MmapProtection::ReadWrite
+            },
+        }
+    }
 }
 
 fn get_frac_available(frac: f32) -> usize {