<div dir="ltr">It seems like patchwork is picking up the patches, but it's unhappy with the formatted corruption caused by my Vim config<div>(basically, tabs were rendered as spaces and caused merge conflicts when included in the patch).</div><div>Let me try one last time before I'm banned from this mailing list for spam :)</div><div><br></div><div><br></div><div><br></div><div><div>From: Radoslaw Burny <rburny at <a href="http://google.com">google.com</a>></div><div><br></div><div>Subject: [PATCH] Fix UB in choose_service_fd_base.</div><div><br></div><div>Signed-off-by: Radoslaw Burny <rburny at <a href="http://google.com">google.com</a>></div><div><br></div><div>---</div><div> criu/util.c | 4 ++--</div><div> 1 file changed, 2 insertions(+), 2 deletions(-)</div><div><br></div><div>diff --git a/criu/util.c b/criu/util.c</div><div>index b19bf517..48ba09a8 100644</div><div>--- a/criu/util.c</div><div>+++ b/criu/util.c</div><div>@@ -588,9 +588,9 @@ static int choose_service_fd_base(struct pstree_item *me)</div><div> <span style="white-space:pre"> </span>nr += 16; /* Safety pad */</div><div> <span style="white-space:pre"> </span>real_nr = nr;</div><div> </div><div>-<span style="white-space:pre"> </span>nr /= (1024 / sizeof(void *));</div><div>+<span style="white-space:pre"> </span>/* Align nr to the power of 2 for easier debugging */</div><div>+<span style="white-space:pre"> </span>BUG_ON(nr <= 0);</div><div> <span style="white-space:pre"> </span>nr = 1 << (32 - __builtin_clz(nr));</div><div>-<span style="white-space:pre"> </span>nr *= (1024 / sizeof(void *));</div><div> </div><div> <span style="white-space:pre"> </span>if (nr > service_fd_rlim_cur) {</div><div> <span style="white-space:pre"> </span>/* Right border is bigger, than rlim. OK, then just aligned value is enough */</div><div>-- </div><div>2.17.0.484.g0c8726318c-goog</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 9, 2018 at 6:19 PM, Kirill Tkhai <span dir="ltr"><<a href="mailto:ktkhai@virtuozzo.com" target="_blank">ktkhai@virtuozzo.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 09.04.2018 19:16, Radoslaw Burny wrote:<br>
> Ah, I see. Even with the fds that CRIU adds, it's still possible to fit<br>
> below 64 or even 32.<br>
> I've reverted the patch to the version you suggested and attached it below<br>
> - I presume this is the code review process for CRIU, right?<br>
<br>
</span>I assume, criu patchwork will be able to pick this patch up from the reply. Andrew, is this OK?<br>
<span class=""><br>
> BTW, if you want to make any changes to the patch before merging it -<br>
> feel free to do so :)<br>
> It will be faster than us two iterating over the email.<br>
><br>
> Thanks!<br>
<br>
</span>Thanks,<br>
Kirill<br>
<div class="HOEnZb"><div class="h5"><br>
> On Mon, Apr 9, 2018 at 6:03 PM, Kirill Tkhai <<a href="mailto:ktkhai@virtuozzo.com">ktkhai@virtuozzo.com</a>> wrote:<br>
><br>
>> On 09.04.2018 17:57, Radoslaw Burny wrote:<br>
>>> From: Radoslaw Burny <<a href="mailto:rburny@google.com">rburny@google.com</a>><br>
>>><br>
>>> Subject: [PATCH] Fix UB in choose_service_fd_base.<br>
>>><br>
>>> Signed-off-by: Radoslaw Burny <<a href="mailto:rburny@google.com">rburny@google.com</a>><br>
>>><br>
>>> ---<br>
>>> criu/util.c | 4 ++--<br>
>>> 1 file changed, 2 insertions(+), 2 deletions(-)<br>
>>><br>
>>> diff --git a/criu/util.c b/criu/util.c<br>
>>> index b19bf517..48ba09a8 100644<br>
>>> --- a/criu/util.c<br>
>>> +++ b/criu/util.c<br>
>>> @@ -588,9 +588,9 @@ static int choose_service_fd_base(struct pstree_item<br>
>>> *me)<br>
>>> nr += 16; /* Safety pad */<br>
>>> real_nr = nr;<br>
>>><br>
>>> - nr /= (1024 / sizeof(void *));<br>
>>> + /* Align nr to the power of 2 for easier debugging */<br>
>>> + BUG_ON(nr <= 0);<br>
>>> nr = 1 << (32 - __builtin_clz(nr));<br>
>>> - nr *= (1024 / sizeof(void *));<br>
>>><br>
>>> if (nr > service_fd_rlim_cur) {<br>
>>> /* Right border is bigger, than rlim. OK, then just<br>
>> aligned<br>
>>> value is enough */<br>
>><br>
>> It's not a round up power of 2, it's round up power of 2 rounded to 128.<br>
>> This goes from kernel: <a href="https://git.kernel.org/pub/" rel="noreferrer" target="_blank">https://git.kernel.org/pub/</a><br>
>> scm/linux/kernel/git/torvalds/<wbr>linux.git/tree/fs/file.c#n85<br>
>><br>
>> #include <limits.h><br>
>> int main(void)<br>
>> {<br>
>> unsigned int i, was, nr;<br>
>><br>
>> for (i = 0; i < INT_MAX; i++) {<br>
>> nr = i;<br>
>> nr /= (1024 / sizeof(void *));<br>
>> if (nr)<br>
>> nr = 1 << (32 - __builtin_clz(nr));<br>
>> else<br>
>> nr = 1;<br>
>> nr *= (1024 / sizeof(void *));<br>
>><br>
>> printf("nr=%d, i=%d\n", nr, i);<br>
>> if (nr < i || (nr-1) & nr) {<br>
>> printf("error\n");<br>
>> exit(1);<br>
>> }<br>
>> }<br>
>><br>
>> return 0;<br>
>><br>
>> }<br>
>><br>
>> nr=128, i=16<br>
>> nr=128, i=17<br>
>> nr=128, i=18<br>
>> nr=128, i=19<br>
>> nr=128, i=20<br>
>> nr=128, i=21<br>
>> nr=128, i=22<br>
>> nr=128, i=23<br>
>> nr=128, i=24<br>
>> nr=128, i=25<br>
>> nr=128, i=26<br>
>> nr=128, i=27<br>
>> nr=128, i=28<br>
>> nr=128, i=29<br>
>> nr=128, i=30<br>
>><br>
><br>
><br>
><br>
> From: Radoslaw Burny <rburny at <a href="http://google.com" rel="noreferrer" target="_blank">google.com</a>><br>
><br>
><br>
><br>
> Subject: [PATCH] Fix UB in choose_service_fd_base (rev 2).<br>
><br>
><br>
><br>
><br>
><br>
> Signed-off-by: Radoslaw Burny <rburny at <a href="http://google.com" rel="noreferrer" target="_blank">google.com</a>><br>
><br>
><br>
><br>
> ---<br>
> criu/util.c | 5 ++++-<br>
> 1 file changed, 4 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/criu/util.c b/criu/util.c<br>
> index b19bf517..56a1683e 100644<br>
> --- a/criu/util.c<br>
><br>
> +++ b/criu/util.c<br>
><br>
> @@ -589,7 +589,10 @@ static int choose_service_fd_base(struct pstree_item<br>
> *me)<br>
> real_nr = nr;<br>
><br>
> nr /= (1024 / sizeof(void *));<br>
> - nr = 1 << (32 - __builtin_clz(nr));<br>
> + if (nr != 0)<br>
> + nr = 1 << (32 - __builtin_clz(nr));<br>
> + else<br>
> + nr = 1;<br>
> nr *= (1024 / sizeof(void *));<br>
><br>
> if (nr > service_fd_rlim_cur) {<br>
> --<br>
> 2.17.0.484.g0c8726318c-goog<br>
><br>
</div></div></blockquote></div><br></div>