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

Tracing function runtime patch #1060

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8d706a5
Add runtime patch and restore calls on trace transition.
Pavel-Durov Mar 27, 2024
e1fd774
Add docstring in patch module
Pavel-Durov Mar 27, 2024
ee56310
Add message to panic on empty trace in swt.
Pavel-Durov Mar 27, 2024
461448f
Add patch calls to side-traces.
Pavel-Durov Mar 27, 2024
e237278
Add panic if runtime patch is called on non-x86 platform.
Pavel-Durov Mar 27, 2024
0699dbf
Add test to swt patch.
Pavel-Durov Mar 27, 2024
0db3e4f
Add restore assertion to patch test.
Pavel-Durov Mar 27, 2024
6994b59
Add assert to function restore.
Pavel-Durov Mar 27, 2024
0e88a29
Add x86 compile-time guard
Pavel-Durov Mar 29, 2024
86e404b
Add compile-time guard.
Pavel-Durov Mar 29, 2024
3725c71
Remove PROT_EXEC on mprotect.
Pavel-Durov Mar 29, 2024
799644e
Revert "Remove PROT_EXEC on mprotect."
Pavel-Durov Mar 29, 2024
d572cab
Add documentation.
Pavel-Durov Mar 29, 2024
0bd8f36
Add module docstring.
Pavel-Durov Mar 29, 2024
08029de
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Mar 29, 2024
b2aee3d
Encapsulate x86 instructions.
Pavel-Durov Mar 29, 2024
f1794a2
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Apr 7, 2024
c46bb5f
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Apr 9, 2024
40c0696
Remove PROT_EXEC from mprotect.
Pavel-Durov Apr 9, 2024
4401153
Format.
Pavel-Durov Apr 9, 2024
5ead893
Remove not needed cfgs.
Pavel-Durov Apr 11, 2024
b6fcbd4
Add x86_64 guard.
Pavel-Durov Apr 11, 2024
b334bfa
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov Apr 11, 2024
6f4bdc3
Set mprotect as read+write on patch.
Pavel-Durov Apr 13, 2024
8e00241
Use Layout::from_size_align for aligned page calculation.
Pavel-Durov Apr 14, 2024
ada68bb
Change match to unwrap().
Pavel-Durov Apr 18, 2024
3e7d023
Recover and panic on mprotect.
Pavel-Durov Apr 18, 2024
18b7fee
Remove +size from start_offset calculation.
Pavel-Durov Apr 18, 2024
e1be48c
Add comments.
Pavel-Durov Apr 18, 2024
0428dc1
Use unwrap over expect.
Pavel-Durov Apr 20, 2024
6be7bef
Early return if mprotect failes to set memmory page as writable.
Pavel-Durov Apr 20, 2024
6f0abb3
Add panic on mprotect failure.
Pavel-Durov Apr 23, 2024
94597ef
Merge branch 'master' into tracing-function-runtime-patch
Pavel-Durov May 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions ykrt/src/trace/swt/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,13 @@ unsafe fn patch_function(function_ptr: usize, code: *const u8, size: size_t) {
let layout = Layout::from_size_align(start_offset, page_size)
.expect("Failed to create layout for function memory page");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just use unwrap as all these expects really bloat the code, especially as they shouldn't ever trigger. [As this suggests we almost never use expect: it does have a place, but it's really rare for us.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.
updated 👉 0428dc1

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of the other expects introduced in this PR too please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any other expects only a single unwrap.
Am I missing it somehow?


// Set function memory page as writable
let result = mprotect(page_address, layout.size(), PROT_READ | PROT_WRITE);
if result != 0 {
panic!("Failed to change memory protection to be writable");
}
// Set function memory page as writable.
// Ignoring mprotect call failure.
mprotect(page_address, layout.size(), PROT_READ | PROT_WRITE);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to ignore the return code: we just want to abort changing the machine code. So we need something like if mprotect(...) { return ... } and then recover from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check: is just return enough for the system to keep running? If so, let's add a comment to that effect.

Alternatively, if we think "if mprotect has failed, we've got something wrong", then we might want to panic if mprotect fails.

Copy link
Contributor Author

@Pavel-Durov Pavel-Durov Apr 23, 2024

Choose a reason for hiding this comment

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

I think it's ok to return if mrotect as it should be transactional and not leave the memory in corrupted state on error. Unless we consider the "runtime patching" as a must-have for SWT, then we should definitely panic.

I tested it with different invalid memory addresses that resulted in mrotect returning a non-0 result and the test suite passed.
We still risk changing memory that will not necessarily cause mrotect to return with an error but will cause some other part of the process to have invalid memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think this runtime patching is an integral part of SWT.
Added panic 👉 6f0abb3

// Copy the new code over
std::ptr::copy_nonoverlapping(code, function_ptr as *mut u8, size);
// Set function memory page as readable
let result = mprotect(page_address, layout.size(), PROT_READ | PROT_EXEC);
if result != 0 {
if mprotect(page_address, layout.size(), PROT_READ | PROT_EXEC) != 0 {
panic!("Failed to change memory protection back to executable");
}
}
Expand Down