* [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.