Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

libct: don't send config to nsexec when joining an existing timens #4636

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Feb 19, 2025

Fix #4635

When we exec a process in a container has private timens, we need to
join the init process's timens path, so, we should not send the timens
config to nsexec, otherwise, runc will try to update the process's time
ns configuration when joining the timens path.
We should configure the process's timens offset only when we need to
create new time namespace, we shouldn't do it if we are joining an
existing time namespace.

@kolyshkin kolyshkin added the backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 label Feb 21, 2025
@kolyshkin
Copy link
Contributor

Nice find, nice fix, thanks! Just a single nit about the test case.

Also, we might need to backport it to 1.2.

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.

Unless I'm mistaken this will stop us from detecting broken configurations (i.e. configurations with timens offsets configured but not joining a time namespace). Of course it's easy to add a step to the validator but the previous version already did validation by getting an error from the kernel if appropriate.

Also, it is theoretically possible for someone to want to join a timens that hasn't been used yet and so configuring a timens offset of a timens we didn't create (while a little weird) is perfectly valid.

Personally, I think the old behaviour was better -- the user gets the same error from the kernel they would get if they tried to apply an incorrect configuration manually.

Signed-off-by: lifubang <lifubang@acmcoder.com>
We should configure the process's timens offset only when we need to
create new time namespace, we shouldn't do it if we are joining an
existing time namespace. (opencontainers#4635)

Signed-off-by: lifubang <lifubang@acmcoder.com>
@lifubang lifubang changed the title libct/nsexec: don't update timens offset when joining a time ns libct: don't send config to nsexec when joining an existing timens Feb 22, 2025
@lifubang
Copy link
Member Author

Unless I'm mistaken this will stop us from detecting broken configurations (i.e. configurations with timens offsets configured but not joining a time namespace). Of course it's easy to add a step to the validator but the previous version already did validation by getting an error from the kernel if appropriate.

Also, it is theoretically possible for someone to want to join a timens that hasn't been used yet and so configuring a timens offset of a timens we didn't create (while a little weird) is perfectly valid.

Personally, I think the old behaviour was better -- the user gets the same error from the kernel they would get if they tried to apply an incorrect configuration manually.

Yes, make sense, I changed the logic to the same way as USERNS.

// write namespace paths only when we are not joining an existing user ns
_, joinExistingUser := nsMaps[configs.NEWUSER]
if !joinExistingUser {

@kolyshkin kolyshkin added this to the 1.3.0-rc.1 milestone Feb 25, 2025
if c.config.TimeOffsets != nil {
// write boottime and monotonic time ns offsets only when we are not joining an existing time ns
_, joinExistingTime := nsMaps[configs.NEWTIME]
if !joinExistingTime && c.config.TimeOffsets != nil {
Copy link
Member

Choose a reason for hiding this comment

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

It is theoretically possible someone will want to be able to do this, but I guess it's a fairly esoteric thing.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rata rata merged commit 352c8d4 into opencontainers:main Feb 26, 2025
34 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
backport/1.2-todo A PR in main branch which needs to be backported to release-1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't exec into a container with private time namespace
4 participants