linux-ext4 - Re: [PATCH v3 5/7] ext2fs: add EXT4_FEATURE_INCOMPAT

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:   Wed, 7 Mar 2018 22:50:09 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Artem Blagodarenko <artem.blagodarenko@...il.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca
Subject: Re: [PATCH v3 5/7] ext2fs: add EXT4_FEATURE_INCOMPAT_64INODE support

On Mar 6, 2018, at 8:18 AM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
> 
> Inodes count and free inodes count should be 64 bit long.

You might consider splitting this into a "add wrappers for inode access"
patch and a separate "change s_inodes_* to 64-bit" patch, like the ext4
patches that went in upstream, to simplify inspection.

> @@ -128,7 +129,7 @@ ext2_ino_t string_to_inode(char *str)
> 		com_err(str, retval, 0);
> 		return 0;
> 	}
> -	if (ino > current_fs->super->s_inodes_count) {
> +	if (ino > ext2fs_get_inodes_count(current_fs->super)) {
> 		com_err(str, 0, "resolves to an illegal inode number: %u\n",
> 			ino);

Should this be "%llu"?

> @@ -433,8 +433,9 @@ static void check_if_skip(e2fsck_t ctx)
> 	/* Print the summary message when we're skipping a full check */
> 	log_out(ctx, _("%s: clean, %u/%u files, %llu/%llu blocks"),
> 		ctx->device_name,
> -		fs->super->s_inodes_count - fs->super->s_free_inodes_count,
> -		fs->super->s_inodes_count,
> +		ext2fs_get_inodes_count(fs->super) -
> +		ext2fs_get_free_inodes_count(fs->super),
> +		ext2fs_get_inodes_count(fs->super),
> 		ext2fs_blocks_count(fs->super) -
> 		ext2fs_free_blocks_count(fs->super),
> 		ext2fs_blocks_count(fs->super));


Also "%llu/%llu"?

> diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> index 1a5b2ebc..9ed1c193 100644
> --- a/e2fsck/pass2.c
> +++ b/e2fsck/pass2.c
> @@ -411,8 +411,15 @@ int e2fsck_check_dirent_data(e2fsck_t ctx, struct ext2_dir_entry *de,
> 		if ((de->name_len >> 8) & ~EXT2_FT_MASK) {
> 			/* clear dirent extra data flags. */
> 			if (fix_problem(ctx, PR_2_CLEAR_DIRDATA, pctx)) {
> -				de->name_len &= (EXT2_FT_MASK << 8) |
> +				if ((de->name_len >> 8) & EXT2_DIRENT_INODE &&
> +				    ctx->fs->super->s_feature_incompat &
> +				    EXT4_FEATURE_INCOMPAT_INODE64) {
> +					ctx->fs->super->s_feature_incompat |=
> +						EXT4_FEATURE_INCOMPAT_DIRDATA;

Isn't it pretty much a given that INCOMPAT_INODE64 requires INCOMPAT_DIRDATA?
That could be checked early on in e2fsck, rather than waiting until finding
a dirent that has the high bits set.

> diff --git a/lib/e2p/ls.c b/lib/e2p/ls.c
> index a7586e09..91a3b0ae 100644
> --- a/lib/e2p/ls.c
> +++ b/lib/e2p/ls.c
> @@ -269,14 +269,17 @@ void list_super2(struct ext2_super_block * sb, FILE *f)
> 	str = e2p_os2string(sb->s_creator_os);
> 	fprintf(f, "Filesystem OS type:       %s\n", str);
> 	free(str);
> -	fprintf(f, "Inode count:              %u\n", sb->s_inodes_count);
> +	fprintf(f, "Inode count:              %u\n",
> +			ext2fs_get_inodes_count(sb));

Should this be "%llu"?

> 	fprintf(f, "Block count:              %llu\n", e2p_blocks_count(sb));
> 	fprintf(f, "Reserved block count:     %llu\n", e2p_r_blocks_count(sb));
> 	if (sb->s_overhead_blocks)
> 		fprintf(f, "Overhead blocks:          %u\n",
> 			sb->s_overhead_blocks);
> -	fprintf(f, "Free blocks:              %llu\n", e2p_free_blocks_count(sb));
> -	fprintf(f, "Free inodes:              %u\n", sb->s_free_inodes_count);
> +	fprintf(f, "Free blocks:              %llu\n",
> +			e2p_free_blocks_count(sb));
> +	fprintf(f, "Free inodes:              %u\n",
> +		ext2fs_get_free_inodes_count(sb));

Similarly, should this be "%lu" or "%llu"?  While the kernel has a limit
of "unsigned long" for inode numbers, it isn't clear whether the same
limit should exist in userspace?  In any case, "%u" is for unsigned int,
which is typically a 32-bit value, and we need at least "%lu" for
64-bit inode numbers.

I suspect that there is a lot more code that needs to be changed to allow
64-bit inode numbers everywhere, since any code using "ext2_ino_t" is
not going to be working correctly if it finds a 64-bit inode.

Cheers, Andreas






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