syscall: add PidFD, CgroupFD, and UseCgroupFD options for Linux clone to SysProcAttr

[kolyshkin]

Linux 5.3 added a new system call, clone3(), which provides a superset of the functionality of the older clone(). In particular, it adds a way for a child to be spawned in a different cgroup, which solves a number of issues (see CLONE_INTO_CGROUP flag in clone3(2) man page for details).

It would be great for some software written in Go to benefit from this new functionality. Obviously, calling clone3() syscall directly won't work since Go runtime needs to perform specific steps around fork and exec.

So, the support for clone3 needs to be in the syscall package, where clone() is called.

Looks like this can be implemented by amending linux SysProcAttr structure with a pointer to CloneArgs (a new structure that mirrors that of C.clone_args). Then, forkAndExecInChild would use clone3 instead of clone if this pointer is non-nil. This can be used from e.g. os/exec can set cmd.SysProcArgs.CloneArgs.

[cuiweixie]

i think this kernel version is too new, the minimal kernel version is still 2.6.23。

[kolyshkin]

think this kernel version is too new, the minimal kernel version is still 2.6.23。

This is not a proposal to switch to the new syscall, this is a proposal to make its functionality available to userspace.

[OneOfOne]

i think this kernel version is too new, the minimal kernel version is still 2.6.23。

Just to add perspective, 5.3 came out in Sept 2019.

[rsc]

Retitled. There are three parts to this:

1. Add CloneArgs struct
2. Add CloneArgs *CloneArgs field to SysProcAttr
3. When SysProcAttr.CloneArgs is set, use clone3 during syscall.ForkExec.

All of this seems unobjectionable.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[prattmic]

Is the intention for SysProcAttr.CloneArgs to be the entirety of the full C clone_args structure?

struct clone_args {
  u64 flags;        /* Flags bit mask */
  u64 pidfd;        /* Where to store PID file descriptor (int *) */
  u64 child_tid;    /* Where to store child TID, in child's memory (pid_t *) */
  u64 parent_tid;   /* Where to store child TID, in parent's memory (pid_t *) */
  u64 exit_signal;  /* Signal to deliver to parent on child termination */
  u64 stack;        /* Pointer to lowest byte of stack */
  u64 stack_size;   /* Size of stack */
  u64 tls;          /* Location of new TLS */
  u64 set_tid;      /* Pointer to a pid_t array (since Linux 5.5) */
  u64 set_tid_size; /* Number of elements in set_tid (since Linux 5.5) */
  u64 cgroup;       /* File descriptor for target cgroup of child (since Linux 5.7) */
};

This is a superset of the arguments to clone(2), and it seems like a significant departure from the existing SysProcAttr to allow unfettered access, rather than providing specific desired features.

Many of these arguments cannot be safely set by Go code. e.g., setting stack, tls, or certain flags is likely to cause the child (or parent!) to crash before it can complete syscall.ForkExec.

As an alternative, we could add specific useful features enabled by this syscall. e.g.,

• SysProcAttr.PidFD *int which is assigned to the pidfd of the child.
• SysProcAttr.CgroupFD *int which the fd for target cgroup of child.

[kolyshkin]

Is the intention for SysProcAttr.CloneArgs to be the entirety of the full C clone_args structure?

Definitely not.

