[CRIU] [PATCH v2 3/3] mount: handle tracefs more gracefully

[Tycho Andersen]

On Thu, May 12, 2016 at 05:22:18PM +0300, Pavel Emelyanov wrote:
> On 05/11/2016 06:21 PM, Tycho Andersen wrote:
> > On Wed, May 11, 2016 at 08:49:19AM -0600, Tycho Andersen wrote:
> >> On Wed, May 11, 2016 at 03:49:07PM +0300, Pavel Emelyanov wrote:
> >>> On 05/11/2016 12:15 AM, Tycho Andersen wrote:
> >>>> See the comment for details, but basically tracefs is automounted by the
> >>>> kernel, so we can just mount debugfs with MS_REC and get the right result.
> >>>>
> >>>> v2: rebase on criu-dev
> >>>>
> >>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> >>>> ---
> >>>> This is kind of similarly ugly to to a cgroup patch I sent earlier, where
> >>>> we needed some filesystem specific code and checks in various parts of
> >>>> mount.c. I somehow can't figure out a nicer way to do this, but I am
> >>>> definitely open to suggestions.
> >>>
> >>> A suggestion :)
> >>>
> >>>> @@ -2301,6 +2310,11 @@ static int do_new_mount(struct mount_info *mi)
> >>>>  	if (remount_ro)
> >>>>  		sflags &= ~MS_RDONLY;
> >>>>  
> >>>> +	if (tp->flags) {
> >>>> +		pr_info("forcing flags: %d\n", tp->flags);
> >>>> +		sflags |= tp->flags;
> >>>> +	}
> >>>> +
> >>>>  	if (do_mount(mi, src, mnt_fsname(mi), sflags) < 0) {
> >>>
> >>> This do_mount can call mi->fs->mount() callback. What if we define one
> >>> for debugfs that would just call do_simple_mount() with updated flags?
> >>
> >> Yes, this sounds a lot better :). I'll resend, thanks for the
> >> suggestion.
> > 
> > Ugh, actually this won't work for us, because liblxc by default bind
> > mounts debugfs into the container, rather than letting the container
> > mount it itself, so the hunk we actually care about for this patch is
> > the bit in do_bind:
> > 
> > @@ -2474,12 +2488,16 @@ do_bind:
> >                 }
> >         }
> > 
> > -       if (mount(root, mi->mountpoint, NULL, MS_BIND, NULL) < 0) {
> > +       if (mount(root, mi->mountpoint, NULL, MS_BIND | mi->fstype->flags, NULL) < 0) {
> >                 pr_perror("Can't mount at %s", mi->mountpoint);
> >                 goto err;
> >         }
> > 
> >         mflags = mi->flags & (~MS_PROPAGATE);
> > +       if (mi->fstype->flags) {
> > +               pr_info("forcing flags: 0x%x\n", mi->fstype->flags);
> > +               mflags |= mi->fstype->flags;
> > +       }
> >         if (!mi->bind || mflags != (mi->bind->flags & (~MS_PROPAGATE)))
> >                 if (mount(NULL, mi->mountpoint, NULL, MS_BIND | MS_REMOUNT | mflags, NULL)) {
> >                         pr_perror("Can't mount at %s", mi->mountpoint);
> 
> Ouch :( And we we need MS_REC on __both__ mount-s -- initial and remount?

I think so, because the overmounting protection code checks it, but
I'll just verify that again to be sure.

> > So we need some way to hook that. Maybe adding another fstype->bind hook, so
> > it's more general than just flags, similar to the ->mount hook above?
> 
> At this point I think the fstype->flags would be better. Though I've found one
> more place in the patch that looks weird:
> 
> > @@ -1380,6 +1380,13 @@ struct mount_info *parse_mountinfo(pid_t pid, struct ns_id *nsid, bool for_dump)
> >  			goto end;
> >  		}
> >  
> > +		if (new->fstype->code == FSTYPE__TRACEFS) {
> > +			pr_info("\tskipping tracefs mounted at %s\n", new->mountpoint + 1);
> > +			mnt_entry_free(new);
> > +			new = NULL;
> > +			goto end;
> > +		}
> > +
> >  		pr_info("\ttype %s source %s mnt_id %d s_dev %#x %s @ %s flags %#x options %s\n",
> >  				fsname, new->source,
> >  				new->mnt_id, new->s_dev, new->root, new->mountpoint,
> 
> Why do we need to drop this entry? AFAIU this is only required to make
> restore skip mounting one and let 'mount -t debugfs' do the thing, right?
> If yes, then we should keep this entry parsed and skip one on restore.
> I would say -- the debugfs' -> parse callback should walk all tracefs'
> MI's and set mounted on them to true.

Sounds good, I'll resend with this change and drop the hunk above if
we don't need it.

Thanks!

Tycho