linux-ext4 - Re: [RFC PATCH 2/8] e2fsck: add support for dirdata feature

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>] [day] [month] [year] [list]

Message-Id: <E5C7221F-6A40-4FF4-9CC2-476B035BB037@dilger.ca>
Date:   Fri, 27 Oct 2017 17:13:09 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Artem Blagodarenko <artem.blagodarenko@...il.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Alexey Lyashkov <alexey.lyashkov@...il.com>
Subject: Re: [RFC PATCH 2/8]  e2fsck: add support for dirdata feature

On Oct 27, 2017, at 11:22 AM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
> From: Andreas Dilger <andreas.dilger@...el.com>
> 
>    Add support for the INCOMPAT_DIRDATA feature, which allows
>    storing extra data in the directory entry beyond the name.
>    This allows the Lustre File IDentifier to be accessed in
>    an efficient manner, and would be useful for expanding a
>    filesystem to allow more than 2^32 inodes in the future.
> 
>      LU-1774 e2fsck: e2fsck -D does not change dirdata content
> 
>      Fix dir optimization to preserver dirdata content for dot
>      and dotdot entries.

(typo) s/preserver/preserve/

> +int e2fsck_check_dirent_data(e2fsck_t ctx, struct ext2_dir_entry *de,
> +			     unsigned int offset, struct problem_context *pctx)
> +{
> +	if (!(ctx->fs->super->s_feature_incompat &
> +			EXT4_FEATURE_INCOMPAT_DIRDATA)) {
> +		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) |
> +						EXT2_NAME_LEN;
> +				return 2;
> +			}
> +		}
> +		return 1;
> +	}
> +	if ((de->name_len >> 8) & ~EXT2_FT_MASK) {
> +		if (de->rec_len >= EXT2_DIR_REC_LEN(de) ||
> +		    de->rec_len + offset == EXT2_BLOCK_SIZE(ctx->fs->super)) {
> +			if (ext2_get_dirent_dirdata_size(de,
> +							 EXT2_DIRENT_LUFID) ==
> +			    EXT2_DIRENT_LUFID_SIZE)
> +				return 0;

One thing that we've been thinking about recently is to store multiple FIDs
in the dirent to allow metadata replication (i.e. the dirent refers to inodes
on two different servers).  It would be better to change this check to be:

			if (ext2_get_dirent_dirdata_size(de, EXT2_DIRENT_LUFID)%
			    EXT2_DIRENT_LUFID_SIZE == 1 /* size */ + 1 /* NUL */)

and change

#define EXT2_DIRENT_LUFID_SIZE 16

so that the dirdata size can be a multiple of 16, with 1 byte for the record
size. The current EXT2_DIRENT_LUFID_SIZE is defined as 17 + 1, which I think
is misleading.  The actual size stored on disk is 17 (16 + sizeof(lu_fid))
and the extra "+ 1" is added by ext2_get_dirent_dirdata_size(), but I don't
think the NUL after the name should be included in LUFID_SIZE.  That wasn't
important when there was only a single FID and a single dirdata entry, but
it makes sense to fix this when landing this patch upstream.

> +		}
> +		/* just clear dirent data flags for now, we should fix FID data
> +		 * in lustre specific pass.
> +		 */
> +		if (fix_problem(ctx, PR_2_CLEAR_DIRDATA, pctx)) {
> +			ext2_fix_dirent_dirdata(de);
> +			if (ext2_get_dirent_dirdata_size(de,
> +							 EXT2_DIRENT_LUFID) !=
> +			    EXT2_DIRENT_LUFID_SIZE)

> +				de->name_len &= ~(EXT2_DIRENT_LUFID << 8);
> +
> +			return 2;
> +		}
> +	}
> +	return 1;
> +}
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index 2bb8d4e2..1891756b 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -1671,6 +1671,11 @@ static struct e2fsck_problem problem_table[] = {
> 	  N_("Encrypted @E is too short.\n"),
> 	  PROMPT_CLEAR, 0 },
> 
> +	/* Directory entry dirdata length set incorrectly */
> +	{ PR_2_CLEAR_DIRDATA,
> +	  N_("@E dirdata length set incorrectly.\n"),
> +	  PROMPT_CLEAR, PR_PREEN_OK },
> +
> 	/* Pass 3 errors */
> 
> 	/* Pass 3: Checking directory connectivity */
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index 49a8c3ad..96862522 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -1004,6 +1004,9 @@ struct problem_context {
> /* Encrypted directory entry is too short */
> #define PR_2_BAD_ENCRYPTED_NAME		0x020050
> 
> +/* Directory dirdata flag set */

Please make this comment match the error message in problem.c:

/* Entry dirdata length set incorrectly */

> +#define PR_2_CLEAR_DIRDATA		0x02f000

This should use the next (or at least close) problem number,
otherwise it is prone to getting the entries in the wrong
numerical order, which will fail.  We only had such a large
value in the Lustre e2fsprogs tree to avoid conflicts from
other patches landing upstream.


Cheers, Andreas






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