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

defer creation of sysctl until after all namespaces have been created #983

Closed
wants to merge 1 commit into from

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Aug 5, 2022

there is a bug in rootless podman that does not allow users to set
kernel.domainname because the uts namespace is not set up before the sysctl's are
added.

I moved the libcrun_set_sysctl down to a point where i think all of the namespaces have been created

Signed-off-by: Charlie Doern [email protected]

@cdoern
Copy link
Contributor Author

cdoern commented Aug 5, 2022

@flouthoc @giuseppe PTAL, I am not even sure if this is the right place to do this.. but thought I'd try it on a hunch. see containers/podman#15200 for more info of why this is needed

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

How does runc does it, i am not sure but upon quick look it looks like it does it much early while processing the init https://github.com/opencontainers/runc/blob/main/libcontainer/standard_init_linux.go#L133

@flouthoc
Copy link
Collaborator

flouthoc commented Aug 5, 2022

Maybe hack would be to only process kernel.domainname after UTS namespace is configured and other sysctls can stay as is.

@flouthoc
Copy link
Collaborator

flouthoc commented Aug 5, 2022

I think domainname could be part of OCI spec and let runtime handle it in a special case but that looks like a longer route.

@flouthoc
Copy link
Collaborator

flouthoc commented Aug 5, 2022

@giuseppe WDYT ?

@cdoern
Copy link
Contributor Author

cdoern commented Aug 5, 2022

@flouthoc can you point out where the UTS NS is configured here? I only see references to userNS. this is my first time really looking around crun

@cdoern cdoern force-pushed the utsns-sysctl branch 2 times, most recently from f30b3b2 to 737b32b Compare August 5, 2022 20:21
@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2022

This pull request introduces 1 alert when merging 737b32b into b66340c - view on LGTM.com

new alerts:

  • 1 for Dead code due to goto or break statement

@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2022

This pull request introduces 1 alert when merging ea16be0 into b66340c - view on LGTM.com

new alerts:

  • 1 for Dead code due to goto or break statement

there is a bug in rootless podman that does not allow users to set
kernel.domainname because the uts namespace is not set up before the sysctl's are
added.

I moved the libcrun_set_sysctl down to a point where i think all of the namespaces have been created

I also made a new function libcrun_set_additional_sysctl in which sysctls that need the namespaces set up are added later

Signed-off-by: Charlie Doern <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2022

This pull request introduces 1 alert when merging 87d102e into b66340c - view on LGTM.com

new alerts:

  • 1 for Dead code due to goto or break statement

@flouthoc
Copy link
Collaborator

flouthoc commented Aug 6, 2022

@flouthoc can you point out where the UTS NS is configured here? I only see references to userNS. this is my first time really looking around crun

@cdoern I think this might be better workaround as suggested in patch here containers/podman#15200 (comment) but modifying OCI spec to support domainname might be the best fix.

@giuseppe
Copy link
Member

giuseppe commented Aug 6, 2022

let's close this PR for now, I don't think this approach can solve the issue we have seen

@giuseppe giuseppe closed this Aug 6, 2022
@cdoern
Copy link
Contributor Author

cdoern commented Aug 6, 2022

Ok, I'll work on the OCI spec approach

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