Indeed, this proposal can be reduced to adding PidFD and CgroupFD, and making use of clone3 CgroupFD is set (note that PidFD can be obtained via clone(2) already, so it's not clear if the code should switch to using clone3 in this case).

I fully agree with your analysis, and should have proposed what you had.

(Not really related to the proposal, but as far as the implementation goes, my biggest roadblock is obsoleted code for generating per-arch syscall numbers in src/syscall. The files that are supposed to be auto-generated are now changed manually so I am not exactly sure how to add a new syscall number.)

[prattmic]

(Not really related to the proposal, but as far as the implementation goes, my biggest roadblock is obsoleted code for generating per-arch syscall numbers in src/syscall. The files that are supposed to be auto-generated are now changed manually so I am not exactly sure how to add a new syscall number.)

Indeed, our system call code is a mess. We hope to clean it up (see #51087 and #15282), but right now it is a pain to change.

[prattmic]

Indeed, this proposal can be reduced to adding PidFD and CgroupFD.

Thanks. In some sense this is really two proposals, I'm not sure if we should split it up.

Following the API we came to in #47049 (comment), I'll adjust the proposed API to adding the following to SysProcAttr:

PidFD *int // set to pid FD of child if not nil
UseCgroup bool
Cgroup int // fd of cgroup to place child in

[prattmic]

Support for the cgroup FD seems fine and uncontroversial to me.

The pid FD case is a bit more interesting. I certainly think it should be possible to get a pid FD. In fact, I think we should use a pid FD whenever possible. I wonder if instead of providing only a low-level SysProcAttr field, we should automatically make os.StartProcess return an os.Process backed by a pid FD rather than the raw pid (Wait, Kill, etc perform pid FD operations). A new os.Process.Fd() could return the pid FD for additional direct use. os.Process closes the pid FD in os.Process.Release (which is also called with a finalizer).

Down a level, syscall.StartProcess could return the pid FD in the handle return slot, which is currently unused on Linux. Some extra argument or SysProcAttr field would be needed to request this, as callers other than os.StartProcess wouldn't know to close the FD by default. (syscall.ForkExec doesn't have this return value and would need a new variant or SysProcAttr field).

One possibility to merge the limitations above would be to add PidFD *int to SysProcAttr, but also make os.StartProcess automatically set this field if not set by callers.

[kolyshkin]

PidFD *int // set to pid FD of child if not nil

So, I played with this a bit, and it seems it's easier to have something like this:

       // PidFD is set to a PID file descriptor referring to the child process,
       // if CLONE_PIDFD flag is set in Cloneflags. Available since Linux 5.2.
       PidFD int

The only thing about it is a slightly unusual way of returning a value. The gist of implementation, using existing clone() call, is this:

@@ -213,12 +218,12 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att
        runtime_BeforeFork()
        locked = true
        switch {
-       case sys.Cloneflags&CLONE_NEWUSER == 0 && sys.Unshareflags&CLONE_NEWUSER == 0:
+       case sys.Cloneflags&(CLONE_NEWUSER|CLONE_PIDFD) == 0 && sys.Unshareflags&CLONE_NEWUSER == 0:
                r1, err1 = rawVforkSyscall(SYS_CLONE, uintptr(SIGCHLD|CLONE_VFORK|CLONE_VM)|sys.Cloneflags)
        case runtime.GOARCH == "s390x":
-               r1, _, err1 = RawSyscall6(SYS_CLONE, 0, uintptr(SIGCHLD)|sys.Cloneflags, 0, 0, 0, 0)
+               r1, _, err1 = RawSyscall6(SYS_CLONE, 0, uintptr(SIGCHLD)|sys.Cloneflags, 0, uintptr(unsafe.Point
er(&sys.PidFD)), 0, 0)
        default:
-               r1, _, err1 = RawSyscall6(SYS_CLONE, uintptr(SIGCHLD)|sys.Cloneflags, 0, 0, 0, 0, 0)
+               r1, _, err1 = RawSyscall6(SYS_CLONE, uintptr(SIGCHLD)|sys.Cloneflags, 0, uintptr(unsafe.Pointer(
&sys.PidFD)), 0, 0, 0)
        }
        if err1 != 0 || r1 != 0 {
                // If we're in the parent, we must return immediately

We can certainly do it with a pointer, too, if that is preferred.

The only problem is adding new constants; at this point, I guess, I'd rather patch all the needed files in place, since the generator is outright broken and fixing this is very out of scope for this.

[ianlancetaylor]

Currently SysProcAttr is not changed by StartProcess. I would be reluctant to introduce such a case. So I think that a pointer is better. Also, using a pointer makes clear when the caller expects the new descriptor; we don't want to create it if the caller doesn't want it.

[rsc]

Retitled per discussion above. The current proposal is #51246 (comment), to add PidFD, Cgroup, Cgroup.

Certainly you would expect "Pid *int" to be a process ID, not a process ID file descriptor. Hence "PidFD *int".
What about Cgroup? Are there integer IDs for cgroups already? Should it be CgroupFD?

(In contrast, we have "Ctty int" but I think most people would expect a tty int to be a file descriptor, not a tty number. The question is whether Cgroup is more like Ctty or more like Pid.)

[prattmic]

Certainly you would expect "Pid *int" to be a process ID, not a process ID file descriptor. Hence "PidFD *int". What about Cgroup? Are there integer IDs for cgroups already? Should it be CgroupFD?

As far as I know there are no standard integer IDs for cgroups. cgroups are primarily managed as directories within a cgroupfs mount. Technically those directories would have integer inode IDs, but I've never heard of those being used for anything.

Even proc files identify cgroups with their string path within the cgroupfs. e.g., the last field below,

$ cat /proc/225775/cgroup 
13:freezer:/
12:memory:/user.slice/user-200669.slice/session-c16.scope
11:perf_event:/
10:pids:/user.slice/user-200669.slice/session-c16.scope
9:misc:/
8:rdma:/
7:hugetlb:/
6:devices:/user.slice
5:net_cls,net_prio:/
4:cpuset:/
3:cpu,cpuacct:/user.slice/user-200669.slice/session-c16.scope
2:blkio:/user.slice
1:name=systemd:/user.slice/user-200669.slice/session-c16.scope
0::/user.slice/user-200669.slice/session-c16.scope

The first field is the "hierarchy ID", which is a numeric description for the cgroup type:

$ cat /proc/cgroups
#subsys_name    hierarchy       num_cgroups     enabled
cpuset  4       2       1
cpu     3       96      1
cpuacct 3       96      1
blkio   2       87      1
memory  12      124     1
devices 6       87      1
freezer 13      2       1
net_cls 5       2       1
perf_event      11      2       1
net_prio        5       2       1
hugetlb 7       2       1
pids    10      96      1
rdma    8       1       1
misc    9       1       1

I suppose that could be confusing, but again this describes a cgroup type, not a specific cgroup.

Despite all that, in hindsight I think CgroupFD is a better name. I suggested Cgroup because that is the name used in Linux's clone_args, but CgroupFD IMO only adds clarity.

Additionally, as far as I know, "cgroup FDs" are not a common concept [1]. In fact, I'm not certain if there is anything you can do with this FD besides pass it to clone. Most cgroup operations are performed on files in the cgroup directory. e.g., an existing process is added to a cgroup by opening the cgroup.procs file in the directory and writing a PID. So I think adding clarity in the name is helpful. (The documentation on the field should also explain how to get this FD).

[1] A "cgroup FD" in the context of clone is just open(O_RDONLY) of the cgroup directory.

[prattmic]

In fact, I'm not certain if there is anything you can do with this FD besides pass it to clone.

Oh, of course, the other obvious thing you could do is use this with openat (e.g., openat(cgroupFD, "cgroup.procs")) to avoid needing to know the name of the cgroup (or even the cgroupfs mount point). But I'm not sure if that is something commonly done.

[kolyshkin]

@prattmic what you look at is cgroup v1, which is a set of cgroups per controller, i.e. we have a forest. This is being superseded by cgroup v2, where we have a single unified cgroup directory for each process (IOW a tree, not a forest). Cgroup v2 is enabled by default in some modern distros, e.g. recent Fedora. For Ubuntu, you need a kernel options

The cgroup parameter to clone3 only supports cgroup v2, and it's a file description to cgroup directory.

You're right that there is not much use for cgroupfd outside of this clone3. One other use I can think of is openat2, but implementing openat2 is a separate unrelated topic. (Update: I missed your last comment, @prattmic, where you say the same about openat)

[rsc]

It sounds like there is enough confusion about cgroups that calling it CgroupFD would help clarify matters.

[rsc]

Retitled. Does anyone object to adding PidFD, CgroupFD, and UseCgroupFD to the Linux SysProcAttr?

[rsc]

@kolyshkin, we might want to reserve access to the pidfd to Go itself, so that we have the ability to use it for signal delivery on new enough Linux systems. If we add this option to SysProcAttr, we are essentially giving away the pidfd and giving up the ability to use it in the Go runtime or os package in the future. We might not want to do that.

What use for pidfd did you have? Perhaps we should figure out an API that would let you do what you need while the Go runtime or os package still owns the pidfd.

(Or maybe if we need two in the future we could use Dup.)

[rsc]

@kolyshkin We are still interested in whether the pidfd itself needs to be exposed, and for what use. Thanks!

[kolyshkin]

Sorry for not replying earlier, was on vacay.

Support for pidfd (I mean full and transparent support, such as embedding it into os.Process and using from e.g. Wait (that would be waitid(P_PIDFD, ...)) and Kill methods) would be nice to have, since using it will protect from pid reuse, and I've seen a few projects that have their own semi-crappy ways of dealing with it (usually by storing and checking process start time).

Obviously, those projects need a way to see if pidfd is supported by the runtime/kernel.

Some low-level software, like runc, may find other uses for pidfd (I'm not sure what would be it exactly, perhaps pidfd_getfd(2), and I guess future kernels may add more ways to use pidfd), so having some method to get it (hidden under Sys() or the like) would be nice to have, too, and could also be used as a way to check if pidfd is supported.

[ianlancetaylor]

Note that the os package already uses waitid to avoid PID reuse. It's not a problem on Linux for code that uses the os or os/exec package to manage child processes.

I think the relevant question is: if we add an option to syscall.SysProcAttr to return the pidfd, will that preclude the Go standard library using a pidfd for its own purposes?

[kolyshkin]

Note that the os package already uses waitid to avoid PID reuse.

What I meant is not just waitid, but waitid with P_PIDFD. The current code is

go/src/os/wait_waitid.go

Line 32 in a682a5c

_, _, e = syscall.Syscall6(syscall.SYS_WAITID, _P_PID, uintptr(p.Pid), uintptr(unsafe.Pointer(psig)), syscall.WEXITED|syscall.WNOWAIT, 0, 0)

That process id may or may not refer to the original process by the time (*Process.Wait) is called (I understand that getting the "wrong" pid won't happen normally and would require deliberately bad code, such as a goroutine calling waitid(P_ALL, ...) for the original PID to be freed and potentially reused).

Complete support for pidfd should include switching to syscall.SYS_WAITID, _P_PIDFD, uintptr(p.PidFD) (if available) in the above code. Surely this is the last issue to worry about since normally pid reuse can't happen.

[kolyshkin]

I am not very familiar with the code, but it seems Linux can reuse the handle field of os.Process struct to store pidfd.

Now,

• os.FindProcess can implement pidfd_open(2) on Linux
• os.Wait can use waitid(P_PIDFD, p.handle)
• os.Kill can use pidfd_send_signal(2)

If all this is out of scope for this proposal, please let me know if you want me to file a new issue regarding pidfd support, and we can limit this one to cgroupfd.

[ianlancetaylor]

For this proposal we don't have to discuss how the os package should use pidfd. What I'm asking is this: if we add PidFD field to syscall.SysProcAttr, will that preclude us from using pidfd for that process? That is, if the os package starts using the pidfd (details don't matter), and somebody uses os.StartProcess to start a process with PidFD set to get the pidfd itself, will the operations that the user program does with the pidfd interfere with the operations that the os package does?

It seems clearly safer to reserve the pidfd value for the os package. But maybe it's OK to also let the user code get the pidfd. I don't know. (And of course user code can always call pidfd_open, but that seems clearly out of scope whereas setting a field in SysProcAttr is clearly in scope.)

[kolyshkin]

if we add PidFD field to syscall.SysProcAttr, will that preclude us from using pidfd for that process? That is, if the os package starts using the pidfd (details don't matter), and somebody uses os.StartProcess to start a process with PidFD set to get the pidfd itself, will the operations that the user program does with the pidfd interfere with the operations that the os package does?

That's a great but tough question.

I thought about various scenarios and I do not foresee any issues. Exposing PidFD enables a user to do a few things that they can already do using Pid, so no changes here.

In addition, a user could do the following operations on PidFD:

1. set it to a different value: this might be dangerous, and it is complicated to assess the consequences.
2. close: seems mildly dangerous, but practically it should result in os.* falling back to using Pid as they were, so apparently no issue.
3. dup: it seems that it won't preclude wait* from reaping a zombie, so again no issue here.
4. set or clear CLOEXEC flag: seems harmless.
5. various fcntls other than the previous two cases -- most probably undefined, so no issue.
6. read/write: these operations are not defined, i.e. they should not change anything and result in EINVAL from the kernel';
7. poll/select/epoll: AFAIK multiple pollers should all receive events, so no issue;

Mostly probably I have missed something, and might make other mistakes -- feel free to correct/contribute.

For item 1, though, making the field private and adding an accessor method (e.g.func (s *SysProcAttr) PidFD() int) would mitigate the isse.

[prattmic]

2. close: seems mildly dangerous, but practically it should result in os.* falling back to using Pid as they were, so apparently no issue.

Ownership seems to be the biggest obvious problem. We need to define who is responsible for closing the FD. If os is ever going to use PidFD, it seems that os must own the FD (i.e., be responsible for closing it) even if full use is not implemented in os, because otherwise os can't assume that it remains safe to use. If the FD is closed early, then the os package could race and operate on a different file descriptor re-opened with the same FD number.

Stepping back a bit, from #51246 (comment) it seems that the primary desire is use within os, and that thus far we have no concrete examples of what applications would use the FD for (@kolyshkin please correct me if I have misinterpreted). Thus it seems safest to me that we hold off on exposing PidFD for now, but do move forward with CgroupFD and using PidFD internally in os (I don't think this would require a proposal at all).

[kolyshkin]

If the FD is closed early, then the os package could race and operate on a different file descriptor re-opened with the same FD number.

OTOH we already have Fd method for os.File, which is susceptible to the same problem. Practically, I guess, no one calls unix.Close(int(f.Fd())) as this is obviously wrong (and this fact is documented in https://pkg.go.dev/os#File.Fd).

thus far we have no concrete examples of what applications would use the FD for

Right.

Thus it seems safest to me that we hold off on exposing PidFD for now, but do move forward with CgroupFD and using PidFD internally in os (I don't think this would require a proposal at all).

I agree. The only thing is, apps would need a way to know if PidFD is supported/used by the runtime (so they can stop using their own mitigations against PID reuse). I was thinking that having a PidFD() accessor method would double as such a way, but if we don't want to expose PidFD, there is a need to have another way.

[rsc]

If we did want to let the os package use the pidfd and a user package wanted it too, is there any problem with just calling dup to get two fds and let them both close it when done?

[mdlayher]

For what it's worth, I have done a bit of playing around with pidfds recently at https://github.com/mdlayher/pidfd. Here's my use case:

My intent with this package is to ultimately use pidfd_getfd(2) to transfer a net.Listener backed by some file descriptor between two processes. In my particular case, I will be operating on long-running processes which are serving traffic indefinitely, and can:

• start up, look up PID of existing process
• use pidfd_open(2) to get a handle to that process
• use pidfd_getfd(2) to dup the listening file descriptor
• use pidfd_send_signal(2) to ultimately notify the first process that it can close its listener and stop running

Since these processes are long-running, I don't expect to run into any races between spawning a process, having it die, and then accidentally getting a pidfd handle to a process which has reused the same PID as my previous one.

---

As for spawning a new process from Go and getting a pidfd assigned immediately by clone3(2): assuming that the pidfd can be dup'd safely and each copy can be used independently by os and user applications, I'd be in favor of seeing that functionality exposed.

If it turns out to be too tricky to expose today, I believe the approach I've highlighted above should work for all but the shortest-lived processes spawned from Go.

[rsc]

It sounds like everyone agrees with adding CgroupFD and UseCgroupFD.
Do I have that right?

[rsc]

For PidFD, it sounds like there should be no problem handing it out to users right now, and if the Go runtime wants a copy, we can call dup on it to get two in the future. So it sounds like we should add PidFD too.

Do I have that right?

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[gopherbot]

Change https://go.dev/cl/407574 mentions this issue: syscall: do not autogenerate Linux CLONE_ flags