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

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

[Guo Chao]

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;
 	}
 	t = (struct dx_tail *)(((struct dx_entry *)c) + limit);
 
-- 
1.7.9.5

[PATCH 2/3] ext4: release buffer in failed path in dx_probe()

[Guo Chao]

If checksum fails, we should also release the buffer
read from previous iteration.

Cc: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 fs/ext4/namei.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 843e29f..e249a47 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -722,7 +722,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
 			ext4_warning(dir->i_sb, "Node failed checksum");
 			brelse(bh);
 			*err = ERR_BAD_DX_DIR;
-			goto fail;
+			goto fail2;
 		}
 		set_buffer_verified(bh);
 
-- 
1.7.9.5

Re: [PATCH 2/3] ext4: release buffer in failed path in dx_probe()

[Darrick J. Wong]

On Sat, Jan 05, 2013 at 03:43:00PM +0800, Guo Chao wrote:
> If checksum fails, we should also release the buffer
> read from previous iteration.
> 
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
> ---
>  fs/ext4/namei.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 843e29f..e249a47 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -722,7 +722,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
>  			ext4_warning(dir->i_sb, "Node failed checksum");
>  			brelse(bh);
>  			*err = ERR_BAD_DX_DIR;
> -			goto fail;
> +			goto fail2;

Good catch!

Acked-by: Darrick J. Wong <darrick.wong@oracle.com>

--D
>  		}
>  		set_buffer_verified(bh);
>  
> -- 
> 1.7.9.5
> 

Re: [PATCH 2/3] ext4: release buffer in failed path in dx_probe()

[Theodore Ts'o]

On Sat, Jan 05, 2013 at 11:41:35AM -0800, Darrick J. Wong wrote:
> On Sat, Jan 05, 2013 at 03:43:00PM +0800, Guo Chao wrote:
> > If checksum fails, we should also release the buffer
> > read from previous iteration.
> > 
> > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>

Thanks, applied.

> Acked-by: Darrick J. Wong <darrick.wong@oracle.com>

BTW, technically speaking it should be "Reviewed-by"; that's what I've
used in the commit.

"Acked-by:" gets used when a commit touches multiple subsystem trees.
For example, suppose someone changes a function in fs/direct-io.c,
which also required changes in btrfs, xfs, and ext4.  This commit
might go through Al Viro's vfs tree, and might have an Acked-by from
Chris Mason, Dave Chinner, and me, indicating that developers who are
well versed in those trees have agreed that the patches which touched
those other subsystems looked sane.

Since in this case the commit only touches ext4 code, and is going
through the ext4 tree, "Reviewed-by" is what normally gets used.

Cheers,

					- Ted

[PATCH 3/3] ext4: remove duplicate assignment in ext4_init_new_dir()

[Guo Chao]

It should be a typo unless I miss something.

Cc: Tao Ma <boyu.mt@taobao.com>
Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
---
 fs/ext4/namei.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e249a47..db3b1e9 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2368,7 +2368,6 @@ static int ext4_init_new_dir(handle_t *handle, struct inode *dir,
 	}
 
 	inode->i_size = EXT4_I(inode)->i_disksize = blocksize;
-	dir_block = ext4_bread(handle, inode, 0, 1, &err);
 	if (!(dir_block = ext4_bread(handle, inode, 0, 1, &err))) {
 		if (!err) {
 			err = -EIO;
-- 
1.7.9.5

Re: [PATCH 3/3] ext4: remove duplicate assignment in ext4_init_new_dir()

[Tao Ma]

On 01/05/2013 03:43 PM, Guo Chao wrote:
> It should be a typo unless I miss something.
> 
> Cc: Tao Ma <boyu.mt@taobao.com>
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>
oh, you are right. Thanks for the fix. And I really can't recall how I
messed this up.

Acked-by: Tao Ma <boyu.mt@taobao.com>
> ---
>  fs/ext4/namei.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e249a47..db3b1e9 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2368,7 +2368,6 @@ static int ext4_init_new_dir(handle_t *handle, struct inode *dir,
>  	}
>  
>  	inode->i_size = EXT4_I(inode)->i_disksize = blocksize;
> -	dir_block = ext4_bread(handle, inode, 0, 1, &err);
>  	if (!(dir_block = ext4_bread(handle, inode, 0, 1, &err))) {
>  		if (!err) {
>  			err = -EIO;
> 

Re: [PATCH 3/3] ext4: remove duplicate assignment in ext4_init_new_dir()

[Theodore Ts'o]

On Sat, Jan 05, 2013 at 03:43:01PM +0800, Guo Chao wrote:
> It should be a typo unless I miss something.
> 
> Cc: Tao Ma <boyu.mt@taobao.com>
> Signed-off-by: Guo Chao <yan@linux.vnet.ibm.com>

Thanks, applied.  Note that this fixes a buffer cache leak when
creating a directory using the mkdir system call.

	   	     	       	     - Ted

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

[Darrick J. Wong]

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
> 

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

[Guo Chao]

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
> >