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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 19 May 2016 21:40:04 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	"Darrick J. Wong" <darrick.wong@...cle.com>
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 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.

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

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






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)