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

[release/5.0] Fix usage of process_vm_readv in createdump #50478

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Mar 31, 2021

Description

This PR tries to use process_vm_readv and falls back to using pread on /proc/mem if the first fails with EPERM. This is needed as docker 19.03 on kernels newer than 4.8 allows to use ptrace as long as yama settings don't restrict it, but it does not allow usage of process_vm_* syscalls. These were added very recently to the allow-list but we need a mechanism to allow customers to collect dumps in their containerized environments.

Customer Impact

Customers are forced to use CAP_SYS_PTRACE to be able to collect dumps in their linux containers running on docker (both per request and crash dumps). This is not recommended as an arbitrary process from the same user context could be inspected. A couple of internal and external customers have already voiced their interest in this change.

Regression

Yes, customers were able to collect dumps in containers without needing PTRACE for most scenarios without needing to add the capability to the containers. This changed as the syscall used was changed in the 5.0 timeframe.

Testing

Manual testing performed to validate fallback with the syscall enabled and disabled as well as with and without CAP_SYS_PTRACE to ensure all code paths were used.

Risk

Minimal, dump time only with the old behavior being preserved as the first option.

Fixes dotnet/diagnostics#2098 on release/5.0
PR for main: #50477

@hoyosjs hoyosjs added this to the 5.0.x milestone Mar 31, 2021
@hoyosjs hoyosjs requested a review from mikem8361 March 31, 2021 11:31
@hoyosjs hoyosjs self-assigned this Mar 31, 2021
@ghost
Copy link

ghost commented Mar 31, 2021

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

This PR tries to use process_vm_readv and falls back to using pread on /proc/mem if the first fails with EPERM. This is needed as docker 19.03 on kernels newer than 4.8 allows to use ptrace as long as yama settings don't restrict it, but it does not allow usage of process_vm_* syscalls. These were added very recently to the allow-list but we need a mechanism to allow customers to collect dumps in their containerized environments.

Customer Impact

Customers are forced to use CAP_SYS_PTRACE to be able to collect dumps in their linux containers running on docker (both per request and crash dumps). This is not recommended as an arbitrary process from the same user context could be inspected.

Regression

Yes, customers were able to collect dumps in containers without needing PTRACE for most scenarios without needing to add the capability to the containers. This changed as the syscall used was changed in the 5.0 timeframe.

Testing

Manual testing performed to validate fallback with the syscall enabled and disabled as well as with and without CAP_SYS_PTRACE to ensure all code paths were used.

Risk

Minimal, dump time only with the old behavior being preserved as the first option.

Fixes dotnet/diagnostics#2098 on release/5.0

Author: hoyosjs
Assignees: hoyosjs
Labels:

area-Diagnostics-coreclr

Milestone: 5.0.x

@hoyosjs hoyosjs marked this pull request as ready for review March 31, 2021 20:50
@hoyosjs hoyosjs added the Servicing-consider Issue for next servicing release review label Mar 31, 2021
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We will take for consideration in 5.0.x

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 1, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.6 Apr 1, 2021
@Anipik Anipik merged commit da22c3d into dotnet:release/5.0 Apr 6, 2021
@hoyosjs hoyosjs deleted the juhoyosa/50-fix-createdump branch April 23, 2021 00:02
@ghost ghost locked as resolved and limited conversation to collaborators May 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants