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

mountinfo: linux: use /proc/thread-self/mountinfo #128

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Aug 24, 2023

In a Go program with many threads, which goroutine is running on the thread-group leader thread is not something programs can controls. Thus, even if you use runtime.LockOSThread() for threads that have different mount namespaces, it's possible that /proc/self will refer to the "wrong" thread.

The solution is to simply use /proc/thread-self, which will always provide the correct result for the calling thread.

The usage of /proc/self/mountinfo caused isuses for a patch to runc which creates a thread to create id-mapped mounts (which requires joining the container mount namespace).

Signed-off-by: Aleksa Sarai [email protected]

@cyphar

This comment was marked as resolved.

@cyphar cyphar marked this pull request as draft August 24, 2023 05:24
@cyphar cyphar force-pushed the mountinfo-linux-thread-self branch from 7049be0 to da2df1c Compare August 24, 2023 12:26
@cyphar cyphar marked this pull request as ready for review August 25, 2023 08:05
@cyphar cyphar marked this pull request as draft August 25, 2023 08:06
@cyphar

This comment was marked as resolved.

@thaJeztah
Copy link
Member

I will need to add some fallback code for pre-3.17 kernels where /proc/thread-self didn't exist.

I guess this was not backported in the RHEL 3.10 kernels? I think RHEL 7 reaches EOL June next year, so ... it's getting closer.

@cyphar
Copy link
Contributor Author

cyphar commented Oct 4, 2023

(For future reference, this was motivated by opencontainers/runc#3985.)

@cyphar cyphar force-pushed the mountinfo-linux-thread-self branch from da2df1c to 170c9be Compare October 26, 2023 06:25
@cyphar cyphar marked this pull request as ready for review October 26, 2023 06:41
@cyphar
Copy link
Contributor Author

cyphar commented Oct 26, 2023

Wdyt @kolyshkin?

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

In a Go program with many threads, which goroutine is running on the
thread-group leader thread is not something programs can controls. Thus,
even if you use runtime.LockOSThread() for threads that have different
mount namespaces, it's possible that /proc/self will refer to the
"wrong" thread.

The solution is to simply use /proc/thread-self, which will always
provide the correct result for the calling thread. For pre-3.17 kernels
we use /proc/self/task/<tid> as a fallback.

The usage of /proc/self/mountinfo caused isuses for a patch to runc
which creates a thread to create id-mapped mounts (which requires
joining the container mount namespace).

Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar cyphar force-pushed the mountinfo-linux-thread-self branch from 170c9be to 12c61a3 Compare October 27, 2023 20:59
Copy link
Collaborator

@kolyshkin kolyshkin 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!

@kolyshkin
Copy link
Collaborator

@thaJeztah ptal

@kolyshkin
Copy link
Collaborator

Merging as this doesn't seem like a breaking change, and we need it in runc (#3985).

@kolyshkin kolyshkin merged commit a4e0878 into moby:main Nov 5, 2023
9 checks passed
@cyphar cyphar deleted the mountinfo-linux-thread-self branch November 6, 2023 05:01
@cyphar
Copy link
Contributor Author

cyphar commented Nov 6, 2023

Ah, it seems that we need the /proc/self fallback that I added to runc in opencontainers/runc#3985 as well (it seems we do get mountinfo in the host rootfs as pid1 in the container...).

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