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] [day] [month] [year] [list]
Message-ID: <20160506193259.GB3057@mail.hallyn.com>
Date:	Fri, 6 May 2016 14:32:59 -0500
From:	"Serge E. Hallyn" <serge@...lyn.com>
To:	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
Cc:	"Serge E. Hallyn" <serge@...lyn.com>,
	"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

Quoting Michael Kerrisk (man-pages) (mtk.manpages@...il.com):
> 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

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@...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
> 
> [...]