Process.Linux: handle when /proc and the process pid namespace don't match.#100076

tmds · 2024-03-21T13:16:38Z

Normally the '/proc' filesystem uses the same pid namespace as the process. With rootless containers, it may happen that these pid namespaces do not match because the container doesn't have permissions to change '/proc' but it can create a new pid namespace.

When that happens, the numeric ids used by the /proc filesystem no longer match with the process pid namespace. We can still access information for the current process using '/proc/self'. For other processes, we can't map pids to the proc pids, which leads to reading information for non-existing/wrong/inaccessible processes.

Closes #99887.

@dotnet/area-system-diagnostics-process @stephentoub ptal.

cc @omajid

…match. Normally the '/proc' filesystem uses the same pid namespace as the process. With rootless containers, it may happen that these pid namespaces do not match because the container doesn't have permissions to change '/proc' but it can create a new pid namespace. When that happens, the numeric ids used by the /proc filesystem no longer match with the process pid namespace. We can still access information for the current process using '/proc/self'. For other processes, we can't map pids to the proc pids, which leads to reading information for non-existing/wrong/inaccessible processes.

dotnet-policy-service · 2024-03-21T13:17:10Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

tmds · 2024-03-21T13:36:39Z

With rootless containers, it may happen that these pid namespaces do not match

To reproduces the issue without containers, you can run sudo unshare -p --fork /bin/sh.

In that environment you can run a program like:

Console.WriteLine(Process.GetCurrentProcess().PeakWorkingSet64);

When running this, sometimes this will print out a value, sometimes throw InvalidOperationException for an exited process.

In all cases, it is using information from /proc for another process than the current one.

tmds · 2024-04-16T08:03:57Z

@dotnet/area-system-diagnostics-process can you take a look?

Though it is not a common scenario, I think it's valuable we make .NET behave correct because it can occur in containers and CI. A user can not fix it and diagnosing the root cause may also be a challenge for them.

am11 · 2024-04-16T10:45:52Z

For "self" case, we could short-circuit to Environment.ProcessId as we are doing that for Environment.ProcessPath in this code. (assuming Environment.ProcessId is returning the correct value in rootless case; I have't tested)

tmds · 2024-04-16T19:20:41Z

For "self" case, we could short-circuit to Environment.ProcessId as we are doing that for Environment.ProcessPath in this code. (assuming Environment.ProcessId is returning the correct value in rootless case; I have't tested)

To get the current process pid we don't use /proc. I don't understand what you are suggesting.

am11 · 2024-04-17T06:49:36Z

I don't understand what you are suggesting.

I was looking at this and thought it could be replaced by procPid == Environment.ProcessId:

tmds · 2024-04-17T15:45:36Z

I was looking at this and thought it could be replaced by procPid == Environment.ProcessId:

We're adding ProcPid to represent identifiers as they are used by /proc. Environment.ProcessId is an identifier as used by the pid namespace. We mustn't compare these directly. TryGetProcPid gets us from ProcessId to ProcPid.Self. Then in GetExePath for ProcPid.Self instead of reading from /proc/self we optimize by using Environment.ProcessPath.

tmds · 2024-04-29T07:01:42Z

@dotnet/area-system-diagnostics-process ptal.

tmds · 2024-05-21T09:12:28Z

@dotnet/area-system-diagnostics-process ptal.

tmds · 2024-06-07T09:05:45Z

@jeffhandley @adamsitnik can you take a look at this PR please?

tmds · 2024-06-14T14:36:40Z

@jeffhandley @adamsitnik can you take a look? I assume the review is slow because this is a niche case. I looked into this due to a support case. The root cause was hard to diagnose for the end-user, and they could not work around it.

stephentoub

Sorry for the delay. LGTM.

adamsitnik

Overall LGTM, but please answer my question before I hit the merge button.

And once again big thanks for your help @tmds and apologies for the delays in my areas (I was 100% focused on the BinaryFormatter removal effort)

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs

adamsitnik · 2024-06-21T10:00:25Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Linux.cs

+                    {
+                        procSelfPid = pid;
+                    }
+                    Debug.Assert(procSelfPid.HasValue);


Debug.Assert(procSelfPid.HasValue) assumes that the value will be always present, but the condition below does not follow this assumption and in such case returns 1 ('/proc' and the process pid namespace match).

Why is that?

It's handling the unexpected (should never happen) case where we were unable to determine the proc fs pid for self.

tmds · 2024-07-08T09:15:43Z

@adamsitnik this should be good to merge.

adamsitnik

LGTM, thank you @tmds !

adamsitnik · 2024-07-15T11:46:11Z

I need to close and re-open the PR so the CI can re-run the failed legs.

am11 · 2024-07-20T10:57:47Z

src/libraries/System.Private.CoreLib/src/System/Environment.SunOS.cs

    {
        public static long WorkingSet =>
-            (long)(Interop.procfs.TryReadProcessStatusInfo(ProcessId, out Interop.procfs.ProcessStatusInfo status) ? status.ResidentSetSize : 0);
+            (long)(Interop.procfs.TryReadProcessStatusInfo(Interop.procfs.ProcPid.Self, out Interop.procfs.ProcessStatusInfo status) ? status.ResidentSetSize : 0);


This breaks the build on illumos. Was this intentional?

No. I assume the fix is straight forward?

Yes, I will push it to my ongoing branch #105178. Was checking if you had other plans for it, I'll just revert.

ghost added the area-System.Diagnostics.Process label Mar 21, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 21, 2024

stephentoub approved these changes Jun 15, 2024

View reviewed changes

adamsitnik reviewed Jun 21, 2024

View reviewed changes

PR feedback.

72112c3

adamsitnik approved these changes Jul 15, 2024

View reviewed changes

adamsitnik closed this Jul 15, 2024

adamsitnik reopened this Jul 15, 2024

adamsitnik merged commit 773f3cd into dotnet:main Jul 16, 2024

adamsitnik added this to the 9.0.0 milestone Jul 16, 2024

am11 reviewed Jul 20, 2024

View reviewed changes

am11 mentioned this pull request Jul 20, 2024

Use memfd_create when available #105178

Merged

github-actions bot locked and limited conversation to collaborators Aug 20, 2024

Comments

Conversation

tmds commented Mar 21, 2024

Uh oh!

dotnet-policy-service bot commented Mar 21, 2024

Uh oh!

tmds commented Mar 21, 2024

Uh oh!

tmds commented Apr 16, 2024

Uh oh!

am11 commented Apr 16, 2024

Uh oh!

tmds commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am11 commented Apr 17, 2024

Uh oh!

tmds commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmds commented Apr 29, 2024

Uh oh!

tmds commented May 21, 2024

Uh oh!

tmds commented Jun 7, 2024

Uh oh!

tmds commented Jun 14, 2024

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamsitnik Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

tmds Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds commented Jul 8, 2024

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Jul 15, 2024

Uh oh!

am11 Jul 20, 2024

Choose a reason for hiding this comment

Uh oh!

tmds Jul 20, 2024

Choose a reason for hiding this comment

Uh oh!

am11 Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds Jul 20, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tmds commented Apr 16, 2024 •

edited

Loading

tmds commented Apr 17, 2024 •

edited

Loading

tmds Jul 2, 2024 •

edited

Loading

am11 Jul 20, 2024 •

edited

Loading