[CRIU] [PATCH v3 1/2] restore: don't pass cgroup nsroot= option to mount()

[Tycho Andersen]

On Tue, Mar 29, 2016 at 01:11:03PM +0300, Pavel Emelyanov wrote:
> On 03/28/2016 05:46 PM, Tycho Andersen wrote:
> > https://lkml.org/lkml/2016/3/21/777 introduces a new cgroup mount option
> > called nsroot, but doesn't allow you to pass it to mount() to actually
> > mount it. Let's chop this option off so that we don't get errors from
> > mount().
> > 
> > There may be better places to chop this off (e.g. perhaps on dump),
> > although I think it is useful to have it in there for debugging purposes,
> > and only costs us a few extra bytes to keep it in the images since we're
> > going to have other cgroup options anyways.
> > 
> > v2: handle the case where nsroot= is not the last mount option
> > 
> > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > ---
> >  criu/mount.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/criu/mount.c b/criu/mount.c
> > index a425828..eb8d058 100644
> > --- a/criu/mount.c
> > +++ b/criu/mount.c
> > @@ -2866,6 +2866,26 @@ static int collect_mnt_from_image(struct mount_info **pms, struct ns_id *nsid)
> >  		/* FIXME: abort unsupported early */
> >  		pm->fstype = decode_fstype(me->fstype, me->fsname);
> >  
> > +		if (pm->fstype->code == FSTYPE__CGROUP) {
> > +			char *start;
> > +
> > +			start = strstr(pm->options, "nsroot=");
> > +			if (start) {
> > +				char *next = strchr(start, ',');
> > +
> > +				if (next) {
> > +					/* skip the `,' but include the terminating \0 */
> > +					memmove(start, next+1, strlen(next+1)+1);
> > +				} else {
> > +					if (start > pm->options && *(start-1) == ',')
> > +						start--;
> > +					*start = 0;
> > +				}
> > +
> > +				pr_info("trimmed nsroot option from cgroup mount, opts now: %s\n", pm->options);
> > +			}
> > +		}
> > +
> 
> OK, so we need to keep options intact to make all the comparing machinery
> work. That's clear.
> 
> But I still want to keep fs-specific code out of the generic one. In the
> fstype we have the ->mount callback that gets called when the mi is about
> to be mounted. Would defining this callback for cgroups like this
> 
> static int cgroup_mount(struct mount_info *mi, ...)
> {
> 	char *options = mi->options;
> 
> 	cut_nsroot_opt(options);
> 
> 	return do_simple_mount(mi, ...);
> }
> 
> (we have the do_simple_mount() call that just calls mount syscall) work?

Actually after talking with Serge last night, I think the most
sensible thing might be to allow this mount option, as long as the
passed in nsroot matches the currently existing one (which would be
the case in CRIU). I'll reply to the kernel thread about that, but
basically then we could drop this patch and only take patch #2.

Tycho

> >  		if (get_mp_root(me, pm))
> >  			goto err;
> >  
> > 
>