Warning: empty root dentry name after lazy mount removal

Warning: empty root dentry name after lazy mount removal

[Ivan Delalande]

Hi,

Since commit 8ed936b "vfs: Lazily remove mounts on unlinked files and
directories.", I’m seeing a new warning from prepend_path() at
fs/dcache.c:2937 saying:

	Root dentry has weird name <>

Our particular occurrence happens because we are using an older version
of iproute (< v3.10, commit bcb9d40, but this also works with the master
and just reverting that commit), and the following scenario:

	term1# ip netns add foo
	term1# ip netns exec foo sh
	term1# ls -l /proc/self/fd/4
	lr-x------ 1 root root 64 May 15 11:02 /proc/self/fd/4 -> /var/run/netns/foo
	term2# ip netns del foo
	term1# ls -l /proc/self/fd/4
	[dmesg] WARNING: [...]
	[dmesg] Root dentry has weird name <>
	lr-x------ 1 root root 64 May 15 11:04 /proc/self/fd/4 -> /

Relevant moutinfo chunks before the `ip netns del`:

	29 0 8:2 / / rw,relatime - ext4 /dev/sda2 rw,data=ordered
	82 29 0:3 / /var/run/netns/foo rw,nosuid,nodev,noexec,relatime - proc proc rw

What I could observe is that after the namespace deletion, that mount 82
get isolated (mnt->parent = mnt), and so prepend_path considers it as
the global root. But the name of that dentry has d_name.len == 0, which
triggers the WARN in that function.

I’m still struggling to understand all the relations between the VFS
structures and at which point they get initialized precisely. Do you
have any idea how to fix this problem?

Thanks,
-- 
Ivan "Colona" Delalande
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Warning: empty root dentry name after lazy mount removal

[Omar Sandoval]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Fri, May 15, 2015 at 08:58:20PM +0200, Ivan Delalande wrote:
> Hi,
> 
> Since commit 8ed936b "vfs: Lazily remove mounts on unlinked files and
> directories.", I’m seeing a new warning from prepend_path() at
> fs/dcache.c:2937 saying:
> 
> 	Root dentry has weird name <>
> 
> Our particular occurrence happens because we are using an older version
> of iproute (< v3.10, commit bcb9d40, but this also works with the master
> and just reverting that commit), and the following scenario:
> 
> 	term1# ip netns add foo
> 	term1# ip netns exec foo sh
> 	term1# ls -l /proc/self/fd/4
> 	lr-x------ 1 root root 64 May 15 11:02 /proc/self/fd/4 -> /var/run/netns/foo
> 	term2# ip netns del foo
> 	term1# ls -l /proc/self/fd/4
> 	[dmesg] WARNING: [...]
> 	[dmesg] Root dentry has weird name <>
> 	lr-x------ 1 root root 64 May 15 11:04 /proc/self/fd/4 -> /
> 
> Relevant moutinfo chunks before the `ip netns del`:
> 
> 	29 0 8:2 / / rw,relatime - ext4 /dev/sda2 rw,data=ordered
> 	82 29 0:3 / /var/run/netns/foo rw,nosuid,nodev,noexec,relatime - proc proc rw
> 
> What I could observe is that after the namespace deletion, that mount 82
> get isolated (mnt->parent = mnt), and so prepend_path considers it as
> the global root. But the name of that dentry has d_name.len == 0, which
> triggers the WARN in that function.
> 
> I’m still struggling to understand all the relations between the VFS
> structures and at which point they get initialized precisely. Do you
> have any idea how to fix this problem?
> 

I'm attaching a minimal script that reproduces this on 4.1-rc4. I'm
taking a look, but Eric or Al will probably figure this out before I get
the chance :)

-- 
Omar

[-- Attachment #2: repro.sh --]
[-- Type: text/plain, Size: 433 bytes --]

#!/bin/sh

set -e

MNT=testmnt

# Create a new namespace and bind-mount it to keep it alive.
(
touch "$MNT"
unshare --net mount --bind /proc/self/ns/net "$MNT"
)

rm -f barrier && mkfifo barrier

# Hold a reference while the namespace gets unmounted.
(
exec 4<"$MNT"
ls -l /proc/self/fd
cat < barrier
ls -l /proc/self/fd
) &

# Unmount the namespace while there's still a reference.
cat /dev/null > barrier
umount --lazy "$MNT"
wait

[PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Eric W. Biederman]

Omar Sandoval <osandov@osandov.com> writes:

> On Fri, May 15, 2015 at 08:58:20PM +0200, Ivan Delalande wrote:
>> Hi,
>> 
>> I’m still struggling to understand all the relations between the VFS
>> structures and at which point they get initialized precisely. Do you
>> have any idea how to fix this problem?

The relationships in this case are quite odd and can be simplified.

> I'm attaching a minimal script that reproduces this on 4.1-rc4. I'm
> taking a look, but Eric or Al will probably figure this out before I get
> the chance :)

