diff mbox

[03/12] e2fsck: ea_inode hash validation

Message ID 20170626134348.1240-3-tahsin@google.com
State Superseded, archived
Headers show

Commit Message

Tahsin Erdogan June 26, 2017, 1:43 p.m. UTC 
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 <tahsin@google.com>
---
 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(-)

Comments

Andreas Dilger June 26, 2017, 9:21 p.m. UTC | #1 
On Jun 26, 2017, at 7:43 AM, Tahsin Erdogan <tahsin@google.com> wrote:
> 
> 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 <tahsin@google.com>
> ---
> 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
> @@ -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;

Shouldn't this be "if (!fix_problem(...))" ?


> 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

It would also be OK to keep the old values, and just skip 0x10084.

> 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
> @@ -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);

Rather than reading the inode again here, it would make more sense to pass the
parent inode from the caller.

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


Cheers, Andreas
diff mbox

Patch

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++;