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

Deprecate old shared memory fd creation #374

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Conversation

Mikopet
Copy link
Contributor

@Mikopet Mikopet commented Nov 29, 2024

During the attempt to fix gaol <> ipc-channel incompatibilities, I was trying to fix the errors first. the memfd_create is fixable on gaol side, but I was not able to fix the shm_open.

I cannot really point out the issue. I suspect some tricky bitflip playing around the usernamespace and permissions, but I have no evidence tbh.


Anyhow, memfd_create is part of the linux kernel from 2014, and also part of FreeBSD from 2021.
I don't see strong reason to keep the older shm_open method.

according to GNU libc manual:

  • shm_open: MT-Safe locale | AS-Unsafe init heap lock | AC-Unsafe lock mem fd
  • memfd_create: MT-Safe | AS-Safe | AC-Safe fd

@@ -26,7 +26,6 @@ harness = false
[features]
default = []
force-inprocess = []
memfd = []
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave the feature with a comment saying it's deprecated. This will allow us to put off having to make a semver-major version change.

Copy link
Contributor Author

@Mikopet Mikopet Nov 30, 2024

Choose a reason for hiding this comment

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

Sort of makes sense, but because its major is <1, semver allows to do anything.
It could be minor or patch version dump too.

However, (I just realize) memfd feature is used in the GHA workflow atm, and that also needs some care.
I'd vote for removing memfd feature completely therefore decreasing unnecessary complexity.

what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to bump anyway, given that this internal change removes support for some old systems.

Copy link
Member

Choose a reason for hiding this comment

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

That position is reasonable. I'll merge this as-is.

@sagudev
Copy link
Member

sagudev commented Nov 30, 2024

BTW for shm_open in gaol, firefox has this in their sandbox code: https://searchfox.org/mozilla-central/source/security/sandbox/linux/Sandbox.cpp#572

@Mikopet
Copy link
Contributor Author

Mikopet commented Nov 30, 2024

BTW for shm_open in gaol, firefox has this in their sandbox code: https://searchfox.org/mozilla-central/source/security/sandbox/linux/Sandbox.cpp#572

I tried it out, but same issue. Tbh I cannot fathom what causes it, but it's also not my field of expertise. (evidently not the syscall itself, as audit doesn't report violation. It has to be something in the environment.)

But as it is major incompatibility, I see no reason to support it. (unless somebody can fix it, then I'm up for it, sure)

@@ -26,7 +26,6 @@ harness = false
[features]
default = []
force-inprocess = []
memfd = []
Copy link
Member

Choose a reason for hiding this comment

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

That position is reasonable. I'll merge this as-is.

@jdm jdm added this pull request to the merge queue Dec 3, 2024
Merged via the queue into servo:main with commit 6b8619f Dec 3, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants