Re: [PATCH] mountinfo: implement show_path for kernfs and cgroup

[Serge E. Hallyn]

Quoting Michael Kerrisk (man-pages) (mtk.manpages@xxxxxxxxx):
> Hi Serge,
> 
> On 6 May 2016 at 19:33, Serge E. Hallyn <serge@xxxxxxxxxx> wrote:
> > Quoting Michael Kerrisk (man-pages) (mtk.manpages@xxxxxxxxx):
> >> Hi Serge,
> >>
> >> I'll add my own notes below, as much as anything in order to convince
> >> myself that I understand what's going on.
> >>
> >> On 05/05/2016 05:20 PM, Serge E. Hallyn wrote:
> >> > Short explanation:
> >> >
> >> > When showing a cgroupfs entry in mountinfo, show the path of the mount
> >> > root dentry relative to the reader's cgroup namespace root.
> >>
> >> As part of the commit message, I think it would be useful to add a
> >> sentence here explain why this is needed / which applications need it.
> >>
> >> > Long version:
> >> >
> >> > When a uid 0 task which is in freezer cgroup /a/b, unshares a new cgroup
> >> > namespace, and then mounts a new instance of the freezer cgroup, the new
> >> > mount will be rooted at /a/b.  The root dentry field of the mountinfo
> >> > entry will show '/a/b'.
> >>
> >> So, the point is that if we create a new cgroup namespace,
> >> then we want both /proc/self/cgroup and /proc/self/mountinfo
> >> to show cgroup paths that are correctly virtualized with
> >> respect to the cgroup mount point. Previous to this patch,
> >> /proc/self/cgroup shows the right info, but
> >> /proc/self/mountinfo does not. (Walk through in a moment.)
> >>
> >> Is the above a correct summary?
> 
> Feel free to add that piece to the commit message :-).
> 
> [...]
> 
> >> So, I applied your patch against a current (i.e., 4.6-rc6) kernel.
> >> Same steps as before, and here's what I see:
> >>
> >> # mkdir -p /sys/fs/cgroup/freezer/a/b
> >> # echo $$ > /sys/fs/cgroup/freezer/a/b/cgroup.procs
> >> # ./cgroup_info.sh
> >>       /proc/self/cgroup:      10:freezer:/a/b
> >>       mountinfo:              /       /sys/fs/cgroup/freezer
> >> # ~mtk/tlpi/code/ns/unshare -Cm bash
> >> # ./cgroup_info.sh
> >>       /proc/self/cgroup:      10:freezer:/
> >>       mountinfo:              /../..  /sys/fs/cgroup/freezer
> >> # mount --make-rslave /
> >> # mkdir -p /mnt/freezer
> >> # umount /sys/fs/cgroup/freezer
> >> # mount -t cgroup -o freezer freezer /mnt/freezer/
> >> # ./cgroup_info.sh
> >>       /proc/self/cgroup:      10:freezer:/
> >>       mountinfo:              /       /mnt/freezer
> >>
> >> Now the root directory path shown by mountinfo is correct,
> >> and when we look inside the mount point, we see that things
> >> look "right" (i.e., a cgroup root directory with no
> >> subdirectories, and the PID of the shell run by unshare is
> >> in the cgroup.procs file of this cgroup):
> >>
> >> # ls /mnt/freezer/
> >> cgroup.clone_children  freezer.parent_freezing  freezer.state      tasks
> >> cgroup.procs           freezer.self_freezing    notify_on_release
> >> # echo $$
> >> 3164
> >> # cat /mnt/freezer/cgroup.procs
> >> 2653                   # First shell that placed in this cgroup
> >> 3164                   # Shell started by 'unshare'
> >> 14197                  # cat(1)
> >>
> >> All makes sense to me.
> >
> > Right.  So in particular, docker wants to do something like:
> >
> > bindpath=`grep freezer /proc/self/mountinfo | tail -n 1 | awk '{ print $4 }'`
> > mountpoint=`grep freezer /proc/self/mountinfo | tail -n 1 | awk '{ print $5 }'`
> > mycg=`awk -F: '/freezer/ { print $3 }' /proc/self/cgroup`
> > cat ${mountpoint}/${bindpath}/${mycg}/cgroup.procs
> >
> > and see its own task.
> 
> I think that'd be a great piece to include in the commit message, near
> the top, as rationale for the patch

Hang on I've messed that up :)  The above is not actually what docker
does.  Rather it expects the $bindpath to be the same as the first part
of $mycg, and and so appends the rest of $mycg to the $mountpoint and
uses that.

> >> Tested-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
> >> Acked-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
> >>
> >> (I did no review of the patch itself though.)
> >
> > Thanks, Michael.
> 
> You're welcome.
> 
> > I'll resend with corrections and a test script of
> > some sort.
> 
> I think including some version of the two walk thoughs (without + with
> patch) would also make for a great commit message :-).
> 
> Cheers,
> 
> Michael
> 
> [...]