This works for me, can you confirm it works for you folks as well?

Thank you,
Eric


From: "Eric W. Biederman" <ebiederm@xmission.com>
Date: Wed, 20 May 2015 16:39:00 -0500
Subject: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

Now that these files are no longer in proc we can stop playing games
with d_alloc_pseudo and d_dname.  This change causes a couple of user
visible changes:

- Opening /proc/*/ns/* and then readlink on /proc/self/fd/N now sees a
  prepended / in the filename.

- /proc/mountinfo now gives useful information on what is mounted.

- Bind mounting /proc/*/ns/*, opening the mounted file, removing the
  bind mount, and readlink on /proc/self/fd/N now sees / (as it should)
  instead of triggering a warning and a kernel stack backtrace from
  prepend_path.

Cc: stable@vger.kernel.org
Reported-by: Ivan Delalande <colona@arista.com>
Reported-by: Omar Sandoval <osandov@osandov.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/dcache.c |  7 +------
 fs/nsfs.c   | 19 +++++--------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 656ce522a218..e0f0967ef24c 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3087,13 +3087,8 @@ char *d_path(const struct path *path, char *buf, int buflen)
 	 * thus don't need to be hashed.  They also don't need a name until a
 	 * user wants to identify the object in /proc/pid/fd/.  The little hack
 	 * below allows us to generate a name for these objects on demand:
-	 *
-	 * Some pseudo inodes are mountable.  When they are mounted
-	 * path->dentry == path->mnt->mnt_root.  In that case don't call d_dname
-	 * and instead have d_path return the mounted path.
 	 */
-	if (path->dentry->d_op && path->dentry->d_op->d_dname &&
-	    (!IS_ROOT(path->dentry) || path->dentry != path->mnt->mnt_root))
+	if (path->dentry->d_op && path->dentry->d_op->d_dname)
 		return path->dentry->d_op->d_dname(path->dentry, buf, buflen);
 
 	rcu_read_lock();
diff --git a/fs/nsfs.c b/fs/nsfs.c
index 99521e7c492b..b9ecfa6cb46d 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -11,15 +11,6 @@ static const struct file_operations ns_file_operations = {
 	.llseek		= no_llseek,
 };
 
