Restrict Checks for Open/MMAPed Files - CLI & Checksum #1131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So, here's the enhanced version of the first try.
Changes are:
1. The wrapper name is criu-ns instead of crns.py
2. The CLI is absolutely the same as for criu, since the script
re-execl-s criu binary. E.g.
scripts/criu-ns dump -t 1234 ...
just works
3. Caller doesn't need to care about substituting CLI options,
instead, the scripts analyzes the command line and
a) replaces -t|--tree argument with virtual pid __if__ the
target task lives in another pidns
b) keeps the current cwd (and root) __if__ switches to another
mntns. A limitation applies here -- cwd path should be the
same in target ns, no "smart path mapping" is performed. So
this script is for now only useful for mntns clones (which
is our main goal at the moment).
Signed-off-by: Pavel Emelyanov <[email protected]>
Looks-good-to: Andrey Vagin <[email protected]>
This is the case when the in/out files are image cache/proxy sockets. Signed-off-by: Rodrigo Bruno <[email protected]> Signed-off-by: Katerina Koukiou <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]>
This patch introduces the --remote option and the necessary code changes to
support it. This leaves user the option to decide if the checkpoint data is to
be stored on disk or sent through the network (through the image-proxy).
The latter forwards the data to the destination node where image-cache
receives it.
The overall communication is performed as follows:
src_node CRIU dump -> (sends images through UNIX sockets) -> image-proxy
|
V
dst_node: CRIU restore <- (receives images through UNIX sockets)<- image-cache
Communication between image-proxy and image-cache is done through a single
TCP connection.
Running criu with --remote option is like this:
dst_node# criu image-cache -d --port <port> -o /tmp/image-cache.log
dst_node# criu restore --remote -o /tmp/image-cache.log
src_node# criu image-proxy -d --port <port> --address <dst_node> -o /tmp/image-proxy.log
src_node# criu dump -t <pid> --remote -o /tmp/dump.log
[ xemul:
here's the list of what should be done with the cache/proxy
in order to have them merged into master.
0. Document the whole thing :)
Please, add articles for newly introduced actions and options to
https://criu.org/CLI page.
Also, it would be good to have an article describing the protocols
involved.
1. Make the unix sockets reside in work-dir.
The good thing is that we've get rid of the socket name option :)
But looking at do_open_remote_image() I see that it fchdir-s to
image dir before connecting to proxy/cache. Better solution is to
put the socket into workdir.
1a. After this the option -D|--images-dir should become optional.
Provided the --remote is given CRIU should work purely on the
work-dir and not generate anything in the images-dir.
2. Tune up the image_cache and image_proxy commands to accept the
--status-fd and --pidfile options.
Presumably the very cr_daemon() call should be equipped with
everything that should be done for daemonizing and proxy/cache
tasks should just call it :)
3. Fix local connections not to generate per-image threads. There
can be many images and it's not nice to stress the system with
such amount of threads. Please, look at how criu/uffd.c manages
multiple descriptors with page-faults using the epoll stuff.
3a. The accept_remote_image_connections() seem not to work well
with opts.ps_socket scenario as the former just calls accept()
on whatever socket is passed there, while the opts.ps_socket
is already an established socket for data transfer.
4. No strings in protocol. Now the hard-coded "RESTORE_FINISH" string
(and DUMP_FINISHED one) is used to terminate the communication.
Need to tune up the protobuf objects to send boolean (or integer)
EOF sign rather that the string.
5. Check how proxy/cache works with incremental dumps. Looking at the
skip_remote_bytes() I think that image-cache and -proxy still do not
work well with stacked pages images. Probably for those we'll need
the page-server or lazy-pages -like protocol that would request the
needed regions and receive it back rather than read bytes from
sockets simply to skip those.
6. Add support for cache/proxy into go-phaul code. I haven't yet finished
with the prototype, but plan to do it soon, so once the above steps
are done we'll be able to proceed with this one.
]
Signed-off-by: Rodrigo Bruno <[email protected]>
Signed-off-by: Katerina Koukiou <[email protected]>
Signed-off-by: Pavel Emelyanov <[email protected]>
The current patch brings the implementation of the image proxy and image cache. These components are necessary to perform in-memory live migration of processes using CRIU. The image proxy receives images from CRIU Dump/Pre-Dump (through UNIX sockets) and forwards them to the image cache (through a TCP socket). The image cache caches image in memory and sends them to CRIU Restore (through UNIX sockets) when requested. Signed-off-by: Rodrigo Bruno <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]>
Signed-off-by: Rodrigo Bruno <[email protected]> Signed-off-by: Katerina Koukiou <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]>
To suppress protobuf's warning: > [libprotobuf WARNING google/protobuf/compiler/parser.cc:546] > No syntax specified for the proto file: remote-image.proto. > Please use 'syntax = "proto2";' or 'syntax = "proto3";' > to specify a syntax version. (Defaulted to proto2 syntax.) Signed-off-by: Dmitry Safonov <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Signed-off-by: Rodrigo Bruno <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Signed-off-by: rodrigo-bruno <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Signed-off-by: rodrigo-bruno <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
I see no need to do dynamic init here. Cc: Rodrigo Bruno <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
OK, so we have pr_perror() for cases where errno is set (and it makes sense to show it), and pr_err() for other errors. A correct function is to be used, depending on the context. 1. pthread_mutex_*() functions don't set errno, therefore pr_perror() should not be used. 2. accept() sets errno => makes sense to use pr_perror(). 3. read_header() arguably sets errno => use pr_err(). 4. open_proc_rw() already prints an error message, there is no need for yet another one. Cc: Rodrigo Bruno <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
1. Use xmalloc() where possible. 2. There is no need to print an error message, as xmalloc() has already printed it for you. Cc: Rodrigo Bruno <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
In those error paths where we don't have errno set, don't use pr_perror(), use pr_err() instead. Cc: Rodrigo Bruno <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
When --remote option is specified, read_local_page tries to pread from a socket, and fails with "Illegal seek" error. Restore single pread call for regular image files case and introduce maybe_read_page_img_cache version of maybe_read_page method. Generally-approved-by: Rodrigo Bruno <[email protected]> Acked-by: Pavel Emelyanov <[email protected]> Signed-off-by: Mike Rapoport <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
There is no real need to have both. Signed-off-by: Omri Kramer <[email protected]> Singed-off-by: Lior Fisch <[email protected]> Reviewed-by: Mike Rapoport <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
It's simply impossible (yet), so emit a warning. Acked-by: Mike Rapoport <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
The opts.remote is always false in this code. Acked-by: Mike Rapoport <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Acked-by: Mike Rapoport <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
We have two places to check for parent via page server -- as a part of _OPEN req and explicit req. Make the latter code be in-sync with the opening one. Acked-by: Mike Rapoport <[email protected]> Signed-off-by: Pavel Emelyanov <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Those may not support sendfiles, so use read/write-s instead Signed-off-by: Pavel Emelyanov <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
Drop the constants for default cache host/port and page size because they are not used anywhere. Signed-off-by: Radostin Stoyanov <[email protected]> Reviewed-by: Dmitry Safonov <[email protected]> Signed-off-by: Andrei Vagin <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
1) fix sfle memory leak on get_fle_for_scm error 2) fix gfd open descriptor leak on get_fle_for_scm error 3-6) fix buf memory leak on read and pwrite errors Signed-off-by: Pavel Tikhomirov <[email protected]>
Import "How to submit patches" article from CRIU wiki and update its format to match GitHub markdown. Signed-off-by: Mike Rapoport <[email protected]>
* Mark lowcase criu as code in the environment section * Add missing brace around the reference to https://criu.org/Secrity * Fixup an admolition block that GitHub cannot render * Spelling fixups * s/github/GitHub/g Signed-off-by: Mike Rapoport <[email protected]>
Shamelessly stolen from the Linux kernel [1], shortened a bit and relaxed to match CRIU. [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html Signed-off-by: Mike Rapoport <[email protected]>
Following the discussion at [1] describe best practices for pull request creation. [1] checkpoint-restore#1096 Signed-off-by: Mike Rapoport <[email protected]>
When uffd_open is called from kerndat_uffd, userfaultfd failure is not considered an error, so the goal is to suppress the error message -- instead, we print this message as info. If the function fails, it is the responsibility of the caller to print the error message. Signed-off-by: Angie Ni <[email protected]>
Signed-off-by: Adrian Reber <[email protected]>
Change made through this commit: - Include copy of flog as a seperate tree. - Modify the makefile to add and compile flog code. Signed-off-by: prakritigoyal19 <[email protected]>
In case if our parent is a dead task (zombie) we should lookup for parent ids which will be inherited on restore. Otherwise parent->ids may be nil and SIGSEGV produced. Signed-off-by: Cyrill Gorcunov <[email protected]> Rework and port from vzcriu: 87b320964 ("vz7: mount: restore_task_mnt_ns - Lookup for mount namespace conditionally") Fixes: checkpoint-restore#1066 Signed-off-by: Pavel Tikhomirov <[email protected]>
In case if our parent is a dead task (zombie) or a helper which in it's turn has zombie parent, and parent thus has zero cg_set we should look for current cgset deeper. Fixes: checkpoint-restore#1066 Signed-off-by: Pavel Tikhomirov <[email protected]>
Create a session leader and it's child - session member, make leader zombie. To restore this criu will need to create a helper task a child of our zombie so that member can inherit session. Before fixes in this patchset we segfault on empty ids and fail to restore cgroups because of empty cg_set Signed-off-by: Pavel Tikhomirov <[email protected]>
avagin
reviewed
Jul 8, 2020
avagin
reviewed
Jul 8, 2020
avagin
reviewed
Jul 8, 2020
criu/files-reg.c
Outdated
| * 10485760 was chosen because regardless of the checksum configuration chosen, | ||
| * at most 10MB worth of data of the file is collectively considered in the | ||
| * calculation of the checksum since the checksum parameter can also never exceed | ||
| * 10485760. |
There was a problem hiding this comment.
I removed the limit for the number of the iterations. Now the problem is what if the user chooses to checksum the entire file and the files turn out to be huge? It would take a very long time to calculate the checksum using all the bytes.
avagin
reviewed
Jul 8, 2020
avagin
reviewed
Jul 8, 2020
criu/include/cr_options.h
Outdated
| #define FILE_VALIDATION_FILE_SIZE 1 | ||
| #define FILE_VALIDATION_BUILD_ID 2 | ||
| #define FILE_VALIDATION_CHKSM 3 | ||
| #define FILE_VALIDATION_CHKSM_EVERY 4 |
There was a problem hiding this comment.
Need to add a comment which describes what each of these constant means.
And I think they have to be in images/regfile.proto as enum
There was a problem hiding this comment.
Why does the enum need to be in images/regfile.proto? Currently, I've left it in criu/include/cr_options.h.
avagin
reviewed
Jul 8, 2020
avagin
reviewed
Jul 8, 2020
avagin
reviewed
Jul 8, 2020
This adds build-id, checksum, checksum-config and checksum-parameter fields to RegFileEntry to store metadata used for file verification. build_id: Holds the build-id if it could be obtained checksum: Holds the checksum if it could be obtained checksum_config: Holds the configuration of bytes for which checksum has been calculated (The entire file, first N bytes or every Nth byte) checksum_parameter: Specifies the value of 'N', if required, for the configuration of bytes Signed-off-by: Ajay Bharadwaj <[email protected]>
efi.h: Required for accessing the build-id of .efi files This adds functions to find, store and compare with the stored build-id. get_build_id() calls 32-bit or 64-bit helper functions depending on the bitness of the ELF file after first ensuring that it is actually an ELF file by checking for the magic number. The number of iterations while searching the elf file for the build-id before giving up (500 while searching the note section) are limited. Signed-off-by: Ajay Bharadwaj <[email protected]>
file_validation_method field added to cr_options structure in "criu/include/cr_options.h" along with the constants: FILE_VALIDATION_FILE_SIZE FILE_VALIDATION_BUILD_ID FILE_VALIDATION_DEFAULT (Equal to FILE_VALIDATION_BUILD_ID) Usage and description information is yet to be added Usage: --file-validation="filesize" (To use only the file size check) --file-validation="buildid" (To try and use only the build-id check) Signed-off-by: Ajay Bharadwaj <[email protected]>
file_validation_chksm_config and file_validation_chksm_parameter fields added to cr_options structure in criu/include/cr_options.h along with the constants: FILE_VALIDATION_CHKSM FILE_VALIDATION_CHKSM_EVERY FILE_VALIDATION_CHKSM_FIRST FILE_VALIDATION_CHKSM_PERIOD FILE_VALIDATION_CHKSM_CONFIG_DEFAULT (Equal to FILE_VALIDATION_CHKSM_FIRST) FILE_VALIDATION_CHKSM_PARAM_DEFAULT (Equal to 1024) Usage and description information is yet to be added. Usage: --file-validation="chksm" --chksm-parameter=[N] (To calculate checksum with the first N bytes of the file, at most 10MB) --file-validation="chksm-full" (To calculate checksum with the entire file, at most 10MB) --file-validation="chksm-period" --chksm-parameter=[N] (To calculate checksum with every Nth byte of the file) Signed-off-by: Ajay Bharadwaj <[email protected]>
This adds functions to find, store and compare with the stored checksum. The file is mapped 10MB at a time. Signed-off-by: Ajay Bharadwaj <[email protected]>
AnorexicAtticusFinch
force-pushed
the
checksum
branch
from
c9b1e36 to
c5af0ad
Compare
|
The criu-dev branch has been force-pushed. Please rebase this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds CLI options and the checksum file validation method in addition to the previous build-ID and file size checks as part of GSoC 2020.
CRIU uses the build-ID validation method by default whenever possible and if that cannot be done, it will try and use the checksum method with the first 1 MB of the file. If this fails as well, it will use only the file size check.
To use the checksum method first and make the build-ID method the fallback:
--file-validation="chksm" --chksm-parameter=[N]
(To calculate checksum with the first N bytes of the file, at most 10 MB)
--file-validation="chksm-full"
(To calculate checksum with the entire file, at most 10 MB)
--file-validation="chksm-period" --chksm-parameter=[N]
(To calculate checksum with every Nth byte of the file)
To explicitly use only the file size check all the time, the following command line option can be used: --file-validation="filesize"
In other words, --file-validation="buildid" will not make a difference as this is the default method
Using the time command to measure the time taken for 2 tests.
zdtm/transition/shmem:
file size: 3.782s
build-id: 4.153s
chksum (Parameter = 1024): 4.465s
chksum-full: 4.722s
chksum-period (Parameter = 1024): 4.498s
zdtm/static/maps04:
file size: 35.317s
build-id: 35.720s
chksum (Parameter = 1024): 35.919s
chksum-full: 36.679s
chksum-period (Parameter = 1024): 36.476s
Mentors: @avagin @0x7f454c46