* [PATCH] Propagate CRAMFS uncompression errors @ 2009-02-07 2:55 David VomLehn (dvomlehn) 2009-02-09 23:16 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: David VomLehn (dvomlehn) @ 2009-02-07 2:55 UTC (permalink / raw) To: linux-kernel If cramfs_uncompress_block detects an error uncompressing it will return a zero value. This patch checks the return value and propagates the error back up to the block layer. Signed-off-by: David VomLehn <dvomlehn@cisco.com> --- fs/cramfs/inode.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-) diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c index a07338d..6ff8a5e 100644 --- a/fs/cramfs/inode.c +++ b/fs/cramfs/inode.c @@ -493,7 +493,15 @@ static int cramfs_readpage(struct file *file, struct page * page) memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); kunmap(page); flush_dcache_page(page); - SetPageUptodate(page); + + if (bytes_filled == 0) { + ClearPageUptodate(page); + SetPageError(page); + } + + else + SetPageUptodate(page); + unlock_page(page); return 0; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate CRAMFS uncompression errors 2009-02-07 2:55 [PATCH] Propagate CRAMFS uncompression errors David VomLehn (dvomlehn) @ 2009-02-09 23:16 ` Andrew Morton 2009-02-10 3:28 ` David VomLehn (dvomlehn) 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2009-02-09 23:16 UTC (permalink / raw) To: David VomLehn (dvomlehn); +Cc: linux-kernel On Fri, 6 Feb 2009 21:55:29 -0500 "David VomLehn (dvomlehn)" <dvomlehn@cisco.com> wrote: > If cramfs_uncompress_block detects an error uncompressing it will > return a zero value. This patch checks the return value and propagates > the error back up to the block layer. > > Signed-off-by: David VomLehn <dvomlehn@cisco.com> > --- > fs/cramfs/inode.c | 10 +++++++++- > 1 files changed, 9 insertions(+), 1 deletions(-) > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c > index a07338d..6ff8a5e 100644 > --- a/fs/cramfs/inode.c > +++ b/fs/cramfs/inode.c > @@ -493,7 +493,15 @@ static int cramfs_readpage(struct file *file, > struct page * page) Your email client is wordwrapping the patches. > memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > bytes_filled); > kunmap(page); > flush_dcache_page(page); > - SetPageUptodate(page); > + > + if (bytes_filled == 0) { > + ClearPageUptodate(page); > + SetPageError(page); > + } > + > + else > + SetPageUptodate(page); > + > unlock_page(page); > return 0; A more typical code layout would be if (bytes_filled == 0) { ClearPageUptodate(page); SetPageError(page); } else SetPageUptodate(page); or (better, IMO): if (bytes_filled == 0) { ClearPageUptodate(page); SetPageError(page); } else { SetPageUptodate(page); } This patch will incorrectly cause the driver to report an IO error if the (page->index < maxblock) test returns false. For example, a pread() which is wholly outside the end-of-file should return zero, not -EIO. cramfs_readpage() handles this case very strangely, although not obviously buggily. Probably this function never even gets called for a read wholly outside i_size. How does this version look to you? --- a/fs/cramfs/inode.c~propagate-cramfs-uncompression-errors +++ a/fs/cramfs/inode.c @@ -488,12 +488,19 @@ static int cramfs_readpage(struct file * compr_len); mutex_unlock(&read_mutex); } - } else - pgdata = kmap(page); - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); - kunmap(page); - flush_dcache_page(page); - SetPageUptodate(page); + + if (bytes_filled == 0) { + /* Decompression error */ + ClearPageUptodate(page); + SetPageError(page); + } else { + memset(pgdata + bytes_filled, 0, + PAGE_CACHE_SIZE - bytes_filled); + flush_dcache_page(page); + SetPageUptodate(page); + } + kunmap(page); + } unlock_page(page); return 0; } _ After applying, this is what we have: static int cramfs_readpage(struct file *file, struct page * page) { struct inode *inode = page->mapping->host; u32 maxblock, bytes_filled; void *pgdata; maxblock = (inode->i_size + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT; bytes_filled = 0; if (page->index < maxblock) { struct super_block *sb = inode->i_sb; u32 blkptr_offset = OFFSET(inode) + page->index*4; u32 start_offset, compr_len; start_offset = OFFSET(inode) + maxblock*4; mutex_lock(&read_mutex); if (page->index) start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4); compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset); mutex_unlock(&read_mutex); pgdata = kmap(page); if (compr_len == 0) ; /* hole */ else if (compr_len > (PAGE_CACHE_SIZE << 1)) printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len); else { mutex_lock(&read_mutex); bytes_filled = cramfs_uncompress_block(pgdata, PAGE_CACHE_SIZE, cramfs_read(sb, start_offset, compr_len), compr_len); mutex_unlock(&read_mutex); } if (bytes_filled == 0) { /* Decompression error */ ClearPageUptodate(page); SetPageError(page); } else { memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); flush_dcache_page(page); SetPageUptodate(page); } kunmap(page); } unlock_page(page); return 0; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] Propagate CRAMFS uncompression errors 2009-02-09 23:16 ` Andrew Morton @ 2009-02-10 3:28 ` David VomLehn (dvomlehn) 2009-02-10 3:48 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: David VomLehn (dvomlehn) @ 2009-02-10 3:28 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel > From: Andrew Morton [mailto:akpm@linux-foundation.org] > Sent: Monday, February 09, 2009 3:17 PM > Subject: Re: [PATCH] Propagate CRAMFS uncompression errors > > "David VomLehn (dvomlehn)" <dvomlehn@cisco.com> wrote: > > > If cramfs_uncompress_block detects an error uncompressing it will > > return a zero value. This patch checks the return value and > propagates > > the error back up to the block layer. > > > > Signed-off-by: David VomLehn <dvomlehn@cisco.com> > > --- > > fs/cramfs/inode.c | 10 +++++++++- > > 1 files changed, 9 insertions(+), 1 deletions(-) > > > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c > > index a07338d..6ff8a5e 100644 > > --- a/fs/cramfs/inode.c > > +++ b/fs/cramfs/inode.c > > @@ -493,7 +493,15 @@ static int cramfs_readpage(struct file *file, > > struct page * page) > > Your email client is wordwrapping the patches. Sorry. I sent it to myself first, and didn't have a problem. I hate email programs. > > memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > > bytes_filled); > > kunmap(page); > > flush_dcache_page(page); > > - SetPageUptodate(page); > > + > > + if (bytes_filled == 0) { > > + ClearPageUptodate(page); > > + SetPageError(page); > > + } > > + > > + else > > + SetPageUptodate(page); > > + > > unlock_page(page); > > return 0; > > A more typical code layout would be > > if (bytes_filled == 0) { > ClearPageUptodate(page); > SetPageError(page); > } else > SetPageUptodate(page); > > or (better, IMO): > > if (bytes_filled == 0) { > ClearPageUptodate(page); > SetPageError(page); > } else { > SetPageUptodate(page); > } > > > This patch will incorrectly cause the driver to report an IO error if > the (page->index < maxblock) test returns false. For example, a > pread() which is wholly outside the end-of-file should return > zero, not > -EIO. > > cramfs_readpage() handles this case very strangely, although not > obviously buggily. Probably this function never even gets > called for a > read wholly outside i_size. > > How does this version look to you? > > --- a/fs/cramfs/inode.c~propagate-cramfs-uncompression-errors > +++ a/fs/cramfs/inode.c > @@ -488,12 +488,19 @@ static int cramfs_readpage(struct file * > compr_len); > mutex_unlock(&read_mutex); > } > - } else > - pgdata = kmap(page); > - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > bytes_filled); > - kunmap(page); > - flush_dcache_page(page); > - SetPageUptodate(page); > + > + if (bytes_filled == 0) { > + /* Decompression error */ > + ClearPageUptodate(page); > + SetPageError(page); > + } else { > + memset(pgdata + bytes_filled, 0, > + PAGE_CACHE_SIZE - bytes_filled); > + flush_dcache_page(page); > + SetPageUptodate(page); > + } > + kunmap(page); > + } > unlock_page(page); > return 0; > } Better, thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate CRAMFS uncompression errors 2009-02-10 3:28 ` David VomLehn (dvomlehn) @ 2009-02-10 3:48 ` Andrew Morton 2009-02-10 4:06 ` David VomLehn 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2009-02-10 3:48 UTC (permalink / raw) To: David VomLehn (dvomlehn); +Cc: linux-kernel On Mon, 9 Feb 2009 22:28:15 -0500 "David VomLehn (dvomlehn)" <dvomlehn@cisco.com> wrote: > > > From: Andrew Morton [mailto:akpm@linux-foundation.org] > > Sent: Monday, February 09, 2009 3:17 PM > > Subject: Re: [PATCH] Propagate CRAMFS uncompression errors > > > > "David VomLehn (dvomlehn)" <dvomlehn@cisco.com> wrote: > > > > > If cramfs_uncompress_block detects an error uncompressing it will > > > return a zero value. This patch checks the return value and > > propagates > > > the error back up to the block layer. > > > > > > Signed-off-by: David VomLehn <dvomlehn@cisco.com> > > > --- > > > fs/cramfs/inode.c | 10 +++++++++- > > > 1 files changed, 9 insertions(+), 1 deletions(-) > > > > > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c > > > index a07338d..6ff8a5e 100644 > > > --- a/fs/cramfs/inode.c > > > +++ b/fs/cramfs/inode.c > > > @@ -493,7 +493,15 @@ static int cramfs_readpage(struct file *file, > > > struct page * page) > > > > Your email client is wordwrapping the patches. > > Sorry. I sent it to myself first, and didn't have a problem. I hate > email programs. > > > > memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > > > bytes_filled); > > > kunmap(page); > > > flush_dcache_page(page); > > > - SetPageUptodate(page); > > > + > > > + if (bytes_filled == 0) { > > > + ClearPageUptodate(page); > > > + SetPageError(page); > > > + } > > > + > > > + else > > > + SetPageUptodate(page); > > > + > > > unlock_page(page); > > > return 0; > > > > A more typical code layout would be > > > > if (bytes_filled == 0) { > > ClearPageUptodate(page); > > SetPageError(page); > > } else > > SetPageUptodate(page); > > > > or (better, IMO): > > > > if (bytes_filled == 0) { > > ClearPageUptodate(page); > > SetPageError(page); > > } else { > > SetPageUptodate(page); > > } > > > > > > This patch will incorrectly cause the driver to report an IO error if > > the (page->index < maxblock) test returns false. For example, a > > pread() which is wholly outside the end-of-file should return > > zero, not > > -EIO. > > > > cramfs_readpage() handles this case very strangely, although not > > obviously buggily. Probably this function never even gets > > called for a > > read wholly outside i_size. > > > > How does this version look to you? > > > > --- a/fs/cramfs/inode.c~propagate-cramfs-uncompression-errors > > +++ a/fs/cramfs/inode.c > > @@ -488,12 +488,19 @@ static int cramfs_readpage(struct file * > > compr_len); > > mutex_unlock(&read_mutex); > > } > > - } else > > - pgdata = kmap(page); > > - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - > > bytes_filled); > > - kunmap(page); > > - flush_dcache_page(page); > > - SetPageUptodate(page); > > + > > + if (bytes_filled == 0) { > > + /* Decompression error */ > > + ClearPageUptodate(page); > > + SetPageError(page); > > + } else { > > + memset(pgdata + bytes_filled, 0, > > + PAGE_CACHE_SIZE - bytes_filled); > > + flush_dcache_page(page); > > + SetPageUptodate(page); > > + } > > + kunmap(page); > > + } > > unlock_page(page); > > return 0; > > } > > Better, thanks! umm... Nope, it's still not right. We'll treat this case: if (compr_len == 0) ; /* hole */ as an IO error. grr. --- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors +++ a/fs/cramfs/inode.c @@ -477,7 +477,7 @@ static int cramfs_readpage(struct file * mutex_unlock(&read_mutex); pgdata = kmap(page); if (compr_len == 0) - ; /* hole */ + goto out; /* hole */ else if (compr_len > (PAGE_CACHE_SIZE << 1)) printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len); else { @@ -488,12 +488,20 @@ static int cramfs_readpage(struct file * compr_len); mutex_unlock(&read_mutex); } - } else - pgdata = kmap(page); - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); - kunmap(page); - flush_dcache_page(page); - SetPageUptodate(page); + + if (bytes_filled == 0) { + /* Decompression error */ + ClearPageUptodate(page); + SetPageError(page); + } else { + memset(pgdata + bytes_filled, 0, + PAGE_CACHE_SIZE - bytes_filled); + flush_dcache_page(page); + SetPageUptodate(page); + } + kunmap(page); + } +out: unlock_page(page); return 0; } _ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate CRAMFS uncompression errors 2009-02-10 3:48 ` Andrew Morton @ 2009-02-10 4:06 ` David VomLehn 2009-02-10 5:10 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: David VomLehn @ 2009-02-10 4:06 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel On Mon, 2009-02-09 at 19:48 -0800, Andrew Morton wrote: ... > > umm... > > Nope, it's still not right. We'll treat this case: > > if (compr_len == 0) > ; /* hole */ > > as an IO error. grr. > > --- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors > +++ a/fs/cramfs/inode.c > @@ -477,7 +477,7 @@ static int cramfs_readpage(struct file * > mutex_unlock(&read_mutex); > pgdata = kmap(page); > if (compr_len == 0) > - ; /* hole */ > + goto out; /* hole */ > else if (compr_len > (PAGE_CACHE_SIZE << 1)) > printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len); > else { > @@ -488,12 +488,20 @@ static int cramfs_readpage(struct file * > compr_len); > mutex_unlock(&read_mutex); > } > - } else > - pgdata = kmap(page); > - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); > - kunmap(page); > - flush_dcache_page(page); > - SetPageUptodate(page); > + > + if (bytes_filled == 0) { > + /* Decompression error */ > + ClearPageUptodate(page); > + SetPageError(page); > + } else { > + memset(pgdata + bytes_filled, 0, > + PAGE_CACHE_SIZE - bytes_filled); > + flush_dcache_page(page); > + SetPageUptodate(page); > + } > + kunmap(page); > + } > +out: > unlock_page(page); > return 0; > } Is that actually an error. The comment in the code is, uh, a bit terse, but I took it to mean that there was a deliberate hole, just like holes in files that should read as all zeroes. But even if it's actually an error, we still need to kunmap the page -- David VomLehn, dvomlehn@cisco.com - - - - - Cisco - - - - - This e-mail and any attachments may contain information which is confidential, proprietary, privileged or otherwise protected by law. The information is solely intended for the named addressee (or a person responsible for delivering it to the addressee). If you are not the intended recipient of this message, you are not authorized to read, print, retain, copy or disseminate this message or any part of it. If you have received this e-mail in error, please notify the sender immediately by return e-mail and delete it from your computer. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Propagate CRAMFS uncompression errors 2009-02-10 4:06 ` David VomLehn @ 2009-02-10 5:10 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2009-02-10 5:10 UTC (permalink / raw) To: dvomlehn; +Cc: linux-kernel On Mon, 09 Feb 2009 20:06:01 -0800 David VomLehn <dvomlehn@cisco.com> wrote: > > On Mon, 2009-02-09 at 19:48 -0800, Andrew Morton wrote: > ... > > > > umm... > > > > Nope, it's still not right. We'll treat this case: > > > > if (compr_len == 0) > > ; /* hole */ > > > > as an IO error. grr. > > > > --- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors > > +++ a/fs/cramfs/inode.c > > @@ -477,7 +477,7 @@ static int cramfs_readpage(struct file * > > mutex_unlock(&read_mutex); > > pgdata = kmap(page); > > if (compr_len == 0) > > - ; /* hole */ > > + goto out; /* hole */ > > else if (compr_len > (PAGE_CACHE_SIZE << 1)) > > printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len); > > else { > > @@ -488,12 +488,20 @@ static int cramfs_readpage(struct file * > > compr_len); > > mutex_unlock(&read_mutex); > > } > > - } else > > - pgdata = kmap(page); > > - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); > > - kunmap(page); > > - flush_dcache_page(page); > > - SetPageUptodate(page); > > + > > + if (bytes_filled == 0) { > > + /* Decompression error */ > > + ClearPageUptodate(page); > > + SetPageError(page); > > + } else { > > + memset(pgdata + bytes_filled, 0, > > + PAGE_CACHE_SIZE - bytes_filled); > > + flush_dcache_page(page); > > + SetPageUptodate(page); > > + } > > + kunmap(page); > > + } > > +out: > > unlock_page(page); > > return 0; > > } > > Is that actually an error. The comment in the code is, uh, a bit terse, > but I took it to mean that there was a deliberate hole, just like holes > in files that should read as all zeroes. But even if it's actually an > error, we still need to kunmap the page Yes, we need to unmap the page. It's better to perform those checks before mapping it. --- a/fs/cramfs/inode.c~cramfs-propagate-uncompression-errors +++ a/fs/cramfs/inode.c @@ -475,25 +475,32 @@ static int cramfs_readpage(struct file * start_offset = *(u32 *) cramfs_read(sb, blkptr_offset-4, 4); compr_len = (*(u32 *) cramfs_read(sb, blkptr_offset, 4) - start_offset); mutex_unlock(&read_mutex); - pgdata = kmap(page); if (compr_len == 0) - ; /* hole */ - else if (compr_len > (PAGE_CACHE_SIZE << 1)) - printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", compr_len); - else { - mutex_lock(&read_mutex); - bytes_filled = cramfs_uncompress_block(pgdata, - PAGE_CACHE_SIZE, - cramfs_read(sb, start_offset, compr_len), - compr_len); - mutex_unlock(&read_mutex); + goto out; /* hole */ + if (compr_len > (PAGE_CACHE_SIZE << 1)) { + printk(KERN_ERR "cramfs: bad compressed blocksize %u\n", + compr_len); + goto out; } - } else pgdata = kmap(page); - memset(pgdata + bytes_filled, 0, PAGE_CACHE_SIZE - bytes_filled); - kunmap(page); - flush_dcache_page(page); - SetPageUptodate(page); + mutex_lock(&read_mutex); + bytes_filled = cramfs_uncompress_block(pgdata, PAGE_CACHE_SIZE, + cramfs_read(sb, start_offset, compr_len), compr_len); + mutex_unlock(&read_mutex); + + if (bytes_filled == 0) { + /* Decompression error */ + ClearPageUptodate(page); + SetPageError(page); + } else { + memset(pgdata + bytes_filled, 0, + PAGE_CACHE_SIZE - bytes_filled); + flush_dcache_page(page); + SetPageUptodate(page); + } + kunmap(page); + } +out: unlock_page(page); return 0; } But if address_space_operations.readpage() encounters a hole, it is supposed to return zeroes to userspace (see, for example, block_read_full_page()'s !buffer_mapped() handling). Sigh. That code needs more thought than I am apparently able to give it :( ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-02-10 5:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-02-07 2:55 [PATCH] Propagate CRAMFS uncompression errors David VomLehn (dvomlehn) 2009-02-09 23:16 ` Andrew Morton 2009-02-10 3:28 ` David VomLehn (dvomlehn) 2009-02-10 3:48 ` Andrew Morton 2009-02-10 4:06 ` David VomLehn 2009-02-10 5:10 ` Andrew Morton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.