2008-06-23 18:25:39

by Miklos Szeredi

[permalink] [raw]
Subject: [patch v3] vfs: fix sys_getcwd for detached mounts

I tested this one in various situations, and it appears to do what
it's supposed to.

A somewhat analogous issue is if we have cwd (or fd) outside
current->root. Should we add a prefix for that case as well? Perhaps
a double slash?

--
From: Miklos Szeredi <[email protected]>

Currently getcwd(2) on a detached mount will give a garbled result:

> mkdir /mnt/foo
> mount --bind /etc /mnt/foo
> cd /mnt/foo/skel
> umount -l /mnt/foo
> /bin/pwd
etcskel

After the patch the result will be "detached:skel".

Or if the root was mounted:

> mount --bind / /mnt/foo
> cd /mnt/foo/etc
> umount -l /mnt/foo
> /bin/pwd
/etc

After the patch the result will be "detached:etc".

Thanks to John Johansen for pointing out this bug.

v2: comment from Al Viro: use "detached:" prefix to differentiate it from
attached paths.

v3: fix case when mounted dentry is root of the filesystem.

Reported-by: John Johansen <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/dcache.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c 2008-06-23 19:34:21.000000000 +0200
+++ linux-2.6/fs/dcache.c 2008-06-23 20:15:09.000000000 +0200
@@ -1762,6 +1762,12 @@ static int prepend_name(char **buffer, i
return prepend(buffer, buflen, name->name, name->len);
}

+static bool is_pseudo_root(struct dentry *dentry)
+{
+ return IS_ROOT(dentry) &&
+ (dentry->d_name.len != 1 || dentry->d_name.name[0] != '/');
+}
+
/**
* __d_path - return the path of a dentry
* @path: the dentry/vfsmount to report
@@ -1828,8 +1834,16 @@ char *__d_path(const struct path *path,

global_root:
retval += 1; /* hit the slash */
- if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
- goto Elong;
+
+ if (vfsmnt->mnt_ns != NULL || is_pseudo_root(dentry)) {
+ /* Attached, or pseudo filesystem with "foo:" prefix */
+ if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
+ goto Elong;
+ } else {
+ /* Detached */
+ if (prepend(&retval, &buflen, "detached:", 9) != 0)
+ goto Elong;
+ }
root->mnt = vfsmnt;
root->dentry = dentry;
return retval;


2008-06-23 18:29:19

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [patch v3] vfs: fix sys_getcwd for detached mounts

On Mon, Jun 23, 2008 at 08:25:26PM +0200, Miklos Szeredi wrote:
> I tested this one in various situations, and it appears to do what
> it's supposed to.
>
> A somewhat analogous issue is if we have cwd (or fd) outside
> current->root. Should we add a prefix for that case as well? Perhaps
> a double slash?

unreachable: ?

--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2008-06-23 18:50:54

by Al Viro

[permalink] [raw]
Subject: Re: [patch v3] vfs: fix sys_getcwd for detached mounts

On Mon, Jun 23, 2008 at 08:25:26PM +0200, Miklos Szeredi wrote:
> I tested this one in various situations, and it appears to do what
> it's supposed to.
>
> A somewhat analogous issue is if we have cwd (or fd) outside
> current->root. Should we add a prefix for that case as well? Perhaps
> a double slash?

OK, I've applied everything except the last one (all in for-linus in
vfs-2.6.git). As for the stuff outside current->root (or other namespace,
for that matter)... I don't like the idea of using // - as it is,
we have a nice sane check (path[0] != '/') and if anything, I'd consider
going for deleted:<path> instead of <path> (deleted) as we have now.
_That_ would eliminate the major annoyance with handling of unlinked/rmdired -
we'd get rid of guessing whether that's a file really called "a (deleted)"
or an unlinked one that used to be called "a"...

2008-06-23 19:06:55

by Miklos Szeredi

[permalink] [raw]
Subject: Re: [patch v3] vfs: fix sys_getcwd for detached mounts

> On Mon, Jun 23, 2008 at 08:25:26PM +0200, Miklos Szeredi wrote:
> > I tested this one in various situations, and it appears to do what
> > it's supposed to.
> >
> > A somewhat analogous issue is if we have cwd (or fd) outside
> > current->root. Should we add a prefix for that case as well? Perhaps
> > a double slash?
>
> OK, I've applied everything except the last one (all in for-linus in
> vfs-2.6.git).

Thanks. Could you please open a branch for this last patch and get
that into linux-next ? I've also got some other cleanup stuff
pending, and Andrew would prefer all VFS stuff to go through you.

> As for the stuff outside current->root (or other
> namespace, for that matter)... I don't like the idea of using // -
> as it is, we have a nice sane check (path[0] != '/') and if
> anything, I'd consider going for deleted:<path> instead of <path>
> (deleted) as we have now. _That_ would eliminate the major
> annoyance with handling of unlinked/rmdired - we'd get rid of
> guessing whether that's a file really called "a (deleted)" or an
> unlinked one that used to be called "a"...

Yes, it would be nice and consistent. OTOH, there is a real chance of
breaking some stuff, that already tries to parse "(deleted)". I don't
think it's worth touching that thing.

Miklos