From: Tahsin Erdogan Subject: [PATCH 03/12] e2fsck: ea_inode hash validation Date: Mon, 26 Jun 2017 06:43:39 -0700 Message-ID: <20170626134348.1240-3-tahsin@google.com> References: <20170626134348.1240-1-tahsin@google.com> Cc: Tahsin Erdogan To: Andreas Dilger , "Darrick J . Wong" , Theodore Ts'o , linux-ext4@vger.kernel.org Return-path: Received: from mail-pf0-f170.google.com ([209.85.192.170]:34842 "EHLO mail-pf0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752343AbdFZNn6 (ORCPT ); Mon, 26 Jun 2017 09:43:58 -0400 Received: by mail-pf0-f170.google.com with SMTP id c73so739343pfk.2 for ; Mon, 26 Jun 2017 06:43:58 -0700 (PDT) In-Reply-To: <20170626134348.1240-1-tahsin@google.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: In original implementation of ea_inode feature, each xattr inode had a single parent. Child inode tracked the parent by storing its inode number in i_mtime field. Also, child's i_generation matched parent's i_generation. With deduplication support, xattr inodes can now be shared so a single backpointer is not sufficient to achieve strong binding. This is now replaced by hash validation. Signed-off-by: Tahsin Erdogan --- e2fsck/pass1.c | 91 +++++++++++++++++++++++++++++++++------------------ e2fsck/problem.c | 5 --- e2fsck/problem.h | 7 ++-- lib/ext2fs/ext2fs.h | 3 ++ lib/ext2fs/ext_attr.c | 67 +++++++++++++++++++++++++++++++++++-- 5 files changed, 128 insertions(+), 45 deletions(-) diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c index 1c68af8990b4..32152f3ec926 100644 --- a/e2fsck/pass1.c +++ b/e2fsck/pass1.c @@ -360,27 +360,54 @@ static problem_t check_large_ea_inode(e2fsck_t ctx, struct problem_context *pctx) { struct ext2_inode inode; + __u32 hash; + errcode_t retval; /* Check if inode is within valid range */ if ((entry->e_value_inum < EXT2_FIRST_INODE(ctx->fs->super)) || - (entry->e_value_inum > ctx->fs->super->s_inodes_count)) + (entry->e_value_inum > ctx->fs->super->s_inodes_count)) { + pctx->num = entry->e_value_inum; return PR_1_ATTR_VALUE_EA_INODE; + } e2fsck_read_inode(ctx, entry->e_value_inum, &inode, "pass1"); + + retval = ext2fs_ext_attr_hash_entry2(ctx->fs, entry, NULL, &hash); + if (retval) { + com_err("check_large_ea_inode", retval, + _("while hashing entry with e_value_inum = %u"), + entry->e_value_inum); + fatal_error(ctx, 0); + } + + if (hash != entry->e_hash) { + /* This might be an old Lustre-style ea_inode reference. */ + if (inode.i_mtime != pctx->ino || + inode.i_generation != pctx->inode->i_generation) { + + /* If target inode is also missing EA_INODE flag, + * this is likely to be a bad reference. + */ + if (!(inode.i_flags & EXT4_EA_INODE_FL)) { + pctx->num = entry->e_value_inum; + return PR_1_ATTR_VALUE_EA_INODE; + } else { + pctx->num = entry->e_hash; + return PR_1_ATTR_HASH; + } + } + } + if (!(inode.i_flags & EXT4_EA_INODE_FL)) { - /* If EXT4_EA_INODE_FL flag is not present but back-pointer - * matches then we should set this flag */ - if (inode.i_mtime == pctx->ino && - inode.i_generation == pctx->inode->i_generation && - fix_problem(ctx, PR_1_ATTR_SET_EA_INODE_FL, pctx)) { + pctx->num = entry->e_value_inum; + if (fix_problem(ctx, PR_1_ATTR_SET_EA_INODE_FL, pctx)) { inode.i_flags |= EXT4_EA_INODE_FL; - ext2fs_write_inode(ctx->fs, entry->e_value_inum,&inode); - } else + ext2fs_write_inode(ctx->fs, entry->e_value_inum, + &inode); + } else { return PR_1_ATTR_NO_EA_INODE_FL; - } else if (inode.i_mtime != pctx->ino || - inode.i_generation != pctx->inode->i_generation) - return PR_1_ATTR_INVAL_EA_INODE; - + } + } return 0; } @@ -458,6 +485,16 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx) problem = PR_1_INODE_EA_ALLOC_COLLISION; goto fix; } + + hash = ext2fs_ext_attr_hash_entry(entry, + start + entry->e_value_offs); + + /* e_hash may be 0 in older inode's ea */ + if (entry->e_hash != 0 && entry->e_hash != hash) { + pctx->num = entry->e_hash; + problem = PR_1_ATTR_HASH; + goto fix; + } } else { problem = check_large_ea_inode(ctx, entry, pctx); if (problem != 0) @@ -466,16 +503,6 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx) mark_inode_ea_map(ctx, pctx, entry->e_value_inum); } - hash = ext2fs_ext_attr_hash_entry(entry, - start + entry->e_value_offs); - - /* e_hash may be 0 in older inode's ea */ - if (entry->e_hash != 0 && entry->e_hash != hash) { - pctx->num = entry->e_hash; - problem = PR_1_ATTR_HASH; - goto fix; - } - /* If EA value is stored in external inode then it does not * consume space here */ if (entry->e_value_inum == 0) @@ -2439,6 +2466,16 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx, pctx)) goto clear_extattr; } + + hash = ext2fs_ext_attr_hash_entry(entry, block_buf + + entry->e_value_offs); + + if (entry->e_hash != hash) { + pctx->num = entry->e_hash; + if (fix_problem(ctx, PR_1_ATTR_HASH, pctx)) + goto clear_extattr; + entry->e_hash = hash; + } } else { problem_t problem; @@ -2450,16 +2487,6 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx, goto clear_extattr; } - hash = ext2fs_ext_attr_hash_entry(entry, block_buf + - entry->e_value_offs); - - if (entry->e_hash != hash) { - pctx->num = entry->e_hash; - if (fix_problem(ctx, PR_1_ATTR_HASH, pctx)) - goto clear_extattr; - entry->e_hash = hash; - } - entry = EXT2_EXT_ATTR_NEXT(entry); } if (region_allocate(region, (char *)entry - (char *)header, 4)) { diff --git a/e2fsck/problem.c b/e2fsck/problem.c index dcdf9abe1abd..deffa4d66a89 100644 --- a/e2fsck/problem.c +++ b/e2fsck/problem.c @@ -1150,11 +1150,6 @@ static struct e2fsck_problem problem_table[] = { N_("@i %i has @I @a value @i %N.\n"), PROMPT_CLEAR, PR_PREEN_OK }, - /* Invalid backpointer from extended attribute inode to parent inode */ - { PR_1_ATTR_INVAL_EA_INODE, - N_("@n backpointer from @a @i %N to parent @i %i.\n"), - PROMPT_CLEAR, PR_PREEN_OK }, - /* Inode has invalid extended attribute. EA inode missing * EA_INODE flag. */ { PR_1_ATTR_NO_EA_INODE_FL, diff --git a/e2fsck/problem.h b/e2fsck/problem.h index dad608c94fd0..8e07c10895c1 100644 --- a/e2fsck/problem.h +++ b/e2fsck/problem.h @@ -678,15 +678,12 @@ struct problem_context { /* Inode has illegal EA value inode */ #define PR_1_ATTR_VALUE_EA_INODE 0x010083 -/* Invalid backpointer from EA inode to parent inode */ -#define PR_1_ATTR_INVAL_EA_INODE 0x010084 - /* Parent inode has invalid EA entry. EA inode does not have * EXT4_EA_INODE_FL flag. Delete EA entry? */ -#define PR_1_ATTR_NO_EA_INODE_FL 0x010085 +#define PR_1_ATTR_NO_EA_INODE_FL 0x010084 /* EA inode for parent inode does not have EXT4_EA_INODE_FL flag */ -#define PR_1_ATTR_SET_EA_INODE_FL 0x010086 +#define PR_1_ATTR_SET_EA_INODE_FL 0x010085 /* diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h index f4131a640f9c..f72cbe17a8cd 100644 --- a/lib/ext2fs/ext2fs.h +++ b/lib/ext2fs/ext2fs.h @@ -1198,6 +1198,9 @@ extern errcode_t ext2fs_expand_dir(ext2_filsys fs, ext2_ino_t dir); /* ext_attr.c */ extern __u32 ext2fs_ext_attr_hash_entry(struct ext2_ext_attr_entry *entry, void *data); +extern errcode_t ext2fs_ext_attr_hash_entry2(ext2_filsys fs, + struct ext2_ext_attr_entry *entry, + void *data, __u32 *hash); extern errcode_t ext2fs_read_ext_attr(ext2_filsys fs, blk_t block, void *buf); extern errcode_t ext2fs_read_ext_attr2(ext2_filsys fs, blk64_t block, void *buf); diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c index 14d05c7e57fb..d48ab5c55270 100644 --- a/lib/ext2fs/ext_attr.c +++ b/lib/ext2fs/ext_attr.c @@ -25,6 +25,18 @@ #include "ext2fs.h" +static errcode_t read_ea_inode_hash(ext2_filsys fs, ext2_ino_t ino, __u32 *hash) +{ + struct ext2_inode inode; + errcode_t retval; + + retval = ext2fs_read_inode(fs, ino, &inode); + if (retval) + return retval; + *hash = inode.i_atime; + return 0; +} + #define NAME_HASH_SHIFT 5 #define VALUE_HASH_SHIFT 16 @@ -59,6 +71,35 @@ __u32 ext2fs_ext_attr_hash_entry(struct ext2_ext_attr_entry *entry, void *data) return hash; } +/* + * ext2fs_ext_attr_hash_entry2() + * + * Compute the hash of an extended attribute. + * This version of the function supports hashing entries that reference + * external inodes (ea_inode feature). + */ +errcode_t ext2fs_ext_attr_hash_entry2(ext2_filsys fs, + struct ext2_ext_attr_entry *entry, + void *data, __u32 *hash) +{ + *hash = ext2fs_ext_attr_hash_entry(entry, data); + + if (entry->e_value_inum) { + __u32 ea_inode_hash; + errcode_t retval; + + retval = read_ea_inode_hash(fs, entry->e_value_inum, + &ea_inode_hash); + if (retval) + return retval; + + *hash = (*hash << VALUE_HASH_SHIFT) ^ + (*hash >> (8*sizeof(*hash) - VALUE_HASH_SHIFT)) ^ + ea_inode_hash; + } + return 0; +} + static errcode_t check_ext_attr_header(struct ext2_ext_attr_header *header) { if ((header->h_magic != EXT2_EXT_ATTR_MAGIC_v1 && @@ -914,9 +955,29 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle, void *data = (entry->e_value_inum != 0) ? 0 : value_start + entry->e_value_offs; - hash = ext2fs_ext_attr_hash_entry(entry, data); - if (entry->e_hash != hash) - return EXT2_ET_BAD_EA_HASH; + err = ext2fs_ext_attr_hash_entry2(handle->fs, entry, + data, &hash); + if (err) + return err; + if (entry->e_hash != hash) { + struct ext2_inode parent, child; + + /* Check whether this is an old Lustre-style + * ea_inode reference. + */ + err = ext2fs_read_inode(handle->fs, handle->ino, + &parent); + if (err) + return err; + err = ext2fs_read_inode(handle->fs, + entry->e_value_inum, + &child); + if (err) + return err; + if (child.i_mtime != handle->ino || + child.i_generation != parent.i_generation) + return EXT2_ET_BAD_EA_HASH; + } } x++; -- 2.13.1.611.g7e3b11ae1-goog