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