[CRIU] [PATCH v2] rework criu check logic
Pavel Emelyanov
xemul at virtuozzo.com
Mon Mar 14 15:20:49 PDT 2016
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
More information about the CRIU
mailing list