Re: [Xen-users] "xl restore" leaks a file descriptor?

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell]

On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
> It's the checkpoint file - i.e. the command line argument to xl
> restore - that is being leaked.

Thanks.

[...]
> So the checkpoint file is clearly being leaked.

Indeed. I confirmed this even with the current development version using ls
-l /proc/<pid>/fd which shows an fd open on a deleted file:

# ps aux| grep xl
root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl restore save
# ls -l /proc/20465/fd
[...]
lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
[...]
# rm /root/save
# ls -l /proc/20465/fd
[...]
lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
[...]

>  Its space is not freed
> until the 'xl restore' process is ended by shutting down the domain:
[...]
> 
> It seems like xl restore should close the checkpoint file as soon as
> it's done restoring the domain, allowing the space to be freed, but
> that's clearly not happening.

Right. In fact xl sets the file to be close-on-exec right after opening it,
which is before the daemonisation step, so it ought to be closed
automatically, but isn't for some reason.

My working theory is that something in the machinery which spawns the save
helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
unsetting CLOEXEC.

Any way, thanks for reporting. I've copied the devel list and 4.6 RM. Wei
this probably ought to be a blocker for 4.6 (and the fix ought ultimately
to be backported to 4.4 onwards at least).

NB: This leak seems to be independent of the switch to migration v2.

Ian.

> -Andrew
> 
> On Aug 11, 2015 04:55, "Ian Campbell" <ian.campbell@citrix.com> wrote:
> > 
> > On Fri, 2015-08-07 at 12:50 -0400, Andrew Armenia wrote:
> > > The issue appears to occur with any state file - not just one in
> > > particular.
> > 
> > Please give some specific examples e.g. paths to some of the files to 
> > which
> > a fd has been leaked. I'm trying to determine which state files I 
> > should be
> > investigating, since there are several things which an end user might
> > consider a "state file".
> > 
> > Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Andrew Cooper]

On 11/08/15 16:48, Ian Campbell wrote:
> On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
>> It's the checkpoint file - i.e. the command line argument to xl
>> restore - that is being leaked.
> Thanks.
>
> [...]
>> So the checkpoint file is clearly being leaked.
> Indeed. I confirmed this even with the current development version using ls
> -l /proc/<pid>/fd which shows an fd open on a deleted file:
>
> # ps aux| grep xl
> root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl restore save
> # ls -l /proc/20465/fd
> [...]
> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
> [...]
> # rm /root/save
> # ls -l /proc/20465/fd
> [...]
> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
> [...]
>
>>  Its space is not freed
>> until the 'xl restore' process is ended by shutting down the domain:
> [...]
>> It seems like xl restore should close the checkpoint file as soon as
>> it's done restoring the domain, allowing the space to be freed, but
>> that's clearly not happening.
> Right. In fact xl sets the file to be close-on-exec right after opening it,
> which is before the daemonisation step, so it ought to be closed
> automatically, but isn't for some reason.
>
> My working theory is that something in the machinery which spawns the save
> helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
> unsetting CLOEXEC.
>
> Any way, thanks for reporting. I've copied the devel list and 4.6 RM. Wei
> this probably ought to be a blocker for 4.6 (and the fix ought ultimately
> to be backported to 4.4 onwards at least).
>
> NB: This leak seems to be independent of the switch to migration v2.

IIRC, the file descriptor for this is fcntl()'d by at least 3 separate
bits of code (libxl, libxl-save-helper, libxc) once it has been passed
into libxl.

I would not be surprised if one of the higher levels accidentally
clobbered CLOEXEC.

~Andrew

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Wei Liu]

