[PATCH stable request 3.10] vfs: Remove incorrect debugging WARN in prepend_path

[PATCH stable request 3.10] vfs: Remove incorrect debugging WARN in prepend_path

[Vinson Lee]

From: Vinson Lee <vlee@twitter.com>

Hi.

Please backport upstream 4.2 commit 93e3bce6287e1fb3e60d3324ed08555b5bbafa89
("vfs: Remove incorrect debugging WARN in prepend_path") to stable kernel 3.10.

This patch has already been backported to stable kernels 3.12, 3.13, 3.14,
3.16, 3.18, 4.0, and 4.1.

The upstream patch does not apply cleanly and a backport is provided.

This patch fixes 3.13 commit f48cfddc6729e
("vfs: In d_path don't call d_dname on a mount point"), but this patch was
itself was backported in 3.10.28.

This patch addresses this warning seen in 3.10.76.

------------[ cut here ]------------
WARNING: at fs/dcache.c:2576 prepend_path+0x12b/0x17c()
Root dentry has weird name <>
Modules linked in: sch_fq_codel sch_htb cls_basic act_mirred cls_u32 veth sch_ingress netconsole configfs ipv6 dm_multipath scsi_dh video sbs sbshc hed acpi_pad a
 parport_pc lp parport tcp_diag inet_diag ipmi_si ipmi_devintf ipmi_msghandler dell_rbu sg iTCO_wdt iTCO_vendor_support dcdbas igb i2c_algo_bit ptp pps_core i7cor
ioatdma i2c_i801 shpchp i2c_core lpc_ich microcode dca mfd_core freq_table mperf ahci libahci libata sd_mod scsi_mod
CPU: 9 PID: 13242 Comm: lsof Tainted: G      D W    3.10.76 #1
 0000000000000000 ffff880b28ef3da0 ffffffff8146857f ffff880b28ef3dd8
 ffffffff8103cc01 ffff880b28ef3de8 ffff88060275fa20 ffff8803d4dbb740
 ffff88060275fa00 0000000000000000 ffff880b28ef3e38 ffffffff8103cc65
Call Trace:
 [<ffffffff8146857f>] dump_stack+0x19/0x1b
 [<ffffffff8103cc01>] warn_slowpath_common+0x65/0x7d
 [<ffffffff8103cc65>] warn_slowpath_fmt+0x4c/0x4e
 [<ffffffff8113dd5b>] prepend_path+0x12b/0x17c
 [<ffffffff8113de85>] d_path+0xd9/0x139
 [<ffffffff8117c35a>] proc_pid_readlink+0x6e/0xd9
 [<ffffffff81131202>] SyS_readlinkat+0x93/0xdc
 [<ffffffff81131266>] SyS_readlink+0x1b/0x1d
 [<ffffffff81473262>] system_call_fastpath+0x16/0x1b
---[ end trace a4b19d6c8a8b2eec ]---

Cheers,
Vinson

Eric W. Biederman (1):
  vfs: Remove incorrect debugging WARN in prepend_path

 fs/dcache.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

[PATCH] vfs: Remove incorrect debugging WARN in prepend_path

[Vinson Lee]

From: Eric W. Biederman <ebiederm@xmission.com>

commit 93e3bce6287e1fb3e60d3324ed08555b5bbafa89 upstream.

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.

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>
[ vlee: Backported to 3.10. Adjusted context. ]
Signed-off-by: Vinson Lee <vlee@twitter.com>
---
 fs/dcache.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 90be280..f1e8017 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2569,15 +2569,6 @@ static int prepend_path(const struct path *path,
 	return error;
 
 global_root:
-	/*
-	 * 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 (!slash)
 		error = prepend(buffer, buflen, "/", 1);
 	if (!error)
-- 
1.7.1

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.

[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