Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @llly)

libos/src/bookkeep/libos_vma.c line 1291 at r1 (raw file):

    int ret = PalVirtualMemoryProtect((void*)vma->begin, vma->end - vma->begin,
                                      LINUX_PROT_TO_PAL(ctx->prot, /*map_flags=*/0));

I think this breaks the whole design of the VMA bookkeeping module. All other bkeep_* functions do only bookkeeping and are supposed to be called before/after the caller did an appropriate PAL API call. Now this single function would work the opposite way.

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

libos/src/bookkeep/libos_vma.c line 1291 at r1 (raw file):

The libos_syscall_mprotect logic can be reworked like

while(bkeep_end < end)
    bkeep_mprotect_one_vma(&bkeep_end, &pal_begin, &pal_end, &old_prot)
    PalVirtualMemoryProtect(pal_begin, pal_end)

In other functions, we only mprotect one VMA and don't need the loop.

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin and @llly)

a discussion (no related file):

In future PR, Pal mprotect will take a new parameter old_prot to extend/retstrict protection accordingly.

Wouldn't it make sense to do smth like this:

1. Introduce a new field in LibOS VMA data structure that reflects current-environment protections.
2. Update this field accordingly in bkeep* functions (I guess only in bkeep_mprotect() this field will be different from prot field).
3. Use a new Pal-to-LibOS callback introduced by @kailun-qin in another EDMM PR, to also provide current-environment permissions.
4. PAL will call the callback on each VMA, and will see if it needs to apply protections or apply only extend/restrict part.
   • This also has the benefit that Linux PAL may not care about current-environment restrictions. Whereas Linux-SGX PAL will have this special new logic.

libos/src/bookkeep/libos_vma.c line 1291 at r1 (raw file):

+1 to @mkow.

The bkeep_* functions are for internal LibOS bookkeeping purposes, and Pal* functions are for external "take-effect" actions.

We do not mix these functions for the main reason: to allow better error handling. Basically, the current VMA subsystem strives to first do all the bookkeeping (first all bkeep_* calls). If some step in the bookkeeping breaks, then LibOS can easily unwind the changes (because they are all internal).

Then the VMA subsystem calls Pal* functions to reflect the internal state of LibOS memory bookkeeping in the Gramine environment (host Linux in case of gramine-direct, SGX enclave in case of gramine-sgx). At the point of first Pal* call, the subsystem basically says "Ok, after that point unwinding the changes will be super-hard or impossible".

I dislike the current rework of the VMA code. It's very different, and it's hard to review.

Why not keep the current implementation of bkeep_mprotect() as is? This will split (if needed) the VMAs, and then we can apply a visitor pattern in libos_syscall_mprotect(), and call corresponding PalVirtualMemoryProtect().

I guess the main problem with the above approach is that at the point of PalVirtualMemoryProtect(), we won't know the previous (= currently applied in the environment) protections? I think we had similar discussions with @kailun-qin during his EDMM work, and we had ideas like keeping the "currently applied" protections in some data structure (e.g. in PAL). Maybe @kailun-qin will suggest something.

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @kailun-qin)

a discussion (no related file):

I'll rework this PR to use callbacks from PAL to LibOS. Since we expect it base on Kailun's EDMM lazy alloc PR, I'll update after Kailun's PR merged.