linux-ext4 - Re: [PATCH] ext4: correct error value of function verifying dx checksum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160520035704.GB4599@birch.djwong.org>
Date:	Thu, 19 May 2016 20:57:04 -0700
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	Daeho Jeong <daeho.jeong@...sung.com>, tytso@....edu,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: correct error value of function verifying dx
 checksum

On Thu, May 19, 2016 at 09:40:04PM -0600, Andreas Dilger wrote:
> On May 19, 2016, at 7:51 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> > 
> > On Fri, May 20, 2016 at 08:54:56AM +0900, Daeho Jeong wrote:
> >> ext4_dx_csum_verify() returns the success return value in two checksum
> >> verification failure cases. We need to set the return values to zero
> >> as failure like ext4_dirent_csum_verify() returning zero when failing
> >> to find a checksum dirent at the tail.
> 
> It would be useful to add a comment block to this function that describes
> the return values.  Clearly, if the author didn't get the return values
> correct, it seems likely that someone else may be confused in the future.
> The function itself isn't named clearly enough to know whether the return
> of "1" or "0" should be considered an error.  If it were named something
> like "ext4_dx_csum_valid()" then clearly "1" would mean it is valid and
> "0" would mean it is invalid.

<shrug> verify->valid would make the name clearer; maybe the return type
ought to be bool too.

(That said, I think I got them right; it's just my evaluation of what
counts as a soft error and what counts as a hard-error-shut-it-down have
shfited over five years.)

> > ISTR deciding back in 2011 that "can't find the checksums" wasn't a hard enough
> > error to warrant shutting down the FS.  Though, being unable to find the limit
> > and count fields of a dx node /is/ bad enough, I think.
> > 
> > 2016 me is more paranoid about soft errors, so:
> > Reviewed-by: Darrick J. Wong <darrick.wong@...cle.com>
> 
> My recollection is that there are some cases where adding a checksum to an
> existing directory that didn't have enough space for the tail would leave the
> directory with no checksum?  What does e2fsck do in this case when adding
> checksums to an existing directory?  Skip the tail or split the block?

It rebuilds the entire directory. :)

--D

> 
> Cheers, Andreas
> 
> >> Signed-off-by: Daeho Jeong <daeho.jeong@...sung.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 48e4b89..ec811bb 100644
> >> --- a/fs/ext4/namei.c
> >> +++ b/fs/ext4/namei.c
> >> @@ -446,14 +446,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
> >> 
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> >> the body of a message to majordomo@...r.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


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