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

From: Paolo 'Blaisorblade' Giarrusso
Date: Sun Sep 18 2005 - 09:24:44 EST


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/