Handle processes with uprobes vma #2717
Conversation
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
5de06ac to
6641937
Compare
|
Hi, @rst0git! As we discussed in the previous comment, I've force pushed the changes to add a new optional boolean |
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
6641937 to
3d47ded
Compare
|
@shashank-mahadasyam Thank you so much for the PR. The patches look good to me. I was able to test the functionality locally and confirm that it works as expected. @avagin, @mihalicyn What do you think about this approach? |
|
Thanks a lot for the reviews and testing, @rst0git! If this approach is okay, I'll add some tests as well |
|
I just realized something. If a uprobe is enabled, but it's never triggered, then the uprobes vma will never be created, but the software breakpoint instruction replacement will have happened. If we then dump, criu will have no way of knowing that there's a uprobe attached. On restore, if the uprobe is still active, then good. Otherwise, a SIGTRAP will be sent if execution reaches the uprobed location. Since the kernel looks up uprobes with the inode number of the executable and file offset as key, should uprobes also be C/R-able? Or at least criu should have some way of identifying uprobes attached to a file. C/R-ing uprobes may be too much because there are at least three ways of creating uprobes: Or am I reading too much into this? Sure, this is a legitimate possibility, but it seems like an edge case. I'm not sure how much it really shows up in the wild. Maybe we can just document this possibility and leave it at that. |
Yes, we need to limit the scope - this PR fixes the problem described in #1961. It is often impractical to support all possible edge cases.
This might better handled at a higher level, similar to when restoring on different kernel version. The user or another tool need to verify system compatibility before restore. |
|
Hi, @avagin, @mihalicyn! Could you please check out this PR? I've also pushed a test now to test the newly introduced option @rst0git, could you please check out the test as well? Thank you! |
|
I see that making the uprobes test depend on libtracefs, libtraceevent, and libelf is breaking CI. Just to confirm, where do I need to add these dependencies? |
|
Dear @shashank-mahadasyam, thanks for working on this! I'm sorry for the delay with review, but I'll be able to look into this over the weekend. Pretty busy week. :( Andrey (@avagin) is on vacation now and (almost) without a computer, that's why he hasn't looked yet.
I guess almost everywhere. Because some of these files are used to build docker images to run tests from it, some are used to prepare a GitHub runner and install necessary dependencies. |
|
Hi, @mihalicyn!
No worries :) Thanks for the update!
Uh oh, I'll try to decipher then |
|
Take a look at d165b94 |
|
I literally just stumbled upon this commit. Thanks, @adrianreber! |
|
@avagin I was wondering if you have any comments about the changes in this PR? |
|
LGTM. Thanks a lot for this contribution. We need to fix CI tests before merging it. |
avagin@31e5d37 should fix most of ci failures. |
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
3323caf to
fa0a4c8
Compare
|
Hi, @avagin! Thanks for the review and the commit to fix the CI tests! I was gone for a few weeks on vacation, but before leaving I was working on adding package-manager specific package list files to make it easier to add new packages. The way we do it now (manually add packages in a bunch of places) is messy. So, e04f1c3 adds the required files and references them in the relevant Dockerfiles/scripts. I've tried to cover as many Dockerfile/scripts I could find out, but I may have missed a few. Commits 44366f4 .. 3a4aa22 consolidate common packages to the relevant package list files. Then, the original commit adding the uprobes test has been rebased onto these commits to be able to add the packages required for the test. I know I should have sent these CI related commits as another PR, but due to certain internal constraints, I would really prefer it to go through this PR. I hope it isn't too annoying :) |
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
fa0a4c8 to
a94ce93
Compare
|
I just rebased this branch onto the latest main branch. Sorry about the mess. |
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
a94ce93 to
0a96494
Compare
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
0a96494 to
fcbae69
Compare
|
Changes:
|
|
@shashank-mahadasyam I opened the following PR with the patch updating the base images for the Java tests: #2761 |
This flag will be used for a "[uprobes]" vma. Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
This commit teaches criu to deal with processes which have a "[uprobes]" vma. This vma is mapped by the kernel when execution hits a uprobe location. This is done so as to execute the uprobe'd instruciton out-of-line in the special vma. The uprobe'd location is replaced by a software breakpoint instruction, which is int3 on x86. When execution reaches that location, control is transferred over to the kernel, which then executes whatever handler code it has to, for the uprobe, and then executed the replaced instruction out-of-line in the special vma. For more details, refer to this commit: torvalds/linux@d4b3b63 Reason for adding a new option ------------------------------ A new option is added instead of making the uprobes vma handling transparent to the user, so that when a dump is attempted on a process tree in which a process has the uprobes vma, criu will error, asking the user to use this option. This gives the user a chance to check what uprobes are attached to the processes being dumped, and try to ensure that those uprobes are active on restore as well. Again, the same reason for requiring this option on restore as well. Because if a process is dumped with an active uprobe, and on restore if the uprobe is not active, then if execution reaches the uprobe location, then the process will be sent a SIGTRAP, whose default behaviour will terminate and core dump the process. This is because the code pages are dumped with the software breakpoint instruction replacement at the uprobe'd locations. On restore, if execution reaches these locations and the kernel sees no associated active uprobes, then it'll send a SIGTRAP. So, using this option is on dump and restore is an implicit guarantee on the user's behalf that they'll take care of the active uprobes and that any future SIGTRAPs because of this are not on us! :) Handling uprobes vma on dump ---------------------------- We don't need to store any information about the uprobes vma because it's completely handled by the kernel, transparent to userspace. So, when a uprobes vma is detected, we check if the --allow-uprobes option was specified or not. If so, then the allow_uprobes boolean in the inventory image is set (this is used on restore). The uprobes vma is skipped from being added to the vma list. Handling uprobes vma on restore ------------------------------- If allow_uprobes is set in the inventory image, then check if --allow-uprobes is specified or not. Restoring the vma is not required. Fixes: checkpoint-restore#1961 Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Most people know this, don't they? :) Suggested-by: Radostin Stoyanov <rstoyanov1@gmail.com> Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
Program flow: - Parse the test's own executable to calculate the file offset of the uprobe target function symbol - Enable the uprobe at the target function - Call the target function to trigger the uprobe, and hence the uprobes vma creation - C/R - Call the target function again to check that no SIGTRAP is sent, since the uprobe is still active At least v1.7 of libtracefs is required because that's when tracefs_instance_reset was introduced. The uprobes API was introduced in v1.4, and the dynamic events API was introduced in v1.3. Ubuntu Focal doesn't have libtracefs. Jammy has v1.2.5, and Noble has v1.7. Signed-off-by: Shashank Balaji <shashank.mahadasyam@sony.com>
shashank-mahadasyam
force-pushed
the
uprobes-support
branch
from
fcbae69 to
349411c
Compare
|
Finally, none of the checks are failing because of this PR 😮💨 |
|
@shashank-mahadasyam Thank you for working through the feedback! |
|
Thanks a lot! It reads well |
5 participants
Currently, uprobes vma are treated as unsupported which causes criu to error with "Unsupported mapping". This PR fixes that.
A uprobes vma is automatically mapped by the kernel when a uprobes location is hit. The TL;DR version of how uprobes works is, a software breakpoint is placed at the uprobe'd location. When execution reaches this place, control is transferred over to the kernel, which then sets up this special vma, if it hasn't done so already. The kernel executes whatever uprobe hooks it has to, and then executes the instruction which was replaced by the software breakpoint. This happens out-of-line in the special vma. For more details, checkout this commit: torvalds/linux@d4b3b63
This vma is not unmapped even when once active uprobes are deleted. So the presence of the vma does not necessarily mean an active uprobe, i.e. software breakpoint instruction replacement.
--allow-uprobesoption, and teaches criu to deal with the uprobes vma.I'm not sure if adding a new option is the best way to solve this problem or not. So, if there are improvements, then I'll rework this PR. If this is accepted, then I'll send another PR adding some tests for this option.
Fixes: #1961