-static char *ns_dname(struct dentry *dentry, char *buffer, int buflen)
-{
-	struct inode *inode = d_inode(dentry);
-	const struct proc_ns_operations *ns_ops = dentry->d_fsdata;
-
-	return dynamic_dname(dentry, buffer, buflen, "%s:[%lu]",
-		ns_ops->name, inode->i_ino);
-}
-
 static void ns_prune_dentry(struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
@@ -33,7 +24,6 @@ const struct dentry_operations ns_dentry_operations =
 {
 	.d_prune	= ns_prune_dentry,
 	.d_delete	= always_delete_dentry,
-	.d_dname	= ns_dname,
 };
 
 static void nsfs_evict(struct inode *inode)
@@ -47,7 +37,7 @@ void *ns_get_path(struct path *path, struct task_struct *task,
 			const struct proc_ns_operations *ns_ops)
 {
 	struct vfsmount *mnt = mntget(nsfs_mnt);
-	struct qstr qname = { .name = "", };
+	char name[DNAME_INLINE_LEN];
 	struct dentry *dentry;
 	struct inode *inode;
 	struct ns_common *ns;
@@ -80,6 +70,7 @@ slow:
 		mntput(mnt);
 		return ERR_PTR(-ENOMEM);
 	}
+	snprintf(name, sizeof(name), "%s:[%u]", ns_ops->name, ns->inum);
 	inode->i_ino = ns->inum;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_flags |= S_IMMUTABLE;
@@ -87,13 +78,13 @@ slow:
 	inode->i_fop = &ns_file_operations;
 	inode->i_private = ns;
 
-	dentry = d_alloc_pseudo(mnt->mnt_sb, &qname);
+	dentry = d_alloc_name(mnt->mnt_root, name);
 	if (!dentry) {
 		iput(inode);
 		mntput(mnt);
 		return ERR_PTR(-ENOMEM);
 	}
-	d_instantiate(dentry, inode);
+	d_add(dentry, inode);
 	dentry->d_fsdata = (void *)ns_ops;
 	d = atomic_long_cmpxchg(&ns->stashed, 0, (unsigned long)dentry);
 	if (d) {
@@ -143,7 +134,7 @@ static const struct super_operations nsfs_ops = {
 static struct dentry *nsfs_mount(struct file_system_type *fs_type,
 			int flags, const char *dev_name, void *data)
 {
-	return mount_pseudo(fs_type, "nsfs:", &nsfs_ops,
+	return mount_pseudo(fs_type, "/", &nsfs_ops,
 			&ns_dentry_operations, NSFS_MAGIC);
 }
 static struct file_system_type nsfs = {
-- 
2.2.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Omar Sandoval]

On Wed, May 20, 2015 at 05:05:30PM -0500, Eric W. Biederman wrote:
> This works for me, can you confirm it works for you folks as well?
> 
> Thank you,
> Eric

Yup, neither reproducer produces the warning anymore.
Reported-and-Tested-by: Omar Sandoval <osandov@osandov.com>

> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Wed, 20 May 2015 16:39:00 -0500
> Subject: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.
> 
> Now that these files are no longer in proc we can stop playing games
> with d_alloc_pseudo and d_dname.  This change causes a couple of user
> visible changes:
> 
> - Opening /proc/*/ns/* and then readlink on /proc/self/fd/N now sees a
>   prepended / in the filename.
> 
> - /proc/mountinfo now gives useful information on what is mounted.
> 
> - Bind mounting /proc/*/ns/*, opening the mounted file, removing the
>   bind mount, and readlink on /proc/self/fd/N now sees / (as it should)

What's the rationale for "/" in this case? (I'm not disagreeing, just
looking to learn something.)

Thanks,
-- 
Omar

Re: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Ivan Delalande]

On Wed, May 20, 2015 at 05:05:30PM -0500, Eric W. Biederman wrote:
> Omar Sandoval <osandov@osandov.com> writes:
>> I'm attaching a minimal script that reproduces this on 4.1-rc4. I'm
>> taking a look, but Eric or Al will probably figure this out before I get
>> the chance :)
> 
> This works for me, can you confirm it works for you folks as well?

Yeah, your patch works with our setup and when running our initial
test-case as well. Thanks a lot Eric.

> From: "Eric W. Biederman" <ebiederm@xmission.com>
> Date: Wed, 20 May 2015 16:39:00 -0500
> Subject: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.
> 
> Now that these files are no longer in proc we can stop playing games
> with d_alloc_pseudo and d_dname.  This change causes a couple of user
> visible changes:
> 
> - Opening /proc/*/ns/* and then readlink on /proc/self/fd/N now sees a
>   prepended / in the filename.
> 
> - /proc/mountinfo now gives useful information on what is mounted.
> 
> - Bind mounting /proc/*/ns/*, opening the mounted file, removing the
>   bind mount, and readlink on /proc/self/fd/N now sees / (as it should)
>   instead of triggering a warning and a kernel stack backtrace from
>   prepend_path.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Ivan Delalande <colona@arista.com>
> Reported-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Tested-by: Ivan Delalande <colona@arista.com>

-- 
Ivan "Colona" Delalande

Re: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Al Viro]

On Wed, May 20, 2015 at 05:05:30PM -0500, Eric W. Biederman wrote:

> -	dentry = d_alloc_pseudo(mnt->mnt_sb, &qname);
> +	dentry = d_alloc_name(mnt->mnt_root, name);
>  	if (!dentry) {
>  		iput(inode);
>  		mntput(mnt);
>  		return ERR_PTR(-ENOMEM);
>  	}
> -	d_instantiate(dentry, inode);
> +	d_add(dentry, inode);

Careful - that might have non-trivial effects.  Namely, you are making
the root dentry of that sucker a contention point and adding to hash
pollution...  It's probably not going to cause visible problems, but
it's worth profiling just to be sure.

Besides, you are violating a bunch of rules here - several hashed
children of the same directory with the same name all at once...
not nice.

Re: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Ivan Delalande]

On Thu, May 21, 2015 at 12:23:25AM +0100, Al Viro wrote:
> On Wed, May 20, 2015 at 05:05:30PM -0500, Eric W. Biederman wrote:
> > -	dentry = d_alloc_pseudo(mnt->mnt_sb, &qname);
> > +	dentry = d_alloc_name(mnt->mnt_root, name);
> >  	if (!dentry) {
> >  		iput(inode);
> >  		mntput(mnt);
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> > -	d_instantiate(dentry, inode);
> > +	d_add(dentry, inode);
> 
> Careful - that might have non-trivial effects.  Namely, you are making
> the root dentry of that sucker a contention point and adding to hash
> pollution...  It's probably not going to cause visible problems, but
> it's worth profiling just to be sure.
> 
> Besides, you are violating a bunch of rules here - several hashed
> children of the same directory with the same name all at once...
> not nice.

Hey Eric, did you have any thought about Al’s concerns?

Thanks,
-- 
Ivan "Colona" Delalande
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Eric W. Biederman]

Ivan Delalande <colona@arista.com> writes:

> On Thu, May 21, 2015 at 12:23:25AM +0100, Al Viro wrote:
>> On Wed, May 20, 2015 at 05:05:30PM -0500, Eric W. Biederman wrote:
>> > -	dentry = d_alloc_pseudo(mnt->mnt_sb, &qname);
>> > +	dentry = d_alloc_name(mnt->mnt_root, name);
>> >  	if (!dentry) {
>> >  		iput(inode);
>> >  		mntput(mnt);
>> >  		return ERR_PTR(-ENOMEM);
>> >  	}
>> > -	d_instantiate(dentry, inode);
>> > +	d_add(dentry, inode);
>> 
>> Careful - that might have non-trivial effects.  Namely, you are making
>> the root dentry of that sucker a contention point and adding to hash
>> pollution...  It's probably not going to cause visible problems, but
>> it's worth profiling just to be sure.
>> 
>> Besides, you are violating a bunch of rules here - several hashed
>> children of the same directory with the same name all at once...
>> not nice.
>
> Hey Eric, did you have any thought about Al’s concerns?

Massive difference in perspective for the most part.  It did cause me to
step back and really look at what that code is doing and why.

For the immediate problem the issue is that the WARN_ON is warning about
something nothing in the kernel has done for 5 years and in practice as
you have seen is actually wrong.  So deleting the warning message
appears the best way to handle the situation you are seeing.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] vfs: Remove incorrect debugging WARN in prepend_path

[Eric W. Biederman]

The warning message in prepend_path is unclear and outdated.  It was
added as a warning that the mechanism for generating names of pseudo
files had been removed from prepend_path and d_dname should be used
instead.  Unfortunately the warning reads like a general warning,
making it unclear what to do with it.

Remove the warning.  The transition it was added to warn about is long
over, and I added code several years ago which in rare cases causes
the warning to fire on legitimate code, and the warning is now firing
and scaring people for no good reason.

Cc: stable@vger.kernel.org
Reported-by: Ivan Delalande <colona@arista.com>
Reported-by: Omar Sandoval <osandov@osandov.com>
Fixes: f48cfddc6729e ("vfs: In d_path don't call d_dname on a mount point")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

Al would you like to pick this one up or should I?

 fs/dcache.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 656ce522a218..615dfc2aa752 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2927,17 +2927,6 @@ restart:
 				vfsmnt = &mnt->mnt;
 				continue;
 			}
-			/*
-			 * Filesystems needing to implement special "root names"
-			 * should do so with ->d_dname()
-			 */
-			if (IS_ROOT(dentry) &&
-			   (dentry->d_name.len != 1 ||
-			    dentry->d_name.name[0] != '/')) {
-				WARN(1, "Root dentry has weird name <%.*s>\n",
-				     (int) dentry->d_name.len,
-				     dentry->d_name.name);
-			}
 			if (!error)
 				error = is_mounted(vfsmnt) ? 1 : 2;
 			break;
-- 
2.2.1

Re: [PATCH] vfs: Fix, simpliy and stop using d_dname for the /proc/*/ns/* files.

[Ivan Delalande]

On Sat, Jun 06, 2015 at 02:27:24PM -0500, Eric W. Biederman wrote:
> Ivan Delalande <colona@arista.com> writes:
> 
> > On Thu, May 21, 2015 at 12:23:25AM +0100, Al Viro wrote:
> >> On Wed, May 20, 2015 at 05:05:30PM -0500, Eric W. Biederman wrote:
> >> > -	dentry = d_alloc_pseudo(mnt->mnt_sb, &qname);
> >> > +	dentry = d_alloc_name(mnt->mnt_root, name);
> >> >  	if (!dentry) {
> >> >  		iput(inode);
> >> >  		mntput(mnt);
> >> >  		return ERR_PTR(-ENOMEM);
> >> >  	}
> >> > -	d_instantiate(dentry, inode);
> >> > +	d_add(dentry, inode);
> >> 
> >> Careful - that might have non-trivial effects.  Namely, you are making
> >> the root dentry of that sucker a contention point and adding to hash
> >> pollution...  It's probably not going to cause visible problems, but
> >> it's worth profiling just to be sure.
> >> 
> >> Besides, you are violating a bunch of rules here - several hashed
> >> children of the same directory with the same name all at once...
> >> not nice.
> >
> > Hey Eric, did you have any thought about Al’s concerns?
> 
> Massive difference in perspective for the most part.  It did cause me to
> step back and really look at what that code is doing and why.
> 
> For the immediate problem the issue is that the WARN_ON is warning about
> something nothing in the kernel has done for 5 years and in practice as
> you have seen is actually wrong.  So deleting the warning message
> appears the best way to handle the situation you are seeing.

Ok, great. Thanks for all the info and the new patch, Eric.

-- 
Ivan "Colona" Delalande
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html