From: John Johansen <[email protected]>
When __d_path() hits a lazily unmounted mount point, it tries to prepend
the name of the lazily unmounted dentry to the path name. It gets this wrong,
and also overwrites the slash that separates the name from the following
pathname component. This patch fixes that; if a process was in directory
/foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
missing slash), while the new result with this patch is ``/foo/bar''.
Signed-off-by: John Johansen <[email protected]>
---
fs/dcache.c | 27 +++++++++++++++++++++++----
1 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 953173a..df49666 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
retval = end-1;
*retval = '/';
- for (;;) {
+ while(dentry != root->dentry || vfsmnt != root->mnt) {
struct dentry * parent;
- if (dentry == root->dentry && vfsmnt == root->mnt)
- break;
if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
/* Global root? */
if (vfsmnt->mnt_parent == vfsmnt) {
@@ -1950,9 +1948,30 @@ out:
return retval;
global_root:
- retval += 1; /* hit the slash */
+ /*
+ * We went past the (vfsmount, dentry) we were looking for and have
+ * either hit a root dentry, a lazily unmounted dentry, an
+ * unconnected dentry, or the file is on a pseudo filesystem.
+ */
+ if ((dentry->d_sb->s_flags & MS_NOUSER) ||
+ (dentry->d_name.len = 1 && *dentry->d_name.name == '/')) {
+ /*
+ * Historically, we also glue together the root dentry and
+ * remaining name for pseudo filesystems like pipefs, which
+ * have the MS_NOUSER flag set. This results in pathnames
+ * like "pipe:[439336]".
+ */
+ retval += 1; /* overwrite the slash */
+ buflen++;
+ }
if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
goto Elong;
+
+ /* connect lazily unmounted mount point */
+ if (*retval != '/' && !(dentry->d_sb->s_flags & MS_NOUSER) &&
+ prepend(&retval, &buflen, "/", 1) != 0)
+ goto Elong;
+
root->mnt = vfsmnt;
root->dentry = dentry;
goto out;
--
1.6.6.1
[email protected] wrote:
> From: John Johansen <[email protected]>
>
> When __d_path() hits a lazily unmounted mount point, it tries to prepend
> the name of the lazily unmounted dentry to the path name. It gets this wrong,
> and also overwrites the slash that separates the name from the following
> pathname component. This patch fixes that; if a process was in directory
> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
> missing slash), while the new result with this patch is ``/foo/bar''.
>
It seems I left out the basic test for this. From a shell you can do
> mkdir /tmp/foo
> sudo mount --bind /home/jj /tmp/foo #substitute /home/jj as you wish
> cd /tmp/foo/bar # assumes /home/jj/bar exists
> /bin/pwd
/tmp/foo/bar
> sudo umount -l /tmp/foo
> /bin/pwd
jjbar
> > cd ..
> > /bin/pwd
jj
Quoting [email protected] ([email protected]):
> From: John Johansen <[email protected]>
>
> When __d_path() hits a lazily unmounted mount point, it tries to prepend
> the name of the lazily unmounted dentry to the path name. It gets this wrong,
> and also overwrites the slash that separates the name from the following
> pathname component. This patch fixes that; if a process was in directory
> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
> missing slash), while the new result with this patch is ``/foo/bar''.
>
> Signed-off-by: John Johansen <[email protected]>
Tested-by: Serge Hallyn <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
In particular, I tested for closed pipes and lazily umounted mounts
and binds - lazily umounted binds are fixed, and no apparent regression
in the others.
Thanks, John.
-serge
> ---
> fs/dcache.c | 27 +++++++++++++++++++++++----
> 1 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 953173a..df49666 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
> retval = end-1;
> *retval = '/';
>
> - for (;;) {
> + while(dentry != root->dentry || vfsmnt != root->mnt) {
> struct dentry * parent;
>
> - if (dentry == root->dentry && vfsmnt == root->mnt)
> - break;
> if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
> /* Global root? */
> if (vfsmnt->mnt_parent == vfsmnt) {
> @@ -1950,9 +1948,30 @@ out:
> return retval;
>
> global_root:
> - retval += 1; /* hit the slash */
> + /*
> + * We went past the (vfsmount, dentry) we were looking for and have
> + * either hit a root dentry, a lazily unmounted dentry, an
> + * unconnected dentry, or the file is on a pseudo filesystem.
> + */
> + if ((dentry->d_sb->s_flags & MS_NOUSER) ||
> + (dentry->d_name.len = 1 && *dentry->d_name.name == '/')) {
> + /*
> + * Historically, we also glue together the root dentry and
> + * remaining name for pseudo filesystems like pipefs, which
> + * have the MS_NOUSER flag set. This results in pathnames
> + * like "pipe:[439336]".
> + */
> + retval += 1; /* overwrite the slash */
> + buflen++;
> + }
> if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
> goto Elong;
> +
> + /* connect lazily unmounted mount point */
> + if (*retval != '/' && !(dentry->d_sb->s_flags & MS_NOUSER) &&
> + prepend(&retval, &buflen, "/", 1) != 0)
> + goto Elong;
> +
> root->mnt = vfsmnt;
> root->dentry = dentry;
> goto out;
> --
> 1.6.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 20 Feb 2010 04:27:38 -0800 [email protected] wrote:
> From: John Johansen <[email protected]>
>
> When __d_path() hits a lazily unmounted mount point, it tries to prepend
> the name of the lazily unmounted dentry to the path name. It gets this wrong,
> and also overwrites the slash that separates the name from the following
> pathname component. This patch fixes that; if a process was in directory
> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
> missing slash), while the new result with this patch is ``/foo/bar''.
>
> Signed-off-by: John Johansen <[email protected]>
> ---
> fs/dcache.c | 27 +++++++++++++++++++++++----
> 1 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 953173a..df49666 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
> retval = end-1;
> *retval = '/';
>
> - for (;;) {
> + while(dentry != root->dentry || vfsmnt != root->mnt) {
Please put a space between the `while' and the `('.
> struct dentry * parent;
>
> - if (dentry == root->dentry && vfsmnt == root->mnt)
> - break;
> if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
> /* Global root? */
> if (vfsmnt->mnt_parent == vfsmnt) {
> @@ -1950,9 +1948,30 @@ out:
> return retval;
>
> global_root:
> - retval += 1; /* hit the slash */
> + /*
> + * We went past the (vfsmount, dentry) we were looking for and have
> + * either hit a root dentry, a lazily unmounted dentry, an
> + * unconnected dentry, or the file is on a pseudo filesystem.
> + */
> + if ((dentry->d_sb->s_flags & MS_NOUSER) ||
> + (dentry->d_name.len = 1 && *dentry->d_name.name == '/')) {
Did you really mean to assign 1 to dentry->d_name.len here? Was `=='
intended? I hope so, because modifying the dentry in d_path() would be odd.
If this was a mistake then why did the patch pass testing?
> + /*
> + * Historically, we also glue together the root dentry and
> + * remaining name for pseudo filesystems like pipefs, which
> + * have the MS_NOUSER flag set. This results in pathnames
> + * like "pipe:[439336]".
> + */
> + retval += 1; /* overwrite the slash */
> + buflen++;
> + }
> if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
> goto Elong;
> +
> + /* connect lazily unmounted mount point */
> + if (*retval != '/' && !(dentry->d_sb->s_flags & MS_NOUSER) &&
> + prepend(&retval, &buflen, "/", 1) != 0)
> + goto Elong;
> +
> root->mnt = vfsmnt;
> root->dentry = dentry;
> goto out;
Quoting Andrew Morton ([email protected]):
> On Sat, 20 Feb 2010 04:27:38 -0800 [email protected] wrote:
>
> > From: John Johansen <[email protected]>
> >
> > When __d_path() hits a lazily unmounted mount point, it tries to prepend
> > the name of the lazily unmounted dentry to the path name. It gets this wrong,
> > and also overwrites the slash that separates the name from the following
> > pathname component. This patch fixes that; if a process was in directory
> > /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
> > missing slash), while the new result with this patch is ``/foo/bar''.
> >
> > Signed-off-by: John Johansen <[email protected]>
> > ---
> > fs/dcache.c | 27 +++++++++++++++++++++++----
> > 1 files changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 953173a..df49666 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
> > retval = end-1;
> > *retval = '/';
> >
> > - for (;;) {
> > + while(dentry != root->dentry || vfsmnt != root->mnt) {
>
> Please put a space between the `while' and the `('.
>
> > struct dentry * parent;
> >
> > - if (dentry == root->dentry && vfsmnt == root->mnt)
> > - break;
> > if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
> > /* Global root? */
> > if (vfsmnt->mnt_parent == vfsmnt) {
> > @@ -1950,9 +1948,30 @@ out:
> > return retval;
> >
> > global_root:
> > - retval += 1; /* hit the slash */
> > + /*
> > + * We went past the (vfsmount, dentry) we were looking for and have
> > + * either hit a root dentry, a lazily unmounted dentry, an
> > + * unconnected dentry, or the file is on a pseudo filesystem.
> > + */
> > + if ((dentry->d_sb->s_flags & MS_NOUSER) ||
> > + (dentry->d_name.len = 1 && *dentry->d_name.name == '/')) {
>
> Did you really mean to assign 1 to dentry->d_name.len here? Was `=='
> intended? I hope so, because modifying the dentry in d_path() would be odd.
>
> If this was a mistake then why did the patch pass testing?
Jinkeys! Well I guess it must've passed testing bc that will only be
done for MS_NOUSER filesystems, and we won't later be doing any 'ls'
so the d_name.len isn't really used?
> > + /*
> > + * Historically, we also glue together the root dentry and
> > + * remaining name for pseudo filesystems like pipefs, which
> > + * have the MS_NOUSER flag set. This results in pathnames
> > + * like "pipe:[439336]".
> > + */
> > + retval += 1; /* overwrite the slash */
> > + buflen++;
> > + }
> > if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
> > goto Elong;
> > +
> > + /* connect lazily unmounted mount point */
> > + if (*retval != '/' && !(dentry->d_sb->s_flags & MS_NOUSER) &&
> > + prepend(&retval, &buflen, "/", 1) != 0)
> > + goto Elong;
> > +
> > root->mnt = vfsmnt;
> > root->dentry = dentry;
> > goto out;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Andrew Morton wrote:
> On Sat, 20 Feb 2010 04:27:38 -0800 [email protected] wrote:
>
>> From: John Johansen <[email protected]>
>>
>> When __d_path() hits a lazily unmounted mount point, it tries to prepend
>> the name of the lazily unmounted dentry to the path name. It gets this wrong,
>> and also overwrites the slash that separates the name from the following
>> pathname component. This patch fixes that; if a process was in directory
>> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
>> missing slash), while the new result with this patch is ``/foo/bar''.
>>
>> Signed-off-by: John Johansen <[email protected]>
>> ---
>> fs/dcache.c | 27 +++++++++++++++++++++++----
>> 1 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 953173a..df49666 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
>> retval = end-1;
>> *retval = '/';
>>
>> - for (;;) {
>> + while(dentry != root->dentry || vfsmnt != root->mnt) {
>
thanks, forgot to refresh after checkpatch
> Please put a space between the `while' and the `('.
>
>> struct dentry * parent;
>>
>> - if (dentry == root->dentry && vfsmnt == root->mnt)
>> - break;
>> if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
>> /* Global root? */
>> if (vfsmnt->mnt_parent == vfsmnt) {
>> @@ -1950,9 +1948,30 @@ out:
>> return retval;
>>
>> global_root:
>> - retval += 1; /* hit the slash */
>> + /*
>> + * We went past the (vfsmount, dentry) we were looking for and have
>> + * either hit a root dentry, a lazily unmounted dentry, an
>> + * unconnected dentry, or the file is on a pseudo filesystem.
>> + */
>> + if ((dentry->d_sb->s_flags & MS_NOUSER) ||
>> + (dentry->d_name.len = 1 && *dentry->d_name.name == '/')) {
>
> Did you really mean to assign 1 to dentry->d_name.len here? Was `=='
> intended? I hope so, because modifying the dentry in d_path() would be odd.
>
> If this was a mistake then why did the patch pass testing?
>
Nope, definite bug, missed that case in testing. In this case every test that
had a leading '/' had a d_name.len == 1 as well.
I haven't seen the case of where a root dentry has a leading / and doesn't have
a d_name.len == 1 and if that case never happens the test wouldn't be needed.
I will respin the patch, and include testing this time
thanks
john
Second iteration of the __d_path patch
Fixes
* formating problem - while(dentry
* the assignment where comparison should be used
Tests to exercise the behavior and test for regressions follows
---
#!/bin/bash
#simple script to test behavior of lazily unmounted bind mount
DIR1=`mktemp -d /tmp/foo.XXXXXXXXXX`
DIR2=`mktemp -d /tmp/test.XXXXXXXXXX`
mkdir "$DIR2/bar"
mount --bind "$DIR2" "$DIR1"
cd "$DIR1/bar"
echo "Before umount -l"
/bin/pwd
umount -l "$DIR1"
echo "After umount -l"
/bin/pwd
echo "After cd .."
cd ..
/bin/pwd
rm -rf "$DIR1" "$DIR2"
From: John Johansen <[email protected]>
When __d_path() hits a lazily unmounted mount point, it tries to prepend
the name of the lazily unmounted dentry to the path name. It gets this wrong,
and also overwrites the slash that separates the name from the following
pathname component. This patch fixes that; if a process was in directory
/foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
missing slash), while the new result with this patch is ``/foo/bar''.
Signed-off-by: John Johansen <[email protected]>
---
fs/dcache.c | 27 +++++++++++++++++++++++----
1 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 953173a..46096b4 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
retval = end-1;
*retval = '/';
- for (;;) {
+ while (dentry != root->dentry || vfsmnt != root->mnt) {
struct dentry * parent;
- if (dentry == root->dentry && vfsmnt == root->mnt)
- break;
if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
/* Global root? */
if (vfsmnt->mnt_parent == vfsmnt) {
@@ -1950,9 +1948,30 @@ out:
return retval;
global_root:
- retval += 1; /* hit the slash */
+ /*
+ * We went past the (vfsmount, dentry) we were looking for and have
+ * either hit a root dentry, a lazily unmounted dentry, an
+ * unconnected dentry, or the file is on a pseudo filesystem.
+ */
+ if ((dentry->d_sb->s_flags & MS_NOUSER) ||
+ (dentry->d_name.len == 1 && *dentry->d_name.name == '/')) {
+ /*
+ * Historically, we also glue together the root dentry and
+ * remaining name for pseudo filesystems like pipefs, which
+ * have the MS_NOUSER flag set. This results in pathnames
+ * like "pipe:[439336]".
+ */
+ retval += 1; /* overwrite the slash */
+ buflen++;
+ }
if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
goto Elong;
+
+ /* connect lazily unmounted mount point */
+ if (*retval != '/' && !(dentry->d_sb->s_flags & MS_NOUSER) &&
+ prepend(&retval, &buflen, "/", 1) != 0)
+ goto Elong;
+
root->mnt = vfsmnt;
root->dentry = dentry;
goto out;
--
1.6.6.1
On Sat, 20 Feb 2010, [email protected] wrote:
> From: John Johansen <[email protected]>
>
> When __d_path() hits a lazily unmounted mount point, it tries to prepend
> the name of the lazily unmounted dentry to the path name. It gets this wrong,
> and also overwrites the slash that separates the name from the following
> pathname component. This patch fixes that; if a process was in directory
> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
> missing slash), while the new result with this patch is ``/foo/bar''.
Example:
# mkdir -p /tmp/foo/bar
# mkdir /tmp/mnt
# mount --bind /tmp/foo /tmp/mnt
# cd /tmp/mnt/bar
# /bin/pwd
/tmp/mnt/bar
# umount -l /tmp/mnt
# /bin/pwd
foobar
After the patch it will be /foo/bar.
Why is the path starting with "/foo"? Does that make any sense?
Last time this was discussed the proposals which are halfway sane
were:
a) "(unreachable)/bar" or something along those lines
b) ENOENT
And with either one care needs to be taken to limit this change to
interfaces (both internal and userspace) where it's not likely to
cause breakage.
Thanks,
Miklos
On 02/26/2010 04:07 AM, Miklos Szeredi wrote:
> On Sat, 20 Feb 2010, [email protected] wrote:
>> From: John Johansen<[email protected]>
>>
>> When __d_path() hits a lazily unmounted mount point, it tries to prepend
>> the name of the lazily unmounted dentry to the path name. It gets this wrong,
>> and also overwrites the slash that separates the name from the following
>> pathname component. This patch fixes that; if a process was in directory
>> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
>> missing slash), while the new result with this patch is ``/foo/bar''.
>
> Example:
>
> # mkdir -p /tmp/foo/bar
> # mkdir /tmp/mnt
> # mount --bind /tmp/foo /tmp/mnt
> # cd /tmp/mnt/bar
> # /bin/pwd
> /tmp/mnt/bar
> # umount -l /tmp/mnt
> # /bin/pwd
> foobar
>
> After the patch it will be /foo/bar.
>
> Why is the path starting with "/foo"? Does that make any sense?
>
not a lot except, connecting disconnected paths to root is what
is currently done for paths that aren't reachable but have an fs
as their root (ie the last dentry is / so it looks connected to
root).
I would be happy in this case to leave bind mounts disconnected
(no leading /) and just fix the overwriting of the internal /.
I'll make the change.
> Last time this was discussed the proposals which are halfway sane
> were:
>
> a) "(unreachable)/bar" or something along those lines
> b) ENOENT
>
right, I actually have another couple of __d_path patches I need
to kick out for discussion. Last time we rolled 3 different
changes into a single patch. This time I wanted to isolate the
changes per patch. I'll kick them all out today.
> And with either one care needs to be taken to limit this change to
> interfaces (both internal and userspace) where it's not likely to
> cause breakage.
>
agreed.