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

[Michael Kerrisk (man-pages)]

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

>> 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

[...]