Re: [patch] fix infinite loop in generic_file_splice_read()

Previous thread: fs time granularity by Adrian Hunter on Wednesday, April 9, 2008 - 8:07 am. (1 message)

Next thread: Re: file offset corruption on 32-bit machines? by Michal Hocko on Thursday, April 10, 2008 - 6:55 am. (22 messages)
From: Miklos Szeredi
Date: Wednesday, April 9, 2008 - 8:57 am

generic_file_splice_read() goes into an infinite loop if it races with
truncation.  I've found this with fsx-linux on NFS over fuse.

Perhaps the whole while() loop is bogus, but I can't tell from a
cursory glance at __generic_file_splice_read() if it will return zero
only on EOF, or it can do that for other reasons as well.  In the
latter case the loop is obviously needed.

This simplistic patch fixes the issue for me.

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

Index: linux/fs/splice.c
===================================================================
--- linux.orig/fs/splice.c	2008-04-02 13:34:58.000000000 +0200
+++ linux/fs/splice.c	2008-04-09 17:35:06.000000000 +0200
@@ -481,19 +481,20 @@ ssize_t generic_file_splice_read(struct 
 {
 	ssize_t spliced;
 	int ret;
-	loff_t isize, left;
-
-	isize = i_size_read(in->f_mapping->host);
-	if (unlikely(*ppos >= isize))
-		return 0;
-
-	left = isize - *ppos;
-	if (unlikely(left < len))
-		len = left;
 
 	ret = 0;
 	spliced = 0;
 	while (len && !spliced) {
+		loff_t isize, left;
+
+		isize = i_size_read(in->f_mapping->host);
+		if (unlikely(*ppos >= isize))
+			return 0;
+
+		left = isize - *ppos;
+		if (unlikely(left < len))
+			len = left;
+
 		ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
 
 		if (ret < 0)
--

From: Oliver Pinter
Date: Wednesday, April 9, 2008 - 10:05 am

SGVsbPMgTWlrbPNzIQoKZXogYSBwYXRjaCBzevxrc+lnZXMgYSAyLjYuMjIueSBrZXJuZWxoZXo/
ICgyLjYuMjIueS1iYSBlbHP1IHLhbul66XNyZQpodW5rIG7pbGv8bCBob3p64WFkaGF08ykKdmFn
eSB24XJqYW0gbWVnLCBhbWlnIGJla2Vy/GwgYSBtYWlubGluZSBrZXJuZWxiZT8KCk9uIDQvOS8w
OCwgTWlrbG9zIFN6ZXJlZGkgPG1pa2xvc0BzemVyZWRpLmh1PiB3cm90ZToKPiBnZW5lcmljX2Zp
bGVfc3BsaWNlX3JlYWQoKSBnb2VzIGludG8gYW4gaW5maW5pdGUgbG9vcCBpZiBpdCByYWNlcyB3
aXRoCj4gdHJ1bmNhdGlvbi4gIEkndmUgZm91bmQgdGhpcyB3aXRoIGZzeC1saW51eCBvbiBORlMg
b3ZlciBmdXNlLgo+Cj4gUGVyaGFwcyB0aGUgd2hvbGUgd2hpbGUoKSBsb29wIGlzIGJvZ3VzLCBi
dXQgSSBjYW4ndCB0ZWxsIGZyb20gYQo+IGN1cnNvcnkgZ2xhbmNlIGF0IF9fZ2VuZXJpY19maWxl
X3NwbGljZV9yZWFkKCkgaWYgaXQgd2lsbCByZXR1cm4gemVybwo+IG9ubHkgb24gRU9GLCBvciBp
dCBjYW4gZG8gdGhhdCBmb3Igb3RoZXIgcmVhc29ucyBhcyB3ZWxsLiAgSW4gdGhlCj4gbGF0dGVy
IGNhc2UgdGhlIGxvb3AgaXMgb2J2aW91c2x5IG5lZWRlZC4KPgo+IFRoaXMgc2ltcGxpc3RpYyBw
YXRjaCBmaXhlcyB0aGUgaXNzdWUgZm9yIG1lLgo+Cj4gU2lnbmVkLW9mZi1ieTogTWlrbG9zIFN6
ZXJlZGkgPG1zemVyZWRpQHN1c2UuY3o+Cj4gLS0tCj4gIGZzL3NwbGljZS5jIHwgICAxOSArKysr
KysrKysrLS0tLS0tLS0tCj4gIDEgZmlsZSBjaGFuZ2VkLCAxMCBpbnNlcnRpb25zKCspLCA5IGRl
bGV0aW9ucygtKQo+Cj4gSW5kZXg6IGxpbnV4L2ZzL3NwbGljZS5jCj4gPT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQo+IC0t
LSBsaW51eC5vcmlnL2ZzL3NwbGljZS5jCTIwMDgtMDQtMDIgMTM6MzQ6NTguMDAwMDAwMDAwICsw
MjAwCj4gKysrIGxpbnV4L2ZzL3NwbGljZS5jCTIwMDgtMDQtMDkgMTc6MzU6MDYuMDAwMDAwMDAw
ICswMjAwCj4gQEAgLTQ4MSwxOSArNDgxLDIwIEBAIHNzaXplX3QgZ2VuZXJpY19maWxlX3NwbGlj
ZV9yZWFkKHN0cnVjdAo+ICB7Cj4gIAlzc2l6ZV90IHNwbGljZWQ7Cj4gIAlpbnQgcmV0Owo+IC0J
bG9mZl90IGlzaXplLCBsZWZ0Owo+IC0KPiAtCWlzaXplID0gaV9zaXplX3JlYWQoaW4tPmZfbWFw
cGluZy0+aG9zdCk7Cj4gLQlpZiAodW5saWtlbHkoKnBwb3MgPj0gaXNpemUpKQo+IC0JCXJldHVy
biAwOwo+IC0KPiAtCWxlZnQgPSBpc2l6ZSAtICpwcG9zOwo+IC0JaWYgKHVubGlrZWx5KGxlZnQg
PCBsZW4pKQo+IC0JCWxlbiA9IGxlZnQ7Cj4KPiAgCXJldCA9IDA7Cj4gIAlzcGxpY2VkID0gMDsK
PiAgCXdoaWxlIChsZW4gJiYgIXNwbGljZWQpIHsKPiArCQlsb2ZmX3QgaXNpemUsIGxlZnQ7Cj4g
Kwo+ICsJCWlzaXplID0gaV9zaXplX3JlYWQoaW4tPmZfbWFwcGluZy0+aG9zdCk7Cj4gKwkJaWYg
KHVubGlrZWx5KCpw ...
From: Andrew Morton
Date: Wednesday, April 9, 2008 - 11:57 am

On Wed, 09 Apr 2008 17:57:56 +0200

We found suspicious-looking code in generic_file_splice_read() back in
February.  See http://lkml.org/lkml/2008/2/29/443.  I suspect that patch
(if it works) will address the truncate lockup as well - it zaps the loop
entirely.

Unfortunately Allard never got back to us (probably because he's running
2.6.24 which has a quite different generic_file_splice_read()) and the
patch didn't get anywhere.

Here it is again.

It needs a changelog.

Nobody has tested this at all, to my knowledge.

It's going to take some serious and sudden effort to get these bugs fixed
for 2.6.25.


 fs/splice.c |   31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff -puN fs/splice.c~generic_file_splice_read-fix-lockups fs/splice.c
--- a/fs/splice.c~generic_file_splice_read-fix-lockups
+++ a/fs/splice.c
@@ -370,8 +370,10 @@ __generic_file_splice_read(struct file *
 			 * for an in-flight io page
 			 */
 			if (flags & SPLICE_F_NONBLOCK) {
-				if (TestSetPageLocked(page))
+				if (TestSetPageLocked(page)) {
+					error = -EAGAIN;
 					break;
+				}
 			} else
 				lock_page(page);
 
@@ -479,9 +481,8 @@ ssize_t generic_file_splice_read(struct 
 				 struct pipe_inode_info *pipe, size_t len,
 				 unsigned int flags)
 {
-	ssize_t spliced;
-	int ret;
 	loff_t isize, left;
+	int ret;
 
 	isize = i_size_read(in->f_mapping->host);
 	if (unlikely(*ppos >= isize))
@@ -491,29 +492,9 @@ ssize_t generic_file_splice_read(struct 
 	if (unlikely(left < len))
 		len = left;
 
-	ret = 0;
-	spliced = 0;
-	while (len && !spliced) {
-		ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
-
-		if (ret < 0)
-			break;
-		else if (!ret) {
-			if (spliced)
-				break;
-			if (flags & SPLICE_F_NONBLOCK) {
-				ret = -EAGAIN;
-				break;
-			}
-		}
-
+	ret = __generic_file_splice_read(in, ppos, pipe, len, flags);
+	if (ret > 0)
 		*ppos += ret;
-		len -= ret;
-		spliced += ret;
-	}
-
-	if ...
From: Miklos Szeredi
Date: Wednesday, April 9, 2008 - 12:25 pm

Patch looks sane, and fixes the fsx-linux/NFS/fuse lockup, as expected.

Thanks,
Miklos
--

From: Jens Axboe
Date: Wednesday, April 9, 2008 - 12:52 pm

Hmm strange, I was pretty sure I pushed my patch back then. I'll double

The original reporter did not, however others did.

-- 
Jens Axboe

--

From: Allard Hoeve
Date: Wednesday, April 9, 2008 - 11:29 pm

Unfortunately, we cannot test the patch on the server that triggered the 
bug in 2.6.24.2, because it's too critical for operation. On other similar 
servers the bug hasn't been encountered and we were unable to reproduce 
it, so it must be some combination of load and typical usage (NFS server).

We are now running 2.6.22.x, which does fine. I hope you understand our 
hesitation of running patched, known-bugged kernels on our fileservers :)

Thanks for all the bughunting here. There is a maintenance window soon, 
I'll try to put the original patch on the agenda.

Regards,

Allard Hoeve
--

Previous thread: fs time granularity by Adrian Hunter on Wednesday, April 9, 2008 - 8:07 am. (1 message)

Next thread: Re: file offset corruption on 32-bit machines? by Michal Hocko on Thursday, April 10, 2008 - 6:55 am. (22 messages)