Re: [PATCH] vfs_read/vfs_write small bug fix (2.5.29)

[torvalds@transmeta.com (Linus Torvalds)]

In article <200207291825.g6TIPj026021@eng2.beaverton.ibm.com>,
Badari Pulavarty  <pbadari@us.ibm.com> wrote:
>
>Here is a patch to fix small bug in for vfs_read/vfs_write.

Hmm. The patch is bogus, but that's not your fault, looking at the code
the patch is no more bogus than the existing code already is.

The fact is, the test for a negative "pos" should not be in
vfs_read/write at all, since it can only happen for pread/pwrite.

And pread/pwrite do not even _take_ a "loff_t" argument, they take a
"off_t", and yet we've just happily claiming they do a loff_t, which
means that they shouldn't work at all unless by pure changce user space
happens to put a zero in memory in the right place.

Cristoph, I think you're the one that did this re-org. I think the code
is wrong, and the right fix is something along these lines (untested,
you get brownie-points for testing against some standards test).

		Linus


----
===== fs/read_write.c 1.12 vs edited =====
--- 1.12/fs/read_write.c	Sat Jul 27 08:21:19 2002
+++ edited/fs/read_write.c	Mon Jul 29 12:51:09 2002
@@ -185,8 +185,6 @@
 		return -EBADF;
 	if (!file->f_op || !file->f_op->read)
 		return -EINVAL;
-	if (pos < 0)
-		return -EINVAL;
 
 	ret = locks_verify_area(FLOCK_VERIFY_READ, inode, file, *pos, count);
 	if (!ret) {
@@ -210,8 +208,6 @@
 		return -EBADF;
 	if (!file->f_op || !file->f_op->write)
 		return -EINVAL;
-	if (pos < 0)
-		return -EINVAL;
 
 	ret = locks_verify_area(FLOCK_VERIFY_WRITE, inode, file, *pos, count);
 	if (!ret) {
@@ -255,14 +251,18 @@
 }
 
 asmlinkage ssize_t sys_pread(unsigned int fd, char *buf,
-			     size_t count, loff_t pos)
+			     size_t count, off_t pos)
 {
 	struct file *file;
 	ssize_t ret = -EBADF;
 
+	if (pos < 0)
+		return -EINVAL;
+
 	file = fget(fd);
 	if (file) {
-		ret = vfs_read(file, buf, count, &pos);
+		loff_t lpos = pos;
+		ret = vfs_read(file, buf, count, &lpos);
 		fput(file);
 	}
 
@@ -270,14 +270,18 @@
 }
 
 asmlinkage ssize_t sys_pwrite(unsigned int fd, const char *buf,
-			      size_t count, loff_t pos)
+			      size_t count, off_t pos)
 {
 	struct file *file;
 	ssize_t ret = -EBADF;
 
+	if (pos < 0)
+		return -EINVAL;
+
 	file = fget(fd);
 	if (file) {
-		ret = vfs_write(file, buf, count, &pos);
+		loff_t lpos = pos;
+		ret = vfs_write(file, buf, count, &lpos);
 		fput(file);
 	}