[CRIU] [PATCH v2] rework criu check logic
Saied Kazemi
saied at google.com
Mon Mar 14 16:14:42 PDT 2016
Yes, there's definitely a bug. Filipe was suggesting that perhaps the
prctl() calls are failing not due to lack of support in the kernel but due
to some other error (e.g., passing wrong parameters). This causes "criu
check" to think the old API is not supported but in reality it is and
that's why c/r works.
Below is a complete log showing all your patches applied. Hope this helps!
--Saied
$ uname -r
3.13.0-77-generic
$ git log -n 3 --oneline
6576008 check: Call sbrk to get current brk
7ae34a7 check: Use errno to check for prctl error code (for exe file)
e37fc4f check: Use errno to check for prctl error code (for brk setting)
$ make clean
...
$ make -j 12
...
$ sudo ./criu/criu check
prctl: PR_SET_MM_MAP is not supported, which is required for restoring user
namespaces
Error (cr-check.c:629): Kernel doesn't support PTRACE_O_SUSPEND_SECCOMP
Error (cr-check.c:673): Dumping seccomp filters not supported: Input/output
error
Error (timerfd.c:55): timerfd: No timerfd support for c/r: Inappropriate
ioctl for device
Error (cr-check.c:307): fdinfo doesn't contain the mnt_id field
Error (cr-check.c:770): AIO remap doesn't work properly
Error (cr-check.c:786): fdinfo doesn't contain the lock field
Error (cr-check.c:833): cgroupns not supported. This is not fatal.
$ echo $?
1
$ sudo ./criu/criu check --ms
Warn (cr-check.c:195): Skipping unssuported PR_SET_MM_MAP
Error (cr-check.c:629): Kernel doesn't support PTRACE_O_SUSPEND_SECCOMP
Error (cr-check.c:673): Dumping seccomp filters not supported: Input/output
error
Error (timerfd.c:55): timerfd: No timerfd support for c/r: Inappropriate
ioctl for device
Error (cr-check.c:307): fdinfo doesn't contain the mnt_id field
Warn (cr-check.c:773): Skipping unsupported AIO remap
Warn (cr-check.c:789): fdinfo doesn't contain the lock field
Warn (cr-check.c:827): Skipping cgroup namespaces check
$ echo $?
1
$ ../a.out &
[1] 21409
$ sudo ./criu/criu dump -D /tmp/criu -o dump.log -v4 -j -t 21409
[1]+ Killed ../a.out
$ echo $?
0
$ sudo ./criu/criu restore -D /tmp/criu -o retore.log -v4 -j -t 21409 -d
$ echo $?
0
$ ps -efl | grep a.out
5 R saied 21409 1 99 80 0 - 1047 - 16:05 pts/0 00:00:06
../a.out
0 S saied 21431 8198 0 80 0 - 5938 pipe_w 16:05 pts/0 00:00:00
grep --color=auto a.out
On Mon, Mar 14, 2016 at 3:20 PM, Pavel Emelyanov <xemul at virtuozzo.com>
wrote:
> On 03/14/2016 08:35 PM, Saied Kazemi wrote:
> >
> >
> > On Mon, Mar 14, 2016 at 1:22 AM, Pavel Emelyanov <xemul at virtuozzo.com
> <mailto:xemul at virtuozzo.com>> wrote:
> >
> > On 03/12/2016 01:53 AM, Saied Kazemi wrote:
> > >
> > >
> > > On Fri, Mar 11, 2016 at 5:04 AM, Pavel Emelyanov <
> xemul at virtuozzo.com <mailto:xemul at virtuozzo.com> <mailto:
> xemul at virtuozzo.com <mailto:xemul at virtuozzo.com>>> wrote:
> > >
> > > On 03/11/2016 12:01 AM, Saied Kazemi wrote:
> > > > Pavel and Adrian,
> > > >
> > > > Please take a look at this patch and let me know what you
> think.
> > > >
> > > > As you will see, I have changed some pr_error() to pr_warn()
> when calling
> > > > "criu check --extra" because we're saying that the extra
> features do not
> > > > necessarily cause dump/restore failures. But we can't
> change all pr_error()
> > > > calls to pr_warn() indiscriminately in the functions that
> are called by
> > > > "criu check --extra" because some of them are real errors.
> > >
> > > Well, yes. If you're talking about pr_perror()-s that are
> called by non cr-check.c
> > > code, we can shut this down by manually lowering the log level.
> > >
> > > As far as such replacements in cr-check.c, this seem to be
> related to which return
> > > code we want to have when extra features checks fail.
> > >
> > > > Another item of discussion is the exit code. Obviously, if
> there are no
> > > > errors or warnings the exit code has to be zero. But what
> if there's a
> > > > warning on an extra or specific feature?
> > > > Currently, the exit code is non-zero but one can argue that
> for warnings it
> > > > should still be zero.
> > >
> > > I would say it should be non-zero, as user _explicitly_ asks
> to check for extra
> > > feature, and if it's absent, then the check has failed. And,
> respectively, the
> > > pr_err/_perror should probably remain such.
> > >
> > > > The current patch is a definite improvement (because it was
> broken) but to make
> > > > it "perfect" will require more time and iterations. It's
> also very difficult to
> > > > test all permutations.
> > >
> > > I like the patch and want to merge it, but have only one
> comment, please, find it
> > > inline:
> > >
> > >
> > > > @@ -182,38 +183,13 @@ static int check_prctl(void)
> > > > return -1;
> > > > }
> > > >
> > > > - /*
> > > > - * Either new or old interface must be supported
> in the kernel.
> > > > - */
> > > > - ret = prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE,
> (unsigned long)&size, 0, 0);
> > > > - if (ret) {
> > > > - if (!opts.check_ms_kernel) {
> > > > - pr_msg("prctl: PR_SET_MM_MAP is
> not supported, which "
> > > > - "is required for
> restoring user namespaces\n");
> > > > - return -1;
> > > > - } else
> > > > - pr_warn("Skipping unssuported
> PR_SET_MM_MAP\n");
> > > > -
> > > > - ret = prctl(PR_SET_MM, PR_SET_MM_BRK,
> brk(0), 0, 0);
> > > > - if (ret) {
> > > > - if (ret == -EPERM)
> > > > - pr_msg("prctl: One needs
> CAP_SYS_RESOURCE capability to perform testing\n");
> > > > - else
> > > > - pr_msg("prctl: PR_SET_MM
> is not supported\n");
> > > > - return -1;
> > > > - }
> > > > -
> > > > - ret = prctl(PR_SET_MM,
> PR_SET_MM_EXE_FILE, -1, 0, 0);
> > > > - if (ret != -EBADF) {
> > > > - pr_msg("prctl:
> PR_SET_MM_EXE_FILE is not supported (%d)\n", ret);
> > > > - return -1;
> > > > - }
> > > > -
> > > > - ret = prctl(PR_SET_MM, PR_SET_MM_AUXV,
> (long)&user_auxv, sizeof(user_auxv), 0);
> > > > - if (ret) {
> > > > - pr_msg("prctl: PR_SET_MM_AUXV is
> not supported\n");
> > > > - return -1;
> > > > - }
> > >
> > > The checks for PR_SET_MM_BRK, _EXE_FILE and _AUXV are still
> necessary, since they
> > > were added exclusively for CRIU. And CRIU still calls this
> stuff actually (in user
> > > mode), so presence of these things should be left in Category
> 1.
> > >
> > > -- Pavel
> > >
> > >
> > > This is a bit confusing. On my system (running 3.13 kernel) criu
> freshly created from the head
> > > will fail both "criu check" and "criu check --ms" which means I do
> not have either the new API
> > > (prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, ...)) or the old API
> (prctl(PR_SET_MM, PR_SET_MM_BRK, ...)).
> > > However, I can successfully dump and restore a simple main() {
> while (1); } C binary.
> >
> > Hm... That's because we have a number of bugs in there :( I'll send
> some patches soon.
> >
> >
> > The patches fix the bugs where ret was confused with errno. But even
> with those patches,
> > both "criu check" and "criu check --ms" fail while I can still
> successfully dump and restore
> > the loop process.
>
> In my 3 patches set there was patch #3, that was not in Filipe's patch --
> it
> fixes the brk() to be sbrk(). Would you check whether this patch helps?
> And if
> no -- would you show the criu check with and without --ms output?
>
> > My expectation is that if "criu check ]--ms]" fails, I should not be
> able to dump and restore.
>
> Yes, there's something weird with these prctls :\
>
> > > Why PR_SET_MM_BRK, _EXE_FILE and _AUXV are absolutely required as
> Category 1 features?
> >
> > Because the PR_SET_MM_MAP_SIZE was introduced in 3.18 (or 3.17) with
> >
> https://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f606b77f1
> >
> >
> > This explains why PR_SET_MM_MAP_SIZE was introduced and how it works.
> But pertaining to
> > my comment above where I can successfully c/r, it does not explain why
> it has to be a Category
> > 1 feature. Sorry if I am missing something obvious.
>
> Well, for now I bet there's still some bug with this code :)
>
> -- Pavel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20160314/0aaf5e62/attachment-0001.html>
More information about the CRIU
mailing list