Patchwork [1/3] ext4: report error if things go wrong when do checksum

login
register
mail settings
Submitter Guo Chao
Date Jan. 5, 2013, 7:42 a.m.
Message ID <1357371781-18194-1-git-send-email-yan@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/209648/
State Rejected
Headers show

Comments

Guo Chao - Jan. 5, 2013, 7:42 a.m.
In ext4_dx_csum_verify(), if we detect corrupted data,
we do not compare checksum because checksum itself may
be wrong, but we should report error in this case.

Cc: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 fs/ext4/namei.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Darrick J. Wong - Jan. 5, 2013, 7:50 p.m.
On Sat, Jan 05, 2013 at 03:42:59PM +0800, Guo Chao wrote:
> In ext4_dx_csum_verify(), if we detect corrupted data,
> we do not compare checksum because checksum itself may
> be wrong, but we should report error in this case.
> 
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> ---
>  fs/ext4/namei.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index cac4482..843e29f 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -370,14 +370,14 @@ static int ext4_dx_csum_verify(struct inode *inode,
>  	c = get_dx_countlimit(inode, dirent, &count_offset);
>  	if (!c) {
>  		EXT4_ERROR_INODE(inode, "dir seems corrupt?  Run e2fsck -D.");
> -		return 1;
> +		return 0;
>  	}
>  	limit = le16_to_cpu(c->limit);
>  	count = le16_to_cpu(c->count);
>  	if (count_offset + (limit * sizeof(struct dx_entry)) >
>  	    EXT4_BLOCK_SIZE(inode->i_sb) - sizeof(struct dx_tail)) {
>  		warn_no_space_for_csum(inode);
> -		return 1;
> +		return 0;

In both of these cases we cannot figure out where the dx block checksum lives,
and therefore we have no stored checksum to compare against.  This can result
from enabling checksums on a existing filesystem and ignoring tune2fs' request
to run fsck -D to rebuild dx blocks that are completely full.  However, since
we haven't a checksum that we could use to decide if there's real corruption,
there's no cause to return -EIO to the user.  Therefore, we print a warning and
trust the sanity checks to catch totally bogus blocks, which is the best we can
hope for.

Sorry, but this doesn't seem necessary.

--D
>  	}
>  	t = (struct dx_tail *)(((struct dx_entry *)c) + limit);
>  
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guo Chao - Jan. 6, 2013, 2:37 a.m.
Hello, Darrick,

On Sat, Jan 05, 2013 at 11:50:54AM -0800, Darrick J. Wong wrote:
> On Sat, Jan 05, 2013 at 03:42:59PM +0800, Guo Chao wrote:
> > In ext4_dx_csum_verify(), if we detect corrupted data,
> > we do not compare checksum because checksum itself may
> > be wrong, but we should report error in this case.
> > 
> > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> > ---
> >  fs/ext4/namei.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> > index cac4482..843e29f 100644
> > --- a/fs/ext4/namei.c
> > +++ b/fs/ext4/namei.c
> > @@ -370,14 +370,14 @@ static int ext4_dx_csum_verify(struct inode *inode,
> >  	c = get_dx_countlimit(inode, dirent, &count_offset);
> >  	if (!c) {
> >  		EXT4_ERROR_INODE(inode, "dir seems corrupt?  Run e2fsck -D.");
> > -		return 1;
> > +		return 0;
> >  	}
> >  	limit = le16_to_cpu(c->limit);
> >  	count = le16_to_cpu(c->count);
> >  	if (count_offset + (limit * sizeof(struct dx_entry)) >
> >  	    EXT4_BLOCK_SIZE(inode->i_sb) - sizeof(struct dx_tail)) {
> >  		warn_no_space_for_csum(inode);
> > -		return 1;
> > +		return 0;
> 
> In both of these cases we cannot figure out where the dx block checksum lives,
> and therefore we have no stored checksum to compare against.  This can result
> from enabling checksums on a existing filesystem and ignoring tune2fs' request
> to run fsck -D to rebuild dx blocks that are completely full.  However, since
> we haven't a checksum that we could use to decide if there's real corruption,
> there's no cause to return -EIO to the user.  Therefore, we print a warning and
> trust the sanity checks to catch totally bogus blocks, which is the best we can
> hope for.
> 
> Sorry, but this doesn't seem necessary.

Thanks for the explaination. 

I think ext4_dirent_csum_verify() can encounter similar problem but return
error. But I'm not sure it's the same case.

Thanks,
Guo Chao

> --D
> >  	}
> >  	t = (struct dx_tail *)(((struct dx_entry *)c) + limit);
> >  
> > -- 
> > 1.7.9.5
> > 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cac4482..843e29f 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -370,14 +370,14 @@  static int ext4_dx_csum_verify(struct inode *inode,
 	c = get_dx_countlimit(inode, dirent, &count_offset);
 	if (!c) {
 		EXT4_ERROR_INODE(inode, "dir seems corrupt?  Run e2fsck -D.");
-		return 1;
+		return 0;
 	}
 	limit = le16_to_cpu(c->limit);
 	count = le16_to_cpu(c->count);
 	if (count_offset + (limit * sizeof(struct dx_entry)) >
 	    EXT4_BLOCK_SIZE(inode->i_sb) - sizeof(struct dx_tail)) {
 		warn_no_space_for_csum(inode);
-		return 1;
+		return 0;
 	}
 	t = (struct dx_tail *)(((struct dx_entry *)c) + limit);