<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 14, 2016 at 1:22 AM, Pavel Emelyanov <span dir="ltr"><<a href="mailto:xemul@virtuozzo.com" target="_blank">xemul@virtuozzo.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 03/12/2016 01:53 AM, Saied Kazemi wrote:<br>
<div><div class="h5">><br>
><br>
> On Fri, Mar 11, 2016 at 5:04 AM, Pavel Emelyanov <<a href="mailto:xemul@virtuozzo.com">xemul@virtuozzo.com</a> <mailto:<a href="mailto:xemul@virtuozzo.com">xemul@virtuozzo.com</a>>> wrote:<br>
><br>
> On 03/11/2016 12:01 AM, Saied Kazemi wrote:<br>
> > Pavel and Adrian,<br>
> ><br>
> > Please take a look at this patch and let me know what you think.<br>
> ><br>
> > As you will see, I have changed some pr_error() to pr_warn() when calling<br>
> > "criu check --extra" because we're saying that the extra features do not<br>
> > necessarily cause dump/restore failures. But we can't change all pr_error()<br>
> > calls to pr_warn() indiscriminately in the functions that are called by<br>
> > "criu check --extra" because some of them are real errors.<br>
><br>
> Well, yes. If you're talking about pr_perror()-s that are called by non cr-check.c<br>
> code, we can shut this down by manually lowering the log level.<br>
><br>
> As far as such replacements in cr-check.c, this seem to be related to which return<br>
> code we want to have when extra features checks fail.<br>
><br>
> > Another item of discussion is the exit code. Obviously, if there are no<br>
> > errors or warnings the exit code has to be zero. But what if there's a<br>
> > warning on an extra or specific feature?<br>
> > Currently, the exit code is non-zero but one can argue that for warnings it<br>
> > should still be zero.<br>
><br>
> I would say it should be non-zero, as user _explicitly_ asks to check for extra<br>
> feature, and if it's absent, then the check has failed. And, respectively, the<br>
> pr_err/_perror should probably remain such.<br>
><br>
> > The current patch is a definite improvement (because it was broken) but to make<br>
> > it "perfect" will require more time and iterations. It's also very difficult to<br>
> > test all permutations.<br>
><br>
> I like the patch and want to merge it, but have only one comment, please, find it<br>
> inline:<br>
><br>
><br>
> > @@ -182,38 +183,13 @@ static int check_prctl(void)<br>
> > return -1;<br>
> > }<br>
> ><br>
> > - /*<br>
> > - * Either new or old interface must be supported in the kernel.<br>
> > - */<br>
> > - ret = prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, (unsigned long)&size, 0, 0);<br>
> > - if (ret) {<br>
> > - if (!opts.check_ms_kernel) {<br>
> > - pr_msg("prctl: PR_SET_MM_MAP is not supported, which "<br>
> > - "is required for restoring user namespaces\n");<br>
> > - return -1;<br>
> > - } else<br>
> > - pr_warn("Skipping unssuported PR_SET_MM_MAP\n");<br>
> > -<br>
> > - ret = prctl(PR_SET_MM, PR_SET_MM_BRK, brk(0), 0, 0);<br>
> > - if (ret) {<br>
> > - if (ret == -EPERM)<br>
> > - pr_msg("prctl: One needs CAP_SYS_RESOURCE capability to perform testing\n");<br>
> > - else<br>
> > - pr_msg("prctl: PR_SET_MM is not supported\n");<br>
> > - return -1;<br>
> > - }<br>
> > -<br>
> > - ret = prctl(PR_SET_MM, PR_SET_MM_EXE_FILE, -1, 0, 0);<br>
> > - if (ret != -EBADF) {<br>
> > - pr_msg("prctl: PR_SET_MM_EXE_FILE is not supported (%d)\n", ret);<br>
> > - return -1;<br>
> > - }<br>
> > -<br>
> > - ret = prctl(PR_SET_MM, PR_SET_MM_AUXV, (long)&user_auxv, sizeof(user_auxv), 0);<br>
> > - if (ret) {<br>
> > - pr_msg("prctl: PR_SET_MM_AUXV is not supported\n");<br>
> > - return -1;<br>
> > - }<br>
><br>
> The checks for PR_SET_MM_BRK, _EXE_FILE and _AUXV are still necessary, since they<br>
> were added exclusively for CRIU. And CRIU still calls this stuff actually (in user<br>
> mode), so presence of these things should be left in Category 1.<br>
><br>
> -- Pavel<br>
><br>
><br>
> This is a bit confusing. On my system (running 3.13 kernel) criu freshly created from the head<br>
> will fail both "criu check" and "criu check --ms" which means I do not have either the new API<br>
> (prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, ...)) or the old API (prctl(PR_SET_MM, PR_SET_MM_BRK, ...)).<br>
> However, I can successfully dump and restore a simple main() { while (1); } C binary.<br>
<br>
</div></div>Hm... That's because we have a number of bugs in there :( I'll send some patches soon.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>My expectation is that if "criu check ]--ms]" fails, I should not be able to dump and restore.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> Why PR_SET_MM_BRK, _EXE_FILE and _AUXV are absolutely required as Category 1 features?<br>
<br>
</span>Because the PR_SET_MM_MAP_SIZE was introduced in 3.18 (or 3.17) with<br>
<a href="https://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f606b77f1" rel="noreferrer" target="_blank">https://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f606b77f1</a></blockquote><div><br></div><div>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.</div><div><br></div><div>--Saied</div><div> </div></div><br></div></div>