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

cr-restore: rseq: dynamically handle *libc with rseq #1937

Merged

Conversation

mihalicyn
Copy link
Member

Before this patch we assumed that CRIU is compiled against
the same GLibc as it runs with. But as we see from real
world examples like #1935 it's not always true.

The idea of this patch is to detect rseq configuration
for the main CRIU process and use it to unregister
rseq for all further child processes. It's correct,
because we restore pstree using clone*() syscalls,
don't use exec*() (!) syscalls, so rseq gets inherited
in the kernel and rseq configuration remains the same
for all children processes.

This will prevent issues like this:
#1935

Suggested-by: Florian Weimer [email protected]
Signed-off-by: Alexander Mikhalitsyn [email protected]

Before this patch we assumed that CRIU is compiled against
the same GLibc as it runs with. But as we see from real
world examples like checkpoint-restore#1935 it's not always true.

The idea of this patch is to detect rseq configuration
for the main CRIU process and use it to unregister
rseq for all further child processes. It's correct,
because we restore pstree using clone*() syscalls,
don't use exec*() (!) syscalls, so rseq gets inherited
in the kernel and rseq configuration remains the same
for all children processes.

This will prevent issues like this:
checkpoint-restore#1935

Suggested-by: Florian Weimer <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
@fweimer-rh
Copy link
Contributor

Would it make sense to remove the glibc-specific code, so that the generic code receives wider test coverage? If you fork & ptrace anyway to get other things, the additional overhead will be really small, I assume.

Let's use dynamic approach to detect built-in *libc rseq in all cases,
and "old" static approach as a fallback path if the user kernel
lacks support of ptrace_get_rseq_conf feature.

Suggested-by: Florian Weimer <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
@mihalicyn
Copy link
Member Author

Would it make sense to remove the glibc-specific code, so that the generic code receives wider test coverage? If you fork & ptrace anyway to get other things, the additional overhead will be really small, I assume.

agreed, Florian. Have done ;) I've left "old approach" only as a fallback

@codecov-commenter
Copy link

Codecov Report

Merging #1937 (279fb63) into criu-dev (4f62ec0) will increase coverage by 1.64%.
The diff coverage is 66.49%.

❗ Current head 279fb63 differs from pull request most recent head bd479a4. Consider uploading reports for the commit bd479a4 to get more accurate results

@@             Coverage Diff              @@
##           criu-dev    #1937      +/-   ##
============================================
+ Coverage     68.98%   70.63%   +1.64%     
============================================
  Files           128      130       +2     
  Lines         33349    33574     +225     
============================================
+ Hits          23005    23714     +709     
+ Misses        10344     9860     -484     
Impacted Files Coverage Δ
compel/arch/x86/src/lib/cpu.c 78.21% <ø> (ø)
compel/arch/x86/src/lib/thread_area.c 86.36% <0.00%> (ø)
compel/src/lib/infect-util.c 33.33% <ø> (ø)
criu/arch/x86/crtools.c 68.19% <ø> (ø)
criu/arch/x86/include/asm/restorer.h 69.23% <ø> (ø)
criu/arch/x86/include/asm/types.h 100.00% <ø> (ø)
criu/arch/x86/sigaction_compat.c 0.00% <0.00%> (ø)
criu/eventpoll.c 79.51% <ø> (ø)
criu/include/autofs.h 100.00% <ø> (ø)
criu/include/files-reg.h 100.00% <ø> (ø)
... and 123 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90c0f08...bd479a4. Read the comment docs.

@minhbq-99
Copy link
Member

LGTM

@avagin avagin merged commit db9781e into checkpoint-restore:criu-dev Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants