This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Use /proc/self instead of /proc/<getpid()> to get mapped addresses
Needs ReviewPublic

Authored by lnyng on Jun 23 2022, 2:28 PM.

Details

Summary

The original code may fail on permission deny if it's called by a process that just enters a new pid namespace but before the procfs is remounted. Changing to use /proc/self fixes it because the magic symlink always points to the right pid directory regardless of the process's current pid namespace.

For example, to create a container, we may want to have the init process enter some new namespaces like pid and mount, and remount procfs. Say its host pid is 10000 and container pid is 1. It may see the procfs from the host for a short period of time while getpid() returns 1. If openmp is used during that period, it will access /proc/1 and get permission deny. Using /proc/self works before (points to /proc/10000) and after (to /proc/1) remounting procfs.

There is no strong requirement to use openmp before procfs is remounted, but it's good to cover more edge cases without compromising common cases.

Diff Detail

Event Timeline

lnyng created this revision.Jun 23 2022, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 2:28 PM
lnyng requested review of this revision.Jun 23 2022, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 2:28 PM
luciang added inline comments.
openmp/runtime/src/z_Linux_util.cpp
2019

You might want to keep the existing getpid()-based version on HURD.

I'm not familiar enough with HURD, but based on the commit that added /proc/self - it seems to be an optional compat option and might not always be configured:
https://git.savannah.gnu.org/cgit/hurd/procfs.git/commit/?id=4665f087fde174a9de3e1c3f3de090dd4bfa85e0

+    { "fake-self", 'S', "PID", OPTION_ARG_OPTIONAL,
+	"Provide a fake \"self\" symlink to the given PID, for compatibility "
+	"purposes.  If PID is omitted, \"self\" will point to init.  "
+	"(default: no self link)" },

https://www.gnu.org/software/hurd/hurd/translator/procfs/jkoenig/discussion.html

lnyng updated this revision to Diff 439547.Jun 23 2022, 3:18 PM

Only apply change to Linux but not Hurd

luciang added inline comments.Jun 23 2022, 3:45 PM
openmp/runtime/src/z_Linux_util.cpp
2025

This will issue warnings/build errors under some configurations. Change the type to:

const char* name
lnyng updated this revision to Diff 439555.Jun 23 2022, 3:51 PM

Address comment

lnyng marked an inline comment as done.Jun 23 2022, 3:51 PM