[PATCH 0/7] dcache fixes and cleanups

[PATCH 0/7] dcache fixes and cleanups

[Miklos Szeredi]

Al,

Could you please add these patches to your vfs queue?

Thanks,
Miklos

[PATCH 1/7] cachefiles: use path_get instead of lone dget

[Miklos Szeredi]

[-- Attachment #1: cachefiles-fix-lone-dentry-ref.patch --]
[-- Type: text/plain, Size: 2551 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Dentry references should not be acquired without a corresponding
vfsmount ref.

CC: David Howells <dhowells@redhat.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/cachefiles/daemon.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/cachefiles/daemon.c
===================================================================
--- linux-2.6.orig/fs/cachefiles/daemon.c	2010-07-05 15:44:37.000000000 +0200
+++ linux-2.6/fs/cachefiles/daemon.c	2010-07-05 15:47:40.000000000 +0200
@@ -553,7 +553,7 @@ static int cachefiles_daemon_tag(struct
 static int cachefiles_daemon_cull(struct cachefiles_cache *cache, char *args)
 {
 	struct fs_struct *fs;
-	struct dentry *dir;
+	struct path path;
 	const struct cred *saved_cred;
 	int ret;
 
@@ -575,22 +575,23 @@ static int cachefiles_daemon_cull(struct
 	/* extract the directory dentry from the cwd */
 	fs = current->fs;
 	read_lock(&fs->lock);
-	dir = dget(fs->pwd.dentry);
+	path = fs->pwd;
+	path_get(&path);
 	read_unlock(&fs->lock);
 
-	if (!S_ISDIR(dir->d_inode->i_mode))
+	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = cachefiles_cull(cache, dir, args);
+	ret = cachefiles_cull(cache, path.dentry, args);
 	cachefiles_end_secure(cache, saved_cred);
 
-	dput(dir);
+	path_put(&path);
 	_leave(" = %d", ret);
 	return ret;
 
 notdir:
-	dput(dir);
+	path_put(&path);
 	kerror("cull command requires dirfd to be a directory");
 	return -ENOTDIR;
 
@@ -629,7 +630,7 @@ inval:
 static int cachefiles_daemon_inuse(struct cachefiles_cache *cache, char *args)
 {
 	struct fs_struct *fs;
-	struct dentry *dir;
+	struct path path;
 	const struct cred *saved_cred;
 	int ret;
 
@@ -651,22 +652,23 @@ static int cachefiles_daemon_inuse(struc
 	/* extract the directory dentry from the cwd */
 	fs = current->fs;
 	read_lock(&fs->lock);
-	dir = dget(fs->pwd.dentry);
+	path = fs->pwd;
+	path_get(&path);
 	read_unlock(&fs->lock);
 
-	if (!S_ISDIR(dir->d_inode->i_mode))
+	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = cachefiles_check_in_use(cache, dir, args);
+	ret = cachefiles_check_in_use(cache, path.dentry, args);
 	cachefiles_end_secure(cache, saved_cred);
 
-	dput(dir);
+	path_put(&path);
 	//_leave(" = %d", ret);
 	return ret;
 
 notdir:
-	dput(dir);
+	path_put(&path);
 	kerror("inuse command requires dirfd to be a directory");
 	return -ENOTDIR;
 

-- 

Re: [PATCH 1/7] cachefiles: use path_get instead of lone dget

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Dentry references should not be acquired without a corresponding
> vfsmount ref.
> 
> CC: David Howells <dhowells@redhat.com>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>

Acked-by: David Howells <dhowells@redhat.com>

[PATCH 2/7] vfs: add helpers to get root and pwd

[Miklos Szeredi]

[-- Attachment #1: vfs-add-get_fs-helpers.patch --]
[-- Type: text/plain, Size: 8473 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Add three helpers that retrieve a refcounted copy of the root and cwd
from the supplied fs_struct.

 get_fs_root()
 get_fs_pwd()
 get_fs_root_and_pwd()

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/cachefiles/daemon.c    |   14 ++------------
 fs/dcache.c               |   12 ++----------
 fs/fs_struct.c            |    7 +------
 fs/namei.c                |   15 +++------------
 fs/namespace.c            |    5 +----
 fs/proc/base.c            |   22 +++++++++++-----------
 include/linux/fs_struct.h |   27 +++++++++++++++++++++++++++
 kernel/auditsc.c          |    9 ++-------
 8 files changed, 49 insertions(+), 62 deletions(-)

Index: linux-2.6/fs/fs_struct.c
===================================================================
--- linux-2.6.orig/fs/fs_struct.c	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/fs/fs_struct.c	2010-07-06 18:08:06.000000000 +0200
@@ -106,12 +106,7 @@ struct fs_struct *copy_fs_struct(struct
 		fs->in_exec = 0;
 		rwlock_init(&fs->lock);
 		fs->umask = old->umask;
-		read_lock(&old->lock);
-		fs->root = old->root;
-		path_get(&old->root);
-		fs->pwd = old->pwd;
-		path_get(&old->pwd);
-		read_unlock(&old->lock);
+		get_fs_root_and_pwd(old, &fs->root, &fs->pwd);
 	}
 	return fs;
 }
Index: linux-2.6/include/linux/fs_struct.h
===================================================================
--- linux-2.6.orig/include/linux/fs_struct.h	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/include/linux/fs_struct.h	2010-07-06 18:08:06.000000000 +0200
@@ -21,4 +21,31 @@ extern void free_fs_struct(struct fs_str
 extern void daemonize_fs_struct(void);
 extern int unshare_fs_struct(void);
 
+static inline void get_fs_root(struct fs_struct *fs, struct path *root)
+{
+	read_lock(&fs->lock);
+	*root = fs->root;
+	path_get(root);
+	read_unlock(&fs->lock);
+}
+
+static inline void get_fs_pwd(struct fs_struct *fs, struct path *pwd)
+{
+	read_lock(&fs->lock);
+	*pwd = fs->pwd;
+	path_get(pwd);
+	read_unlock(&fs->lock);
+}
+
+static inline void get_fs_root_and_pwd(struct fs_struct *fs, struct path *root,
+				       struct path *pwd)
+{
+	read_lock(&fs->lock);
+	*root = fs->root;
+	path_get(root);
+	*pwd = fs->pwd;
+	path_get(pwd);
+	read_unlock(&fs->lock);
+}
+
 #endif /* _LINUX_FS_STRUCT_H */
Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/fs/dcache.c	2010-07-06 18:08:06.000000000 +0200
@@ -2012,10 +2012,7 @@ char *d_path(const struct path *path, ch
 	if (path->dentry->d_op && path->dentry->d_op->d_dname)
 		return path->dentry->d_op->d_dname(path->dentry, buf, buflen);
 
-	read_lock(&current->fs->lock);
-	root = current->fs->root;
-	path_get(&root);
-	read_unlock(&current->fs->lock);
+	get_fs_root(current->fs, &root);
 	spin_lock(&dcache_lock);
 	tmp = root;
 	res = __d_path(path, &tmp, buf, buflen);
@@ -2110,12 +2107,7 @@ SYSCALL_DEFINE2(getcwd, char __user *, b
 	if (!page)
 		return -ENOMEM;
 
-	read_lock(&current->fs->lock);
-	pwd = current->fs->pwd;
-	path_get(&pwd);
-	root = current->fs->root;
-	path_get(&root);
-	read_unlock(&current->fs->lock);
+	get_fs_root_and_pwd(current->fs, &root, &pwd);
 
 	error = -ENOENT;
 	spin_lock(&dcache_lock);
Index: linux-2.6/fs/proc/base.c
===================================================================
--- linux-2.6.orig/fs/proc/base.c	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/fs/proc/base.c	2010-07-06 18:08:06.000000000 +0200
@@ -148,18 +148,13 @@ static unsigned int pid_entry_count_dirs
 	return count;
 }
 
-static int get_fs_path(struct task_struct *task, struct path *path, bool root)
+static int get_task_root(struct task_struct *task, struct path *root)
 {
-	struct fs_struct *fs;
 	int result = -ENOENT;
 
 	task_lock(task);
-	fs = task->fs;
-	if (fs) {
-		read_lock(&fs->lock);
-		*path = root ? fs->root : fs->pwd;
-		path_get(path);
-		read_unlock(&fs->lock);
+	if (task->fs) {
+		get_fs_root(task->fs, root);
 		result = 0;
 	}
 	task_unlock(task);
@@ -172,7 +167,12 @@ static int proc_cwd_link(struct inode *i
 	int result = -ENOENT;
 
 	if (task) {
-		result = get_fs_path(task, path, 0);
+		task_lock(task);
+		if (task->fs) {
+			get_fs_pwd(task->fs, path);
+			result = 0;
+		}
+		task_unlock(task);
 		put_task_struct(task);
 	}
 	return result;
@@ -184,7 +184,7 @@ static int proc_root_link(struct inode *
 	int result = -ENOENT;
 
 	if (task) {
-		result = get_fs_path(task, path, 1);
+		result = get_task_root(task, path);
 		put_task_struct(task);
 	}
 	return result;
@@ -589,7 +589,7 @@ static int mounts_open_common(struct ino
 				get_mnt_ns(ns);
 		}
 		rcu_read_unlock();
-		if (ns && get_fs_path(task, &root, 1) == 0)
+		if (ns && get_task_root(task, &root) == 0)
 			ret = 0;
 		put_task_struct(task);
 	}
Index: linux-2.6/fs/cachefiles/daemon.c
===================================================================
--- linux-2.6.orig/fs/cachefiles/daemon.c	2010-07-06 18:08:00.000000000 +0200
+++ linux-2.6/fs/cachefiles/daemon.c	2010-07-06 18:08:06.000000000 +0200
@@ -552,7 +552,6 @@ static int cachefiles_daemon_tag(struct
  */
 static int cachefiles_daemon_cull(struct cachefiles_cache *cache, char *args)
 {
-	struct fs_struct *fs;
 	struct path path;
 	const struct cred *saved_cred;
 	int ret;
@@ -573,11 +572,7 @@ static int cachefiles_daemon_cull(struct
 	}
 
 	/* extract the directory dentry from the cwd */
-	fs = current->fs;
-	read_lock(&fs->lock);
-	path = fs->pwd;
-	path_get(&path);
-	read_unlock(&fs->lock);
+	get_fs_pwd(current->fs, &path);
 
 	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
@@ -629,7 +624,6 @@ inval:
  */
 static int cachefiles_daemon_inuse(struct cachefiles_cache *cache, char *args)
 {
-	struct fs_struct *fs;
 	struct path path;
 	const struct cred *saved_cred;
 	int ret;
@@ -650,11 +644,7 @@ static int cachefiles_daemon_inuse(struc
 	}
 
 	/* extract the directory dentry from the cwd */
-	fs = current->fs;
-	read_lock(&fs->lock);
-	path = fs->pwd;
-	path_get(&path);
-	read_unlock(&fs->lock);
+	get_fs_pwd(current->fs, &path);
 
 	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/fs/namei.c	2010-07-06 18:08:06.000000000 +0200
@@ -484,13 +484,8 @@ ok:
 
 static __always_inline void set_root(struct nameidata *nd)
 {
-	if (!nd->root.mnt) {
-		struct fs_struct *fs = current->fs;
-		read_lock(&fs->lock);
-		nd->root = fs->root;
-		path_get(&nd->root);
-		read_unlock(&fs->lock);
-	}
+	if (!nd->root.mnt)
+		get_fs_root(current->fs, &nd->root);
 }
 
 static int link_path_walk(const char *, struct nameidata *);
@@ -1016,11 +1011,7 @@ static int path_init(int dfd, const char
 		nd->path = nd->root;
 		path_get(&nd->root);
 	} else if (dfd == AT_FDCWD) {
-		struct fs_struct *fs = current->fs;
-		read_lock(&fs->lock);
-		nd->path = fs->pwd;
-		path_get(&fs->pwd);
-		read_unlock(&fs->lock);
+		get_fs_pwd(current->fs, &nd->path);
 	} else {
 		struct dentry *dentry;
 
Index: linux-2.6/kernel/auditsc.c
===================================================================
--- linux-2.6.orig/kernel/auditsc.c	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/kernel/auditsc.c	2010-07-06 18:08:06.000000000 +0200
@@ -1837,13 +1837,8 @@ void __audit_getname(const char *name)
 	context->names[context->name_count].ino  = (unsigned long)-1;
 	context->names[context->name_count].osid = 0;
 	++context->name_count;
-	if (!context->pwd.dentry) {
-		read_lock(&current->fs->lock);
-		context->pwd = current->fs->pwd;
-		path_get(&current->fs->pwd);
-		read_unlock(&current->fs->lock);
-	}
-
+	if (!context->pwd.dentry)
+		get_fs_pwd(current->fs, &context->pwd);
 }
 
 /* audit_putname - intercept a putname request
Index: linux-2.6/fs/namespace.c
===================================================================
--- linux-2.6.orig/fs/namespace.c	2010-07-06 17:59:05.000000000 +0200
+++ linux-2.6/fs/namespace.c	2010-07-06 18:08:06.000000000 +0200
@@ -2208,10 +2208,7 @@ SYSCALL_DEFINE2(pivot_root, const char _
 		goto out1;
 	}
 
-	read_lock(&current->fs->lock);
-	root = current->fs->root;
-	path_get(&current->fs->root);
-	read_unlock(&current->fs->lock);
+	get_fs_root(current->fs, &root);
 	down_write(&namespace_sem);
 	mutex_lock(&old.dentry->d_inode->i_mutex);
 	error = -EINVAL;

-- 

[PATCH 3/7] ia64: perfmon: add d_dname method

[Miklos Szeredi]

[-- Attachment #1: vfs-add-d_dname-method.patch --]
[-- Type: text/plain, Size: 1588 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Switch ia64/perfmon to using the d_dname() instead of relying on
__d_path() to prepend the name of the root dentry to the path.

CC: Tony Luck <tony.luck@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 arch/ia64/kernel/perfmon.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6/arch/ia64/kernel/perfmon.c
===================================================================
--- linux-2.6.orig/arch/ia64/kernel/perfmon.c	2010-07-06 17:59:04.000000000 +0200
+++ linux-2.6/arch/ia64/kernel/perfmon.c	2010-07-06 18:08:09.000000000 +0200
@@ -2191,8 +2191,15 @@ pfmfs_delete_dentry(struct dentry *dentr
 	return 1;
 }
 
+static char *pfmfs_dname(struct dentry *dentry, char *buffer, int buflen)
+{
+	return dynamic_dname(dentry, buffer, buflen, "pfm:[%lu]",
+			     dentry->d_inode->i_ino);
+}
+
 static const struct dentry_operations pfmfs_dentry_operations = {
 	.d_delete = pfmfs_delete_dentry,
+	.d_dname = pfmfs_dname,
 };
 
 
@@ -2202,8 +2209,7 @@ pfm_alloc_file(pfm_context_t *ctx)
 	struct file *file;
 	struct inode *inode;
 	struct path path;
-	char name[32];
-	struct qstr this;
+	struct qstr this = { .name = "" };
 
 	/*
 	 * allocate a new inode
@@ -2218,11 +2224,6 @@ pfm_alloc_file(pfm_context_t *ctx)
 	inode->i_uid  = current_fsuid();
 	inode->i_gid  = current_fsgid();
 
-	sprintf(name, "[%lu]", inode->i_ino);
-	this.name = name;
-	this.len  = strlen(name);
-	this.hash = inode->i_ino;
-
 	/*
 	 * allocate a new dcache entry
 	 */

-- 

[PATCH 4/7] vfs: __d_path: dont prepend the name of the root dentry

[Miklos Szeredi]

[-- Attachment #1: vfs-dont-prepend-name-of-root-dentry.patch --]
[-- Type: text/plain, Size: 1866 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

In the old times pseudo-filesystems set the name of theroot dentry to
some prefix like "pipe:" and the name of the child dentry to "[123]"
and relied on a hack in __d_path() to replace the preceding slash with
the root's name to get "pipe:[123]".

Then the d_dname() dentry operation was introduced which solved the
same problem without having to pre-fill the name in each dentry.

Currently the following pseudo filesystems exist in the kernel:

perfmon
mtd
anon_inode
bdev
pipe
socket

Of these only perfmon, anon_inode, pipe and socket create
sub-dentries, all of which have now been switched to using d_dname().

bdev and mtd only create inodes.

This means that now the hack to overwrite the slash can be removed, so
for unreachable paths (e.g. within a detached mount) the path string
won't be polluted with garbage.  For these cases a subsequent patch
will add a prefix, indicating that the path is unreachable.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/dcache.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-07-06 18:08:06.000000000 +0200
+++ linux-2.6/fs/dcache.c	2010-07-06 18:08:12.000000000 +0200
@@ -1968,9 +1968,15 @@ out:
 	return retval;
 
 global_root:
-	retval += 1;	/* hit the slash */
-	if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
-		goto Elong;
+	/*
+	 * 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);
+	}
 	root->mnt = vfsmnt;
 	root->dentry = dentry;
 	goto out;

-- 

[PATCH 5/7] vfs: add prepend_path() helper

[Miklos Szeredi]

[-- Attachment #1: vfs-add-prepend_path-helper.patch --]
[-- Type: text/plain, Size: 4391 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Split off prepend_path() from __d_path().  This new helper takes an
end-of-buffer pointer and buffer-length pointer just like the other
prepend_* functions.  Move the " (deleted)" postfix out to __d_path().

This patch doesn't change any functionality but paves the way for the
following patches.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/dcache.c |   94 +++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 58 insertions(+), 36 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-07-06 18:08:12.000000000 +0200
+++ linux-2.6/fs/dcache.c	2010-07-06 18:08:16.000000000 +0200
@@ -1903,48 +1903,30 @@ static int prepend_name(char **buffer, i
 }
 
 /**
- * __d_path - return the path of a dentry
+ * Prepend path string to a buffer
+ *
  * @path: the dentry/vfsmount to report
  * @root: root vfsmnt/dentry (may be modified by this function)
- * @buffer: buffer to return value in
- * @buflen: buffer length
+ * @buffer: pointer to the end of the buffer
+ * @buflen: pointer to buffer length
  *
- * Convert a dentry into an ASCII path name. If the entry has been deleted
- * the string " (deleted)" is appended. Note that this is ambiguous.
- *
- * Returns a pointer into the buffer or an error code if the
- * path was too long.
- *
- * "buflen" should be positive. Caller holds the dcache_lock.
+ * Caller holds the dcache_lock.
  *
  * If path is not reachable from the supplied root, then the value of
  * root is changed (without modifying refcounts).
  */
-char *__d_path(const struct path *path, struct path *root,
-	       char *buffer, int buflen)
+static int prepend_path(const struct path *path, struct path *root,
+			char **buffer, int *buflen)
 {
 	struct dentry *dentry = path->dentry;
 	struct vfsmount *vfsmnt = path->mnt;
-	char *end = buffer + buflen;
-	char *retval;
+	bool slash = false;
+	int error = 0;
 
 	spin_lock(&vfsmount_lock);
-	prepend(&end, &buflen, "\0", 1);
-	if (d_unlinked(dentry) &&
-		(prepend(&end, &buflen, " (deleted)", 10) != 0))
-			goto Elong;
-
-	if (buflen < 1)
-		goto Elong;
-	/* Get '/' right */
-	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) {
@@ -1956,16 +1938,22 @@ char *__d_path(const struct path *path,
 		}
 		parent = dentry->d_parent;
 		prefetch(parent);
-		if ((prepend_name(&end, &buflen, &dentry->d_name) != 0) ||
-		    (prepend(&end, &buflen, "/", 1) != 0))
-			goto Elong;
-		retval = end;
+		error = prepend_name(buffer, buflen, &dentry->d_name);
+		if (!error)
+			error = prepend(buffer, buflen, "/", 1);
+		if (error)
+			break;
+
+		slash = true;
 		dentry = parent;
 	}
 
 out:
+	if (!error && !slash)
+		error = prepend(buffer, buflen, "/", 1);
+
 	spin_unlock(&vfsmount_lock);
-	return retval;
+	return error;
 
 global_root:
 	/*
@@ -1980,10 +1968,44 @@ global_root:
 	root->mnt = vfsmnt;
 	root->dentry = dentry;
 	goto out;
+}
 
-Elong:
-	retval = ERR_PTR(-ENAMETOOLONG);
-	goto out;
+/**
+ * __d_path - return the path of a dentry
+ * @path: the dentry/vfsmount to report
+ * @root: root vfsmnt/dentry (may be modified by this function)
+ * @buffer: buffer to return value in
+ * @buflen: buffer length
+ *
+ * Convert a dentry into an ASCII path name. If the entry has been deleted
+ * the string " (deleted)" is appended. Note that this is ambiguous.
+ *
+ * Returns a pointer into the buffer or an error code if the
+ * path was too long.
+ *
+ * "buflen" should be positive. Caller holds the dcache_lock.
+ *
+ * If path is not reachable from the supplied root, then the value of
+ * root is changed (without modifying refcounts).
+ */
+char *__d_path(const struct path *path, struct path *root,
+	       char *buf, int buflen)
+{
+	char *res = buf + buflen;
+	int error;
+
+	prepend(&res, &buflen, "\0", 1);
+	if (d_unlinked(path->dentry)) {
+		error = prepend(&res, &buflen, " (deleted)", 10);
+		if (error)
+			return ERR_PTR(error);
+	}
+
+	error = prepend_path(path, root, &res, &buflen);
+	if (error)
+		return ERR_PTR(error);
+
+	return res;
 }
 
 /**

-- 

[PATCH 6/7] vfs: only add " (deleted)" where necessary

[Miklos Szeredi]

[-- Attachment #1: vfs-d_path-move-deleted-into-callers.patch --]
[-- Type: text/plain, Size: 2827 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

__d_path() has 4 callers:

  d_path()
  sys_getcwd()
  seq_path_root()
  tomoyo_realpath_from_path2()

Of these the only one which needs the " (deleted)" ending is d_path().

sys_getcwd() checks for existence before calling __d_path().

seq_path_root() is used to show the mountpoint path in
/proc/PID/mountinfo, which is always a positive.

And tomoyo doesn't want the deleted ending.

Create a helper "path_with_deleted()" as subsequent patches will need
this in multiple places.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/dcache.c |   32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-07-06 18:08:16.000000000 +0200
+++ linux-2.6/fs/dcache.c	2010-07-06 18:08:19.000000000 +0200
@@ -1977,8 +1977,7 @@ global_root:
  * @buffer: buffer to return value in
  * @buflen: buffer length
  *
- * Convert a dentry into an ASCII path name. If the entry has been deleted
- * the string " (deleted)" is appended. Note that this is ambiguous.
+ * Convert a dentry into an ASCII path name.
  *
  * Returns a pointer into the buffer or an error code if the
  * path was too long.
@@ -1995,12 +1994,6 @@ char *__d_path(const struct path *path,
 	int error;
 
 	prepend(&res, &buflen, "\0", 1);
-	if (d_unlinked(path->dentry)) {
-		error = prepend(&res, &buflen, " (deleted)", 10);
-		if (error)
-			return ERR_PTR(error);
-	}
-
 	error = prepend_path(path, root, &res, &buflen);
 	if (error)
 		return ERR_PTR(error);
@@ -2008,6 +2001,22 @@ char *__d_path(const struct path *path,
 	return res;
 }
 
+/*
+ * same as __d_path but appends "(deleted)" for unlinked files.
+ */
+static int path_with_deleted(const struct path *path, struct path *root,
+				 char **buf, int *buflen)
+{
+	prepend(buf, buflen, "\0", 1);
+	if (d_unlinked(path->dentry)) {
+		int error = prepend(buf, buflen, " (deleted)", 10);
+		if (error)
+			return error;
+	}
+
+	return prepend_path(path, root, buf, buflen);
+}
+
 /**
  * d_path - return the path of a dentry
  * @path: path to report
@@ -2026,9 +2035,10 @@ char *__d_path(const struct path *path,
  */
 char *d_path(const struct path *path, char *buf, int buflen)
 {
-	char *res;
+	char *res = buf + buflen;
 	struct path root;
 	struct path tmp;
+	int error;
 
 	/*
 	 * We have various synthetic filesystems that never get mounted.  On
@@ -2043,7 +2053,9 @@ char *d_path(const struct path *path, ch
 	get_fs_root(current->fs, &root);
 	spin_lock(&dcache_lock);
 	tmp = root;
-	res = __d_path(path, &tmp, buf, buflen);
+	error = path_with_deleted(path, &tmp, &res, &buflen);
+	if (error)
+		res = ERR_PTR(error);
 	spin_unlock(&dcache_lock);
 	path_put(&root);
 	return res;

-- 

Re: [PATCH 6/7] vfs: only add " (deleted)" where necessary

[Valdis.Kletnieks]

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

On Mon, 02 Aug 2010 13:20:01 +0200, Miklos Szeredi said:

> Index: linux-2.6/fs/dcache.c
> ===================================================================
> --- linux-2.6.orig/fs/dcache.c	2010-07-06 18:08:16.000000000 +0200
> +++ linux-2.6/fs/dcache.c	2010-07-06 18:08:19.000000000 +0200
> @@ -1977,8 +1977,7 @@ global_root:
>   * @buffer: buffer to return value in
>   * @buflen: buffer length
>   *
> - * Convert a dentry into an ASCII path name. If the entry has been deleted
> - * the string " (deleted)" is appended. Note that this is ambiguous.
> + * Convert a dentry into an ASCII path name.
>   *
>   * Returns a pointer into the buffer or an error code if the
>   * path was too long.

I'd prefer the comment about it being ambiguous remain.  I'm waiting to see how
long it takes for somebody to create a security hole by creating a file called
'/etc/some/thing/important (deleted)' and having some software Do The Wrong
Thing instead to /etc/some/thing/important.


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 6/7] vfs: only add " (deleted)" where necessary

[Bastien ROUCARIES]

On Mon, Aug 2, 2010 at 3:00 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Mon, 02 Aug 2010 13:20:01 +0200, Miklos Szeredi said:
>
>> Index: linux-2.6/fs/dcache.c
>> ===================================================================
>> --- linux-2.6.orig/fs/dcache.c        2010-07-06 18:08:16.000000000 +0200
>> +++ linux-2.6/fs/dcache.c     2010-07-06 18:08:19.000000000 +0200
>> @@ -1977,8 +1977,7 @@ global_root:
>>   * @buffer: buffer to return value in
>>   * @buflen: buffer length
>>   *
>> - * Convert a dentry into an ASCII path name. If the entry has been deleted
>> - * the string " (deleted)" is appended. Note that this is ambiguous.
>> + * Convert a dentry into an ASCII path name.
>>   *
>>   * Returns a pointer into the buffer or an error code if the
>>   * path was too long.
>
> I'd prefer the comment about it being ambiguous remain.  I'm waiting to see how
> long it takes for somebody to create a security hole by creating a file called
> '/etc/some/thing/important (deleted)' and having some software Do The Wrong
> Thing instead to /etc/some/thing/important.
>

In order to close this kind of hole why not creating a deleted
directory on /proc and redirect symbolic link to this directory.
And do the same for unreachable. If we use the good permission it will
work from a backaward compatibily point of view

bastien

Re: [PATCH 6/7] vfs: only add " (deleted)" where necessary

[Miklos Szeredi]

On Mon, 2 Aug 2010, Bastien ROUCARIES wrote:
> On Mon, Aug 2, 2010 at 3:00 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> > On Mon, 02 Aug 2010 13:20:01 +0200, Miklos Szeredi said:
> >
> >> Index: linux-2.6/fs/dcache.c
> >> ===================================================================
> >> --- linux-2.6.orig/fs/dcache.c        2010-07-06 18:08:16.000000000 +0200
> >> +++ linux-2.6/fs/dcache.c     2010-07-06 18:08:19.000000000 +0200
> >> @@ -1977,8 +1977,7 @@ global_root:
> >>   * @buffer: buffer to return value in
> >>   * @buflen: buffer length
> >>   *
> >> - * Convert a dentry into an ASCII path name. If the entry has been deleted
> >> - * the string " (deleted)" is appended. Note that this is ambiguous.
> >> + * Convert a dentry into an ASCII path name.
> >>   *
> >>   * Returns a pointer into the buffer or an error code if the
> >>   * path was too long.
> >
> > I'd prefer the comment about it being ambiguous remain.  I'm waiting to see how
> > long it takes for somebody to create a security hole by creating a file called
> > '/etc/some/thing/important (deleted)' and having some software Do The Wrong
> > Thing instead to /etc/some/thing/important.
> >
> 
> In order to close this kind of hole why not creating a deleted
> directory on /proc and redirect symbolic link to this directory.
> And do the same for unreachable. If we use the good permission it will
> work from a backaward compatibily point of view

Unfortunately the "(deleted)" thing is part of the userspace ABI and
can't be changed.

The only thing we can do is make sure new interfaces use a saner
convention.

Thanks,
Miklos

Re: [PATCH 6/7] vfs: only add " (deleted)" where necessary

[Miklos Szeredi]

On Mon, 02 Aug 2010, Valdis.Kletnieks@vt.ed wrote:
> On Mon, 02 Aug 2010 13:20:01 +0200, Miklos Szeredi said:
> 
> > Index: linux-2.6/fs/dcache.c
> > ===================================================================
> > --- linux-2.6.orig/fs/dcache.c	2010-07-06 18:08:16.000000000 +0200
> > +++ linux-2.6/fs/dcache.c	2010-07-06 18:08:19.000000000 +0200
> > @@ -1977,8 +1977,7 @@ global_root:
> >   * @buffer: buffer to return value in
> >   * @buflen: buffer length
> >   *
> > - * Convert a dentry into an ASCII path name. If the entry has been deleted
> > - * the string " (deleted)" is appended. Note that this is ambiguous.
> > + * Convert a dentry into an ASCII path name.
> >   *
> >   * Returns a pointer into the buffer or an error code if the
> >   * path was too long.
> 
> I'd prefer the comment about it being ambiguous remain.  I'm waiting
> to see how long it takes for somebody to create a security hole by
> creating a file called '/etc/some/thing/important (deleted)' and
> having some software Do The Wrong Thing instead to
> /etc/some/thing/important.

The same comment is left intact on top of d_path().

It is removed from __d_path() because this function no longer appends
"(deleted)".

Thanks,
Miklos

Re: [PATCH 6/7] vfs: only add " (deleted)" where necessary

[Valdis.Kletnieks]

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

On Mon, 02 Aug 2010 15:35:20 +0200, Miklos Szeredi said:

> > I'd prefer the comment about it being ambiguous remain.  I'm waiting
> > to see how long it takes for somebody to create a security hole by
> > creating a file called '/etc/some/thing/important (deleted)' and
> > having some software Do The Wrong Thing instead to
> > /etc/some/thing/important.
> 
> The same comment is left intact on top of d_path().
> 
> It is removed from __d_path() because this function no longer appends
> "(deleted)".

Oh, OK.  Obviously -ENOCAFFEINE on my part, I thought you were nuking the
d_path() copy. ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

[PATCH 7/7] vfs: show unreachable paths in getcwd and proc

[Miklos Szeredi]

[-- Attachment #1: vfs-unreachable-paths.patch --]
[-- Type: text/plain, Size: 4615 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Prepend "(unreachable)" to path strings if the path is not reachable
from the current root.

Two places updated are
 - the return string from getcwd()
 - and symlinks under /proc/$PID.

Other uses of d_path() are left unchanged (we know that some old
software crashes if /proc/mounts is changed).

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/dcache.c            |   54 +++++++++++++++++++++++++++++++++++++++++++++----
 fs/proc/base.c         |    2 -
 include/linux/dcache.h |    1 
 include/linux/path.h   |    5 ++++
 4 files changed, 57 insertions(+), 5 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-07-06 18:08:19.000000000 +0200
+++ linux-2.6/fs/dcache.c	2010-07-06 18:08:22.000000000 +0200
@@ -2017,6 +2017,11 @@ static int path_with_deleted(const struc
 	return prepend_path(path, root, buf, buflen);
 }
 
+static int prepend_unreachable(char **buffer, int *buflen)
+{
+	return prepend(buffer, buflen, "(unreachable)", 13);
+}
+
 /**
  * d_path - return the path of a dentry
  * @path: path to report
@@ -2062,6 +2067,39 @@ char *d_path(const struct path *path, ch
 }
 EXPORT_SYMBOL(d_path);
 
+/**
+ * d_path_with_unreachable - return the path of a dentry
+ * @path: path to report
+ * @buf: buffer to return value in
+ * @buflen: buffer length
+ *
+ * The difference from d_path() is that this prepends "(unreachable)"
+ * to paths which are unreachable from the current process' root.
+ */
+char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
+{
+	char *res = buf + buflen;
+	struct path root;
+	struct path tmp;
+	int error;
+
+	if (path->dentry->d_op && path->dentry->d_op->d_dname)
+		return path->dentry->d_op->d_dname(path->dentry, buf, buflen);
+
+	get_fs_root(current->fs, &root);
+	spin_lock(&dcache_lock);
+	tmp = root;
+	error = path_with_deleted(path, &tmp, &res, &buflen);
+	if (!error && !path_equal(&tmp, &root))
+		error = prepend_unreachable(&res, &buflen);
+	spin_unlock(&dcache_lock);
+	path_put(&root);
+	if (error)
+		res =  ERR_PTR(error);
+
+	return res;
+}
+
 /*
  * Helper function for dentry_operations.d_dname() members
  */
@@ -2154,15 +2192,23 @@ SYSCALL_DEFINE2(getcwd, char __user *, b
 	if (!d_unlinked(pwd.dentry)) {
 		unsigned long len;
 		struct path tmp = root;
-		char * cwd;
+		char *cwd = page + PAGE_SIZE;
+		int buflen = PAGE_SIZE;
 
-		cwd = __d_path(&pwd, &tmp, page, PAGE_SIZE);
+		prepend(&cwd, &buflen, "\0", 1);
+		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
 		spin_unlock(&dcache_lock);
 
-		error = PTR_ERR(cwd);
-		if (IS_ERR(cwd))
+		if (error)
 			goto out;
 
+		/* Unreachable from current root */
+		if (!path_equal(&tmp, &root)) {
+			error = prepend_unreachable(&cwd, &buflen);
+			if (error)
+				goto out;
+		}
+
 		error = -ERANGE;
 		len = PAGE_SIZE + page - cwd;
 		if (len <= size) {
Index: linux-2.6/include/linux/dcache.h
===================================================================
--- linux-2.6.orig/include/linux/dcache.h	2010-07-06 17:59:04.000000000 +0200
+++ linux-2.6/include/linux/dcache.h	2010-07-06 18:08:22.000000000 +0200
@@ -315,6 +315,7 @@ extern char *dynamic_dname(struct dentry
 
 extern char *__d_path(const struct path *path, struct path *root, char *, int);
 extern char *d_path(const struct path *, char *, int);
+extern char *d_path_with_unreachable(const struct path *, char *, int);
 extern char *dentry_path(struct dentry *, char *, int);
 
 /* Allocation counts.. */
Index: linux-2.6/fs/proc/base.c
===================================================================
--- linux-2.6.orig/fs/proc/base.c	2010-07-06 18:08:06.000000000 +0200
+++ linux-2.6/fs/proc/base.c	2010-07-06 18:08:22.000000000 +0200
@@ -1426,7 +1426,7 @@ static int do_proc_readlink(struct path
 	if (!tmp)
 		return -ENOMEM;
 
-	pathname = d_path(path, tmp, PAGE_SIZE);
+	pathname = d_path_with_unreachable(path, tmp, PAGE_SIZE);
 	len = PTR_ERR(pathname);
 	if (IS_ERR(pathname))
 		goto out;
Index: linux-2.6/include/linux/path.h
===================================================================
--- linux-2.6.orig/include/linux/path.h	2010-07-06 17:59:04.000000000 +0200
+++ linux-2.6/include/linux/path.h	2010-07-06 18:08:22.000000000 +0200
@@ -12,4 +12,9 @@ struct path {
 extern void path_get(struct path *);
 extern void path_put(struct path *);
 
+static inline int path_equal(const struct path *path1, const struct path *path2)
+{
+	return path1->mnt == path2->mnt && path1->dentry == path2->dentry;
+}
+
 #endif  /* _LINUX_PATH_H */

-- 

[PATCH 0/7] dcache fixes and cleanups

[Miklos Szeredi]

Al,

Could you please add these patches to your vfs queue?

Thanks,
Miklos

[PATCH 1/7] cachefiles: use path_get instead of lone dget

[Miklos Szeredi]

[-- Attachment #1: cachefiles-fix-lone-dentry-ref.patch --]
[-- Type: text/plain, Size: 2557 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Dentry references should not be acquired without a corresponding
vfsmount ref.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Acked-by: David Howells <dhowells@redhat.com>
---
 fs/cachefiles/daemon.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/cachefiles/daemon.c
===================================================================
--- linux-2.6.orig/fs/cachefiles/daemon.c	2010-07-05 15:44:37.000000000 +0200
+++ linux-2.6/fs/cachefiles/daemon.c	2010-07-05 15:47:40.000000000 +0200
@@ -553,7 +553,7 @@ static int cachefiles_daemon_tag(struct
 static int cachefiles_daemon_cull(struct cachefiles_cache *cache, char *args)
 {
 	struct fs_struct *fs;
-	struct dentry *dir;
+	struct path path;
 	const struct cred *saved_cred;
 	int ret;
 
@@ -575,22 +575,23 @@ static int cachefiles_daemon_cull(struct
 	/* extract the directory dentry from the cwd */
 	fs = current->fs;
 	read_lock(&fs->lock);
-	dir = dget(fs->pwd.dentry);
+	path = fs->pwd;
+	path_get(&path);
 	read_unlock(&fs->lock);
 
-	if (!S_ISDIR(dir->d_inode->i_mode))
+	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = cachefiles_cull(cache, dir, args);
+	ret = cachefiles_cull(cache, path.dentry, args);
 	cachefiles_end_secure(cache, saved_cred);
 
-	dput(dir);
+	path_put(&path);
 	_leave(" = %d", ret);
 	return ret;
 
 notdir:
-	dput(dir);
+	path_put(&path);
 	kerror("cull command requires dirfd to be a directory");
 	return -ENOTDIR;
 
@@ -629,7 +630,7 @@ inval:
 static int cachefiles_daemon_inuse(struct cachefiles_cache *cache, char *args)
 {
 	struct fs_struct *fs;
-	struct dentry *dir;
+	struct path path;
 	const struct cred *saved_cred;
 	int ret;
 
@@ -651,22 +652,23 @@ static int cachefiles_daemon_inuse(struc
 	/* extract the directory dentry from the cwd */
 	fs = current->fs;
 	read_lock(&fs->lock);
-	dir = dget(fs->pwd.dentry);
+	path = fs->pwd;
+	path_get(&path);
 	read_unlock(&fs->lock);
 
-	if (!S_ISDIR(dir->d_inode->i_mode))
+	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = cachefiles_check_in_use(cache, dir, args);
+	ret = cachefiles_check_in_use(cache, path.dentry, args);
 	cachefiles_end_secure(cache, saved_cred);
 
-	dput(dir);
+	path_put(&path);
 	//_leave(" = %d", ret);
 	return ret;
 
 notdir:
-	dput(dir);
+	path_put(&path);
 	kerror("inuse command requires dirfd to be a directory");
 	return -ENOTDIR;
 

-- 

Re: [PATCH 1/7] cachefiles: use path_get instead of lone dget

[Miklos Szeredi]

On Tue, 06 Jul 2010, David Howells wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> > Dentry references should not be acquired without a corresponding
> > vfsmount ref.
> 
> Which we have: current->fs holds one.  Or is this unsafe due to CLONE_FS?

current->fs doesn't change, but the contents can (e.g. another thread
does chdir()).

Thanks,
Miklos

[PATCH 0/7] dcache fixes and cleanups

[Miklos Szeredi]

This series fixes sys_getcwd() and symlinks under /proc in case of
unreachable (e.g. lazy unmounted) paths.

The behavior of d_name() is untouched, new helper functions are
introduced instead.

Changes from the previous posting:

 - add helpers to get root and pwd from fs_struct (Nick Piggin's comment)
 - found and fixed a bug in CacheFiles in this area
 - changed ia64/perfmon filesystem to use ->d_dname
 - split up into more managable pataches

Thanks,
Miklos
--

[PATCH 1/7] cachefiles: use path_get instead of lone dget

[Miklos Szeredi]

[-- Attachment #1: cachefiles-fix-lone-dentry-ref.patch --]
[-- Type: text/plain, Size: 2510 bytes --]

Dentry references should not be acquired without a corresponding
vfsmount ref.

CC: David Howells <dhowells@redhat.com>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/cachefiles/daemon.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/cachefiles/daemon.c
===================================================================
--- linux-2.6.orig/fs/cachefiles/daemon.c	2010-07-05 15:44:37.000000000 +0200
+++ linux-2.6/fs/cachefiles/daemon.c	2010-07-05 15:47:40.000000000 +0200
@@ -553,7 +553,7 @@ static int cachefiles_daemon_tag(struct
 static int cachefiles_daemon_cull(struct cachefiles_cache *cache, char *args)
 {
 	struct fs_struct *fs;
-	struct dentry *dir;
+	struct path path;
 	const struct cred *saved_cred;
 	int ret;
 
@@ -575,22 +575,23 @@ static int cachefiles_daemon_cull(struct
 	/* extract the directory dentry from the cwd */
 	fs = current->fs;
 	read_lock(&fs->lock);
-	dir = dget(fs->pwd.dentry);
+	path = fs->pwd;
+	path_get(&path);
 	read_unlock(&fs->lock);
 
-	if (!S_ISDIR(dir->d_inode->i_mode))
+	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = cachefiles_cull(cache, dir, args);
+	ret = cachefiles_cull(cache, path.dentry, args);
 	cachefiles_end_secure(cache, saved_cred);
 
-	dput(dir);
+	path_put(&path);
 	_leave(" = %d", ret);
 	return ret;
 
 notdir:
-	dput(dir);
+	path_put(&path);
 	kerror("cull command requires dirfd to be a directory");
 	return -ENOTDIR;
 
@@ -629,7 +630,7 @@ inval:
 static int cachefiles_daemon_inuse(struct cachefiles_cache *cache, char *args)
 {
 	struct fs_struct *fs;
-	struct dentry *dir;
+	struct path path;
 	const struct cred *saved_cred;
 	int ret;
 
@@ -651,22 +652,23 @@ static int cachefiles_daemon_inuse(struc
 	/* extract the directory dentry from the cwd */
 	fs = current->fs;
 	read_lock(&fs->lock);
-	dir = dget(fs->pwd.dentry);
+	path = fs->pwd;
+	path_get(&path);
 	read_unlock(&fs->lock);
 
-	if (!S_ISDIR(dir->d_inode->i_mode))
+	if (!S_ISDIR(path.dentry->d_inode->i_mode))
 		goto notdir;
 
 	cachefiles_begin_secure(cache, &saved_cred);
-	ret = cachefiles_check_in_use(cache, dir, args);
+	ret = cachefiles_check_in_use(cache, path.dentry, args);
 	cachefiles_end_secure(cache, saved_cred);
 
-	dput(dir);
+	path_put(&path);
 	//_leave(" = %d", ret);
 	return ret;
 
 notdir:
-	dput(dir);
+	path_put(&path);
 	kerror("inuse command requires dirfd to be a directory");
 	return -ENOTDIR;
 

--