On Tue, Aug 11, 2015 at 04:48:13PM +0100, Ian Campbell wrote:
> On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
> > It's the checkpoint file - i.e. the command line argument to xl
> > restore - that is being leaked.
> 
> Thanks.
> 
> [...]
> > So the checkpoint file is clearly being leaked.
> 
> Indeed. I confirmed this even with the current development version using ls
> -l /proc/<pid>/fd which shows an fd open on a deleted file:
> 
> # ps aux| grep xl
> root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl restore save
> # ls -l /proc/20465/fd
> [...]
> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
> [...]
> # rm /root/save
> # ls -l /proc/20465/fd
> [...]
> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
> [...]
> 
> >  Its space is not freed
> > until the 'xl restore' process is ended by shutting down the domain:
> [...]
> > 
> > It seems like xl restore should close the checkpoint file as soon as
> > it's done restoring the domain, allowing the space to be freed, but
> > that's clearly not happening.
> 
> Right. In fact xl sets the file to be close-on-exec right after opening it,
> which is before the daemonisation step, so it ought to be closed
> automatically, but isn't for some reason.
> 
> My working theory is that something in the machinery which spawns the save
> helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
> unsetting CLOEXEC.
> 
> Any way, thanks for reporting. I've copied the devel list and 4.6 RM. Wei
> this probably ought to be a blocker for 4.6 (and the fix ought ultimately
> to be backported to 4.4 onwards at least).
> 
> NB: This leak seems to be independent of the switch to migration v2.
> 
> Ian.

Maybe this is just because we leak a fd.

I don't see how CLOEXEC would be of any use if xl doesn't actually exec
anything.

Below is a PoC patch which seems to fix the problem for me.

---8<---
commit 7b5f466d5977dc9f41991ca0c2227023ac07709d
Author: Wei Liu <wei.liu2@citrix.com>
Date:   Tue Aug 11 18:02:25 2015 +0100

    xl: close restore_fd when we finish with it
    
    Signed-off-by: Wei Liu <wei.liu2@citrix.com>

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 499a05c..525cd24 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2846,6 +2846,10 @@ start:
         ret = libxl_domain_create_new(ctx, &d_config, &domid,
                                       0, autoconnect_console_how);
     }
+
+    if (migrate_fd < 0)
+        close(restore_fd);
+
     if ( ret )
         goto error_out;

> 
> > -Andrew
> > 
> > On Aug 11, 2015 04:55, "Ian Campbell" <ian.campbell@citrix.com> wrote:
> > > 
> > > On Fri, 2015-08-07 at 12:50 -0400, Andrew Armenia wrote:
> > > > The issue appears to occur with any state file - not just one in
> > > > particular.
> > > 
> > > Please give some specific examples e.g. paths to some of the files to 
> > > which
> > > a fd has been leaked. I'm trying to determine which state files I 
> > > should be
> > > investigating, since there are several things which an end user might
> > > consider a "state file".
> > > 
> > > Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Andrew Cooper]

On 11/08/15 18:07, Wei Liu wrote:
> On Tue, Aug 11, 2015 at 04:48:13PM +0100, Ian Campbell wrote:
>> On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
>>> It's the checkpoint file - i.e. the command line argument to xl
>>> restore - that is being leaked.
>> Thanks.
>>
>> [...]
>>> So the checkpoint file is clearly being leaked.
>> Indeed. I confirmed this even with the current development version using ls
>> -l /proc/<pid>/fd which shows an fd open on a deleted file:
>>
>> # ps aux| grep xl
>> root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl restore save
>> # ls -l /proc/20465/fd
>> [...]
>> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
>> [...]
>> # rm /root/save
>> # ls -l /proc/20465/fd
>> [...]
>> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
>> [...]
>>
>>>  Its space is not freed
>>> until the 'xl restore' process is ended by shutting down the domain:
>> [...]
>>> It seems like xl restore should close the checkpoint file as soon as
>>> it's done restoring the domain, allowing the space to be freed, but
>>> that's clearly not happening.
>> Right. In fact xl sets the file to be close-on-exec right after opening it,
>> which is before the daemonisation step, so it ought to be closed
>> automatically, but isn't for some reason.
>>
>> My working theory is that something in the machinery which spawns the save
>> helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
>> unsetting CLOEXEC.
>>
>> Any way, thanks for reporting. I've copied the devel list and 4.6 RM. Wei
>> this probably ought to be a blocker for 4.6 (and the fix ought ultimately
>> to be backported to 4.4 onwards at least).
>>
>> NB: This leak seems to be independent of the switch to migration v2.
>>
>> Ian.
> Maybe this is just because we leak a fd.
>
> I don't see how CLOEXEC would be of any use if xl doesn't actually exec
> anything.
>
> Below is a PoC patch which seems to fix the problem for me.
>
> ---8<---
> commit 7b5f466d5977dc9f41991ca0c2227023ac07709d
> Author: Wei Liu <wei.liu2@citrix.com>
> Date:   Tue Aug 11 18:02:25 2015 +0100
>
>     xl: close restore_fd when we finish with it
>     
>     Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 499a05c..525cd24 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2846,6 +2846,10 @@ start:
>          ret = libxl_domain_create_new(ctx, &d_config, &domid,
>                                        0, autoconnect_console_how);
>      }
> +
> +    if (migrate_fd < 0)
> +        close(restore_fd);
> +

