os: make use of pidfd for linux #62654

kolyshkin · 2023-09-14T21:54:27Z

Based on a discussion in #51246 (in particular, #51246 (comment)), this is a proposal to implement pidfd support for process methods in the os package -- StartProcess, FindProcess, Kill, Wait.

Reuse the handle field of struct Process to keep pidfd. Since 0 is a valid value, use ^uintptr(0) as a default value (if pidfd is not known).
Instrument ~~syscall.StartProcess~~ os.StartProcess to get pidfd from the kernel and return it as a handle. The initial idea was to modify syscall.StartProcess as it returns a handle (which we're reusing for pidfd) but that would be a change is not backward-compatible and can result in leaking pidfd as callers are not expecting it.
Use pidfdSendSignal from (*process).Signal, if handle is known and the syscall is available; fallback to old code using syscall.Kill if syscall is not available (e.g. disabled by seccomp).
Use waitid(P_PIDFD, ...) in Wait, if pidfd is set; fall back to old code if P_PIDFD is not supported.

an implementation of 1-4: CL 528438

Amend FindProcess to use pidfd_open(2), optionally returning ErrProcessGone, and modify the documentation accordingly (adding that it can return ErrProcessGone on Linux).

an implementation of 5: CL 528798

(Optional) Add (*Process).Handle() uintptr method to return process handle on Windows and pidfd on Linux. This might be useful for low-level operations that require handle/pidfd (e.g. pidfd_getfd on Linux), or to ensure that pidfd (rather than pid) is being used for kill/wait.

The text was updated successfully, but these errors were encountered:

gopherbot · 2023-09-14T21:58:14Z

Change https://go.dev/cl/528436 mentions this issue: internal/syscall/unix: add PidFDSendSignal for Linux

ianlancetaylor · 2023-09-15T01:18:00Z

I don't think there is an API change here (before optional item 7) so taking this out of the proposal process.

CC @golang/runtime

kolyshkin · 2023-09-15T02:56:35Z

I don't think there is an API change here (before optional item 7) so taking this out of the proposal process.

@ianlancetaylor can you please elaborate? Is this about FindProcess returning ErrProcessGone, or something else?

If this is about FindProcess returning an error, as I pointed out in 4 above, this is entirely optional. We can make it succeed every time.

ianlancetaylor · 2023-09-15T19:17:54Z

What I mean is that we only need a proposal for an API change or a significant functionality change. This issue doesn't seem to be either, so we should treat it as an ordinary feature request issue, rather than running it through the proposal process. I hope that makes sense. Thanks.

kolyshkin · 2023-09-15T20:49:57Z

Thanks for the explanation, makes sense.I will keep working on implementation.

gopherbot · 2023-09-16T03:06:45Z

Change https://go.dev/cl/528438 mentions this issue: syscall: make StartProcess return pidfd on Linux

gopherbot · 2023-09-16T03:06:45Z

Change https://go.dev/cl/528798 mentions this issue: os: make use of pidfd on linux

metux · 2023-10-03T09:28:06Z

I'm a bit unhappy w/ #4:
API shouldn't behave different on individual OS. So I'd put it this way:

ErrProcessGone could happen on any OS. Just practically only happens on Linux for now. So, developers always need to be prepared for this.

kolyshkin · 2023-10-11T00:38:49Z

@golang/runtime could you please take a look at this proposal as well as CLs implementing it?
This is https://go.dev/cl/528436 and the two related ones, implementing items 1 to 4 described above.

I would appreciate any feedback.

prattmic · 2023-10-24T19:04:28Z

Hi @kolyshkin,

Sorry for the delay. I took a look at your CLs. I'm leaving some feedback here since it is about potential API changes.

https://go.dev/cl/528438 changes syscall.StartProcess to automatically set ProcAttr.Sys.PidFD in order to always return a pidfd via handle. I don't think this is a safe change. Existing callers will not realize that handle is a file descriptor which needs to be closed, and will thus leak the file descriptor on every call. I think it would be better for os.startProcess to set ProcAttr.Sys.PidFD prior to calling syscall.StartProcess. os.Process will retain ownership of the file descriptor and know when to close it.
Should os.Process.Wait close the pidfd? Right now, only os.Process.Release does so.
Regarding os.FindProcess returning ErrProcessGone, I think this is a nice improvement to the API, but I worry that this might break some callers. If we do this, I think we should add a new GODEBUG flag to switch back to the old behavior.

kolyshkin · 2023-11-21T06:46:20Z

Hi @kolyshkin,

Sorry for the delay. I took a look at your CLs. I'm leaving some feedback here since it is about potential API changes.

https://go.dev/cl/528438 changes syscall.StartProcess to automatically set ProcAttr.Sys.PidFD in order to always return a pidfd via handle. I don't think this is a safe change. Existing callers will not realize that handle is a file descriptor which needs to be closed, and will thus leak the file descriptor on every call. I think it would be better for os.startProcess to set ProcAttr.Sys.PidFD prior to calling syscall.StartProcess. os.Process will retain ownership of the file descriptor and know when to close it.

Should os.Process.Wait close the pidfd? Right now, only os.Process.Release does so.

Regarding os.FindProcess returning ErrProcessGone, I think this is a nice improvement to the API, but I worry that this might break some callers. If we do this, I think we should add a new GODEBUG flag to switch back to the old behavior.

@prattmic thanks for review comments, I updated the code based on your suggestions (except for the item 3, see below). In the process, I found a bug in my older PidFD implementation (see https://go.dev/cl/542698 for the fix). The series is now ready for (re-)review, I'd be grateful about any feedback.

As for using GODEBUG, I feel the overall change is major and might break some setups (in particular, new syscalls are used, which might be blocked on some older systems, and the runtime workarounds may be too complex (see e.g. canUsePidfdSendSignal in https://go.dev/cl/528438). Therefore I guess it might make sense to gate the whole "use pidfd from os" feature (including FindProcess change) by having something like GODEBUG=osusepidfd=0.

What do you think?

CC @golang/runtime

prattmic · 2023-11-21T19:42:12Z

Therefore I guess it might make sense to gate the whole "use pidfd from os" feature (including FindProcess change) by having something like GODEBUG=osusepidfd=0.

I agree, this seems reasonable.

CL 520266 added pidfd_send_signal linux syscall numbers to the syscall package for the sake of a unit test. As pidfd_send_signal will be used from the os package, let's revert the changes to syscall package, add the pidfd_send_signal syscall numbers and the implementation to internal/syscall/unix, and change the above test to use it. Updates #51246. For #62654. Change-Id: I862174c3c1a64baf1080792bdb3a1c1d1b417bb4 Reviewed-on: https://go-review.googlesource.com/c/go/+/528436 Run-TryBot: Kirill Kolyshkin <kolyshkin@gmail.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com>

kolyshkin · 2023-11-29T23:24:35Z

I agree, this seems reasonable.

@prattmic Implemented as discussed; PTAL https://go.dev/cl/528438

Use Process.handle field to store pidfd, and make use of it. Only use pidfd functionality if all the needed syscalls are available. 1. StartProcess: obtain the pidfd from the kernel, if available, using the functionality added by CL 520266. Note we could not modify syscall.StartProcess to return pidfd directly because it is a public API and its callers do not expect it, so we have to use ensurePidfd and getPidfd. 2. (*Process).Kill: use pidfdSendSignal, if the syscall is available and pidfd is known. This is slightly more complicated than it should be, since the syscall can be blocked by e.g. seccomp security policy, therefore the need for a function to check if it's actually working, and a soft fallback to kill. Perhaps this precaution is not really needed. 3. (*Process).Wait: use pidfdWait, if available, otherwise fall back to using waitid/wait4. This is also more complicated than expected due to struct siginfo_t idiosyncrasy. NOTE pidfdSendSignal and pidfdWait are used without a race workaround (blockUntilWaitable and sigMu, added by CL 23967) because with pidfd, PID recycle issue doesn't exist (IOW, pidfd, unlike PID, is guaranteed to refer to one particular process) and thus the race doesn't exist either. For #62654. Updates #13987. Change-Id: I22ebcc7142b16a3a94c422d2f32504d1a80e8a8f Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/528438 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopherbot · 2024-02-22T21:45:42Z

Change https://go.dev/cl/566179 mentions this issue: os: FindProcess must not return nil

gopherbot · 2024-02-22T21:45:42Z

Change https://go.dev/cl/566180 mentions this issue: os: add TestFindProcessDone for Linux

gopherbot · 2024-02-22T21:45:43Z

Change https://go.dev/cl/566181 mentions this issue: os: fix FindError doc typo

gopherbot · 2024-02-23T00:20:35Z

Change https://go.dev/cl/566182 mentions this issue: os: fix TestUNIXProcessAlive wrt FindProcess

mvdan · 2024-02-23T09:16:32Z

Therefore I guess it might make sense to gate the whole "use pidfd from os" feature (including FindProcess change) by having something like GODEBUG=osusepidfd=0.

Seeing the main CL land in master, I was a bit surprised to not see any GODEBUG support included, given the discussion here. For those reading along like me, it seems like it was discarded in favor of a fallback to the old mechanism, per https://go-review.googlesource.com/c/go/+/528438/comment/12693995_7cc3bdc5/.

gopherbot · 2024-02-23T18:15:07Z

Change https://go.dev/cl/566476 mentions this issue: Revert "os: make FindProcess use pidfd on Linux"

gopherbot · 2024-02-23T18:15:09Z

Change https://go.dev/cl/566477 mentions this issue: Revert "os: make use of pidfd on linux"

This reverts CL 542699. Reason for revert: Some applications assume FindProcess does not return errors. For #62654. Fixes #65866. Change-Id: Ic185a6253c8e508b08150b618c39a9905f6cdd60 Reviewed-on: https://go-review.googlesource.com/c/go/+/566476 Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Michael Pratt <mpratt@google.com>

This reverts CL 528438. Reason for revert: Implicated in "bad FD" test failures. Full extent of issue still unclear. For #62654. Fixes #65857. Change-Id: I066e38040544c506917e90255bd0e330964a0276 Reviewed-on: https://go-review.googlesource.com/c/go/+/566477 Auto-Submit: Michael Pratt <mpratt@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

ianlancetaylor · 2024-02-23T20:07:16Z

@mvdan Note that CL 542699 (now rolled back) did use a GODEBUG setting for the behavior change in FindProcess.

gopherbot · 2024-03-08T18:19:46Z

Change https://go.dev/cl/570036 mentions this issue: os: make use of pidfd on linux

gopherbot · 2024-03-11T21:34:49Z

Change https://go.dev/cl/570681 mentions this issue: os: make FindProcess use pidfd on Linux

kolyshkin added the Proposal label Sep 14, 2023

gopherbot added this to the Proposal milestone Sep 14, 2023

ianlancetaylor changed the title ~~proposal: os: make use of pidfd for linux~~ os: make use of pidfd for linux Sep 15, 2023

ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Sep 15, 2023

ianlancetaylor modified the milestones: Proposal, Backlog Sep 15, 2023

thanm mentioned this issue Oct 24, 2023

Go compiler and runtime meeting notes #43930

Open

prattmic mentioned this issue Feb 22, 2024

os: crashes due to new error from FindProcess (CL 542699) #65866

Closed

kolyshkin mentioned this issue Mar 11, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: make use of pidfd for linux #62654

os: make use of pidfd for linux #62654

kolyshkin commented Sep 14, 2023 •

edited

gopherbot commented Sep 14, 2023

ianlancetaylor commented Sep 15, 2023

kolyshkin commented Sep 15, 2023

ianlancetaylor commented Sep 15, 2023

kolyshkin commented Sep 15, 2023

gopherbot commented Sep 16, 2023

gopherbot commented Sep 16, 2023

metux commented Oct 3, 2023

kolyshkin commented Oct 11, 2023

prattmic commented Oct 24, 2023

kolyshkin commented Nov 21, 2023

prattmic commented Nov 21, 2023

kolyshkin commented Nov 29, 2023

gopherbot commented Feb 22, 2024

gopherbot commented Feb 22, 2024

gopherbot commented Feb 22, 2024

gopherbot commented Feb 23, 2024

mvdan commented Feb 23, 2024

gopherbot commented Feb 23, 2024

gopherbot commented Feb 23, 2024

ianlancetaylor commented Feb 23, 2024

gopherbot commented Mar 8, 2024

gopherbot commented Mar 11, 2024

os: make use of pidfd for linux #62654

os: make use of pidfd for linux #62654

Comments

kolyshkin commented Sep 14, 2023 • edited

gopherbot commented Sep 14, 2023

ianlancetaylor commented Sep 15, 2023

kolyshkin commented Sep 15, 2023

ianlancetaylor commented Sep 15, 2023

kolyshkin commented Sep 15, 2023

gopherbot commented Sep 16, 2023

gopherbot commented Sep 16, 2023

metux commented Oct 3, 2023

kolyshkin commented Oct 11, 2023

prattmic commented Oct 24, 2023

kolyshkin commented Nov 21, 2023

prattmic commented Nov 21, 2023

kolyshkin commented Nov 29, 2023

gopherbot commented Feb 22, 2024

gopherbot commented Feb 22, 2024

gopherbot commented Feb 22, 2024

gopherbot commented Feb 23, 2024

mvdan commented Feb 23, 2024

gopherbot commented Feb 23, 2024

gopherbot commented Feb 23, 2024

ianlancetaylor commented Feb 23, 2024

gopherbot commented Mar 8, 2024

gopherbot commented Mar 11, 2024

kolyshkin commented Sep 14, 2023 •

edited