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

Hi Serge,

On 6 May 2016 at 19:33, Serge E. Hallyn [off-list ref] wrote:

Quoting Michael Kerrisk (man-pages) (mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org):

quoted

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:

quoted

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.

quoted

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 :-).

[...]

quoted

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

quoted

Tested-by: Michael Kerrisk <redacted>
Acked-by: Michael Kerrisk <redacted>

(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

[...]