You surely need check for restore_fd >= 0, to avoid a potential EBADF ?

~Andrew

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Wei Liu]

On Tue, Aug 11, 2015 at 06:21:18PM +0100, Andrew Cooper wrote:
> On 11/08/15 18:07, Wei Liu wrote:
> > On Tue, Aug 11, 2015 at 04:48:13PM +0100, Ian Campbell wrote:
> >> On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
> >>> It's the checkpoint file - i.e. the command line argument to xl
> >>> restore - that is being leaked.
> >> Thanks.
> >>
> >> [...]
> >>> So the checkpoint file is clearly being leaked.
> >> Indeed. I confirmed this even with the current development version using ls
> >> -l /proc/<pid>/fd which shows an fd open on a deleted file:
> >>
> >> # ps aux| grep xl
> >> root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl restore save
> >> # ls -l /proc/20465/fd
> >> [...]
> >> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
> >> [...]
> >> # rm /root/save
> >> # ls -l /proc/20465/fd
> >> [...]
> >> lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
> >> [...]
> >>
> >>>  Its space is not freed
> >>> until the 'xl restore' process is ended by shutting down the domain:
> >> [...]
> >>> It seems like xl restore should close the checkpoint file as soon as
> >>> it's done restoring the domain, allowing the space to be freed, but
> >>> that's clearly not happening.
> >> Right. In fact xl sets the file to be close-on-exec right after opening it,
> >> which is before the daemonisation step, so it ought to be closed
> >> automatically, but isn't for some reason.
> >>
> >> My working theory is that something in the machinery which spawns the save
> >> helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
> >> unsetting CLOEXEC.
> >>
> >> Any way, thanks for reporting. I've copied the devel list and 4.6 RM. Wei
> >> this probably ought to be a blocker for 4.6 (and the fix ought ultimately
> >> to be backported to 4.4 onwards at least).
> >>
> >> NB: This leak seems to be independent of the switch to migration v2.
> >>
> >> Ian.
> > Maybe this is just because we leak a fd.
> >
> > I don't see how CLOEXEC would be of any use if xl doesn't actually exec
> > anything.
> >
> > Below is a PoC patch which seems to fix the problem for me.
> >
> > ---8<---
> > commit 7b5f466d5977dc9f41991ca0c2227023ac07709d
> > Author: Wei Liu <wei.liu2@citrix.com>
> > Date:   Tue Aug 11 18:02:25 2015 +0100
> >
> >     xl: close restore_fd when we finish with it
> >     
> >     Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 499a05c..525cd24 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -2846,6 +2846,10 @@ start:
> >          ret = libxl_domain_create_new(ctx, &d_config, &domid,
> >                                        0, autoconnect_console_how);
> >      }
> > +
> > +    if (migrate_fd < 0)
> > +        close(restore_fd);
> > +
> 
> You surely need check for restore_fd >= 0, to avoid a potential EBADF ?
> 

Indeed. When we create a new domain, restore_fd is -1.

Wei.

> ~Andrew

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell]

