Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

[LibOS] Error out on anonymous shared mappings in mmap syscall #2147

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mkow
Copy link
Member

@mkow mkow commented Feb 8, 2021

Description of the changes

LibOS can't implement such a combination of flags because PAL doesn't support shared mappings. I'm not sure about the non-anonymous shared case, as it currently works on Linux PAL, but only by an accident? (at least it seems...)

Fixes #2146.

How to test this PR?

Check if mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, -1, 0) fails. And wait for CI to see if this breaks any of our examples.


This change is Reviewable

LibOS can't implement such a combination of flags because PAL doesn't
support shared mappings. I'm not sure about the non-anonymous shared
case, as it currently works on Linux PAL, but only by an accident? (at
least it seems...)

Signed-off-by: Michał Kowalczyk <[email protected]>
Copy link
Contributor

@boryspoplawski boryspoplawski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)


LibOS/shim/src/sys/shim_mmap.c, line 80 at r1 (raw file):

        switch (flags & MAP_TYPE) {
            case MAP_SHARED:
                /* PAL API doesn't support anonymous shared memory. */

It does, see PAL_PROT_WRITECOPY.

boryspoplawski
boryspoplawski previously approved these changes Feb 8, 2021
Copy link
Contributor

@boryspoplawski boryspoplawski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners


LibOS/shim/src/sys/shim_mmap.c, line 80 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

It does, see PAL_PROT_WRITECOPY.

Except PAL_PROT_WRITECOPY is broken by design and probably unfixable...

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" found in commit messages' one-liners (waiting on @mkow)


.ci/lib/stage-clean-check.jenkinsfile, line 24 at r2 (raw file):

        make -C Examples/redis distclean
        make -C Examples/lighttpd distclean
        #make -C Examples/nginx distclean

OMG. Graphene had a silent MAP_SHARED -> MAP_PRIVATE error in Nginx and Apache all these years? And despite this, both Nginx and Apache worked? This is sad.

Upon further inspection in Nginx, I see src/os/unix/ngx_shmem.c: ngx_shm_alloc() which is used in several places in Nginx. I don't see a way to disable shared memory (we can only switch between 3 implementations: NGX_HAVE_MAP_ANON, NGX_HAVE_MAP_DEVZERO and NGX_HAVE_SYSVSHM).

We are doomed :( At least for Nginx, I don't see how we can simply error out on mmap(MAP_ANON | MAP_SHARED). There is no config option in Nginx to disable shared memory.

Copy link
Member Author

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" found in commit messages' one-liners (waiting on @dimakuv)


.ci/lib/stage-clean-check.jenkinsfile, line 24 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

OMG. Graphene had a silent MAP_SHARED -> MAP_PRIVATE error in Nginx and Apache all these years? And despite this, both Nginx and Apache worked? This is sad.

Upon further inspection in Nginx, I see src/os/unix/ngx_shmem.c: ngx_shm_alloc() which is used in several places in Nginx. I don't see a way to disable shared memory (we can only switch between 3 implementations: NGX_HAVE_MAP_ANON, NGX_HAVE_MAP_DEVZERO and NGX_HAVE_SYSVSHM).

We are doomed :( At least for Nginx, I don't see how we can simply error out on mmap(MAP_ANON | MAP_SHARED). There is no config option in Nginx to disable shared memory.

Uh, that's sad...
Do you know if the shared memory was actually used after forking? How could it work if we aren't really allocating it with MAP_SHARED?

If this mapping isn't really used then we could push down the error to the first shared usage by returning a normal mapping but marking it as "remove-on-fork" internally and then remove it after fork (and reserve this region in bookkeeping). This way fork+execve would still work, but fork+using the memory in the child would error out. Cons: requires SGX2 and EDMM.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" found in commit messages' one-liners (waiting on @mkow)


.ci/lib/stage-clean-check.jenkinsfile, line 24 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Uh, that's sad...
Do you know if the shared memory was actually used after forking? How could it work if we aren't really allocating it with MAP_SHARED?

If this mapping isn't really used then we could push down the error to the first shared usage by returning a normal mapping but marking it as "remove-on-fork" internally and then remove it after fork (and reserve this region in bookkeeping). This way fork+execve would still work, but fork+using the memory in the child would error out. Cons: requires SGX2 and EDMM.

From what I understood, Nginx pre-allocates a shared memory region (of a predefined size) for future use by any Nginx modules at runtime. I.e., it's like a scratch space for Nginx processes. So yes, at least in our simple flows, it seems to be unused, so we could use your trick of "push the error down".

I didn't look into Apache code.

@boryspoplawski
Copy link
Contributor

Jenkins, retest this please (just making jenkins load for other tests, unrelated to this PR)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LibOS,PAL] MAP_SHARED flag is not propagated to PAL
3 participants