linux-kernel - Re: [PATCH] mountinfo: implement show

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgNAkhCZrZevznOX_PqcckX0oJEMYbOir1tK7J+4qKtzUBwpg@mail.gmail.com>
Date:	Fri, 6 May 2016 19:42:48 +0200
From:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	"Serge E. Hallyn" <serge.hallyn@...ntu.com>,
	Tejun Heo <tj@...nel.org>, lkml <linux-kernel@...r.kernel.org>,
	Linux API <linux-api@...r.kernel.org>,
	Containers <containers@...ts.linux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Greg KH <gregkh@...uxfoundation.org>, cgroups@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] mountinfo: implement show_path for kernfs and cgroup

Hi Serge,

On 6 May 2016 at 19:33, Serge E. Hallyn <serge@...lyn.com> wrote:
> Quoting Michael Kerrisk (man-pages) (mtk.manpages@...il.com):
>> 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@...il.com>
>> Acked-by: Michael Kerrisk <mtk.manpages@...il.com>
>>
>> (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

[...]