On Tue, 2015-08-11 at 18:07 +0100, Wei Liu wrote:
> On Tue, Aug 11, 2015 at 04:48:13PM +0100, Ian Campbell wrote:
> > On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
> > > It's the checkpoint file - i.e. the command line argument to xl
> > > restore - that is being leaked.
> > 
> > Thanks.
> > 
> > [...]
> > > So the checkpoint file is clearly being leaked.
> > 
> > Indeed. I confirmed this even with the current development version 
> > using ls
> > -l /proc/<pid>/fd which shows an fd open on a deleted file:
> > 
> > # ps aux| grep xl
> > root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl 
> > restore save
> > # ls -l /proc/20465/fd
> > [...]
> > lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
> > [...]
> > # rm /root/save
> > # ls -l /proc/20465/fd
> > [...]
> > lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
> > [...]
> > 
> > >  Its space is not freed
> > > until the 'xl restore' process is ended by shutting down the domain:
> > [...]
> > > 
> > > It seems like xl restore should close the checkpoint file as soon as
> > > it's done restoring the domain, allowing the space to be freed, but
> > > that's clearly not happening.
> > 
> > Right. In fact xl sets the file to be close-on-exec right after opening 
> > it,
> > which is before the daemonisation step, so it ought to be closed
> > automatically, but isn't for some reason.
> > 
> > My working theory is that something in the machinery which spawns the 
> > save
> > helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
> > unsetting CLOEXEC.
> > 
> > Any way, thanks for reporting. I've copied the devel list and 4.6 RM. 
> > Wei
> > this probably ought to be a blocker for 4.6 (and the fix ought 
> > ultimately
> > to be backported to 4.4 onwards at least).
> > 
> > NB: This leak seems to be independent of the switch to migration v2.
> > 
> > Ian.
> 
> Maybe this is just because we leak a fd.
> 
> I don't see how CLOEXEC would be of any use if xl doesn't actually exec
> anything.

Duh, for some reason I thought daemonize would activate the CLOEXEC, but
it's just fork without exec. Silly me.

> 
> Below is a PoC patch which seems to fix the problem for me.
> 
> ---8<---
> commit 7b5f466d5977dc9f41991ca0c2227023ac07709d
> Author: Wei Liu <wei.liu2@citrix.com>
> Date:   Tue Aug 11 18:02:25 2015 +0100
> 
>     xl: close restore_fd when we finish with it
>     
>     Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 499a05c..525cd24 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2846,6 +2846,10 @@ start:
>          ret = libxl_domain_create_new(ctx, &d_config, &domid,
>                                        0, autoconnect_console_how);
>      }
> +
> +    if (migrate_fd < 0)
> +        close(restore_fd);

As Andy says I think we want restore_fd in the check, I can't see any
reason we wouldn't want to close the socket too.

For reboot handing you would need to reset the fd to < 0, otherwise when we
come back around on reboot we will close this again.

Would it be less error prone to put this in the if (restoring) just above,
i.e. exactly where restore_fd is used and which already has the reboot
logic in place with restoring = 0.

Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell]

On Wed, 2015-08-12 at 09:41 +0100, Ian Campbell wrote:
> On Tue, 2015-08-11 at 18:07 +0100, Wei Liu wrote:
> > 
> > I don't see how CLOEXEC would be of any use if xl doesn't actually exec
> > anything.
> 
> Duh, for some reason I thought daemonize would activate the CLOEXEC, but
> it's just fork without exec. Silly me.

FWIW I did manage to confirm that libxl isn't messing up the CLOEXEC state
of the FD, it correctly only clears it between the fork and exec when
spawning the save restore helper. Any messing which libxc does is safely
contained in that subprocess.

Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Wei Liu]

On Wed, Aug 12, 2015 at 09:41:13AM +0100, Ian Campbell wrote:
> On Tue, 2015-08-11 at 18:07 +0100, Wei Liu wrote:
> > On Tue, Aug 11, 2015 at 04:48:13PM +0100, Ian Campbell wrote:
> > > On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
> > > > It's the checkpoint file - i.e. the command line argument to xl
> > > > restore - that is being leaked.
> > > 
> > > Thanks.
> > > 
> > > [...]
> > > > So the checkpoint file is clearly being leaked.
> > > 
> > > Indeed. I confirmed this even with the current development version 
> > > using ls
> > > -l /proc/<pid>/fd which shows an fd open on a deleted file:
> > > 
> > > # ps aux| grep xl
> > > root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl 
> > > restore save
> > > # ls -l /proc/20465/fd
> > > [...]
> > > lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
> > > [...]
> > > # rm /root/save
> > > # ls -l /proc/20465/fd
> > > [...]
> > > lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
> > > [...]
> > > 
> > > >  Its space is not freed
> > > > until the 'xl restore' process is ended by shutting down the domain:
> > > [...]
> > > > 
> > > > It seems like xl restore should close the checkpoint file as soon as
> > > > it's done restoring the domain, allowing the space to be freed, but
> > > > that's clearly not happening.
> > > 
> > > Right. In fact xl sets the file to be close-on-exec right after opening 
> > > it,
> > > which is before the daemonisation step, so it ought to be closed
> > > automatically, but isn't for some reason.
> > > 
> > > My working theory is that something in the machinery which spawns the 
> > > save
> > > helper is defeating the use of CLOEXEC, perhaps by dup2() or perhaps by
> > > unsetting CLOEXEC.
> > > 
> > > Any way, thanks for reporting. I've copied the devel list and 4.6 RM. 
> > > Wei
> > > this probably ought to be a blocker for 4.6 (and the fix ought 
> > > ultimately
> > > to be backported to 4.4 onwards at least).
> > > 
> > > NB: This leak seems to be independent of the switch to migration v2.
> > > 
> > > Ian.
> > 
> > Maybe this is just because we leak a fd.
> > 
> > I don't see how CLOEXEC would be of any use if xl doesn't actually exec
> > anything.
> 
> Duh, for some reason I thought daemonize would activate the CLOEXEC, but
> it's just fork without exec. Silly me.
> 
> > 
> > Below is a PoC patch which seems to fix the problem for me.
> > 
> > ---8<---
> > commit 7b5f466d5977dc9f41991ca0c2227023ac07709d
> > Author: Wei Liu <wei.liu2@citrix.com>
> > Date:   Tue Aug 11 18:02:25 2015 +0100
> > 
> >     xl: close restore_fd when we finish with it
> >     
> >     Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 499a05c..525cd24 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -2846,6 +2846,10 @@ start:
> >          ret = libxl_domain_create_new(ctx, &d_config, &domid,
> >                                        0, autoconnect_console_how);
> >      }
> > +
> > +    if (migrate_fd < 0)
> > +        close(restore_fd);
> 
> As Andy says I think we want restore_fd in the check, I can't see any
> reason we wouldn't want to close the socket too.
> 

Do you mean migrate_fd when you say "socket"? I tried that, but that led
to failure because toolstack still needs to get controlling information
out of it (the "GO" message).

Maybe I close this too early. I will have a closer look today.

> For reboot handing you would need to reset the fd to < 0, otherwise when we
> come back around on reboot we will close this again.
> 
> Would it be less error prone to put this in the if (restoring) just above,
> i.e. exactly where restore_fd is used and which already has the reboot
> logic in place with restoring = 0.
> 

Depending on whether we can close migrate_fd.

Wei.

> Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell]

On Wed, 2015-08-12 at 10:49 +0100, Wei Liu wrote:
> On Wed, Aug 12, 2015 at 09:41:13AM +0100, Ian Campbell wrote:
> > On Tue, 2015-08-11 at 18:07 +0100, Wei Liu wrote:
> > > On Tue, Aug 11, 2015 at 04:48:13PM +0100, Ian Campbell wrote:
> > > > On Tue, 2015-08-11 at 11:13 -0400, Andrew Armenia wrote:
> > > > > It's the checkpoint file - i.e. the command line argument to xl
> > > > > restore - that is being leaked.
> > > > 
> > > > Thanks.
> > > > 
> > > > [...]
> > > > > So the checkpoint file is clearly being leaked.
> > > > 
> > > > Indeed. I confirmed this even with the current development version 
> > > > using ls
> > > > -l /proc/<pid>/fd which shows an fd open on a deleted file:
> > > > 
> > > > # ps aux| grep xl
> > > > root     20465  0.0  0.2 106036   984 ?        SLsl 15:42   0:00 xl 
> > > > 
> > > > restore save
> > > > # ls -l /proc/20465/fd
> > > > [...]
> > > > lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save
> > > > [...]
> > > > # rm /root/save
> > > > # ls -l /proc/20465/fd
> > > > [...]
> > > > lr-x------. 1 root root 64 Aug 11 15:42 7 -> /root/save (deleted)
> > > > [...]
> > > > 
> > > > >  Its space is not freed
> > > > > until the 'xl restore' process is ended by shutting down the 
> > > > > domain:
> > > > [...]
> > > > > 
> > > > > It seems like xl restore should close the checkpoint file as soon 
> > > > > as
> > > > > it's done restoring the domain, allowing the space to be freed, 
> > > > > but
> > > > > that's clearly not happening.
> > > > 
> > > > Right. In fact xl sets the file to be close-on-exec right after 
> > > > opening 
> > > > it,
> > > > which is before the daemonisation step, so it ought to be closed
> > > > automatically, but isn't for some reason.
> > > > 
> > > > My working theory is that something in the machinery which spawns 
> > > > the 
> > > > save
> > > > helper is defeating the use of CLOEXEC, perhaps by dup2() or 
> > > > perhaps by
> > > > unsetting CLOEXEC.
> > > > 
> > > > Any way, thanks for reporting. I've copied the devel list and 4.6 
> > > > RM. 
> > > > Wei
> > > > this probably ought to be a blocker for 4.6 (and the fix ought 
> > > > ultimately
> > > > to be backported to 4.4 onwards at least).
> > > > 
> > > > NB: This leak seems to be independent of the switch to migration 
> > > > v2.
> > > > 
> > > > Ian.
> > > 
> > > Maybe this is just because we leak a fd.
> > > 
> > > I don't see how CLOEXEC would be of any use if xl doesn't actually 
> > > exec
> > > anything.
> > 
> > Duh, for some reason I thought daemonize would activate the CLOEXEC, 
> > but
> > it's just fork without exec. Silly me.
> > 
> > > 
> > > Below is a PoC patch which seems to fix the problem for me.
> > > 
> > > ---8<---
> > > commit 7b5f466d5977dc9f41991ca0c2227023ac07709d
> > > Author: Wei Liu <wei.liu2@citrix.com>
> > > Date:   Tue Aug 11 18:02:25 2015 +0100
> > > 
> > >     xl: close restore_fd when we finish with it
> > >     
> > >     Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index 499a05c..525cd24 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -2846,6 +2846,10 @@ start:
> > >          ret = libxl_domain_create_new(ctx, &d_config, &domid,
> > >                                        0, autoconnect_console_how);
> > >      }
> > > +
> > > +    if (migrate_fd < 0)
> > > +        close(restore_fd);
> > 
> > As Andy says I think we want restore_fd in the check, I can't see any
> > reason we wouldn't want to close the socket too.
> > 
> 
> Do you mean migrate_fd when you say "socket"?

In the migrate case we do "restore_fd = migrate_fd;", so yes, indirectly.


>  I tried that, but that led
> to failure because toolstack still needs to get controlling information
> out of it (the "GO" message).
> 
> Maybe I close this too early.

Right.


>  I will have a closer look today.
> 
> > For reboot handing you would need to reset the fd to < 0, otherwise 
> > when we
> > come back around on reboot we will close this again.
> > 
> > Would it be less error prone to put this in the if (restoring) just 
> > above,
> > i.e. exactly where restore_fd is used and which already has the reboot
> > logic in place with restoring = 0.
> > 
> 
> Depending on whether we can close migrate_fd.
> 
> Wei.
> 
> > Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Wei Liu]

On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
[...]
> > > 
> > > As Andy says I think we want restore_fd in the check, I can't see any
> > > reason we wouldn't want to close the socket too.
> > > 
> > 
> > Do you mean migrate_fd when you say "socket"?
> 
> In the migrate case we do "restore_fd = migrate_fd;", so yes, indirectly.
> 
> 
> >  I tried that, but that led
> > to failure because toolstack still needs to get controlling information
> > out of it (the "GO" message).
> > 
> > Maybe I close this too early.
> 
> Right.
> 

I look at the code. Even if we should close that socket, it should not
happen inside create_domain, because the caller (migrate_receive) needs
that fd.

IMO create_domain should only close restore_fd if that fd is opened by
itself.

Whether we should close send_fd and recv_fd in migrate_receive is
another matter. I don't think we should. They are just stdin and stdout,
not closing them wouldn't cause us any trouble.

Wei.

> 
> >  I will have a closer look today.
> > 
> > > For reboot handing you would need to reset the fd to < 0, otherwise 
> > > when we
> > > come back around on reboot we will close this again.
> > > 
> > > Would it be less error prone to put this in the if (restoring) just 
> > > above,
> > > i.e. exactly where restore_fd is used and which already has the reboot
> > > logic in place with restoring = 0.
> > > 
> > 
> > Depending on whether we can close migrate_fd.
> > 
> > Wei.
> > 
> > > Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell]

