VFS readahead bug in 2.6.8-rc[1-3]
Subsystems (2)
PATCH 1/1 VFS readahead bug in 2.6.8-rc[1-3]
Hi,
There is a readahead bug in do_generic_mapping_read (filemap.c). This bug appears to have been introduced in 2.6.8-rc1. Specifically the bug is caused by an incorrect code change which causes VFS to call readpage() for indexes beyond the end of files where the file length is zero or a 4k multiple.
In Squashfs this causes a variety of almost immediate OOPes because Squashfs trusts the VFS not to pass invalid index values. For other filesystems it may also be causing subtle bugs. I have received prune_dcache oopes similar to Gene Heskett's (which was also pointer corruption), and so it may fix this and other reported readahead bugs.
The patch is against 2.6.8-rc3.
Regards
Phillip Lougher
mm/filemap.c 2004-08-05 18:15:00.000000000 +0100 renamed from mm/filemap.c 2004-08-05 02:14:39.000000000 +0100 +9 -9
@@ -674,6 +674,15 @@ unsigned long nr, ret; cond_resched(); + + /* nr is the maximum number of bytes to copy from this page */ + nr = PAGE_CACHE_SIZE; + if (index == end_index) { + nr = isize & ~PAGE_CACHE_MASK; + if (nr <= offset) + goto out; + } + page_cache_readahead(mapping, &ra, filp, index); find_page:
@@ -685,15 +694,6 @@ if (!PageUptodate(page)) goto page_not_up_to_date; page_ok: - /* nr is the maximum number of bytes to copy from this page */ - nr = PAGE_CACHE_SIZE; - if (index == end_index) { - nr = isize & ~PAGE_CACHE_MASK; - if (nr <= offset) { - page_cache_release(page); - goto out; - } - } nr = nr - offset; /* If users can be writing to this page using arbitrary
Quoted 8 lines · Phillip Lougher
Hi,
There is a readahead bug in
do_generic_mapping_read(filemap.c). This bug appears to have been introduced in 2.6.8-rc1. Specifically the bug is caused by an incorrect code change which causes VFS to callreadpage()for indexes beyond the end of files where the file length is zero or a 4k multiple.
Quoted 9 lines
In Squashfs this causes a variety of almost immediate OOPes because Squashfs trusts the VFS not to pass invalid index values. For other filesystems it may also be causing subtle bugs. I have received
prune_dcacheoopes similar to Gene Heskett's (which was also pointer corruption), and so it may fix this and other reported readahead bugs.The patch is against 2.6.8-rc3.
Good work - bug is mine, sorry.
You actually re-introduce a bug where read can return incorrect data due to i_size changing from under it (I introduced this bug while fixing that one).
My fix was to re-check i_size and update 'nr' after doing the ->readpage. You could probably fix up both problems with your patch and also copying the hunk down to after i_size gets rechecked. Does that sound ok?
The root of the problem is that i_size gets checked from multiple places that it can get out of synch. A nice fix would be to snapshot i_size once, and pass that around everywhere. Unfortunately this is very intrusive.
Quoted 17 lines · Nick Piggin
Quoted 15 lines · Phillip Lougher
Hi,
There is a readahead bug in
do_generic_mapping_read(filemap.c). This bug appears to have been introduced in 2.6.8-rc1. Specifically the bug is caused by an incorrect code change which causes VFS to callreadpage()for indexes beyond the end of files where the file length is zero or a 4k multiple.In Squashfs this causes a variety of almost immediate OOPes because Squashfs trusts the VFS not to pass invalid index values. For other filesystems it may also be causing subtle bugs. I have received
prune_dcacheoopes similar to Gene Heskett's (which was also pointer corruption), and so it may fix this and other reported readahead bugs.
Quoted 5 lines
Quoted 2 lines
The patch is against 2.6.8-rc3.
Good work - bug is mine, sorry.
On second thought, maybe not. I think your filesystem is at fault.
Firstly, there possibly is a bug in do_pagecache_readahead which allows it to read off the end of the file in a completely serialised situation. But even so, notice the absence of locking - i_size can change at any time can't it? Then your fix will blow up when you race with a truncate, right?
I think you need to handle reading off the end of the file properly: I'm not too familiar with this code, but IIRC you are allowed to setup pagecache past the end of the file - it gets handled correctly in do_generic_mapping_read, and ends up falling off the LRU. This should help you.
Quoted 1 line · Nick Piggin
On second thought, maybe not. I think your filesystem is at fault.
No I'm not wrong here. With a read-only filesystem i_size can never change, there are no possible race conditions. If a too large index is passed it is a VFS bug. Are you suggesting I should start to code assuming the VFS is broken?
Quoted 10 lines · Phillip Lougher
Quoted 1 line · Nick Piggin
On second thought, maybe not. I think your filesystem is at fault.
No I'm not wrong here. With a read-only filesystem
i_sizecan never change, there are no possible race conditions. If a too large index is passed it is a VFS bug. Are you suggesting I should start to code assuming the VFS is broken?
No, I suggest you start to code assuming this interface does what it does. I didn't say there is no bug here, but nobody else's filesystem breaks.
Quoted 14 lines · Nick Piggin
Quoted 11 lines · Phillip Lougher
Quoted 1 line · Nick Piggin
On second thought, maybe not. I think your filesystem is at fault.
No I'm not wrong here. With a read-only filesystem
i_sizecan never change, there are no possible race conditions. If a too large index is passed it is a VFS bug. Are you suggesting I should start to code assuming the VFS is broken?
Quoted 4 lines
No, I suggest you start to code assuming this interface does what it does. I didn't say there is no bug here, but nobody else's filesystem breaks.
Point one: The interface didn't do this UNTIL you changed the code Point two: Just because no one has reported other filesystem breakage, it doesn't mean other filesystems have not broken.
Perhaps I should have the check in my code. However, it is still stupid to fix an occasional race by ensuring readpage() is always called with an out of bounds index when files are 0 or a 4K multiple. The filesystem check may prevent a crash, but it is needlessly wasteful by design, not through any inadvertant race condition.
think is it stupid to fix an o. However, I it
stupid to fix a race co
ll think it stupid to fix a race you en
Quoted 3 lines · Phillip Lougher
Point one: The interface didn't do this UNTIL you changed the code Point two: Just because no one has reported other filesystem breakage, it doesn't mean other filesystems have not broken.
Point three: it was never promised *AND* never intended.
Quoted 4 lines · Nick Piggin
No, I suggest you start to code assuming this interface does what it does. I didn't say there is no bug here, but nobody else's filesystem breaks.
To stop this silly argument from escalating, I will patch my code.
Phillip
Quoted 10 lines · Phillip Lougher
Quoted 4 lines · Nick Piggin
No, I suggest you start to code assuming this interface does what it does. I didn't say there is no bug here, but nobody else's filesystem breaks.
To stop this silly argument from escalating, I will patch my code.
Well I don't think it's silly.
We are deterministically asking the fs to read a page which lies outside EOF, and we shouldn't. If for no other reason than that the ever-popular "read a million 4k files" workload will consume extra CPU and twice the pagecache.
Quoted 12 lines · Andrew Morton
Quoted 10 lines · Phillip Lougher
Quoted 4 lines · Nick Piggin
No, I suggest you start to code assuming this interface does what it does. I didn't say there is no bug here, but nobody else's filesystem breaks.
To stop this silly argument from escalating, I will patch my code.
Quoted 6 lines
Well I don't think it's silly.
We are deterministically asking the fs to read a page which lies outside EOF, and we shouldn't. If for no other reason than that the ever-popular "read a million 4k files" workload will consume extra CPU and twice the pagecache.
Andrew,
Enclosed a patch developed with Nick Piggin. It takes care of the bug.
Thanks,
RP
mm/filemap.c renamed from linux-2.6.8.1/mm/filemap.c +6 -2
@@ -665,14 +665,18 @@ void do_generic_mapping_read(struct addr offset = *ppos & ~PAGE_CACHE_MASK ; isize =i_size_read (inode); - end_index = isize >>PAGE_CACHE_SHIFT ; -if (index > end_index) +if (!isize) goto out; + + end_index = isize >>PAGE_CACHE_SHIFT ;for (;;) {struct page *page;unsigned long nr, ret; +if (index > end_index) + goto out; +cond_resched ();page_cache_readahead (mapping, &ra, filp, index);
Quoted 12 lines · Nick Piggin
Quoted 10 lines · Phillip Lougher
Quoted 1 line · Nick Piggin
On second thought, maybe not. I think your filesystem is at fault.
No I'm not wrong here. With a read-only filesystem
i_sizecan never change, there are no possible race conditions. If a too large index is passed it is a VFS bug. Are you suggesting I should start to code assuming the VFS is broken?
Quoted 3 lines
No, I suggest you start to code assuming this interface does what it does. I didn't say there is no bug here, but nobody else's filesystem breaks.
Well, that might not exactly be true. We have noticed the same problem with ext2 too. Maybe I can send an example initrd image which triggers the bug on Monday.
Signature · 1 line
Pozsar Balazs