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

open pidfd in child process and send to the parent via SOCK_SEQPACKET+CMSG #113939

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Jul 22, 2023

This avoids using clone3 when a pidfd is requested while still getting it in a 100% race-free manner by passing it up from the child process.
This should solve most concerns in #82971

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 22, 2023
@the8472
Copy link
Member Author

the8472 commented Aug 2, 2023

ping @Mark-Simulacrum

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

r=me modulo comment fix and/or if removal

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2023
msg.msg_controllen = mem::size_of::<Cmsg>() as _;
msg.msg_control = &mut cmsg as *mut _ as *mut _;

match cvt_r(|| libc::recvmsg(sock.as_raw(), &mut msg, 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a timeout here (or something like it) at all? E.g., if we're unlucky and the child process is kill'd by something before we get the pidfd sent back? Or will that close the stream and end here?

My sense is that we should be ok, but wanted to raise in case I overlooked something.

Copy link
Member Author

Choose a reason for hiding this comment

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

The parent closes its output, so only the child holds the remote side of the socket open. If the child gets killed that gets closed, which disconnects the socket which in turn leads to a 0-length read in the parent and a return from this function.

@the8472
Copy link
Member Author

the8472 commented Aug 7, 2023

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 7, 2023

📌 Commit 4673247bf49c260e952082d463fb74342aa81434 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2023
@bors
Copy link
Contributor

bors commented Aug 8, 2023

⌛ Testing commit 4673247bf49c260e952082d463fb74342aa81434 with merge 829cf9a30cfb622ec07dbad9fddeffdae487b630...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 8, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 8, 2023
…+CMSG

This is a 100% race-free way to obtain a child's pidfd while
avoiding `clone3`.
@the8472
Copy link
Member Author

the8472 commented Aug 8, 2023

fixed cfg on the test

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 8, 2023

📌 Commit 8d349c1 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#113939 (open pidfd in child process and send to the parent via SOCK_SEQPACKET+CMSG)
 - rust-lang#114548 (Migrate a trait selection error to use diagnostic translation)
 - rust-lang#114606 (fix: not insert missing lifetime for `ConstParamTy`)
 - rust-lang#114634 (Mention riscv64-linux-android support in Android documentation)
 - rust-lang#114638 (Remove old RPITIT tests (revisions were removed))
 - rust-lang#114641 (Rename copying `ascii::Char` methods from `as_` to `to_`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3feab00 into rust-lang:master Aug 9, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 9, 2023
@the8472 the8472 mentioned this pull request Aug 9, 2023
9 tasks
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2024
…ochenkov

Clean up after clone3 removal from pidfd code (docs and tests)

rust-lang#113939 removed clone3 from pidfd code. This patchset does necessary clean up: fixes docs and tests
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2024
…ochenkov

Clean up after clone3 removal from pidfd code (docs and tests)

rust-lang#113939 removed clone3 from pidfd code. This patchset does necessary clean up: fixes docs and tests
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 26, 2024
Rollup merge of rust-lang#120306 - safinaskar:clone3-clean-up, r=petrochenkov

Clean up after clone3 removal from pidfd code (docs and tests)

rust-lang#113939 removed clone3 from pidfd code. This patchset does necessary clean up: fixes docs and tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants