[CRIU] [PATCH 1/2] finalize cgroups correctly

[Pavel Emelyanov]

On 03/24/2015 09:30 PM, Saied Kazemi wrote:
> 
> 
> On Tue, Mar 24, 2015 at 10:57 AM, Pavel Emelyanov <xemul at parallels.com <mailto:xemul at parallels.com>> wrote:
> 
>     On 03/24/2015 08:25 PM, Saied Kazemi wrote:
>     > For the easy case of comparing pathname strings on the command line (-D and --root), I
>     > think it's better to error and exit instead of restoring with all the cgyard mounts
>     > leaked into the restored process tree.
> 
>     I rather agree with this. It's quite uncommon (from my perspective) that images sit
>     in the container's root, so preventing this usage doesn't (shouldn't) break anything
>     critical. Does it?
> 
> 
> It wouldn't break anything because nothing has been merged yet.

I meant -- right now we allow for images to be under root, but we plan
to ban this and if we do, nothing should get broken.

>     > For a more general solution, is it possible to make sure that after fini_cgroup() the
>     > cgyard mount points have been successfully removed from the restored tree?
> 
>     What if we call umount with detach flag on cgyard dir inside restore_task_mnt_ns()?
>     Would this resolve this?
> 
> 
> I just quickly tried the following patch, first calling umount_cgyard() before if and then
> after if.  Neither case worked :(

The cg_yard variable is just a ".criu.cgyard.XXXXX" relative path. When
calling umount2 on it kernel would try to resolve one relative to task's
cwd.

As far "calling this after if" is concerned -- yes, it's supposed to fail
since restoring task's mntns implies calling pivot_roo() which moves task's
cwd into new root and thus the cg_yard starts pointing to non-existing 
directory.

But "before if"... I have no explanation for it. With what error does the
umount fail in this case?

> --Saied
> 
> diff --git a/cgroup.c b/cgroup.c
> index b1a2f92..fa94853 100644
> --- a/cgroup.c
> +++ b/cgroup.c
> @@ -951,6 +951,12 @@ void fini_cgroup(void)
>         cg_yard = NULL;
>  }
>  
> +void umount_cgyard(void)
> +{
> +       if (cg_yard)
> +               umount2(cg_yard, MNT_DETACH);
> +}
> +
>  static int restore_cgroup_prop(const CgroupPropEntry * cg_prop_entry_p,
>                                char *path, int off)
>  {
> diff --git a/include/cgroup.h b/include/cgroup.h
> index 14fef67..5bcf028 100644
> --- a/include/cgroup.h
> +++ b/include/cgroup.h
> @@ -10,6 +10,7 @@ int prepare_cgroup(void);
>  /* Restore things like cpu_limit in known cgroups. */
>  int prepare_cgroup_properties(void);
>  void fini_cgroup(void);
> +void umount_cgyard(void);
>  
>  struct cg_controller;
>  
> diff --git a/mount.c b/mount.c
> index e59cb0d..b0b34cb 100644
> --- a/mount.c
> +++ b/mount.c
> @@ -2028,6 +2028,7 @@ int restore_task_mnt_ns(struct pstree_item *current)
>  
>                 if (do_restore_task_mnt_ns(nsid))
>                         return -1;
> +               umount_cgyard();
>         }
>  
>         return 0;
>