[PATCH] ROMFS 0 byte file read error

[PATCH] ROMFS 0 byte file read error

[Chris Fester]

I've verified that the git tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

also has the zero byte file problem for romfs.  This patch
fixes the problem.  Avoids calling romfs_copyfrom when in 
the 0 size case.

Thanks!
Chris Fester

Signed-off-by: Chris Fester <cfester@wms.com>
---

diff --git a/fs/romfs/inode.c b/fs/romfs/inode.c
index 8e51a2a..5d24113 100644
--- a/fs/romfs/inode.c
+++ b/fs/romfs/inode.c
@@ -430,10 +430,10 @@ romfs_readpage(struct file *file, struct page * page)
 
        /* 32 bit warning -- but not for us :) */
        offset = page_offset(page);
-       if (offset < i_size_read(inode)) {
+       if (offset <= i_size_read(inode)) {
                avail = inode->i_size-offset;
                readlen = min_t(unsigned long, avail, PAGE_SIZE);
-               if (romfs_copyfrom(inode, buf, ROMFS_I(inode)->i_dataoffset+offset, readlen) == readlen) {
+               if (!readlen || romfs_copyfrom(inode, buf, ROMFS_I(inode)->i_dataoffset+offset, readlen) == readlen) {
                        if (readlen < PAGE_SIZE) {
                                memset(buf + readlen,0,PAGE_SIZE-readlen);
                        }

Re: [PATCH] ROMFS 0 byte file read error

[Linus Torvalds]

On Wed, 30 Jul 2008, Chris Fester wrote:
>
> I've verified that the git tree at:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> 
> also has the zero byte file problem for romfs.  This patch
> fixes the problem.  Avoids calling romfs_copyfrom when in 
> the 0 size case.

Hmm. Who calls 'readpage()' with an offset past the end of the file 
anyway? This _really_ shouldn't matter.

But regardless, isn't the bug that 'romfs_readpage()' sets an error bit 
by default - ie even if there was no actual IO error? The thing is also 
very confused about types: if it really wants to be safe in "loff_t, then 
it had bettr not do the "min_t()" in just "unsigned long".

So this whole routine really seems to be much more broken than your patch 
implies, and your patch just works around some brokenness.

Of course, I think it's all 32-bit and some of these problems cannot 
actually happen, but wouldn't it be more obvious to rewrite it to have a 
separate error return ("result") and a variable saying how much of the 
page was filled ("filled")?

Something like this. UNTESTED. Pls verify and send back if this is ok. The 
patch is certainly bigger, but I think that the end result is more 
obvious.

		Linus

---
 fs/romfs/inode.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/fs/romfs/inode.c b/fs/romfs/inode.c
index 8e51a2a..60d2f82 100644
--- a/fs/romfs/inode.c
+++ b/fs/romfs/inode.c
@@ -418,7 +418,8 @@ static int
 romfs_readpage(struct file *file, struct page * page)
 {
 	struct inode *inode = page->mapping->host;
-	loff_t offset, avail, readlen;
+	loff_t offset, size;
+	unsigned long filled;
 	void *buf;
 	int result = -EIO;
 
@@ -430,21 +431,29 @@ romfs_readpage(struct file *file, struct page * page)
 
 	/* 32 bit warning -- but not for us :) */
 	offset = page_offset(page);
-	if (offset < i_size_read(inode)) {
-		avail = inode->i_size-offset;
-		readlen = min_t(unsigned long, avail, PAGE_SIZE);
-		if (romfs_copyfrom(inode, buf, ROMFS_I(inode)->i_dataoffset+offset, readlen) == readlen) {
-			if (readlen < PAGE_SIZE) {
-				memset(buf + readlen,0,PAGE_SIZE-readlen);
-			}
-			SetPageUptodate(page);
-			result = 0;
+	size = i_size_read(inode);
+	filled = 0;
+	result = 0;
+	if (offset < size) {
+		unsigned long readlen;
+
+		size -= offset;
+		readlen = size > PAGE_SIZE ? PAGE_SIZE : size;
+
+		filled = romfs_copyfrom(inode, buf, ROMFS_I(inode)->i_dataoffset+offset, readlen);
+
+		if (filled != readlen) {
+			SetPageError(page);
+			filled = 0;
+			result = -EIO;
 		}
 	}
-	if (result) {
-		memset(buf, 0, PAGE_SIZE);
-		SetPageError(page);
-	}
+
+	if (filled < PAGE_SIZE)
+		memset(buf + filled, 0, PAGE_SIZE-filled);
+
+	if (!result)
+		SetPageUptodate(page);
 	flush_dcache_page(page);
 
 	unlock_page(page);

Re: [PATCH] ROMFS 0 byte file read error

[Chris Fester]

On Wed, Jul 30, 2008 at 11:06:29AM -0700, Linus Torvalds wrote:
> Something like this. UNTESTED. Pls verify and send back if this is ok. The 
> patch is certainly bigger, but I think that the end result is more 
> obvious.
> 
> 		Linus

I tested this patch by:
1.)  cat'ing the 0 length file.
2.)  doing a diff between the loop mounted filesystem and the source
directory for the romfs image.
3.)  cp -rdp'ing the contents of the loop mounted filesystem to another
directory, diffing that directory to the source directory.

All of those tests pass.  The image I'm testing with is about 23MB, with
900 files and directories.

I agree that your patch generates far more straightforward code than the
original.  Coolbeans!

Let me know if there's anything else I should do to test.  Also, let me
know if there's other pressing work to be done to this driver and I'll
try my best at de-crustifying it.

Thanks,
Chris Fester

> 
> ---
>  fs/romfs/inode.c |   37 +++++++++++++++++++++++--------------
>  1 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/romfs/inode.c b/fs/romfs/inode.c
> index 8e51a2a..60d2f82 100644
> --- a/fs/romfs/inode.c
> +++ b/fs/romfs/inode.c
> @@ -418,7 +418,8 @@ static int
>  romfs_readpage(struct file *file, struct page * page)
>  {
>  	struct inode *inode = page->mapping->host;
> -	loff_t offset, avail, readlen;
> +	loff_t offset, size;
> +	unsigned long filled;
>  	void *buf;
>  	int result = -EIO;
>  
> @@ -430,21 +431,29 @@ romfs_readpage(struct file *file, struct page * page)
>  
>  	/* 32 bit warning -- but not for us :) */
>  	offset = page_offset(page);
> -	if (offset < i_size_read(inode)) {
> -		avail = inode->i_size-offset;
> -		readlen = min_t(unsigned long, avail, PAGE_SIZE);
> -		if (romfs_copyfrom(inode, buf, ROMFS_I(inode)->i_dataoffset+offset, readlen) == readlen) {
> -			if (readlen < PAGE_SIZE) {
> -				memset(buf + readlen,0,PAGE_SIZE-readlen);
> -			}
> -			SetPageUptodate(page);
> -			result = 0;
> +	size = i_size_read(inode);
> +	filled = 0;
> +	result = 0;
> +	if (offset < size) {
> +		unsigned long readlen;
> +
> +		size -= offset;
> +		readlen = size > PAGE_SIZE ? PAGE_SIZE : size;
> +
> +		filled = romfs_copyfrom(inode, buf, ROMFS_I(inode)->i_dataoffset+offset, readlen);
> +
> +		if (filled != readlen) {
> +			SetPageError(page);
> +			filled = 0;
> +			result = -EIO;
>  		}
>  	}
> -	if (result) {
> -		memset(buf, 0, PAGE_SIZE);
> -		SetPageError(page);
> -	}
> +
> +	if (filled < PAGE_SIZE)
> +		memset(buf + filled, 0, PAGE_SIZE-filled);
> +
> +	if (!result)
> +		SetPageUptodate(page);
>  	flush_dcache_page(page);
>  
>  	unlock_page(page);