cgroup: actually detect cgroupns bind mounts correctly

Submitted by Tycho Andersen on Aug. 8, 2016, 10:36 p.m.

Details

Message ID 1470695806-5526-1-git-send-email-tycho.andersen@canonical.com
State Rejected
Series "cgroup: actually detect cgroupns bind mounts correctly"
Headers show

Commit Message

Tycho Andersen Aug. 8, 2016, 10:36 p.m.
Initially, the cgroupns patchset that went into the Ubuntu kernels had the
nsroot= argument in mountopts, which allowed us to distinguish between
cgroupfs mounts. The upstream kernel doesn't have this (the reasoning was
that it showed up in the root field of mountinfo), so all cgroup mounts
look idential if we don't consider the root mount, and we wrongly detect
the mounts as bind mounts. Instead, let's remember the root path and
compare this when considering when a mount is a bind mount.

This patch is a little ugly because it intorduces cgroup specific behavior
into the generic code, which we've thus far been able to avoid. One way to
deal with this might be able to add a ->sb_equal hook to the fstype struct
that could compare mountinfos' superblocks, and then fall back to
mounts_sb_equal if that isn't present. Or we can leave this specific code
in the generic bits until more of these appear. Either way is fine with me.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
---
 criu/mount.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index f90ec69..de13915 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -721,6 +721,9 @@  static struct mount_info *find_best_external_match(struct mount_info *list, stru
 		if (!mounts_sb_equal(info, it))
 			continue;
 
+		if (info->fstype->code == FSTYPE__CGROUP && strcmp(info->private, it->private))
+			continue;
+
 		/*
 		 * This means we have a situation like:
 		 *
@@ -1602,7 +1605,7 @@  static int cgroup_parse(struct mount_info *pm)
 	/* cgroup namespaced mounts don't look rooted to CRIU, so let's fake it
 	 * here.
 	 */
-	xfree(pm->root);
+	pm->private = pm->root;
 	pm->root = xstrdup("/");
 	if (!pm->root)
 		return -1;

Comments

Pavel Emelianov Aug. 10, 2016, 1:35 p.m.
On 08/09/2016 01:36 AM, Tycho Andersen wrote:
> Initially, the cgroupns patchset that went into the Ubuntu kernels had the
> nsroot= argument in mountopts, which allowed us to distinguish between
> cgroupfs mounts. The upstream kernel doesn't have this (the reasoning was
> that it showed up in the root field of mountinfo), so all cgroup mounts
> look idential if we don't consider the root mount, and we wrongly detect
> the mounts as bind mounts. Instead, let's remember the root path and
> compare this when considering when a mount is a bind mount.
> 
> This patch is a little ugly because it intorduces cgroup specific behavior
> into the generic code, which we've thus far been able to avoid. One way to
> deal with this might be able to add a ->sb_equal hook to the fstype struct
> that could compare mountinfos' superblocks, and then fall back to
> mounts_sb_equal if that isn't present. Or we can leave this specific code
> in the generic bits until more of these appear. Either way is fine with me.
> 
> Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> ---
>  criu/mount.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index f90ec69..de13915 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -721,6 +721,9 @@ static struct mount_info *find_best_external_match(struct mount_info *list, stru
>  		if (!mounts_sb_equal(info, it))
>  			continue;
>  
> +		if (info->fstype->code == FSTYPE__CGROUP && strcmp(info->private, it->private))
> +			continue;

Shouldn't this be rather in mounts_sb_equal()?
And, BTW, what's there in cgroup's mi->private?

> +
>  		/*
>  		 * This means we have a situation like:
>  		 *
> @@ -1602,7 +1605,7 @@ static int cgroup_parse(struct mount_info *pm)
>  	/* cgroup namespaced mounts don't look rooted to CRIU, so let's fake it
>  	 * here.
>  	 */
> -	xfree(pm->root);
> +	pm->private = pm->root;
>  	pm->root = xstrdup("/");
>  	if (!pm->root)
>  		return -1;
>
Tycho Andersen Aug. 11, 2016, 2:05 p.m.
On Wed, Aug 10, 2016 at 04:35:18PM +0300, Pavel Emelyanov wrote:
> On 08/09/2016 01:36 AM, Tycho Andersen wrote:
> > Initially, the cgroupns patchset that went into the Ubuntu kernels had the
> > nsroot= argument in mountopts, which allowed us to distinguish between
> > cgroupfs mounts. The upstream kernel doesn't have this (the reasoning was
> > that it showed up in the root field of mountinfo), so all cgroup mounts
> > look idential if we don't consider the root mount, and we wrongly detect
> > the mounts as bind mounts. Instead, let's remember the root path and
> > compare this when considering when a mount is a bind mount.
> > 
> > This patch is a little ugly because it intorduces cgroup specific behavior
> > into the generic code, which we've thus far been able to avoid. One way to
> > deal with this might be able to add a ->sb_equal hook to the fstype struct
> > that could compare mountinfos' superblocks, and then fall back to
> > mounts_sb_equal if that isn't present. Or we can leave this specific code
> > in the generic bits until more of these appear. Either way is fine with me.
> > 
> > Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> > ---
> >  criu/mount.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/criu/mount.c b/criu/mount.c
> > index f90ec69..de13915 100644
> > --- a/criu/mount.c
> > +++ b/criu/mount.c
> > @@ -721,6 +721,9 @@ static struct mount_info *find_best_external_match(struct mount_info *list, stru
> >  		if (!mounts_sb_equal(info, it))
> >  			continue;
> >  
> > +		if (info->fstype->code == FSTYPE__CGROUP && strcmp(info->private, it->private))
> > +			continue;
> 
> Shouldn't this be rather in mounts_sb_equal()?

It could be, and I suppose it should be too, I can resend.

> And, BTW, what's there in cgroup's mi->private?

It's the original root path from pm->root, see the hunk below.

Tycho

> > +
> >  		/*
> >  		 * This means we have a situation like:
> >  		 *
> > @@ -1602,7 +1605,7 @@ static int cgroup_parse(struct mount_info *pm)
> >  	/* cgroup namespaced mounts don't look rooted to CRIU, so let's fake it
> >  	 * here.
> >  	 */
> > -	xfree(pm->root);
> > +	pm->private = pm->root;
> >  	pm->root = xstrdup("/");
> >  	if (!pm->root)
> >  		return -1;
> > 
>