On Wed, 2015-08-12 at 18:12 +0100, Wei Liu wrote:
> On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
> [...]
> > > > 
> > > > As Andy says I think we want restore_fd in the check, I can't see 
> > > > any
> > > > reason we wouldn't want to close the socket too.
> > > > 
> > > 
> > > Do you mean migrate_fd when you say "socket"?
> > 
> > In the migrate case we do "restore_fd = migrate_fd;", so yes, 
> > indirectly.
> > 
> > 
> > >  I tried that, but that led
> > > to failure because toolstack still needs to get controlling 
> > > information
> > > out of it (the "GO" message).
> > > 
> > > Maybe I close this too early.
> > 
> > Right.
> > 
> 
> I look at the code. Even if we should close that socket, it should not
> happen inside create_domain, because the caller (migrate_receive) needs
> that fd.
> 
> IMO create_domain should only close restore_fd if that fd is opened by
> itself.

That makes sense, yes. The close should probably have an associated comment
since this will be a bit subtle.

Perhaps rather than trying to repeat the conditions which lead to it being
opened we should just do:
    int restore_fd_to_close = -1;
    ...
    restore_fd_to_close = restore_fd = open(restore_file, O_RDONLY);
    ...
    if (restore_fd_to_close >= 0) {
        close(restore_fd_to_close);
        restore_fd_to_close = -1;
    }

Strictly speaking we ought to check the return of close too I suppose.

> Whether we should close send_fd and recv_fd in migrate_receive is
> another matter. I don't think we should. They are just stdin and stdout,
> not closing them wouldn't cause us any trouble.

The trouble they cause is holding kernel resources associated with the
socket, not to mention leaving a possible (perhaps unlikely) avenue of
attack from the network to a process which isn't expecting it...

Any we should be redirecting those to /dev/null as part of daemonising as a
matter of course and it looks like do_daemonize does that, so this is
already fine I think.

Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Wei Liu]

On Thu, Aug 13, 2015 at 09:39:36AM +0100, Ian Campbell wrote:
> On Wed, 2015-08-12 at 18:12 +0100, Wei Liu wrote:
> > On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
> > [...]
> > > > > 
> > > > > As Andy says I think we want restore_fd in the check, I can't see 
> > > > > any
> > > > > reason we wouldn't want to close the socket too.
> > > > > 
> > > > 
> > > > Do you mean migrate_fd when you say "socket"?
> > > 
> > > In the migrate case we do "restore_fd = migrate_fd;", so yes, 
> > > indirectly.
> > > 
> > > 
> > > >  I tried that, but that led
> > > > to failure because toolstack still needs to get controlling 
> > > > information
> > > > out of it (the "GO" message).
> > > > 
> > > > Maybe I close this too early.
> > > 
> > > Right.
> > > 
> > 
> > I look at the code. Even if we should close that socket, it should not
> > happen inside create_domain, because the caller (migrate_receive) needs
> > that fd.
> > 
> > IMO create_domain should only close restore_fd if that fd is opened by
> > itself.
> 
> That makes sense, yes. The close should probably have an associated comment
> since this will be a bit subtle.
> 
> Perhaps rather than trying to repeat the conditions which lead to it being
> opened we should just do:
>     int restore_fd_to_close = -1;
>     ...
>     restore_fd_to_close = restore_fd = open(restore_file, O_RDONLY);
>     ...
>     if (restore_fd_to_close >= 0) {
>         close(restore_fd_to_close);
>         restore_fd_to_close = -1;
>     }
> 
> Strictly speaking we ought to check the return of close too I suppose.
> 

What would we do in case close fails?

> > Whether we should close send_fd and recv_fd in migrate_receive is
> > another matter. I don't think we should. They are just stdin and stdout,
> > not closing them wouldn't cause us any trouble.
> 
> The trouble they cause is holding kernel resources associated with the
> socket, not to mention leaving a possible (perhaps unlikely) avenue of
> attack from the network to a process which isn't expecting it...
> 
> Any we should be redirecting those to /dev/null as part of daemonising as a
> matter of course and it looks like do_daemonize does that, so this is
> already fine I think.
> 

Right.

Wei.

> Ian.

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell]

On Thu, 2015-08-13 at 09:50 +0100, Wei Liu wrote:
> On Thu, Aug 13, 2015 at 09:39:36AM +0100, Ian Campbell wrote:
> > On Wed, 2015-08-12 at 18:12 +0100, Wei Liu wrote:
> > > On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
> > > [...]
> > > > > > 
> > > > > > As Andy says I think we want restore_fd in the check, I can't 
> > > > > > see 
> > > > > > any
> > > > > > reason we wouldn't want to close the socket too.
> > > > > > 
> > > > > 
> > > > > Do you mean migrate_fd when you say "socket"?
> > > > 
> > > > In the migrate case we do "restore_fd = migrate_fd;", so yes, 
> > > > indirectly.
> > > > 
> > > > 
> > > > >  I tried that, but that led
> > > > > to failure because toolstack still needs to get controlling 
> > > > > information
> > > > > out of it (the "GO" message).
> > > > > 
> > > > > Maybe I close this too early.
> > > > 
> > > > Right.
> > > > 
> > > 
> > > I look at the code. Even if we should close that socket, it should 
> > > not
> > > happen inside create_domain, because the caller (migrate_receive) 
> > > needs
> > > that fd.
> > > 
> > > IMO create_domain should only close restore_fd if that fd is opened 
> > > by
> > > itself.
> > 
> > That makes sense, yes. The close should probably have an associated 
> > comment
> > since this will be a bit subtle.
> > 
> > Perhaps rather than trying to repeat the conditions which lead to it 
> > being
> > opened we should just do:
> >     int restore_fd_to_close = -1;
> >     ...
> >     restore_fd_to_close = restore_fd = open(restore_file, O_RDONLY);
> >     ...
> >     if (restore_fd_to_close >= 0) {
> >         close(restore_fd_to_close);
> >         restore_fd_to_close = -1;
> >     }
> > 
> > Strictly speaking we ought to check the return of close too I suppose.
> > 
> 
> What would we do in case close fails?

Maybe just log?

http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html says
we can get:

EBADF, which would be an error in our code.
EINTR, ...
EIO, which would be from disk full or a disk dying or something.

We (and most code in general) tends to not worry about close failing too
much, there's an argument for that too...

Re: [Xen-users] "xl restore" leaks a file descriptor?

[Andrew Cooper]

On 13/08/15 10:17, Ian Campbell wrote:
> On Thu, 2015-08-13 at 09:50 +0100, Wei Liu wrote:
>> On Thu, Aug 13, 2015 at 09:39:36AM +0100, Ian Campbell wrote:
>>> On Wed, 2015-08-12 at 18:12 +0100, Wei Liu wrote:
>>>> On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
>>>> [...]
>>>>>>> As Andy says I think we want restore_fd in the check, I can't 
>>>>>>> see 
>>>>>>> any
>>>>>>> reason we wouldn't want to close the socket too.
>>>>>>>
>>>>>> Do you mean migrate_fd when you say "socket"?
>>>>> In the migrate case we do "restore_fd = migrate_fd;", so yes, 
>>>>> indirectly.
>>>>>
>>>>>
>>>>>>  I tried that, but that led
>>>>>> to failure because toolstack still needs to get controlling 
>>>>>> information
>>>>>> out of it (the "GO" message).
>>>>>>
>>>>>> Maybe I close this too early.
>>>>> Right.
>>>>>
>>>> I look at the code. Even if we should close that socket, it should 
>>>> not
>>>> happen inside create_domain, because the caller (migrate_receive) 
>>>> needs
>>>> that fd.
>>>>
>>>> IMO create_domain should only close restore_fd if that fd is opened 
>>>> by
>>>> itself.
>>> That makes sense, yes. The close should probably have an associated 
>>> comment
>>> since this will be a bit subtle.
>>>
>>> Perhaps rather than trying to repeat the conditions which lead to it 
>>> being
>>> opened we should just do:
>>>     int restore_fd_to_close = -1;
>>>     ...
>>>     restore_fd_to_close = restore_fd = open(restore_file, O_RDONLY);
>>>     ...
>>>     if (restore_fd_to_close >= 0) {
>>>         close(restore_fd_to_close);
>>>         restore_fd_to_close = -1;
>>>     }
>>>
>>> Strictly speaking we ought to check the return of close too I suppose.
>>>
>> What would we do in case close fails?
> Maybe just log?
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html says
> we can get:
>
> EBADF, which would be an error in our code.
> EINTR, ...
> EIO, which would be from disk full or a disk dying or something.
>
> We (and most code in general) tends to not worry about close failing too
> much, there's an argument for that too...

The return value really shouldn't be ignored.

Logging loudly is about all which can be done, and that is fine, but the
user really does need to be aware that something bad went wrong.

~Andrew