Messages in this thread | | | Date | Thu, 1 Jun 2006 12:12:00 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] cramfs corruption after BLKFLSBUF on loop device |
| |
On Thu, 1 Jun 2006 20:49:38 +0200 Olaf Hering <olh@suse.de> wrote:
> > This script will cause cramfs decompression errors, on SMP at least: > > #!/bin/bash > while :;do blockdev --flushbufs /dev/loop0;done </dev/null &>/dev/null& > while :;do ps faxs </dev/null &>/dev/null&done </dev/null &>/dev/null& > while :;do dmesg </dev/null &>/dev/null&done </dev/null &>/dev/null& > while :;do find /mounts/instsys -type f -print0|xargs -0 cat &>/dev/null;done > > (The used executables come from the symlinked /mounts/instsys directory) > > ... > Error -3 while decompressing! > c0000000009592a2(2649)->c0000000edf87000(4096) > Error -3 while decompressing! > c000000000959298(2520)->c0000000edbc7000(4096) > Error -3 while decompressing! > c000000000959c70(2489)->c0000000f1482000(4096) > Error -3 while decompressing! > c00000000095a629(2355)->c0000000edaff000(4096) > Error -3 while decompressing! > ... > > Its a long standing bug, introduced in 2.6.2. > > cramfs_read() clears parts of the src buffer because the page is not uptodate. > invalidate_bdev() called from block_ioctl(BLKFLSBUF) will set ClearPageUptodate() > after cramfs_read() got the page from read_cache_page() > If PageUptodate() fails, read the page again before using it. > There is still a small window were the page may not be uptodate before copying > its contents away. > > evms_access does the BLKFLSBUF ioctl (lots of them) on the loop device. This will > corrupt the SuSE installation image on SMP kernels, leading to random segfaults. >
OK, invalidate_inode_pages().
> + > for (i = 0; i < BLKS_PER_BUF; i++) { > - struct page *page = pages[i]; > - if (page) { > - memcpy(data, kmap(page), PAGE_CACHE_SIZE); > - kunmap(page); > - page_cache_release(page); > - } else > - memset(data, 0, PAGE_CACHE_SIZE); > + if (blocknr + i < devsize) { > + page = NULL; > + for (readagain = 0; readagain < 5; readagain++) { > + page = read_cache_page(mapping, blocknr + i, > + (filler_t *)mapping->a_ops->readpage, > + NULL); > + /* synchronous error? */ > + if (IS_ERR(page)) { > + page = NULL; > + break; > + } > + wait_on_page_locked(page); > + if (PageUptodate(page)) > + break; > + /* asynchronous error */ > + /* maybe BLKFLSBUF flushed the page */ > + page_cache_release(page); > + page = NULL; > + } > + if (page) { > + memcpy(data, kmap(page), PAGE_CACHE_SIZE); > + kunmap(page); > + page_cache_release(page); > + } else > + memset(data, 0, PAGE_CACHE_SIZE); > + if (readagain) > + printk(KERN_DEBUG "cramfs_read got %s Uptodate page after %d attempt(s)\n", > + page ? "an" : "no", readagain); > + }
This code absolutely needs a comment telling the poor reader what it's in there for.
It's still racy. Do the memcpy while the page is locked:
retry: read_cache_page() lock_page() if (!PageUptodate()) { if (readagain-- != 0) goto retry; give_up(); } memcpy(); unlock_page();
Also, let's use kmap_atomic() while we're in there.
Of course, this will all just fail less often than it presently does. We'd be better off taking a lock if poss to keep the ioctl away. I'd have thought that it'd be appropriate to take i_mutex while running invalidate_inode_pages(). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |