[RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Dmitry V. Levin]

Date: Wed, 17 Oct 2012 20:29:55 +0400

Change show_vfsmnt() and show_vfsstat() to show mountpoints relative
to the root directory and skip mountpoints outside of chroot jail
the same way as show_mountinfo() does.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 fs/proc_namespace.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 789e8d1..0f96f71 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -91,6 +91,7 @@ static void show_type(struct seq_file *m, struct super_block *sb)
 
 static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 {
+	struct proc_mounts *p = proc_mounts(m);
 	struct mount *r = real_mount(mnt);
 	int err = 0;
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };
@@ -104,7 +105,10 @@ static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 		mangle(m, r->mnt_devname ? r->mnt_devname : "none");
 	}
 	seq_putc(m, ' ');
-	seq_path(m, &mnt_path, " \t\n\\");
+	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
+	err = seq_path_root(m, &mnt_path, &p->root, " \t\n\\");
+	if (err)
+		goto out;
 	seq_putc(m, ' ');
 	show_type(m, sb);
 	seq_puts(m, __mnt_is_readonly(mnt) ? " ro" : " rw");
@@ -181,6 +185,7 @@ out:
 
 static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 {
+	struct proc_mounts *p = proc_mounts(m);
 	struct mount *r = real_mount(mnt);
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };
 	struct super_block *sb = mnt_path.dentry->d_sb;
@@ -200,7 +205,10 @@ static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 
 	/* mount point */
 	seq_puts(m, " mounted on ");
-	seq_path(m, &mnt_path, " \t\n\\");
+	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
+	err = seq_path_root(m, &mnt_path, &p->root, " \t\n\\");
+	if (err)
+		goto out;
 	seq_putc(m, ' ');
 
 	/* file system type */
@@ -215,6 +223,7 @@ static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 	}
 
 	seq_putc(m, '\n');
+out:
 	return err;
 }
 
-- 
ldv

Re: [RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Al Viro]

On Mon, Sep 29, 2014 at 10:05:23PM +0400, Dmitry V. Levin wrote:
> Date: Wed, 17 Oct 2012 20:29:55 +0400
> 
> Change show_vfsmnt() and show_vfsstat() to show mountpoints relative
> to the root directory and skip mountpoints outside of chroot jail
> the same way as show_mountinfo() does.

Sigh...  Repeat after me:  We  Do  Not  Break  Userland.  Care to explain
why that change won't do just that?

Re: [RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Dmitry V. Levin]

On Mon, Sep 29, 2014 at 07:32:22PM +0100, Al Viro wrote:
> On Mon, Sep 29, 2014 at 10:05:23PM +0400, Dmitry V. Levin wrote:
> > Date: Wed, 17 Oct 2012 20:29:55 +0400
> > 
> > Change show_vfsmnt() and show_vfsstat() to show mountpoints relative
> > to the root directory and skip mountpoints outside of chroot jail
> > the same way as show_mountinfo() does.
> 
> Sigh...  Repeat after me:  We  Do  Not  Break  Userland.  Care to explain
> why that change won't do just that?

This is definitely an information leak, and I cannot imagine how these
unaccessible entries listed in /proc/self/mounts and /proc/self/mountstats
could be used sanely.


-- 
ldv

Re: [RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Al Viro]

On Mon, Sep 29, 2014 at 10:43:33PM +0400, Dmitry V. Levin wrote:
> On Mon, Sep 29, 2014 at 07:32:22PM +0100, Al Viro wrote:
> > On Mon, Sep 29, 2014 at 10:05:23PM +0400, Dmitry V. Levin wrote:
> > > Date: Wed, 17 Oct 2012 20:29:55 +0400
> > > 
> > > Change show_vfsmnt() and show_vfsstat() to show mountpoints relative
> > > to the root directory and skip mountpoints outside of chroot jail
> > > the same way as show_mountinfo() does.
> > 
> > Sigh...  Repeat after me:  We  Do  Not  Break  Userland.  Care to explain
> > why that change won't do just that?
> 
> This is definitely an information leak, and I cannot imagine how these
> unaccessible entries listed in /proc/self/mounts and /proc/self/mountstats
> could be used sanely.

How do you think any existing script running chrooted is going to parse
/proc/mounts?  One obvious way is to filter on known prefix and strip it
away.  With your change it will instantly break.  Moreover, any script
working with *new* semantics will break as soon as you boot an older
kernel.

