[CRIU] [PATCH v2] rework criu check logic

[Saied Kazemi]

On Mon, Mar 14, 2016 at 1:22 AM, Pavel Emelyanov <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>> 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.

My expectation is that if "criu check ]--ms]" fails, I should not be able
to dump and restore.



> > 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.

--Saied
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20160314/85756553/attachment-0001.html>