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

drop runc-dmz solution according to overlay solution #4482

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

lifubang
Copy link
Member

Because we have the overlay solution, we can drop runc-dmz binary solution since it has too many limitations.

The original post is here: #4450 (comment)

Because of there are many commits about runc-dmz binary solution, so doing git revert action is very hard, let's drop these code line by line.

@AkihiroSuda AkihiroSuda added this to the 1.3.0 milestone Oct 28, 2024
README.md Show resolved Hide resolved
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

One minor nit.

Comment on lines +3 to +5
`runc` normally has to make a binary copy of itself when constructing a
container process in order to defend against certain container runtime attacks
such as CVE-2019-5736.
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I forgot to document how the new overlay mode works in #4448. I'll open a separate PR for that.

Copy link

Choose a reason for hiding this comment

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

this is more of a discussion, so with the overlay change i guess memfd-bind is no longer needed? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is being discussed in #4450 (comment). Short answer: it has fewer upsides now and there is a fairly strong case for removing it.

libcontainer/container_linux.go Outdated Show resolved Hide resolved
@lifubang lifubang force-pushed the drop-dmz-binary branch 4 times, most recently from fad48fe to 330755c Compare October 28, 2024 13:39

The following build tags were used earlier, but are now obsoleted:
- **runc_nodmz** (since runc v1.2.1 runc dmz binary is dropped)
Copy link
Member

Choose a reason for hiding this comment

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

(Considering the amount of the changes I was wondering if this is going to be v1.3.0, but probably safe to cherrypick to v1.2.1, as dmz was experimental and opt-in)

Copy link
Member

Choose a reason for hiding this comment

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

Probably https://github.com/opencontainers/runc/blob/main/docs/experimental.md should be updated to reflect the history

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to drop runc-dmz now since no one is using runc_dmz (yet). Adding a backport label.

Because we have the overlay solution, we can drop runc-dmz binary
solution since it has too many limitations.

Signed-off-by: lifubang <[email protected]>
@kolyshkin kolyshkin added the backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 label Oct 28, 2024
@cyphar cyphar merged commit 68bef80 into opencontainers:main Oct 29, 2024
40 checks passed
@lifubang lifubang added backport/1.2-done A PR in main branch which has been backported to release-1.2 and removed backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 labels Oct 29, 2024
@lifubang lifubang deleted the drop-dmz-binary branch October 29, 2024 09:54
@kolyshkin kolyshkin removed this from the 1.2.1 milestone Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2-done A PR in main branch which has been backported to release-1.2 impact/changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants