[CRIU] [PATCH v2] rework criu check logic

[Pavel Emelyanov]

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.

> 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

-- Pavel