BUG: CRIU corrupt floating point state after checkpoint

Submitted by Dmitry Safonov on Sept. 25, 2019, 1:51 a.m.

Details

Message ID c96c4697-01a2-c5df-8c68-e3e7d27e0802@gmail.com
State New
Series "BUG: CRIU corrupt floating point state after checkpoint"
Headers show

Commit Message

Dmitry Safonov Sept. 25, 2019, 1:51 a.m.
On 9/25/19 2:41 AM, Diyu Zhou wrote:
> Sure. See:
> https://github.com/vmexit/fpu-debug
> 
> I'm very new to github, so let me know if there is any issue
> accessing the link.

Ok, might this quick-fast oneliner help?

--->8---

Patch hide | download patch | download mbox

diff --git a/compel/src/main.c b/compel/src/main.c
index 51bac099fe5e..ca3c0318093e 100644
--- a/compel/src/main.c
+++ b/compel/src/main.c
@@ -41,7 +41,7 @@  typedef struct {
 static const flags_t flags = {
 #if defined CONFIG_X86_64
        .arch           = "x86",
-       .cflags         = COMPEL_CFLAGS_PIE,
+       .cflags         = COMPEL_CFLAGS_PIE " -mfpmath=387",
        .cflags_compat  = COMPEL_CFLAGS_NOPIC,
 #elif defined CONFIG_AARCH64
        .arch           = "aarch64",

Comments

Dmitry Safonov Sept. 25, 2019, 1:54 a.m.
On 9/25/19 2:51 AM, Dmitry Safonov wrote:
> On 9/25/19 2:41 AM, Diyu Zhou wrote:
>> Sure. See:
>> https://github.com/vmexit/fpu-debug
>>
>> I'm very new to github, so let me know if there is any issue
>> accessing the link.
> 
> Ok, might this quick-fast oneliner help?
> 
> --->8---
> diff --git a/compel/src/main.c b/compel/src/main.c
> index 51bac099fe5e..ca3c0318093e 100644
> --- a/compel/src/main.c
> +++ b/compel/src/main.c
> @@ -41,7 +41,7 @@ typedef struct {
>  static const flags_t flags = {
>  #if defined CONFIG_X86_64
>         .arch           = "x86",
> -       .cflags         = COMPEL_CFLAGS_PIE,
> +       .cflags         = COMPEL_CFLAGS_PIE " -mfpmath=387",

Maybe the right one would be actually -msoft-float  ^^
That should insert calls to a library, which will break the build if it
actually happens.

Thanks,
          Dmitry
Diyu Zhou Sept. 25, 2019, 2:02 a.m.
Unfortunately, neither option works.
The problem still persists.

On Tue, Sep 24, 2019 at 6:54 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
> On 9/25/19 2:51 AM, Dmitry Safonov wrote:
> > On 9/25/19 2:41 AM, Diyu Zhou wrote:
> >> Sure. See:
> >> https://github.com/vmexit/fpu-debug
> >>
> >> I'm very new to github, so let me know if there is any issue
> >> accessing the link.
> >
> > Ok, might this quick-fast oneliner help?
> >
> > --->8---
> > diff --git a/compel/src/main.c b/compel/src/main.c
> > index 51bac099fe5e..ca3c0318093e 100644
> > --- a/compel/src/main.c
> > +++ b/compel/src/main.c
> > @@ -41,7 +41,7 @@ typedef struct {
> >  static const flags_t flags = {
> >  #if defined CONFIG_X86_64
> >         .arch           = "x86",
> > -       .cflags         = COMPEL_CFLAGS_PIE,
> > +       .cflags         = COMPEL_CFLAGS_PIE " -mfpmath=387",
>
> Maybe the right one would be actually -msoft-float  ^^
> That should insert calls to a library, which will break the build if it
> actually happens.
>
> Thanks,
>           Dmitry
Dmitry Safonov Sept. 25, 2019, 2:08 a.m.
On 9/25/19 3:02 AM, Diyu Zhou wrote:
> Unfortunately, neither option works.
> The problem still persists.

Note, that before rebuild the pie directory should be cleaned like:
`git clean -fdx criu/pie`
seems like we missing some dependency line in a Makefile to rebuild pies
on changes to compel.

> On Tue, Sep 24, 2019 at 6:54 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>>
>> On 9/25/19 2:51 AM, Dmitry Safonov wrote:
>>> On 9/25/19 2:41 AM, Diyu Zhou wrote:
>>>> Sure. See:
>>>> https://github.com/vmexit/fpu-debug
>>>>
>>>> I'm very new to github, so let me know if there is any issue
>>>> accessing the link.
>>>
>>> Ok, might this quick-fast oneliner help?
>>>
>>> --->8---
>>> diff --git a/compel/src/main.c b/compel/src/main.c
>>> index 51bac099fe5e..ca3c0318093e 100644
>>> --- a/compel/src/main.c
>>> +++ b/compel/src/main.c
>>> @@ -41,7 +41,7 @@ typedef struct {
>>>  static const flags_t flags = {
>>>  #if defined CONFIG_X86_64
>>>         .arch           = "x86",
>>> -       .cflags         = COMPEL_CFLAGS_PIE,
>>> +       .cflags         = COMPEL_CFLAGS_PIE " -mfpmath=387",
>>
>> Maybe the right one would be actually -msoft-float  ^^
>> That should insert calls to a library, which will break the build if it
>> actually happens.

Hmm, that's fun: -msoft-float doesn't break the build for me..
But together " -msoft-float -mno-sse" breaks. Ugh:

[..]
  CC       criu/pie/util-vdso.o
  CC       criu/pie/util.o
  CC       criu/pie/util-vdso-elf32.o
In file included from /usr/include/stdlib.h:1010,
                 from criu/pie/util-vdso-elf32.c:1:
/usr/include/bits/stdlib-float.h: In function ‘atof’:
/usr/include/bits/stdlib-float.h:26:1: error: SSE register return with
SSE disabled
   26 | {
      | ^
In file included from /usr/include/stdlib.h:1010,
                 from criu/pie/util-vdso.c:1:
/usr/include/bits/stdlib-float.h: In function ‘atof’:
/usr/include/bits/stdlib-float.h:26:1: error: SSE register return with
SSE disabled
   26 | {
      | ^

I presume that we don't use atof() in parasite, but the include
<stdlib.h> prevents using -mno-sse.

[it's a night-time in my time zone, so I'm AFK for some hours]

Thanks,
          Dmitry
Diyu Zhou Sept. 25, 2019, 2:16 a.m.
I did a clean before building it. The program still persists.

> Hmm, that's fun: -msoft-float doesn't break the build for me..
> But together " -msoft-float -mno-sse" breaks. Ugh:

That breaks my compilation as well.

On Tue, Sep 24, 2019 at 7:08 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
> On 9/25/19 3:02 AM, Diyu Zhou wrote:
> > Unfortunately, neither option works.
> > The problem still persists.
>
> Note, that before rebuild the pie directory should be cleaned like:
> `git clean -fdx criu/pie`
> seems like we missing some dependency line in a Makefile to rebuild pies
> on changes to compel.
>
> > On Tue, Sep 24, 2019 at 6:54 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
> >>
> >> On 9/25/19 2:51 AM, Dmitry Safonov wrote:
> >>> On 9/25/19 2:41 AM, Diyu Zhou wrote:
> >>>> Sure. See:
> >>>> https://github.com/vmexit/fpu-debug
> >>>>
> >>>> I'm very new to github, so let me know if there is any issue
> >>>> accessing the link.
> >>>
> >>> Ok, might this quick-fast oneliner help?
> >>>
> >>> --->8---
> >>> diff --git a/compel/src/main.c b/compel/src/main.c
> >>> index 51bac099fe5e..ca3c0318093e 100644
> >>> --- a/compel/src/main.c
> >>> +++ b/compel/src/main.c
> >>> @@ -41,7 +41,7 @@ typedef struct {
> >>>  static const flags_t flags = {
> >>>  #if defined CONFIG_X86_64
> >>>         .arch           = "x86",
> >>> -       .cflags         = COMPEL_CFLAGS_PIE,
> >>> +       .cflags         = COMPEL_CFLAGS_PIE " -mfpmath=387",
> >>
> >> Maybe the right one would be actually -msoft-float  ^^
> >> That should insert calls to a library, which will break the build if it
> >> actually happens.
>
> Hmm, that's fun: -msoft-float doesn't break the build for me..
> But together " -msoft-float -mno-sse" breaks. Ugh:
>
> [..]
>   CC       criu/pie/util-vdso.o
>   CC       criu/pie/util.o
>   CC       criu/pie/util-vdso-elf32.o
> In file included from /usr/include/stdlib.h:1010,
>                  from criu/pie/util-vdso-elf32.c:1:
> /usr/include/bits/stdlib-float.h: In function ‘atof’:
> /usr/include/bits/stdlib-float.h:26:1: error: SSE register return with
> SSE disabled
>    26 | {
>       | ^
> In file included from /usr/include/stdlib.h:1010,
>                  from criu/pie/util-vdso.c:1:
> /usr/include/bits/stdlib-float.h: In function ‘atof’:
> /usr/include/bits/stdlib-float.h:26:1: error: SSE register return with
> SSE disabled
>    26 | {
>       | ^
>
> I presume that we don't use atof() in parasite, but the include
> <stdlib.h> prevents using -mno-sse.
>
> [it's a night-time in my time zone, so I'm AFK for some hours]
>
> Thanks,
>           Dmitry
Cyrill Gorcunov Sept. 25, 2019, 6:40 a.m.
On Wed, Sep 25, 2019 at 02:51:37AM +0100, Dmitry Safonov wrote:
> On 9/25/19 2:41 AM, Diyu Zhou wrote:
> > Sure. See:
> > https://github.com/vmexit/fpu-debug
> > 
> > I'm very new to github, so let me know if there is any issue
> > accessing the link.
> 
> Ok, might this quick-fast oneliner help?
> 
> --->8---
> diff --git a/compel/src/main.c b/compel/src/main.c
> index 51bac099fe5e..ca3c0318093e 100644
> --- a/compel/src/main.c
> +++ b/compel/src/main.c
> @@ -41,7 +41,7 @@ typedef struct {
>  static const flags_t flags = {
>  #if defined CONFIG_X86_64
>         .arch           = "x86",
> -       .cflags         = COMPEL_CFLAGS_PIE,
> +       .cflags         = COMPEL_CFLAGS_PIE " -mfpmath=387",
>         .cflags_compat  = COMPEL_CFLAGS_NOPIC,
>  #elif defined CONFIG_AARCH64
>         .arch           = "aarch64",

The parasite code indeed does use fpu :/ Crap. Still I think
we might simply need to save fpu state when entering parasite
and restore on cleanup, simply because compiler might use
fpu instructions for better performance on say u64 and etc.
Cyrill Gorcunov Sept. 25, 2019, 8:23 a.m.
On Wed, Sep 25, 2019 at 09:40:07AM +0300, Cyrill Gorcunov wrote:
> 
> The parasite code indeed does use fpu :/ Crap. Still I think
> we might simply need to save fpu state when entering parasite
> and restore on cleanup, simply because compiler might use
> fpu instructions for better performance on say u64 and etc.

I just recall that we are saving fpu frame on parasite injection,
thus something get screwed. Will take a look closer once time
permit.
Diyu Zhou Sept. 25, 2019, 4:24 p.m.
I think in the CRIU code,  the fpu frame saving and restoring is only performed
for the main thread. Other threads do not do that and thus cause the corruption.

I did a few experiment with the CRIU code. I'm confident the floating
point corruption occurs inside the function parasite_dump_thread_seized
in criu/parasite-syscall.c. Specifically, I suspect the parasite code run
by compel_run_in_thread(tctl, PARASITE_CMD_DUMP_THREAD) causes the floating
point corruption. I added a return 0; before that function and the
floating point corruption does not occur anymore.


On Wed, Sep 25, 2019 at 1:23 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Wed, Sep 25, 2019 at 09:40:07AM +0300, Cyrill Gorcunov wrote:
> >
> > The parasite code indeed does use fpu :/ Crap. Still I think
> > we might simply need to save fpu state when entering parasite
> > and restore on cleanup, simply because compiler might use
> > fpu instructions for better performance on say u64 and etc.
>
> I just recall that we are saving fpu frame on parasite injection,
> thus something get screwed. Will take a look closer once time
> permit.
Cyrill Gorcunov Sept. 25, 2019, 6:23 p.m.
On Wed, Sep 25, 2019 at 09:24:11AM -0700, Diyu Zhou wrote:
> I think in the CRIU code,  the fpu frame saving and restoring is only performed
> for the main thread. Other threads do not do that and thus cause the corruption.
> 
> I did a few experiment with the CRIU code. I'm confident the floating
> point corruption occurs inside the function parasite_dump_thread_seized
> in criu/parasite-syscall.c. Specifically, I suspect the parasite code run
> by compel_run_in_thread(tctl, PARASITE_CMD_DUMP_THREAD) causes the floating
> point corruption. I added a return 0; before that function and the
> floating point corruption does not occur anymore.

Great, thanks! So you've narrowed down the bug. Will take a look, thanks!
Diyu Zhou Sept. 25, 2019, 7:55 p.m.
You are welcome. Thank you all for your help and the wonderful tool: CRIU you
have created!

On Wed, Sep 25, 2019 at 11:23 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Wed, Sep 25, 2019 at 09:24:11AM -0700, Diyu Zhou wrote:
> > I think in the CRIU code,  the fpu frame saving and restoring is only performed
> > for the main thread. Other threads do not do that and thus cause the corruption.
> >
> > I did a few experiment with the CRIU code. I'm confident the floating
> > point corruption occurs inside the function parasite_dump_thread_seized
> > in criu/parasite-syscall.c. Specifically, I suspect the parasite code run
> > by compel_run_in_thread(tctl, PARASITE_CMD_DUMP_THREAD) causes the floating
> > point corruption. I added a return 0; before that function and the
> > floating point corruption does not occur anymore.
>
> Great, thanks! So you've narrowed down the bug. Will take a look, thanks!
Cyrill Gorcunov Sept. 27, 2019, 7:52 a.m.
jfyi, lm working on the issue. once i manage to narrow down the real cause
of it, will ping you.

On Wed, Sep 25, 2019, 22:55 Diyu Zhou <zhoudiyupku@gmail.com> wrote:

> You are welcome. Thank you all for your help and the wonderful tool: CRIU
> you
> have created!
>
> On Wed, Sep 25, 2019 at 11:23 AM Cyrill Gorcunov <gorcunov@gmail.com>
> wrote:
> >
> > On Wed, Sep 25, 2019 at 09:24:11AM -0700, Diyu Zhou wrote:
> > > I think in the CRIU code,  the fpu frame saving and restoring is only
> performed
> > > for the main thread. Other threads do not do that and thus cause the
> corruption.
> > >
> > > I did a few experiment with the CRIU code. I'm confident the floating
> > > point corruption occurs inside the function parasite_dump_thread_seized
> > > in criu/parasite-syscall.c. Specifically, I suspect the parasite code
> run
> > > by compel_run_in_thread(tctl, PARASITE_CMD_DUMP_THREAD) causes the
> floating
> > > point corruption. I added a return 0; before that function and the
> > > floating point corruption does not occur anymore.
> >
> > Great, thanks! So you've narrowed down the bug. Will take a look, thanks!
>
Cyrill Gorcunov Sept. 27, 2019, 1:14 p.m.
On Wed, Sep 25, 2019 at 09:23:23PM +0300, Cyrill Gorcunov wrote:
> 
> Great, thanks! So you've narrowed down the bug. Will take a look, thanks!

1) You've a bug in your program

for  (thrd = 0 ; thrd < nthreads ; thrd++)  {
   if  ((wspace[thrd] = calloc(1,sizeof(double *) * nelem)) == NULL)
     ERRperror("calloc workspaces")
}

you allocate _pointers_ to doubles not doubles themself and then
threat this space as full of doubles, which is obviously wrong.

Actually I fixed this problem, but issue remans, so I think there
might be other bugs in the code. Continue investigating...
Diyu Zhou Sept. 27, 2019, 4:09 p.m.
Thanks for pointing that out. It is indeed an embarrassing bug.

On Fri, Sep 27, 2019 at 6:14 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Wed, Sep 25, 2019 at 09:23:23PM +0300, Cyrill Gorcunov wrote:
> >
> > Great, thanks! So you've narrowed down the bug. Will take a look, thanks!
>
> 1) You've a bug in your program
>
> for  (thrd = 0 ; thrd < nthreads ; thrd++)  {
>    if  ((wspace[thrd] = calloc(1,sizeof(double *) * nelem)) == NULL)
>      ERRperror("calloc workspaces")
> }
>
> you allocate _pointers_ to doubles not doubles themself and then
> threat this space as full of doubles, which is obviously wrong.
>
> Actually I fixed this problem, but issue remans, so I think there
> might be other bugs in the code. Continue investigating...
Cyrill Gorcunov Sept. 30, 2019, 9:05 a.m.
On Fri, Sep 27, 2019 at 09:09:16AM -0700, Diyu Zhou wrote:
> Thanks for pointing that out. It is indeed an embarrassing bug.
> 

After modifying sources (to fix the issue with memory allocation)
and using explicit pid instead of `ps` output it doesnt trigger
anymore.

Could you please use the archive I'm attaching to verify if it
stop faulting for you?

Please note there is a symlink to criu itself inside sources
so adjust it the way it would point to your criu instance.
Diyu Zhou Sept. 30, 2019, 10:43 p.m.
The problem indeed goes away.

Do you have any insight why the corruption goes? As far as I can tell, the only
two bugs in the original code is the calloc and the thread worker function does
not return. After I have fixed these two bugs, with the original code,
the corruption is still there.

On Mon, Sep 30, 2019 at 2:05 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Fri, Sep 27, 2019 at 09:09:16AM -0700, Diyu Zhou wrote:
> > Thanks for pointing that out. It is indeed an embarrassing bug.
> >
>
> After modifying sources (to fix the issue with memory allocation)
> and using explicit pid instead of `ps` output it doesnt trigger
> anymore.
>
> Could you please use the archive I'm attaching to verify if it
> stop faulting for you?
>
> Please note there is a symlink to criu itself inside sources
> so adjust it the way it would point to your criu instance.
>
Cyrill Gorcunov Oct. 1, 2019, 6:56 a.m.
On Mon, Sep 30, 2019 at 03:43:43PM -0700, Diyu Zhou wrote:
> The problem indeed goes away.
> 
> Do you have any insight why the corruption goes? As far as I can tell, the only
> two bugs in the original code is the calloc and the thread worker function does
> not return. After I have fixed these two bugs, with the original code,
> the corruption is still there.

Not only these:

 - we should use explicit pid, not something we got from `ps` output
   (hell knows, which exactly process otherwise is being seized)

 - we should use barriers with threads, to make them working
   syncronically. Strictly speaking pthread_create/join should
   be like a sync points on their own but better to be on safe side

Anyway could you please add at least "pidfile" option (from the
code I've sent you) to your source code and check if it does
matter (don't forget to update duming shell script to use
explicit pid from a file)?
Diyu Zhou Oct. 2, 2019, 4:14 p.m.
> Anyway could you please add at least "pidfile" option (from the
> code I've sent you) to your source code and check if it does
> matter (don't forget to update duming shell script to use
> explicit pid from a file)?

Thanks for your help.

I have tried to let the program output pid to the file, the problemis still
there.

On Mon, Sep 30, 2019 at 11:56 PM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Mon, Sep 30, 2019 at 03:43:43PM -0700, Diyu Zhou wrote:
> > The problem indeed goes away.
> >
> > Do you have any insight why the corruption goes? As far as I can tell, the only
> > two bugs in the original code is the calloc and the thread worker function does
> > not return. After I have fixed these two bugs, with the original code,
> > the corruption is still there.
>
> Not only these:
>
>  - we should use explicit pid, not something we got from `ps` output
>    (hell knows, which exactly process otherwise is being seized)
>
>  - we should use barriers with threads, to make them working
>    syncronically. Strictly speaking pthread_create/join should
>    be like a sync points on their own but better to be on safe side
>
> Anyway could you please add at least "pidfile" option (from the
> code I've sent you) to your source code and check if it does
> matter (don't forget to update duming shell script to use
> explicit pid from a file)?
Cyrill Gorcunov Oct. 2, 2019, 4:23 p.m.
On Wed, Oct 02, 2019 at 09:14:50AM -0700, Diyu Zhou wrote:
> > Anyway could you please add at least "pidfile" option (from the
> > code I've sent you) to your source code and check if it does
> > matter (don't forget to update duming shell script to use
> > explicit pid from a file)?
> 
> Thanks for your help.
> 
> I have tried to let the program output pid to the file, the problemis still there.

Please attach the your new sources (just to be sure we're on the
same codebase), will try to take a look once time permit.
Diyu Zhou Oct. 2, 2019, 4:31 p.m.
Sure, please see attached.

On Wed, Oct 2, 2019 at 9:23 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Wed, Oct 02, 2019 at 09:14:50AM -0700, Diyu Zhou wrote:
> > > Anyway could you please add at least "pidfile" option (from the
> > > code I've sent you) to your source code and check if it does
> > > matter (don't forget to update duming shell script to use
> > > explicit pid from a file)?
> >
> > Thanks for your help.
> >
> > I have tried to let the program output pid to the file, the problemis still there.
>
> Please attach the your new sources (just to be sure we're on the
> same codebase), will try to take a look once time permit.