Re: [PATCH] cramfs corruption after BLKFLSBUF on loop device

[Andrew Morton <akpm@osdl.org>]

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().