diff mbox

[11/18] libext2/fsck: correctly preserve fs flags when modifying ignore-csum-error flag

Message ID 20140726003447.28334.51963.stgit@birch.djwong.org
State Superseded, archived
Headers show

Commit Message

Darrick Wong July 26, 2014, 12:34 a.m. UTC 
When we need to modify the "ignore checksum error" behavior flag to
get us past a library call, it's possible that the library call can
result in other flag bits being changed.  Therefore, it is not correct
to restore unconditionally the previous flags value, since this will
have unintended side effects on the other fs->flags; nor is it correct
to assume that we can unconditionally set (or clear) the "ignore csum
error" flag bit.  Therefore, we must merge the previous value of the
"ignore csum error" flag with the value of flags after the call.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 e2fsck/pass2.c     |   14 ++++++++++----
 e2fsck/pass3.c     |   11 ++++++++---
 e2fsck/rehash.c    |    4 +++-
 e2fsck/util.c      |    5 ++++-
 lib/ext2fs/inode.c |    3 ++-
 5 files changed, 27 insertions(+), 10 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Theodore Ts'o July 27, 2014, 11:27 p.m. UTC | #1 
On Fri, Jul 25, 2014 at 05:34:47PM -0700, Darrick J. Wong wrote:
> nor is it correct
> to assume that we can unconditionally set (or clear) the "ignore csum
> error" flag bit.

For functions inside e2fsck, why is this true?  All of the places
where we are are EXT2_FLAG_IGNORE_CSUM_ERRORS are places where we set
it, and then clear it after an ext2 call.  So as near as I can tell it
shouldn't matter.

I can understand why we need to be careful for functions inside
libext2fs, but in that particular case, none of the downstream
functions of ext2fs_read_inode_full() modify fs->flags.

So I'm really puzzled what problem this patch actually solves.  Was
this a theoretical concern, or was there something I missed?

       		   	       	   	 - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick Wong July 28, 2014, 8:06 a.m. UTC | #2 
On Sun, Jul 27, 2014 at 07:27:46PM -0400, Theodore Ts'o wrote:
> On Fri, Jul 25, 2014 at 05:34:47PM -0700, Darrick J. Wong wrote:
> > nor is it correct
> > to assume that we can unconditionally set (or clear) the "ignore csum
> > error" flag bit.
> 
> For functions inside e2fsck, why is this true?  All of the places
> where we are are EXT2_FLAG_IGNORE_CSUM_ERRORS are places where we set
> it, and then clear it after an ext2 call.  So as near as I can tell it
> shouldn't matter.
> 
> I can understand why we need to be careful for functions inside
> libext2fs, but in that particular case, none of the downstream
> functions of ext2fs_read_inode_full() modify fs->flags.
> 
> So I'm really puzzled what problem this patch actually solves.  Was
> this a theoretical concern, or was there something I missed?

It's mostly a theoretical concern -- rather than having to decide where it's ok
to set the flag and unset it later vs. where we have to set the flag and then
restore it to the previous value, I'd rather just be careful every time, so
that the next time I read through this code I'll know that IGNORE_CSUM_ERRORS
is always put back to its previous value and the other flags are left alone, no
matter how much e2fsck or libext2fs might get refactored.

One of those sites where we modify IGNORE_CSUM_ERRORS actually would
incorrectly clobber the other flag bits that were set inside the library (I
think it's the write_dir_block4 in pass2.c), so I decided to fix all of the
sites.

--D
> 
>        		   	       	   	 - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
index 5e31343..9394a29 100644
--- a/e2fsck/pass2.c
+++ b/e2fsck/pass2.c
@@ -1306,6 +1306,7 @@  skip_checksum:
 		}
 	}
 	if (dir_modified) {
+		int	flags, will_rehash;
 		/* leaf block with no tail?  Rehash dirs later. */
 		if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
 				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
@@ -1318,8 +1319,11 @@  skip_checksum:
 		}
 
 write_and_fix:
-		if (e2fsck_dir_will_be_rehashed(ctx, ino))
+		will_rehash = e2fsck_dir_will_be_rehashed(ctx, ino);
+		if (will_rehash) {
+			flags = ctx->fs->flags;
 			ctx->fs->flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
+		}
 		if (inline_data_size) {
 			cd->pctx.errcode =
 				ext2fs_inline_data_set(fs, ino, 0, buf,
@@ -1327,8 +1331,11 @@  write_and_fix:
 		} else
 			cd->pctx.errcode = ext2fs_write_dir_block4(fs, block_nr,
 								   buf, 0, ino);
-		if (e2fsck_dir_will_be_rehashed(ctx, ino))
-			ctx->fs->flags &= ~EXT2_FLAG_IGNORE_CSUM_ERRORS;
+		if (will_rehash)
+			ctx->fs->flags = (flags &
+					  EXT2_FLAG_IGNORE_CSUM_ERRORS) |
+					 (ctx->fs->flags &
+					  ~EXT2_FLAG_IGNORE_CSUM_ERRORS);
 		if (cd->pctx.errcode) {
 			if (!fix_problem(ctx, PR_2_WRITE_DIRBLOCK,
 					 &cd->pctx))
@@ -1604,7 +1611,6 @@  int e2fsck_process_bad_inode(e2fsck_t ctx, ext2_ino_t dir,
 	return 0;
 }
 
-
 /*
  * allocate_dir_block --- this function allocates a new directory
  * 	block for a particular inode; this is done if a directory has
diff --git a/e2fsck/pass3.c b/e2fsck/pass3.c
index 1ec4015..9eb859e 100644
--- a/e2fsck/pass3.c
+++ b/e2fsck/pass3.c
@@ -687,6 +687,7 @@  static void fix_dotdot(e2fsck_t ctx, ext2_ino_t ino, ext2_ino_t parent)
 	errcode_t	retval;
 	struct fix_dotdot_struct fp;
 	struct problem_context pctx;
+	int		flags, will_rehash;
 
 	fp.fs = fs;
 	fp.parent = parent;
@@ -699,12 +700,16 @@  static void fix_dotdot(e2fsck_t ctx, ext2_ino_t ino, ext2_ino_t parent)
 
 	clear_problem_context(&pctx);
 	pctx.ino = ino;
-	if (e2fsck_dir_will_be_rehashed(ctx, ino))
+	will_rehash = e2fsck_dir_will_be_rehashed(ctx, ino);
+	if (will_rehash) {
+		flags = ctx->fs->flags;
 		ctx->fs->flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
+	}
 	retval = ext2fs_dir_iterate(fs, ino, DIRENT_FLAG_INCLUDE_EMPTY,
 				    0, fix_dotdot_proc, &fp);
-	if (e2fsck_dir_will_be_rehashed(ctx, ino))
-		ctx->fs->flags &= ~EXT2_FLAG_IGNORE_CSUM_ERRORS;
+	if (will_rehash)
+		ctx->fs->flags = (flags & EXT2_FLAG_IGNORE_CSUM_ERRORS) |
+			(ctx->fs->flags & ~EXT2_FLAG_IGNORE_CSUM_ERRORS);
 	if (retval || !fp.done) {
 		pctx.errcode = retval;
 		fix_problem(ctx, retval ? PR_3_FIX_PARENT_ERR :
diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
index 5913ae7..6d18348 100644
--- a/e2fsck/rehash.c
+++ b/e2fsck/rehash.c
@@ -126,10 +126,12 @@  static int fill_dir_block(ext2_filsys fs,
 		dirent = (struct ext2_dir_entry *) dir;
 		(void) ext2fs_set_rec_len(fs, fs->blocksize, dirent);
 	} else {
+		int flags = fs->flags;
 		fs->flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
 		fd->err = ext2fs_read_dir_block4(fs, *block_nr, dir, 0,
 						 fd->dir);
-		fs->flags &= ~EXT2_FLAG_IGNORE_CSUM_ERRORS;
+		fs->flags = (flags & EXT2_FLAG_IGNORE_CSUM_ERRORS) |
+			    (fs->flags & ~EXT2_FLAG_IGNORE_CSUM_ERRORS);
 		if (fd->err)
 			return BLOCK_ABORT;
 	}
diff --git a/e2fsck/util.c b/e2fsck/util.c
index e36e902..8237328 100644
--- a/e2fsck/util.c
+++ b/e2fsck/util.c
@@ -267,6 +267,7 @@  void e2fsck_read_bitmaps(e2fsck_t ctx)
 	errcode_t	retval;
 	const char	*old_op;
 	unsigned int	save_type;
+	int		flags;
 
 	if (ctx->invalid_bitmaps) {
 		com_err(ctx->program_name, 0,
@@ -278,9 +279,11 @@  void e2fsck_read_bitmaps(e2fsck_t ctx)
 	old_op = ehandler_operation(_("reading inode and block bitmaps"));
 	e2fsck_set_bitmap_type(fs, EXT2FS_BMAP64_RBTREE, "fs_bitmaps",
 			       &save_type);
+	flags = ctx->fs->flags;
 	ctx->fs->flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
 	retval = ext2fs_read_bitmaps(fs);
-	ctx->fs->flags &= ~EXT2_FLAG_IGNORE_CSUM_ERRORS;
+	ctx->fs->flags = (flags & EXT2_FLAG_IGNORE_CSUM_ERRORS) |
+			 (ctx->fs->flags & ~EXT2_FLAG_IGNORE_CSUM_ERRORS);
 	fs->default_bitmap_type = save_type;
 	ehandler_operation(old_op);
 	if (retval) {
diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index 0cea9f0..ca97ab8 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -717,7 +717,8 @@  errcode_t ext2fs_write_inode_full(ext2_filsys fs, ext2_ino_t ino,
 		retval = ext2fs_read_inode_full(fs, ino,
 						(struct ext2_inode *)w_inode,
 						length);
-		fs->flags = old_flags;
+		fs->flags = (old_flags & EXT2_FLAG_IGNORE_CSUM_ERRORS) |
+			    (fs->flags & ~EXT2_FLAG_IGNORE_CSUM_ERRORS);
 		if (retval)
 			goto errout;
 	}