Skip to content
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

Merged
merged 15 commits into from
Aug 20, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 20, 2024

Closes #92905

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2024
@am11 am11 force-pushed the feature/system.io.mmf/unix-memfdcreate branch 2 times, most recently from a1520ca to 972fb0d Compare July 20, 2024 10:50
@am11
Copy link
Member Author

am11 commented Jul 20, 2024

Armel failure is unrelated, fixed by dotnet/dotnet-buildtools-prereqs-docker#1141.

fd = Interop.Sys.ShmOpen(mapName, flags, (int)perms); // Create the shared memory object.
if (Interop.Sys.MemfdSupported)
{
fd = Interop.Sys.MemfdCreate(mapName);
Copy link
Member

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?

Copy link
Member Author

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 with memfd_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 keep MFD_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 with shm_open (default: CLOEXEC, clear flag if Inheritable is requested from line 244).

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

@am11 am11 Jul 22, 2024

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 with mmap 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 can write(fd) unless we implement write sealing: am11@f421782. This will make it readonly for current process (same as shm_open) as well as other processes (different than shm_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?

Copy link
Member

@stephentoub stephentoub Jul 22, 2024

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.

@am11
Copy link
Member Author

am11 commented Jul 22, 2024

CI is using older glibc 2.23 (Ubuntu 16.04), which doesn't have the API.

  -- Looking for memfd_create
  -- Looking for memfd_create - not found

so it's using shm_open fallback.

I'll change it to make syscall directly (which is what libc 2.27 onwards are doing).

@adamsitnik adamsitnik added this to the 10.0.0 milestone Jul 23, 2024
@adamsitnik adamsitnik added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 23, 2024
@adamsitnik
Copy link
Member

@am11 Once again big thanks for your contribution. We have missed the Preview 7 snap by very little and now we unfortunately have to wait until main branch becomes .NET 10.

I added the "NO MERGE" label and set the 10.0 milestone to express that. The PR is good in the current shape, we just need to wait.

Thanks!

@adamsitnik adamsitnik added the blocked Issue/PR is blocked on something - see comments label Jul 23, 2024
@am11 am11 removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Aug 19, 2024
@am11
Copy link
Member Author

am11 commented Aug 19, 2024

Branch is now opened.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you again @am11 !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using memfd_create instead of shm_open in MemoryMappedFile if it's available
7 participants