Existing behaviour had been there since mid-90s (at least) and I'm absolutely
certain that such scripts do exist.  I know that, since I had to write such.
Again, we do not break userland.  Especially since the behaviour you want
is trivial to obtain - just create a new namespace instead of messing with
chroot.  Then, in that namespace, pivot_root the sucker to root and unmount
everything else.  Voila.

NAK.  And you really need to realize that changing ABI in a way that breaks
real-world userland code is not acceptable.  Even when the new semantics is
nicer.  And when you cannot imagine how somebody could possibly write code
that would depend on the old one.

Re: [RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Dmitry V. Levin]

On Mon, Sep 29, 2014 at 08:33:29PM +0100, Al Viro wrote:
> On Mon, Sep 29, 2014 at 10:43:33PM +0400, Dmitry V. Levin wrote:
> > On Mon, Sep 29, 2014 at 07:32:22PM +0100, Al Viro wrote:
> > > On Mon, Sep 29, 2014 at 10:05:23PM +0400, Dmitry V. Levin wrote:
> > > > Date: Wed, 17 Oct 2012 20:29:55 +0400
> > > > 
> > > > Change show_vfsmnt() and show_vfsstat() to show mountpoints relative
> > > > to the root directory and skip mountpoints outside of chroot jail
> > > > the same way as show_mountinfo() does.
> > > 
> > > Sigh...  Repeat after me:  We  Do  Not  Break  Userland.  Care to explain
> > > why that change won't do just that?
> > 
> > This is definitely an information leak, and I cannot imagine how these
> > unaccessible entries listed in /proc/self/mounts and /proc/self/mountstats
> > could be used sanely.
> 
> How do you think any existing script running chrooted is going to parse
> /proc/mounts?  One obvious way is to filter on known prefix and strip it
> away.  With your change it will instantly break.  Moreover, any script
> working with *new* semantics will break as soon as you boot an older
> kernel.

If a script relies on this behaviour, it is likely to be broken already
because the kernel strips chroot prefix somehow (I've just checked back to
2.6.32).  The main purpose of this patch is to exclude from listing those
mount points that do not belong to the chroot at all.

> Existing behaviour had been there since mid-90s (at least) and I'm absolutely
> certain that such scripts do exist.  I know that, since I had to write such.

I'm very curious, what could your scripts do with paths outside chroot,
assuming that prefix removal was done by the kernel?

> Again, we do not break userland.

Occasionally, we do.  If you say there was no chroot prefix removal in the
kernel before, and there seems to be a prefix removal for at least 5 years,
then there must have been a break some years ago that went unnoticed.


-- 
ldv

Re: [RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Al Viro]

On Tue, Sep 30, 2014 at 12:26:59AM +0400, Dmitry V. Levin wrote:

> > Again, we do not break userland.
> 
> Occasionally, we do.  If you say there was no chroot prefix removal in the
> kernel before, and there seems to be a prefix removal for at least 5 years,
> then there must have been a break some years ago that went unnoticed.

D'oh!  Yes, we did.  And yes, it was quite a few years ago, actually - back
in 2000.  I plead being low on sleep and lower on caffeine...

For a moment I was even afraid that it was my own doing, but no - it came
in https://lkml.org/lkml/2000/1/16/54.  Werner Almesberger.  IIRC, I hadn't
realized that it was going to cause fun problems back then - basically,
by that point 2.3 and 2.2 had already diverged a lot and quite a few things
got written off as "oh, well - upgrade from 2.2 to 2.4 is going to be not
far from building the system from scratch anyway, and nobody sane would be
using 2.3 on anything other than scratch filesystem - IDE broken more often
than not, memory corruption aplenty, etc."

My apologies.  Hmm...  Filtering the out-of-root ones out is probably OK,
but let's put that into -next after 3.18-rc1 and see if anyone yells.

Re: [RESEND v3 PATCH 3/3] vfs: make mounts and mountstats honor root dir like mountinfo does

[Dmitry V. Levin]

