Two fixes (reported by coverity) and a minor nitpick:
1. Fix checking error from open_proc().
2. Fix buffer overflow. MAX_ULONG can be 20 characters long, so
ret = read() can return 20 and buf[ret] = 0 will overrun the buf.
Make a buf one character longer (an extra byte for \0) and pass
sizeof(buf) - 1 to read to fix it.
3. Call close() right after read().
This is a fixup to commit e68bded.
Reported by Coverity, CID 168505, 168504.
Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Kir Kolyshkin <kir@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
Currently, when we use cgroup freezer to seize the tasks we start freezer
and then without waiting the completion of transition procedure we are
seizing tasks read from freezer @tasks file, using fgets.
This is fragile construction because fgets uses internal buffer and tasks
we've read might be exiting same time while we're freezing them,
the kernel won't freeze these exiting tasks because they are dying
anyway and I fear we might read a pid here which is not even in
our cgroup anymore but reused with another out of cgroup task.
Thus lets do the following: use iterations to freeze tasks waiting
for freezer to change its state and then collect/seize all tasks
in one pass.
For example on container I'm playing with it takes just one iteration
| (00.013690) cg: Set 1 is criu one
| (00.013705) freezing processes: 1800000 attempst with 100 ms steps
| (00.013720) freezer.state=THAWED
| (00.013795) freezer.state=FREEZING
| (00.113962) freezer.state=FROZEN
| (00.113990) freezing processes: 1 attempts done
| (00.114073) SEIZE 240893 (comm systemd): success
| (00.114110) Warn (ptrace.c:121): Unable to interrupt task: 240905 (comm kthreadd/1) (Operation not permitted)
| (00.114136) Warn (ptrace.c:121): Unable to interrupt task: 240906 (comm khelper) (Operation not permitted)
| (00.114155) SEIZE 240969 (comm screen): success
| (00.114166) SEIZE 240970 (comm sendmail): success
| (00.114179) SEIZE 240971 (comm sendmail): success
| (00.114189) SEIZE 240972 (comm saslauthd): success
| (00.114202) SEIZE 240973 (comm crond): success
| (00.114211) SEIZE 240974 (comm agetty): success
| (00.114221) SEIZE 240975 (comm agetty): success
| ...
Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
Acked-by: Andrew Vagin <avagin@gmail.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
When we're freezing processes we don't count on anything but
rather do 5 attempts with constantly increasing delay.
Lets rather do the following:
- take --timeout option into account (which is 5 seconds
by default) and split it into 100 ms steps;
- when frezing processes check freezer status every 100 ms.
Same time it looks that 5 seconds by default is too small
for high loaded containers. Lets increase it to 10 seconds
by default.
[ skinsbursky@:
Frankly speaking, in this particular case increasing intervals are not nice.
This is not a network issue or something.
Usually freezing takes less than a second, but more, that, say 200ms.
Otherwise it takes quite o lot of time.
If step size is growing all the time, in most of the cases criu will
waste hundreds of milliseconds between iterX (say, third) and (iterX+1)
because of the growing step size.
100ms step looks solid enough: not to small to produce a lot of syscalls
and not to large to waste a lot of time.
With previous scheme freezing was usually taking half a second more that
it should because of this growing step.
[ gorcunov@:
You won't belive, but been able to sepcify --timeout 0 here allowed
me and Stas to catch serieous problem in freezer code.
https://lkml.org/lkml/2016/8/3/317
Without this feature we would have to patch criu instead. So you know,
this would be great to keep it for catching more kernel bugs ;)
Reported-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
This is an unix dgram socket which doesn't have an address and
isn't connected to somewhere, so we can use one socket for all processes.
v2: return non-zero code in error cases
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
These files have to be removed after successful restore.
v2:
Check link remap files only for tests with "--link-remap" option in
descriptor.
Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
It should be faster and we don't need to parse a string.
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
Currently if socket() or connect() syscall-s failed, parasite cures itself,
but criu has not got any signals and waits on accept().
This patch adds a futex to synchronize parasite and criu. The server socket
is created with SOCK_NONBLOCK and waits on the futex when a parasite
connects to it, only then criu calls accept() and it returns immediately.
Reported-by: Yohei Kamitsukasa <uhoidx@gmail.com>
Cc: Yohei Kamitsukasa <uhoidx@gmail.com>
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Reviewed-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
We found that sometimes a restored tcp socket doesn't work.
A reason of this bug is incorrect window parameters and in this case
tcp_acceptable_seq() returns tcp_wnd_end(tp) instead of tp->snd_nxt. The
other side drops packets with this seq, because seq is less than
tp->rcv_nxt ( tcp_sequence() ).
We need to restore window parameters to avoid such side effects.
https://github.com/xemul/criu/issues/168
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
It's a new option to get/set window parameters.
v2: don't do this check to unprivileged users, because TCP_REPAIR
is protected by CAP_NET_ADMIN.
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
When using --root option in criu dump, when the mountpoint passed
contains a symbolic link, criu does not resolve its parent correctly.
e.g when passing --root /run/rootfs, dump finishes successfully;
but when /var/run/rootfs is passed, where /var/run is symbolic link to
/run it exits with error "New root and old root are the same".
Signed-off-by: Katerina Koukiou <k.koukiou@gmail.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
realloc() may move a memory chunk in case of shrink.
v4: New
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
For `criu exec` we are searching for a place for syscall injection.
While searching for a VMA with PROT_EXEC and with needed size,
we check that VMA is lower than task_size.
The callpath for it is:
cr_exec => parasite_prep_ctl => get_vma_by_ip
Firstly, I thought to omit kdat.task_size checking if it's not inited:
> if (vma_area->e->start >= kdat.task_size && kdat.task_size)
but I think it's a hack then a proper solution.
Besides, this code still can choose VMA over task_size on ARM
and try to inject syscall there (IIRC, ARM has kernel-mapped
VMA in that area).
So, lets init kdat.task_size for `criu exec`.
Also lets init kdat.has_compat_sigreturn so we could exec into
compatible applications.
Cc: Christopher Covington <cov@codeaurora.org>
Cc: Andrew Vagin <avagin@virtuozzo.com>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Reviewed-by: Christopher Covington <cov@codeaurora.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
So, how it was working:
1. build-as was declared with $$(1) and $$(2) which were expanded
on entering the submake;
2. function $(call build-as,...) performed the second expansion of
build-as.
Cons: build-as works only in sub-makefile, no sub-sub-makefile, no
upper/top makefile.
Simplify this by single $(1).
Then build-as variable will be used _only_ in makefile, not in
sub-makefiles.
This is for now fine, as each file, that calls $(MAKE) with
$(build)=dir or $(call build-as,makefile,dir) will include main.mk
from NMK, which has build-as definition (from include.mk).
In the future, we'll get rid of $(build) and $(build-as) workarounds
as finally switch to building from a global makefile.
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
It looks like, there is not so much that needs to be fixed for
building criu from a top directory.
After the patch it's possible to do `make criu/mount.o` i.e.
It will build protobuf, compel as dependencies (if they are not built),
but no more from criu objects. If something breaks, you can
do make from vim and jump to error. Nice.
Mostly the patch corrects pathes to objects - I tried to make them
depend on $(obj) or $(SRC_DIR)/criu, where it's possible.
After it tested:
`make -j 10`, `make criu/log.o`, `make clean`, `make mrproper`,
`make install DESTDIR=/tmp/criu`, `make uninstall DESTDIR=/tmp/criu`
Note: I improperly called v1 for this patch as "return to make from
top Makefile" -- but I didn't mean that (and it was friday ;)
This patch doesn't yet switch to top-Makefile building, but that's
a step in that way (building from a top Makefile needs correct pathes
in makefiles) which also adds ability to build objects in subdirectories
and etc.
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
For example, if a zombie has a helper that sets up its session id, the
zombie will be reparented to the init task, which will then potentially get
a SIGCHLD for a task which isn't its direct child zombie, which we didn't
handle. Instead, let's find all the zombies for the init task, in case they
get reparented this way.
v2: only the zombies need to be recursively collected, helpers wait on
their children before they exit and will never be reparented
v4: the root task waits all zombies
Reported-by: Tycho Andersen <tycho.andersen@canonical.com>
Cc: Tycho Andersen <tycho.andersen@canonical.com>
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
Seems this snippet escaeped from commit
84bf1ad467942a145a9df99d1397a63817893a91
so we may get -EBUSY in open_detach_mount.
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Andrey Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
VDSO_SYMBOL_MAX is max number of symbols, not their max length.
Fixes my buggy commit: 4c69339cd2b7 ("string.h/pie: use builtin strncmp
instead of strcmp"). Sorry for that bogus misprinting.
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
When restoring on a different node, it may happen that pid_max is
below one of the pid we wanted to recreate.
This leads to a restore error when cloning the restarted process:
(00.011172) Forking task with 44794 pid (flags 0x0)
(00.011205) Error (cr-restore.c:1008): 44794: Write 44793 to sys/kernel/ns_last_pid: Invalid argument
This patch computes the largest pid value and sets the kernel pid_max if
necessary.
If the user don't have the permission to do so, the restart is
failing mentioning that we can't push the pid_max limit.
Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
It looks like, it's completely not needed here.
criu/cgroup.c:582:4: warning: Value stored to 'name' is never read
name = cc->name + 5;
^ ~~~~~~~~~~~~
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
In cr-restore printf() format is mixing "%p" and the prefix "0x" which is
already managed by "%p". This leads to log lines like:
(00.053282) 38744: Found bootstrap VMA hint at: 0x0x100000 (needs ~576K)
Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
Instead, let's skip it before we fork.
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
We masked off SIGCHLD in wait_on_helpers_zombies(), but in fact this is too
late: zombies can die any time after CR_STATE_RESTORE before this function
is called, which lead to us getting "unexpected" deaths. Instead, we should
mask off SIGCHLD before the helpers finish CR_STATE_RESTORE, since they're
explicitly going to wait on all their kids to make sure they don't die
anyway.
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
Acked-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
v2: drop /bin/ps from test deps
v3: wait for the zombie to make sure it exits
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
Acked-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
In the next patch, we'll introduce an option to allow for leaving zombie
processes in the pid ns for the test so that we can test the behavior of
zombies. Let's not reap everything after restore, since we'll reap the
restored zombies as well.
v2: restore the old behavior when in reap mode
CC: Andrey Vagin <avagin@openvz.org>
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
Acked-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
We'll use this variable in the next test to make sure the test suite
doesn't accidentally reap the zombie we want to leave around for the actual
test.
This is kind of ugly and there might be a better way to pass information to
the test's init, I'm open for suggestions :)
CC: Andrey Vagin <avagin@openvz.org>
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
Acked-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
When fixing mprotected (ro) sysvshmems I used the PROT_EXEC flag
to keep the information about whether the segment itself should
be rw or ro. This flag leaked to sys_mprotect and some attachments
of the segment became executable after restore.
Fix this by dropping the EXEC flag.
https://github.com/xemul/criu/issues/180
Reported-by: Andrey Vagin <avagin@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
Acked-by: Andrew Vagin <avagin@virtuozzo.com>
It is always not NULL in sigreturn_restore().
CID 164716 (#1 of 1): Dereference after null check (FORWARD_NULL)
64. var_deref_model: Passing tcore to construct_sigframe, which dereferences null tcore->thread_core. [show details]
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
It's generated and cleaned in the top Makefile.
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
I think, we can simplify criu's makefile by moving packages
checks out to special makefile.
Now we only need to make criu's target depend on 'check-packages'.
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
The root yard is used to clean up ghost files.
Now try_clean_remaps() is called from depopulate_roots_yard(), so
the code about switching mount namespaces was moved to
depopulate_roots_yard().
v2: call clean_remaps() when processes are restored in
the host mount namespace.
Now depopulate_roots_yard() is called from the root task before
finishing CR_STATE_FORKING.
I moved it to the criu process and do it after clean_remaps(), because
clean_remaps() uses the roots yard.
It's called after openning all files, because only at this moment we can
be sure that all link remap files can be removed.
restore_task_with_children() | restore_root_task()
-----------------------------------------------------------------------
depopulate_roots_yard() |
restore_finish_stage(CR_STATE_FORKING) |
prepare_fds() |
open_vmas() |
| restore_switch_stage(CR_STATE_RESTORE_SIGCHLD)
| clean_remaps = 0;
If something fails between CR_STATE_FORKING and CR_STATE_RESTORE_SIGCHLD,
try_clean_remaps will be called().
try_clean_remaps()
try_clean_ghost()
rst_get_mnt_root()
print_ns_root()
snprintf(buf, bs, "%s/%d", mnt_roots, ns->id);
it uses mnt_roots, actually it is what we called the roots yard.
Signed-off-by: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
It looks like, it's possible that cores for some threads
were allocated, but not for all - allocation failed in
pstree_alloc_cores(). And after that we will dereference
NULL pointer as pstree_free_cores() doesn't check pointer:
pstree.c:28:6: warning: Access to field 'tc' results in a dereference of a null pointer (loaded from variable 'core')
if (core->tc && core->tc->timers)
^~~~~~~~
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
plugin.c:123:3: warning: Potential leak of memory pointed to by 'd'
dlclose(h);
^~~~~~~
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
path.c:98:2: warning: Value stored to 'len' is never read
len -= off;
^ ~~~
path.c:99:2: warning: Value stored to 'path' is never read
path += off;
^ ~~~
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
There is call to close_page_read on open_pages_image_at failure,
also on failure of init_pagemaps. pmes[] is uninitialized here
and free_pagemaps() will try to walk them and call xfree().
Which surely would lead to crash.
pagemap.c:317:6: warning: Branch condition evaluates to a garbage value
if (pr->pmes)
^~~~~~~~
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
cr-restore.c:1516:9: warning: Value stored to 'pid' during its initialization is never read
pid_t pid = item->pid.real;
^~~ ~~~~~~~~~~~~~~
cr-restore.c:1570:9: warning: Value stored to 'pid' during its initialization is never read
pid_t pid = item->pid.real;
^~~ ~~~~~~~~~~~~~~
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
Should we like, free them?
cgroup.c:890:11: warning: Potential leak of memory pointed to by 'cg.sets'
return -1;
^
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
There is a bug, that if vsnprintf() wrote nothing to buffer:
that may be xstrcat(0, "%s", "") or something like that,
than vsnprintf's return value is 0, which will be lesser than
delta. The code before would do following:
o first cycle:
1. relocate str to new (str is not allocated anymore)
2. vsnprintf() retured 0, delta is greater.
o second cycle:
1. relocate previously freed str to new..^C ^C
Segmentation fault (core dumped)
Weeell, I do think, we can do better job here.
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
This is a little bit of a hack. The problem is that we can't actually write
this value if memory.use_hierarchy is set, which it is by default.
Additionally, we can't do a hack like unsetting memory.use_hierarchy and
then writing this, because if the bit is set on the parent, unsetting it
will fail. So the restore *can* succeed if things are configured correctly
initially, but won't by default, which is annoying for the tests.
Plus in the case of systemd, there are child cgroups, so we can't ever
unset the root's memroy.use_hierarchy anyway, meaning we could never
actually restore correctly. Instead, let's just not try to write the
default value, which is probably what everyone is using anyway.
Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
CC: Andrew Vagin <avagin@virtuozzo.com>
Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
