[PATCH 04/12] HPPFS: fix access to ppos and file->f_pos

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@xxxxxxxx>, Al Viro

Access to ->f_pos is racy, and it's not the fs task to update it.

Also, this code is still in the pre-2.6.8 world, when ppos was compared
against &file->f_pos to distinguish between normal reads and pread()s for
unseekable files, and so it performs dirty stuff to follow this rule for
the underlying procfs. (see http://lwn.net/Articles/96662/ - safe seeks).

For inner "struct file"s opened with dentry_open(), we can access safely
their ->f_pos field inside dentry_open(), when we are called by
hppfs_open() - in fact, there's one of them per file descriptor, and
there's no race as the fd has not yet been returned to userspace. See new
read_proc() comment.

Instead, we use the VFS readdir locking on inode->i_sem in hppfs_readdir.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@xxxxxxxx>
---

 fs/hppfs/hppfs_kern.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/hppfs/hppfs_kern.c b/fs/hppfs/hppfs_kern.c
--- a/fs/hppfs/hppfs_kern.c
+++ b/fs/hppfs/hppfs_kern.c
@@ -216,6 +216,15 @@ static struct dentry *hppfs_lookup(struc
 static struct inode_operations hppfs_file_iops = {
 };
 
+/* Pass down ppos when you're being called from userspace.
+ *
+ * Otherwise, if you pass NULL, we'll store the file position in file->f_pos,
+ * and you must have a lock on it. Since we're called only by hppfs_open ->
+ * hppfs_get_data, and open is serialized by the VFS, we're safe.
+ *
+ * We also know if we're called from userspace from is_user, which is used for
+ * set_fs(). I'm leaving this redundancy to bite any wrong caller.
+ */
 static ssize_t read_proc(struct file *file, char *buf, ssize_t count,
 			 loff_t *ppos, int is_user)
 {
@@ -228,17 +237,21 @@ static ssize_t read_proc(struct file *fi
 	if (read == NULL)
 		return -EINVAL;
 
+	WARN_ON(is_user != (ppos != NULL));
+
 	if (!is_user) {
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
 	}
 
-	n = (*read)(file, buf, count, &file->f_pos);
+	if (!ppos)
+		ppos = &file->f_pos;
+
+	n = (*read)(file, buf, count, ppos);
 
 	if (!is_user)
 		set_fs(old_fs);
 
-	if(ppos) *ppos = file->f_pos;
 	return n;
 }
 
@@ -330,9 +343,7 @@ static ssize_t hppfs_write(struct file *
 	if (write == NULL)
 		return -EINVAL;
 
-	proc_file->f_pos = file->f_pos;
-	err = (*write)(proc_file, buf, len, &proc_file->f_pos);
-	file->f_pos = proc_file->f_pos;
+	err = (*write)(proc_file, buf, len, ppos);
 
 	return(err);
 }
@@ -613,6 +624,7 @@ static int hppfs_readdir(struct file *fi
 	if (readdir == NULL)
 		return -ENOTDIR;
 
+	/* XXX: race on f_pos? Should be safe because we hold inode->i_sem. */
 	proc_file->f_pos = file->f_pos;
 	err = (*readdir)(proc_file, &dirent, hppfs_filldir);
 	file->f_pos = proc_file->f_pos;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/