On Mon, Sep 29, 2014 at 10:20:57PM +0100, Al Viro wrote:
> On Tue, Sep 30, 2014 at 12:26:59AM +0400, Dmitry V. Levin wrote:
> 
> > > Again, we do not break userland.
> > 
> > Occasionally, we do.  If you say there was no chroot prefix removal in the
> > kernel before, and there seems to be a prefix removal for at least 5 years,
> > then there must have been a break some years ago that went unnoticed.
> 
> D'oh!  Yes, we did.  And yes, it was quite a few years ago, actually - back
> in 2000.  I plead being low on sleep and lower on caffeine...
> 
> For a moment I was even afraid that it was my own doing, but no - it came
> in https://lkml.org/lkml/2000/1/16/54.  Werner Almesberger.  IIRC, I hadn't
> realized that it was going to cause fun problems back then - basically,
> by that point 2.3 and 2.2 had already diverged a lot and quite a few things
> got written off as "oh, well - upgrade from 2.2 to 2.4 is going to be not
> far from building the system from scratch anyway, and nobody sane would be
> using 2.3 on anything other than scratch filesystem - IDE broken more often
> than not, memory corruption aplenty, etc."

Thanks, I won't be able to locate the change that fast.

It also means that the phrase "show mountpoints relative to the root
directory" I used in the commit message is irrelevant and should be
removed to avoid confusion.

> My apologies.  Hmm...  Filtering the out-of-root ones out is probably OK,
> but let's put that into -next after 3.18-rc1 and see if anyone yells.

BTW, the cleanup patch (vfs: cleanup show_mountinfo) could be safely
applied anytime - it's completely harmless.


-- 
ldv

[RESEND v4 PATCH 1/2] vfs: cleanup show_mountinfo

[Dmitry V. Levin]

Date: Wed, 17 Oct 2012 20:29:36 +0400

Starting with commit v3.2-rc4-1-g02125a8, seq_path_root() no longer
changes the value of its "struct path *root" argument.
Starting with commit v3.2-rc7-104-g8c9379e, the "struct path *root"
argument of seq_path_root() is const.
As result, the temporary variable "root" in show_mountinfo() that
holds a copy of struct path root is no longer needed.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 fs/proc_namespace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 73ca174..789e8d1 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -125,7 +125,6 @@ static int show_mountinfo(struct seq_file *m, struct vfsmount *mnt)
 	struct mount *r = real_mount(mnt);
 	struct super_block *sb = mnt->mnt_sb;
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };
-	struct path root = p->root;
 	int err = 0;
 
 	seq_printf(m, "%i %i %u:%u ", r->mnt_id, r->mnt_parent->mnt_id,
@@ -139,7 +138,7 @@ static int show_mountinfo(struct seq_file *m, struct vfsmount *mnt)
 	seq_putc(m, ' ');
 
 	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
-	err = seq_path_root(m, &mnt_path, &root, " \t\n\\");
+	err = seq_path_root(m, &mnt_path, &p->root, " \t\n\\");
 	if (err)
 		goto out;
 
-- 
ldv

[RESEND v4 PATCH 2/2] vfs: make mounts and mountstats honor root dir like mountinfo does

[Dmitry V. Levin]

As we already show mountpoints relative to the root directory, thanks
to the change made back in 2000, change show_vfsmnt() and show_vfsstat()
to skip out-of-root mountpoints the same way as show_mountinfo() does.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 fs/proc_namespace.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 789e8d1..0f96f71 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -91,6 +91,7 @@ static void show_type(struct seq_file *m, struct super_block *sb)
 
 static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 {
+	struct proc_mounts *p = proc_mounts(m);
 	struct mount *r = real_mount(mnt);
 	int err = 0;
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };
@@ -104,7 +105,10 @@ static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 		mangle(m, r->mnt_devname ? r->mnt_devname : "none");
 	}
 	seq_putc(m, ' ');
-	seq_path(m, &mnt_path, " \t\n\\");
+	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
+	err = seq_path_root(m, &mnt_path, &p->root, " \t\n\\");
+	if (err)
+		goto out;
 	seq_putc(m, ' ');
 	show_type(m, sb);
 	seq_puts(m, __mnt_is_readonly(mnt) ? " ro" : " rw");
@@ -181,6 +185,7 @@ out:
 
 static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 {
+	struct proc_mounts *p = proc_mounts(m);
 	struct mount *r = real_mount(mnt);
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };
 	struct super_block *sb = mnt_path.dentry->d_sb;
@@ -200,7 +205,10 @@ static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 
 	/* mount point */
 	seq_puts(m, " mounted on ");
-	seq_path(m, &mnt_path, " \t\n\\");
+	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
+	err = seq_path_root(m, &mnt_path, &p->root, " \t\n\\");
+	if (err)
+		goto out;
 	seq_putc(m, ' ');
 
 	/* file system type */
@@ -215,6 +223,7 @@ static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 	}
 
 	seq_putc(m, '\n');
+out:
 	return err;
 }
 
-- 
ldv