[PATCH 1/2] vfs: seq_file: add helpers for data filling

[PATCH 1/2] vfs: seq_file: add helpers for data filling

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Add two helpers that allow access to the seq_file's own buffer, but
hide the internal details of seq_files.

This allows easier implementation of special purpose filling
functions.  It also cleans up some existing functions which duplicated
the seq_file logic.

Make these inline functions in seq_file.h, as suggested by Al.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---
 fs/seq_file.c            |   74 ++++++++++++++++++++++++-----------------------
 include/linux/seq_file.h |   38 ++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 35 deletions(-)

Index: linux-2.6/fs/seq_file.c
===================================================================
--- linux-2.6.orig/fs/seq_file.c	2009-09-09 13:29:21.000000000 +0200
+++ linux-2.6/fs/seq_file.c	2009-09-21 14:17:10.000000000 +0200
@@ -429,20 +429,21 @@ EXPORT_SYMBOL(mangle_path);
  */
 int seq_path(struct seq_file *m, struct path *path, char *esc)
 {
-	if (m->count < m->size) {
-		char *s = m->buf + m->count;
-		char *p = d_path(path, s, m->size - m->count);
+	char *buf;
+	size_t size = seq_get_buf(m, &buf);
+	int res = -1;
+
+	if (size) {
+		char *p = d_path(path, buf, size);
 		if (!IS_ERR(p)) {
-			s = mangle_path(s, p, esc);
-			if (s) {
-				p = m->buf + m->count;
-				m->count = s - m->buf;
-				return s - p;
-			}
+			char *end = mangle_path(buf, p, esc);
+			if (end)
+				res = end - buf;
 		}
 	}
-	m->count = m->size;
-	return -1;
+	seq_commit(m, res);
+
+	return res;
 }
 EXPORT_SYMBOL(seq_path);
 
@@ -454,26 +455,28 @@ EXPORT_SYMBOL(seq_path);
 int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 		  char *esc)
 {
-	int err = -ENAMETOOLONG;
-	if (m->count < m->size) {
-		char *s = m->buf + m->count;
+	char *buf;
+	size_t size = seq_get_buf(m, &buf);
+	int res = -ENAMETOOLONG;
+
+	if (size) {
 		char *p;
 
 		spin_lock(&dcache_lock);
-		p = __d_path(path, root, s, m->size - m->count);
+		p = __d_path(path, root, buf, size);
 		spin_unlock(&dcache_lock);
-		err = PTR_ERR(p);
+		res = PTR_ERR(p);
 		if (!IS_ERR(p)) {
-			s = mangle_path(s, p, esc);
-			if (s) {
-				p = m->buf + m->count;
-				m->count = s - m->buf;
-				return 0;
-			}
+			char *end = mangle_path(buf, p, esc);
+			if (end)
+				res = end - buf;
+			else
+				res = -ENAMETOOLONG;
 		}
 	}
-	m->count = m->size;
-	return err;
+	seq_commit(m, res);
+
+	return res < 0 ? res : 0;
 }
 
 /*
@@ -481,20 +484,21 @@ int seq_path_root(struct seq_file *m, st
  */
 int seq_dentry(struct seq_file *m, struct dentry *dentry, char *esc)
 {
-	if (m->count < m->size) {
-		char *s = m->buf + m->count;
-		char *p = dentry_path(dentry, s, m->size - m->count);
+	char *buf;
+	size_t size = seq_get_buf(m, &buf);
+	int res = -1;
+
+	if (size) {
+		char *p = dentry_path(dentry, buf, size);
 		if (!IS_ERR(p)) {
-			s = mangle_path(s, p, esc);
-			if (s) {
-				p = m->buf + m->count;
-				m->count = s - m->buf;
-				return s - p;
-			}
+			char *end = mangle_path(buf, p, esc);
+			if (end)
+				res = end - buf;
 		}
 	}
-	m->count = m->size;
-	return -1;
+	seq_commit(m, res);
+
+	return res;
 }
 
 int seq_bitmap(struct seq_file *m, const unsigned long *bits,
Index: linux-2.6/include/linux/seq_file.h
===================================================================
--- linux-2.6.orig/include/linux/seq_file.h	2009-09-09 13:29:21.000000000 +0200
+++ linux-2.6/include/linux/seq_file.h	2009-09-21 14:15:40.000000000 +0200
@@ -35,6 +35,44 @@ struct seq_operations {
 
 #define SEQ_SKIP 1
 
+/**
+ * seq_get_buf - get buffer to write arbitrary data to
+ * @m: the seq_file handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_get_buf(struct seq_file *m, char **bufp)
+{
+	BUG_ON(m->count > m->size);
+	if (m->count < m->size)
+		*bufp = m->buf + m->count;
+	else
+		*bufp = NULL;
+
+	return m->size - m->count;
+}
+
+/**
+ * seq_commit - commit data to the buffer
+ * @m: the seq_file handle
+ * @num: the number of bytes to commit
+ *
+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get.  To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_commit(struct seq_file *m, int num)
+{
+	if (num < 0) {
+		m->count = m->size;
+	} else {
+		BUG_ON(m->count + num > m->size);
+		m->count += num;
+	}
+}
+
 char *mangle_path(char *s, char *p, char *esc);
 int seq_open(struct file *, const struct seq_operations *);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);

[PATCH 2/2] vfs: fix d_path() for unreachable paths

[Miklos Szeredi]

(The two fixes have been folded into this one)

----
From: Miklos Szeredi <mszeredi@suse.cz>

John Johansen pointed out, that getcwd(2) will give a garbled result
if a bind mount of a non-filesystem-root directory is detached:

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

If it was the root of the filesystem which was detached, it will give
a saner looking result, but it still won't be a valid absolute path by
which the CWD can be reached (assuming the process's root is not also
on the detached mount).

A similar issue happens if the CWD is outside the process's root or in
a different namespace.  These problems are relevant to symlinks under
/proc/<pid>/ and /proc/<pid>/fd/ as well.

This patch addresses all these issues, by prefixing such unreachable
paths with "(unreachable)".  This isn't perfect since the returned
path may still be a valid _relative_ path, and applications may not
check the result of getcwd() for starting with a '/' before using it.

For this reason Andreas Gruenbacher thinks getcwd(2) should return
ENOENT in these cases, but that breaks /bin/pwd and bash in the above
cases.

Hugh Dickins reported that an old version of gnome-vfs-daemon crashes
because it finds an entry in /proc/mounts where the mountpoint is
unreachable.  So revert /proc/mounts to the old behavior (or rather a
less crazy version of the old behavior).

Also revert the effect on /proc/${PID}/maps for memory maps set up
with shmem_file_setup() or hugetlb_file_setup().  These functions set
up unlinked files under a kernel-private vfsmount.  Since this
vfsmount is unreachable from userspace, these maps will be reported
with the "(unreachable)" prefix, which is undesirable, because it
changes the kernel ABI and might break applications for no good
reason.

Reported-by: John Johansen <jjohansen@suse.de>
CC: Matthew Wilcox <matthew@wil.cx>
CC: Andreas Gruenbacher <agruen@suse.de>
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>

---
 fs/dcache.c          |   20 ++++++++++++++++-
 fs/hugetlbfs/inode.c |   17 +++++++++++++++
 fs/namespace.c       |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 mm/shmem.c           |   17 +++++++++++++++
 4 files changed, 108 insertions(+), 3 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2009-09-21 14:31:35.000000000 +0200
+++ linux-2.6/fs/dcache.c	2009-09-21 14:31:37.000000000 +0200
@@ -1883,6 +1883,12 @@ static int prepend_name(char **buffer, i
 	return prepend(buffer, buflen, name->name, name->len);
 }
 
+static bool is_pseudo_root(struct dentry *dentry)
+{
+	return IS_ROOT(dentry) &&
+		(dentry->d_name.len != 1 || dentry->d_name.name[0] != '/');
+}
+
 /**
  * __d_path - return the path of a dentry
  * @path: the dentry/vfsmount to report
@@ -1950,8 +1956,18 @@ out:
 
 global_root:
 	retval += 1;	/* hit the slash */
-	if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
-		goto Elong;
+
+	if (is_pseudo_root(dentry)) {
+		/* Pseudo filesystem with "foo:" prefix */
+		if (prepend_name(&retval, &buflen, &dentry->d_name) != 0)
+			goto Elong;
+	} else {
+		/*
+		 * Unreachable (detached or outside root or outside namespace)
+		 */
+		if (prepend(&retval, &buflen, "(unreachable)/", 14) != 0)
+			goto Elong;
+	}
 	root->mnt = vfsmnt;
 	root->dentry = dentry;
 	goto out;
Index: linux-2.6/fs/hugetlbfs/inode.c
===================================================================
--- linux-2.6.orig/fs/hugetlbfs/inode.c	2009-09-21 14:31:35.000000000 +0200
+++ linux-2.6/fs/hugetlbfs/inode.c	2009-09-21 14:31:37.000000000 +0200
@@ -931,6 +931,21 @@ static struct file_system_type hugetlbfs
 
 static struct vfsmount *hugetlbfs_vfsmount;
 
+/*
+ * Do a special d_dname function so that these are not prefixed by
+ * "(unreachable)".
+ */
+static char *hugetlb_unlinked_d_dname(struct dentry *dentry, char *buf,
+				      int buflen)
+{
+	return dynamic_dname(dentry, buf, buflen, "/%s (deleted)",
+			     dentry->d_name.name);
+}
+
+static struct dentry_operations hugetlb_unlinked_dentry_operations = {
+	.d_dname = hugetlb_unlinked_d_dname,
+};
+
 static int can_do_hugetlb_shm(void)
 {
 	return capable(CAP_IPC_LOCK) || in_group_p(sysctl_hugetlb_shm_group);
@@ -968,6 +983,8 @@ struct file *hugetlb_file_setup(const ch
 	if (!dentry)
 		goto out_shm_unlock;
 
+	dentry->d_op = &hugetlb_unlinked_dentry_operations;
+
 	error = -ENOSPC;
 	inode = hugetlbfs_get_inode(root->d_sb, current_fsuid(),
 				current_fsgid(), S_IFREG | S_IRWXUGO, 0);
Index: linux-2.6/fs/namespace.c
===================================================================
--- linux-2.6.orig/fs/namespace.c	2009-09-21 14:31:35.000000000 +0200
+++ linux-2.6/fs/namespace.c	2009-09-21 14:31:37.000000000 +0200
@@ -789,6 +789,61 @@ static void show_type(struct seq_file *m
 	}
 }
 
+/*
+ * Same as d_path() except it doesn't stick "(unreachable)" in front
+ * of unreachable paths.
+ */
+static char *d_path_compat(struct path *path, char *buf, int buflen)
+{
+	char *res;
+	struct path root;
+	struct path tmp;
+
+	read_lock(&current->fs->lock);
+	root = current->fs->root;
+	path_get(&root);
+	read_unlock(&current->fs->lock);
+	spin_lock(&dcache_lock);
+	tmp = root;
+	res = __d_path(path, &tmp, buf, buflen);
+	if (!IS_ERR(res) &&
+	    (tmp.mnt != root.mnt || tmp.dentry != root.dentry)) {
+		/*
+		 * Unreachable path found, redo with the global root
+		 * so we get a normal looking path.
+		 */
+		res = __d_path(path, &tmp, buf, buflen);
+	}
+	spin_unlock(&dcache_lock);
+	path_put(&root);
+
+	return res;
+}
+
+/*
+ * Some old programs break if /proc/mounts contains a mountpoint
+ * beginning with "(unreachable)".  Revert this back to the old way of
+ * displaying the path from the global root instead.
+ */
+static int show_path_old(struct seq_file *m, struct path *path, char *esc)
+{
+	char *buf;
+	size_t size = seq_get_buf(m, &buf);
+	int res = -1;
+
+	if (size) {
+		char *p = d_path_compat(path, buf, size);
+		if (!IS_ERR(p)) {
+			char *end = mangle_path(buf, p, esc);
+			if (end)
+				res = end - buf;
+		}
+	}
+	seq_commit(m, res);
+
+	return res;
+}
+
 static int show_vfsmnt(struct seq_file *m, void *v)
 {
 	struct vfsmount *mnt = list_entry(v, struct vfsmount, mnt_list);
@@ -797,7 +852,7 @@ static int show_vfsmnt(struct seq_file *
 
 	mangle(m, mnt->mnt_devname ? mnt->mnt_devname : "none");
 	seq_putc(m, ' ');
-	seq_path(m, &mnt_path, " \t\n\\");
+	show_path_old(m, &mnt_path, " \t\n\\");
 	seq_putc(m, ' ');
 	show_type(m, mnt->mnt_sb);
 	seq_puts(m, __mnt_is_readonly(mnt) ? " ro" : " rw");
Index: linux-2.6/mm/shmem.c
===================================================================
--- linux-2.6.orig/mm/shmem.c	2009-09-21 14:31:35.000000000 +0200
+++ linux-2.6/mm/shmem.c	2009-09-21 14:31:37.000000000 +0200
@@ -2601,6 +2601,21 @@ int shmem_unuse(swp_entry_t entry, struc
 
 /* common code */
 
+/*
+ * Do a special d_dname function so that these are not prefixed by
+ * "(unreachable)".
+ */
+static char *shmem_unlinked_d_dname(struct dentry *dentry, char *buf,
+				    int buflen)
+{
+	return dynamic_dname(dentry, buf, buflen, "/%s (deleted)",
+			     dentry->d_name.name);
+}
+
+static struct dentry_operations shmem_unlinked_dentry_operations = {
+	.d_dname = shmem_unlinked_d_dname,
+};
+
 /**
  * shmem_file_setup - get an unlinked file living in tmpfs
  * @name: name for dentry (to be seen in /proc/<pid>/maps
@@ -2633,6 +2648,8 @@ struct file *shmem_file_setup(const char
 	if (!dentry)
 		goto put_memory;
 
+	dentry->d_op = &shmem_unlinked_dentry_operations;
+
 	error = -ENFILE;
 	file = get_empty_filp();
 	if (!file)

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Al Viro]

On Mon, Sep 21, 2009 at 02:51:42PM +0200, Miklos Szeredi wrote:
> Hugh Dickins reported that an old version of gnome-vfs-daemon crashes
> because it finds an entry in /proc/mounts where the mountpoint is
> unreachable.  So revert /proc/mounts to the old behavior (or rather a
> less crazy version of the old behavior).
> 
> Also revert the effect on /proc/${PID}/maps for memory maps set up
> with shmem_file_setup() or hugetlb_file_setup().  These functions set
> up unlinked files under a kernel-private vfsmount.  Since this
> vfsmount is unreachable from userspace, these maps will be reported
> with the "(unreachable)" prefix, which is undesirable, because it
> changes the kernel ABI and might break applications for no good
> reason.

Ho-hum...
	a) d_op you've got there look like a candidate for libfs, if
we go for that at all
	b) I *really* don't like changing the behaviour of d_path() instead
of doing new behaviour in a new function and deciding where to use it on
case-by-case basis
	c) having just grepped for d_path()... oh, man
* blackfin cplbinfo: utter crap; it's used to decide which procfs file
is being opened - by dumping full pathname into a (on-stack) buffer
and then parsing it.  Stupid *and* broken.
* blackfin traps.c:decode_address(): for one thing, pathnames has been
known to be longer than 256 bytes.  For another... locking in that loop
over processes and VMAs of each looks very suspicios, while we are at it.
* pohmelfs_construct_path_string(): just what will happen if we are called
from process chrooted into that bugger?  d_path() is badly abused there.
* ext4_file_open(): d_path() per se is probably OK, but initializing
path.mnt/path.dentry isn't.  mount --move racing with that thing => loads
of fun.
* sysfs_open_file(): racy in an obvious way, but probably won't do anything
worse than garbage in oopsen.

I'm very uncomfortable with the silent change of behaviour, especially since
existing callers seem to be split between "part of user-visible ABI" and
"generally bogus, waiting for a gnat to fart".  At the very least it needs
a serious audit of callers.

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Mike Frysinger]

On Mon, Sep 21, 2009 at 10:02, Al Viro wrote:
> * blackfin cplbinfo: utter crap; it's used to decide which procfs file
> is being opened - by dumping full pathname into a (on-stack) buffer
> and then parsing it.  Stupid *and* broken.

it works without having to copy & paste the same exact structures over
and over.  a suggestion as how to do it cleanly without bloating the
code is certainly welcome.  it doesnt really matter that it's on the
stack as the usage is small and d_path() is given the size of the
buffer, so it isnt going to overflow.

> * blackfin traps.c:decode_address(): for one thing, pathnames has been
> known to be longer than 256 bytes.

we know the paths are longer than 256 bytes.  the output is to give a
reasonable idea of what is crashing.  realistically, nothing
executable resides in a 256+ byte path on an embedded system.

>  For another... locking in that loop
> over processes and VMAs of each looks very suspicios, while we are at it.

we've reviewed it several times and exercised it in multiple ways.  so
unless you have a real idea of something being wrong, the code has
been vetted heavily.
-mike

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Al Viro]

On Mon, Sep 21, 2009 at 10:10:17AM -0400, Mike Frysinger wrote:

> it works without having to copy & paste the same exact structures over
> and over.  a suggestion as how to do it cleanly without bloating the
> code is certainly welcome.  it doesnt really matter that it's on the
> stack as the usage is small and d_path() is given the size of the
> buffer, so it isnt going to overflow.

Umm...  Surely, you can put a CPU number + one bit into PDE->data?  As in
	proc_create_data("icplb", ....., (void *)(cpu * 2))
	proc_create_data("dcplb", ....., (void *)(cpu * 2 + 1))
and
	struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
	unsigned long n = (unsigned long) pde->data;
	...
	cpu = n / 2;
	is_D = n & 1;

> > ??For another... locking in that loop
> > over processes and VMAs of each looks very suspicios, while we are at it.
> 
> we've reviewed it several times and exercised it in multiple ways.  so
> unless you have a real idea of something being wrong, the code has
> been vetted heavily.

write_lock_irq tasklist_lock
loop over processes
	get_task_mm [now VM is pinned down, but not locked]
	walk the VMA list without any locking

Something on another CPU (you have SMP systems, judging by the previously
discussed code, right?) does munmap().

Boom.

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Al Viro]

On Mon, Sep 21, 2009 at 03:38:57PM +0100, Al Viro wrote:
> On Mon, Sep 21, 2009 at 10:10:17AM -0400, Mike Frysinger wrote:
> 
> > it works without having to copy & paste the same exact structures over
> > and over.  a suggestion as how to do it cleanly without bloating the
> > code is certainly welcome.  it doesnt really matter that it's on the
> > stack as the usage is small and d_path() is given the size of the
> > buffer, so it isnt going to overflow.
> 
> Umm...  Surely, you can put a CPU number + one bit into PDE->data?  As in
> 	proc_create_data("icplb", ....., (void *)(cpu * 2))
> 	proc_create_data("dcplb", ....., (void *)(cpu * 2 + 1))
> and
> 	struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
> 	unsigned long n = (unsigned long) pde->data;
> 	...
> 	cpu = n / 2;
> 	is_D = n & 1;

PS: as to why it is broken...  Consider e.g.
	mount --bind /proc/cplbinfo/cpu0 /mnt
	cat /mnt/icplb
Or, better yet,
	mount -t proc none /mnt/cpu
	cat /mnt/cpu/cplbinfo/cpu0/icplb

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Mike Frysinger]

On Mon, Sep 21, 2009 at 10:43, Al Viro wrote:
> On Mon, Sep 21, 2009 at 03:38:57PM +0100, Al Viro wrote:
>> On Mon, Sep 21, 2009 at 10:10:17AM -0400, Mike Frysinger wrote:
>> > it works without having to copy & paste the same exact structures over
>> > and over.  a suggestion as how to do it cleanly without bloating the
>> > code is certainly welcome.  it doesnt really matter that it's on the
>> > stack as the usage is small and d_path() is given the size of the
>> > buffer, so it isnt going to overflow.
>
> PS: as to why it is broken...  Consider e.g.
>        mount --bind /proc/cplbinfo/cpu0 /mnt
>        cat /mnt/icplb
> Or, better yet,
>        mount -t proc none /mnt/cpu
>        cat /mnt/cpu/cplbinfo/cpu0/icplb

i'm not disagreeing that it doesnt work under all random VFS
scenarios.  just that the realistic ones all work.
-mike

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Mike Frysinger]

On Mon, Sep 21, 2009 at 10:38, Al Viro wrote:
> On Mon, Sep 21, 2009 at 10:10:17AM -0400, Mike Frysinger wrote:
>> it works without having to copy & paste the same exact structures over
>> and over.  a suggestion as how to do it cleanly without bloating the
>> code is certainly welcome.  it doesnt really matter that it's on the
>> stack as the usage is small and d_path() is given the size of the
>> buffer, so it isnt going to overflow.
>
> Umm...  Surely, you can put a CPU number + one bit into PDE->data?  As in
>        proc_create_data("icplb", ....., (void *)(cpu * 2))
>        proc_create_data("dcplb", ....., (void *)(cpu * 2 + 1))
> and
>        struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
>        unsigned long n = (unsigned long) pde->data;
>        ...
>        cpu = n / 2;
>        is_D = n & 1;

if i'd known this route, i'd have used it ;).  i'll look into changing
it, thanks.

>> > ??For another... locking in that loop
>> > over processes and VMAs of each looks very suspicios, while we are at it.
>>
>> we've reviewed it several times and exercised it in multiple ways.  so
>> unless you have a real idea of something being wrong, the code has
>> been vetted heavily.
>
> write_lock_irq tasklist_lock
> loop over processes
>        get_task_mm [now VM is pinned down, but not locked]
>        walk the VMA list without any locking
>
> Something on another CPU (you have SMP systems, judging by the previously
> discussed code, right?) does munmap().

the SMP port is limited due to lack of hardware cache coherency, but
we'll check out this call path
-mike

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Mike Frysinger]

On Mon, Sep 21, 2009 at 10:38, Al Viro wrote:
> On Mon, Sep 21, 2009 at 10:10:17AM -0400, Mike Frysinger wrote:
>> it works without having to copy & paste the same exact structures over
>> and over.  a suggestion as how to do it cleanly without bloating the
>> code is certainly welcome.  it doesnt really matter that it's on the
>> stack as the usage is small and d_path() is given the size of the
>> buffer, so it isnt going to overflow.
>
> Umm...  Surely, you can put a CPU number + one bit into PDE->data?  As in
>        proc_create_data("icplb", ....., (void *)(cpu * 2))
>        proc_create_data("dcplb", ....., (void *)(cpu * 2 + 1))
> and
>        struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
>        unsigned long n = (unsigned long) pde->data;
>        ...
>        cpu = n / 2;
>        is_D = n & 1;

if i'd known this route, i'd have used it ;).  i'll look into changing
it, thanks.

>> > ??For another... locking in that loop
>> > over processes and VMAs of each looks very suspicios, while we are at it.
>>
>> we've reviewed it several times and exercised it in multiple ways.  so
>> unless you have a real idea of something being wrong, the code has
>> been vetted heavily.
>
> write_lock_irq tasklist_lock
> loop over processes
>        get_task_mm [now VM is pinned down, but not locked]
>        walk the VMA list without any locking
>
> Something on another CPU (you have SMP systems, judging by the previously
> discussed code, right?) does munmap().

the SMP port is limited due to lack of hardware cache coherency, but
we'll check out this call path
-mike
--
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] Blackfin: cplbinfo: drop d_path() hacks

[Mike Frysinger]

The cplbinfo was using d_path() to figure out which cpu/cplb was being
parsed.  As Al pointed out, this isn't exactly reliable as it assumes the
static VFS path to be unchanged, and it's just poor form.  So use the
proc_create_data() to properly (and internally) pass the exact cpu/cplb
requested to the parser function.

Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
just a heads up -- here's what i ended up going with.  thanks for the tips.

 arch/blackfin/kernel/cplbinfo.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/blackfin/kernel/cplbinfo.c b/arch/blackfin/kernel/cplbinfo.c
index 64d7830..7e5731c 100644
--- a/arch/blackfin/kernel/cplbinfo.c
+++ b/arch/blackfin/kernel/cplbinfo.c
@@ -111,24 +111,21 @@ static const struct seq_operations cplbinfo_sops = {
 	.show  = cplbinfo_show,
 };
 
+#define CPLBINFO_DCPLB_FLAG 0x80000000
+
 static int cplbinfo_open(struct inode *inode, struct file *file)
 {
-	char buf[256], *path, *p;
+	struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
+	char cplb_type;
 	unsigned int cpu;
-	char *s_cpu, *s_cplb;
 	int ret;
 	struct seq_file *m;
 	struct cplbinfo_data *cdata;
 
-	path = d_path(&file->f_path, buf, sizeof(buf));
-	if (IS_ERR(path))
-		return PTR_ERR(path);
-	s_cpu = strstr(path, "/cpu");
-	s_cplb = strrchr(path, '/');
-	if (!s_cpu || !s_cplb)
-		return -EINVAL;
+	cpu = (unsigned int)pde->data;
+	cplb_type = cpu & CPLBINFO_DCPLB_FLAG ? 'D' : 'I';
+	cpu &= ~CPLBINFO_DCPLB_FLAG;
 
-	cpu = simple_strtoul(s_cpu + 4, &p, 10);
 	if (!cpu_online(cpu))
 		return -ENODEV;
 
@@ -139,7 +136,7 @@ static int cplbinfo_open(struct inode *inode, struct file *file)
 	cdata = m->private;
 
 	cdata->pos = 0;
-	cdata->cplb_type = toupper(s_cplb[1]);
+	cdata->cplb_type = cplb_type;
 	cplbinfo_seq_init(cdata, cpu);
 
 	return 0;
@@ -168,8 +165,10 @@ static int __init cplbinfo_init(void)
 		if (!cpu_dir)
 			return -ENOMEM;
 
-		proc_create("icplb", S_IRUGO, cpu_dir, &cplbinfo_fops);
-		proc_create("dcplb", S_IRUGO, cpu_dir, &cplbinfo_fops);
+		proc_create_data("icplb", S_IRUGO, cpu_dir, &cplbinfo_fops,
+			(void *)cpu);
+		proc_create_data("dcplb", S_IRUGO, cpu_dir, &cplbinfo_fops,
+			(void *)(cpu | CPLBINFO_DCPLB_FLAG));
 	}
 
 	return 0;
-- 
1.6.5.rc1

Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

[Miklos Szeredi]

On Mon, 21 Sep 2009, Al Viro wrote:
> On Mon, Sep 21, 2009 at 02:51:42PM +0200, Miklos Szeredi wrote:
> > Hugh Dickins reported that an old version of gnome-vfs-daemon crashes
> > because it finds an entry in /proc/mounts where the mountpoint is
> > unreachable.  So revert /proc/mounts to the old behavior (or rather a
> > less crazy version of the old behavior).
> > 
> > Also revert the effect on /proc/${PID}/maps for memory maps set up
> > with shmem_file_setup() or hugetlb_file_setup().  These functions set
> > up unlinked files under a kernel-private vfsmount.  Since this
> > vfsmount is unreachable from userspace, these maps will be reported
> > with the "(unreachable)" prefix, which is undesirable, because it
> > changes the kernel ABI and might break applications for no good
> > reason.
> 
> Ho-hum...
> 	a) d_op you've got there look like a candidate for libfs, if
> we go for that at all
> 	b) I *really* don't like changing the behaviour of d_path() instead
> of doing new behaviour in a new function and deciding where to use it on
> case-by-case basis
> 	c) having just grepped for d_path()... oh, man
> * blackfin cplbinfo: utter crap; it's used to decide which procfs file
> is being opened - by dumping full pathname into a (on-stack) buffer
> and then parsing it.  Stupid *and* broken.
> * blackfin traps.c:decode_address(): for one thing, pathnames has been
> known to be longer than 256 bytes.  For another... locking in that loop
> over processes and VMAs of each looks very suspicios, while we are at it.
> * pohmelfs_construct_path_string(): just what will happen if we are called
> from process chrooted into that bugger?  d_path() is badly abused there.
> * ext4_file_open(): d_path() per se is probably OK, but initializing
> path.mnt/path.dentry isn't.  mount --move racing with that thing => loads
> of fun.
> * sysfs_open_file(): racy in an obvious way, but probably won't do anything
> worse than garbage in oopsen.
> 
> I'm very uncomfortable with the silent change of behaviour, especially since
> existing callers seem to be split between "part of user-visible ABI" and
> "generally bogus, waiting for a gnat to fart".  At the very least it needs
> a serious audit of callers.


Fair enough, I should have done a review of internal callers...  Will
do that now.

The big question is, however, if we accept the unknown risk of
changing the user-visible ABI from "broken, but path at least *looks*
normal" to "paths don't necessarily begin with a slash anymore".

Hmm?

Thanks,
Miklos