Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use memfd_create when available #105178
Use memfd_create when available #105178
Changes from 1 commit
15e4957
b946a15
78da311
56a091d
74adb28
d2ec4c9
67ca808
6a93c49
b8e89bc
4425b0e
4c3b65b
f69b0e4
26f991e
75faa79
22be83c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the information in flags and perms isn't factored in here, where does it get incorporated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memfd_create
flags do not have direct equivalents for read-only or read-write permissions. The flags used withmemfd_create
are mainly related to file descriptor behavior (e.g., closing on exec and allowing sealing), not the memory protection levels. Therefore, it makes sense to keepMFD_CLOEXEC
hardcoded in C.It was missing
mmap
call to set the protection, which I have just added. Inheritance is set the same way as withshm_open
(default: CLOEXEC, clear flag if Inheritable is requested from line 244).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If flags and perms aren't relevant to the if block, should they be moved to the else block? They're only ever used there. I realize it's inside of a retry loop, but we expect retries to be rare bordering on non-existent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, would there be any hardening benefits to using seals as a stand-in for what perms was being used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using
shm_open
with read-only permissions (e.g.,O_RDONLY
) and mapping it withmmap
with read-only protections (e.g.,PROT_READ
), the resulting memory mapping will not allow writing through that specific file descriptor and mapping. However, if another process has opened the same shared memory object with read-write permissions (e.g.,O_RDWR
), it can still write to the shared memory, and those changes will be visible to the read-only mappings.With
memfd_create
there is no protection on fd by default. We canwrite(fd)
unless we implement write sealing: am11@f421782. This will make it readonly for current process (same asshm_open
) as well as other processes (different thanshm_open
).While it is not exactly the drop-in replacement, but I think it is a goodness that we will be more hardened than
shm_open
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, other than the extra syscall, there doesn't appear to be a downside to setting seals and it will help to harden the permissions. I suggest we add it in. At that point, since there's then multiple interop calls involved, having completely separate code paths for memfd_create vs shmopen, including error handling, would seem to make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid
SetSealWrite
P/Invoke call if we call fcntl here. I will test.Current benchmakrs: