linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling
@ 2020-12-16 10:18 Jan Kara
  2020-12-16 10:18 ` [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info() Jan Kara
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

Hello,

This is repost of the second half of the series originally posted here [1].
I'm reposting just this part because Ted has already picked up first 7 patches.

Changes since v1:
* Added missed bdev_read_only() check spotted by Harshad
* Added one more fix and one more cleanup for related problems noticed by
  Andreas

								Honza

[1] https://lore.kernel.org/linux-ext4/20201127113405.26867-1-jack@suse.cz/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info()
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 15:50   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 2/8] ext4: Drop sync argument of ext4_commit_super() Jan Kara
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

save_error_info() is always called together with ext4_handle_error().
Combine them into a single call and move unconditional bits out of
save_error_info() into ext4_handle_error().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 2d7dc0908cdd..72a21f0abe87 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -592,9 +592,6 @@ static void __save_error_info(struct super_block *sb, int error,
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
-	if (bdev_read_only(sb->s_bdev))
-		return;
 	/* We default to EFSCORRUPTED error... */
 	if (error == 0)
 		error = EFSCORRUPTED;
@@ -647,13 +644,19 @@ static void save_error_info(struct super_block *sb, int error,
  * used to deal with unrecoverable failures such as journal IO errors or ENOMEM
  * at a critical moment in log management.
  */
-static void ext4_handle_error(struct super_block *sb, bool force_ro)
+static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
+			      __u32 ino, __u64 block,
+			      const char *func, unsigned int line)
 {
 	journal_t *journal = EXT4_SB(sb)->s_journal;
 
+	EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
 	if (test_opt(sb, WARN_ON_ERROR))
 		WARN_ON_ONCE(1);
 
+	if (!bdev_read_only(sb->s_bdev))
+		save_error_info(sb, error, ino, block, func, line);
+
 	if (sb_rdonly(sb) || (!force_ro && test_opt(sb, ERRORS_CONT)))
 		return;
 
@@ -710,8 +713,7 @@ void __ext4_error(struct super_block *sb, const char *function,
 		       sb->s_id, function, line, current->comm, &vaf);
 		va_end(args);
 	}
-	save_error_info(sb, error, 0, block, function, line);
-	ext4_handle_error(sb, force_ro);
+	ext4_handle_error(sb, force_ro, error, 0, block, function, line);
 }
 
 void __ext4_error_inode(struct inode *inode, const char *function,
@@ -741,9 +743,8 @@ void __ext4_error_inode(struct inode *inode, const char *function,
 			       current->comm, &vaf);
 		va_end(args);
 	}
-	save_error_info(inode->i_sb, error, inode->i_ino, block,
-			function, line);
-	ext4_handle_error(inode->i_sb, false);
+	ext4_handle_error(inode->i_sb, false, error, inode->i_ino, block,
+			  function, line);
 }
 
 void __ext4_error_file(struct file *file, const char *function,
@@ -780,9 +781,8 @@ void __ext4_error_file(struct file *file, const char *function,
 			       current->comm, path, &vaf);
 		va_end(args);
 	}
-	save_error_info(inode->i_sb, EFSCORRUPTED, inode->i_ino, block,
-			function, line);
-	ext4_handle_error(inode->i_sb, false);
+	ext4_handle_error(inode->i_sb, false, EFSCORRUPTED, inode->i_ino, block,
+			  function, line);
 }
 
 const char *ext4_decode_error(struct super_block *sb, int errno,
@@ -849,8 +849,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
 		       sb->s_id, function, line, errstr);
 	}
 
-	save_error_info(sb, -errno, 0, 0, function, line);
-	ext4_handle_error(sb, false);
+	ext4_handle_error(sb, false, -errno, 0, 0, function, line);
 }
 
 void __ext4_msg(struct super_block *sb,
@@ -944,13 +943,14 @@ __acquires(bitlock)
 	if (test_opt(sb, ERRORS_CONT)) {
 		if (test_opt(sb, WARN_ON_ERROR))
 			WARN_ON_ONCE(1);
+		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
 		__save_error_info(sb, EFSCORRUPTED, ino, block, function, line);
-		schedule_work(&EXT4_SB(sb)->s_error_work);
+		if (!bdev_read_only(sb->s_bdev))
+			schedule_work(&EXT4_SB(sb)->s_error_work);
 		return;
 	}
 	ext4_unlock_group(sb, grp);
-	save_error_info(sb, EFSCORRUPTED, ino, block, function, line);
-	ext4_handle_error(sb, false);
+	ext4_handle_error(sb, false, EFSCORRUPTED, ino, block, function, line);
 	/*
 	 * We only get here in the ERRORS_RO case; relocking the group
 	 * may be dangerous, but nothing bad will happen since the
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/8] ext4: Drop sync argument of ext4_commit_super()
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
  2020-12-16 10:18 ` [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info() Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 15:51   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 3/8] ext4: Protect superblock modifications with a buffer lock Jan Kara
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

Everybody passes 1 as sync argument of ext4_commit_super(). Just drop
it.

Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 72a21f0abe87..67fff3104624 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -65,7 +65,7 @@ static struct ratelimit_state ext4_mount_msg_ratelimit;
 static int ext4_load_journal(struct super_block *, struct ext4_super_block *,
 			     unsigned long journal_devnum);
 static int ext4_show_options(struct seq_file *seq, struct dentry *root);
-static int ext4_commit_super(struct super_block *sb, int sync);
+static int ext4_commit_super(struct super_block *sb);
 static int ext4_mark_recovery_complete(struct super_block *sb,
 					struct ext4_super_block *es);
 static int ext4_clear_journal_err(struct super_block *sb,
@@ -621,7 +621,7 @@ static void save_error_info(struct super_block *sb, int error,
 {
 	__save_error_info(sb, error, ino, block, func, line);
 	if (!bdev_read_only(sb->s_bdev))
-		ext4_commit_super(sb, 1);
+		ext4_commit_super(sb);
 }
 
 /* Deal with the reporting of failure conditions on a filesystem such as
@@ -686,7 +686,7 @@ static void flush_stashed_error_work(struct work_struct *work)
 	struct ext4_sb_info *sbi = container_of(work, struct ext4_sb_info,
 						s_error_work);
 
-	ext4_commit_super(sbi->s_sb, 1);
+	ext4_commit_super(sbi->s_sb);
 }
 
 #define ext4_error_ratelimit(sb)					\
@@ -1152,7 +1152,7 @@ static void ext4_put_super(struct super_block *sb)
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
 	}
 	if (!sb_rdonly(sb))
-		ext4_commit_super(sb, 1);
+		ext4_commit_super(sb);
 
 	rcu_read_lock();
 	group_desc = rcu_dereference(sbi->s_group_desc);
@@ -2642,7 +2642,7 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
 	if (sbi->s_journal)
 		ext4_set_feature_journal_needs_recovery(sb);
 
-	err = ext4_commit_super(sb, 1);
+	err = ext4_commit_super(sb);
 done:
 	if (test_opt(sb, DEBUG))
 		printk(KERN_INFO "[EXT4 FS bs=%lu, gc=%u, "
@@ -4863,7 +4863,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (DUMMY_ENCRYPTION_ENABLED(sbi) && !sb_rdonly(sb) &&
 	    !ext4_has_feature_encrypt(sb)) {
 		ext4_set_feature_encrypt(sb);
-		ext4_commit_super(sb, 1);
+		ext4_commit_super(sb);
 	}
 
 	/*
@@ -5416,7 +5416,7 @@ static int ext4_load_journal(struct super_block *sb,
 		es->s_journal_dev = cpu_to_le32(journal_devnum);
 
 		/* Make sure we flush the recovery flag to disk. */
-		ext4_commit_super(sb, 1);
+		ext4_commit_super(sb);
 	}
 
 	return 0;
@@ -5426,7 +5426,7 @@ static int ext4_load_journal(struct super_block *sb,
 	return err;
 }
 
-static int ext4_commit_super(struct super_block *sb, int sync)
+static int ext4_commit_super(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
@@ -5503,8 +5503,7 @@ static int ext4_commit_super(struct super_block *sb, int sync)
 
 	BUFFER_TRACE(sbh, "marking dirty");
 	ext4_superblock_csum_set(sb);
-	if (sync)
-		lock_buffer(sbh);
+	lock_buffer(sbh);
 	if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) {
 		/*
 		 * Oh, dear.  A previous attempt to write the
@@ -5520,16 +5519,14 @@ static int ext4_commit_super(struct super_block *sb, int sync)
 		set_buffer_uptodate(sbh);
 	}
 	mark_buffer_dirty(sbh);
-	if (sync) {
-		unlock_buffer(sbh);
-		error = __sync_dirty_buffer(sbh,
-			REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0));
-		if (buffer_write_io_error(sbh)) {
-			ext4_msg(sb, KERN_ERR, "I/O error while writing "
-			       "superblock");
-			clear_buffer_write_io_error(sbh);
-			set_buffer_uptodate(sbh);
-		}
+	unlock_buffer(sbh);
+	error = __sync_dirty_buffer(sbh,
+		REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0));
+	if (buffer_write_io_error(sbh)) {
+		ext4_msg(sb, KERN_ERR, "I/O error while writing "
+		       "superblock");
+		clear_buffer_write_io_error(sbh);
+		set_buffer_uptodate(sbh);
 	}
 	return error;
 }
@@ -5560,7 +5557,7 @@ static int ext4_mark_recovery_complete(struct super_block *sb,
 
 	if (ext4_has_feature_journal_needs_recovery(sb) && sb_rdonly(sb)) {
 		ext4_clear_feature_journal_needs_recovery(sb);
-		ext4_commit_super(sb, 1);
+		ext4_commit_super(sb);
 	}
 out:
 	jbd2_journal_unlock_updates(journal);
@@ -5602,7 +5599,7 @@ static int ext4_clear_journal_err(struct super_block *sb,
 
 		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
-		ext4_commit_super(sb, 1);
+		ext4_commit_super(sb);
 
 		jbd2_journal_clear_err(journal);
 		jbd2_journal_update_sb_errno(journal);
@@ -5704,7 +5701,7 @@ static int ext4_freeze(struct super_block *sb)
 		ext4_clear_feature_journal_needs_recovery(sb);
 	}
 
-	error = ext4_commit_super(sb, 1);
+	error = ext4_commit_super(sb);
 out:
 	if (journal)
 		/* we rely on upper layer to stop further updates */
@@ -5726,7 +5723,7 @@ static int ext4_unfreeze(struct super_block *sb)
 		ext4_set_feature_journal_needs_recovery(sb);
 	}
 
-	ext4_commit_super(sb, 1);
+	ext4_commit_super(sb);
 	return 0;
 }
 
@@ -5986,7 +5983,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	}
 
 	if (sbi->s_journal == NULL && !(old_sb_flags & SB_RDONLY)) {
-		err = ext4_commit_super(sb, 1);
+		err = ext4_commit_super(sb);
 		if (err)
 			goto restore_opts;
 	}
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/8] ext4: Protect superblock modifications with a buffer lock
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
  2020-12-16 10:18 ` [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info() Jan Kara
  2020-12-16 10:18 ` [PATCH 2/8] ext4: Drop sync argument of ext4_commit_super() Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 15:56   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 4/8] ext4: Save error info to sb through journal if available Jan Kara
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

Protect all superblock modifications (including checksum computation)
with a superblock buffer lock. That way we are sure computed checksum
matches current superblock contents (a mismatch could cause checksum
failures in nojournal mode or if an unjournalled superblock update races
with a journalled one). Also we avoid modifying superblock contents
while it is being written out (which can cause DIF/DIX failures if we
are running in nojournal mode).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.c |  1 -
 fs/ext4/file.c      |  3 +++
 fs/ext4/inode.c     |  3 +++
 fs/ext4/namei.c     |  6 ++++++
 fs/ext4/resize.c    | 12 ++++++++++++
 fs/ext4/super.c     |  2 +-
 fs/ext4/xattr.c     |  3 +++
 7 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 1a0a827a7f34..c7e410c4ab7d 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -379,7 +379,6 @@ int __ext4_handle_dirty_super(const char *where, unsigned int line,
 	struct buffer_head *bh = EXT4_SB(sb)->s_sbh;
 	int err = 0;
 
-	ext4_superblock_csum_set(sb);
 	if (ext4_handle_valid(handle)) {
 		err = jbd2_journal_dirty_metadata(handle, bh);
 		if (err)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 3ed8c048fb12..26907d5835d0 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -809,8 +809,11 @@ static int ext4_sample_last_mounted(struct super_block *sb,
 	err = ext4_journal_get_write_access(handle, sbi->s_sbh);
 	if (err)
 		goto out_journal;
+	lock_buffer(sbi->s_sbh);
 	strlcpy(sbi->s_es->s_last_mounted, cp,
 		sizeof(sbi->s_es->s_last_mounted));
+	ext4_superblock_csum_set(sb);
+	unlock_buffer(sbi->s_sbh);
 	ext4_handle_dirty_super(handle, sb);
 out_journal:
 	ext4_journal_stop(handle);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3a39fa0d6a3a..72534319fae5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5141,7 +5141,10 @@ static int ext4_do_update_inode(handle_t *handle,
 		err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh);
 		if (err)
 			goto out_brelse;
+		lock_buffer(EXT4_SB(sb)->s_sbh);
 		ext4_set_feature_large_file(sb);
+		ext4_superblock_csum_set(sb);
+		unlock_buffer(EXT4_SB(sb)->s_sbh);
 		ext4_handle_sync(handle);
 		err = ext4_handle_dirty_super(handle, sb);
 	}
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 33509266f5a0..d804505e1a32 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2982,7 +2982,10 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode)
 	    (le32_to_cpu(sbi->s_es->s_inodes_count))) {
 		/* Insert this inode at the head of the on-disk orphan list */
 		NEXT_ORPHAN(inode) = le32_to_cpu(sbi->s_es->s_last_orphan);
+		lock_buffer(sbi->s_sbh);
 		sbi->s_es->s_last_orphan = cpu_to_le32(inode->i_ino);
+		ext4_superblock_csum_set(sb);
+		unlock_buffer(sbi->s_sbh);
 		dirty = true;
 	}
 	list_add(&EXT4_I(inode)->i_orphan, &sbi->s_orphan);
@@ -3065,7 +3068,10 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode)
 			mutex_unlock(&sbi->s_orphan_lock);
 			goto out_brelse;
 		}
+		lock_buffer(sbi->s_sbh);
 		sbi->s_es->s_last_orphan = cpu_to_le32(ino_next);
+		ext4_superblock_csum_set(inode->i_sb);
+		unlock_buffer(sbi->s_sbh);
 		mutex_unlock(&sbi->s_orphan_lock);
 		err = ext4_handle_dirty_super(handle, inode->i_sb);
 	} else {
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 928700d57eb6..6155f2b9538c 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -899,7 +899,10 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	EXT4_SB(sb)->s_gdb_count++;
 	ext4_kvfree_array_rcu(o_group_desc);
 
+	lock_buffer(EXT4_SB(sb)->s_sbh);
 	le16_add_cpu(&es->s_reserved_gdt_blocks, -1);
+	ext4_superblock_csum_set(sb);
+	unlock_buffer(EXT4_SB(sb)->s_sbh);
 	err = ext4_handle_dirty_super(handle, sb);
 	if (err)
 		ext4_std_error(sb, err);
@@ -1384,6 +1387,7 @@ static void ext4_update_super(struct super_block *sb,
 	reserved_blocks *= blocks_count;
 	do_div(reserved_blocks, 100);
 
+	lock_buffer(sbi->s_sbh);
 	ext4_blocks_count_set(es, ext4_blocks_count(es) + blocks_count);
 	ext4_free_blocks_count_set(es, ext4_free_blocks_count(es) + free_blocks);
 	le32_add_cpu(&es->s_inodes_count, EXT4_INODES_PER_GROUP(sb) *
@@ -1421,6 +1425,8 @@ static void ext4_update_super(struct super_block *sb,
 	 * active. */
 	ext4_r_blocks_count_set(es, ext4_r_blocks_count(es) +
 				reserved_blocks);
+	ext4_superblock_csum_set(sb);
+	unlock_buffer(sbi->s_sbh);
 
 	/* Update the free space counts */
 	percpu_counter_add(&sbi->s_freeclusters_counter,
@@ -1717,8 +1723,11 @@ static int ext4_group_extend_no_check(struct super_block *sb,
 		goto errout;
 	}
 
+	lock_buffer(EXT4_SB(sb)->s_sbh);
 	ext4_blocks_count_set(es, o_blocks_count + add);
 	ext4_free_blocks_count_set(es, ext4_free_blocks_count(es) + add);
+	ext4_superblock_csum_set(sb);
+	unlock_buffer(EXT4_SB(sb)->s_sbh);
 	ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
 	/* We add the blocks to the bitmap and set the group need init bit */
@@ -1874,10 +1883,13 @@ static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
 	if (err)
 		goto errout;
 
+	lock_buffer(sbi->s_sbh);
 	ext4_clear_feature_resize_inode(sb);
 	ext4_set_feature_meta_bg(sb);
 	sbi->s_es->s_first_meta_bg =
 		cpu_to_le32(num_desc_blocks(sb, sbi->s_groups_count));
+	ext4_superblock_csum_set(sb);
+	unlock_buffer(sbi->s_sbh);
 
 	err = ext4_handle_dirty_super(handle, sb);
 	if (err) {
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 67fff3104624..3d41714f3569 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5436,6 +5436,7 @@ static int ext4_commit_super(struct super_block *sb)
 	if (!sbh || block_device_ejected(sb))
 		return error;
 
+	lock_buffer(sbh);
 	/*
 	 * If the file system is mounted read-only, don't update the
 	 * superblock write time.  This avoids updating the superblock
@@ -5503,7 +5504,6 @@ static int ext4_commit_super(struct super_block *sb)
 
 	BUFFER_TRACE(sbh, "marking dirty");
 	ext4_superblock_csum_set(sb);
-	lock_buffer(sbh);
 	if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) {
 		/*
 		 * Oh, dear.  A previous attempt to write the
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 6127e94ea4f5..ab46aa447baa 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -792,7 +792,10 @@ static void ext4_xattr_update_super_block(handle_t *handle,
 
 	BUFFER_TRACE(EXT4_SB(sb)->s_sbh, "get_write_access");
 	if (ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh) == 0) {
+		lock_buffer(EXT4_SB(sb)->s_sbh);
 		ext4_set_feature_xattr(sb);
+		ext4_superblock_csum_set(sb);
+		unlock_buffer(EXT4_SB(sb)->s_sbh);
 		ext4_handle_dirty_super(handle, sb);
 	}
 }
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/8] ext4: Save error info to sb through journal if available
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
                   ` (2 preceding siblings ...)
  2020-12-16 10:18 ` [PATCH 3/8] ext4: Protect superblock modifications with a buffer lock Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 15:58   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 5/8] ext4: Use sbi instead of EXT4_SB(sb) in ext4_update_super() Jan Kara
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

If journalling is still working at the moment we get to writing error
information to the superblock we cannot write directly to the superblock
as such write could race with journalled update of the superblock and
cause journal checksum failures, writing inconsistent information to the
journal or other problems. We cannot journal the superblock directly
from the error handling functions as we are running in uncertain context
and could deadlock so just punt journalled superblock update to a
workqueue.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 101 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 75 insertions(+), 26 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3d41714f3569..3f6ea48e7b4b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -65,6 +65,7 @@ static struct ratelimit_state ext4_mount_msg_ratelimit;
 static int ext4_load_journal(struct super_block *, struct ext4_super_block *,
 			     unsigned long journal_devnum);
 static int ext4_show_options(struct seq_file *seq, struct dentry *root);
+static void ext4_update_super(struct super_block *sb);
 static int ext4_commit_super(struct super_block *sb);
 static int ext4_mark_recovery_complete(struct super_block *sb,
 					struct ext4_super_block *es);
@@ -586,9 +587,9 @@ static int ext4_errno_to_code(int errno)
 	return EXT4_ERR_UNKNOWN;
 }
 
-static void __save_error_info(struct super_block *sb, int error,
-			      __u32 ino, __u64 block,
-			      const char *func, unsigned int line)
+static void save_error_info(struct super_block *sb, int error,
+			    __u32 ino, __u64 block,
+			    const char *func, unsigned int line)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
@@ -615,15 +616,6 @@ static void __save_error_info(struct super_block *sb, int error,
 	spin_unlock(&sbi->s_error_lock);
 }
 
-static void save_error_info(struct super_block *sb, int error,
-			    __u32 ino, __u64 block,
-			    const char *func, unsigned int line)
-{
-	__save_error_info(sb, error, ino, block, func, line);
-	if (!bdev_read_only(sb->s_bdev))
-		ext4_commit_super(sb);
-}
-
 /* Deal with the reporting of failure conditions on a filesystem such as
  * inconsistencies detected or read IO failures.
  *
@@ -649,20 +641,35 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 			      const char *func, unsigned int line)
 {
 	journal_t *journal = EXT4_SB(sb)->s_journal;
+	bool continue_fs = !force_ro && test_opt(sb, ERRORS_CONT);
 
 	EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
 	if (test_opt(sb, WARN_ON_ERROR))
 		WARN_ON_ONCE(1);
 
-	if (!bdev_read_only(sb->s_bdev))
+	if (!continue_fs && !sb_rdonly(sb)) {
+		ext4_set_mount_flag(sb, EXT4_MF_FS_ABORTED);
+		if (journal)
+			jbd2_journal_abort(journal, -EIO);
+	}
+
+	if (!bdev_read_only(sb->s_bdev)) {
 		save_error_info(sb, error, ino, block, func, line);
+		/*
+		 * In case the fs should keep running, we need to writeout
+		 * superblock through the journal. Due to lock ordering
+		 * constraints, it may not be safe to do it right here so we
+		 * defer superblock flushing to a workqueue.
+		 */
+		if (continue_fs)
+			schedule_work(&EXT4_SB(sb)->s_error_work);
+		else
+			ext4_commit_super(sb);
+	}
 
-	if (sb_rdonly(sb) || (!force_ro && test_opt(sb, ERRORS_CONT)))
+	if (sb_rdonly(sb) || continue_fs)
 		return;
 
-	ext4_set_mount_flag(sb, EXT4_MF_FS_ABORTED);
-	if (journal)
-		jbd2_journal_abort(journal, -EIO);
 	/*
 	 * We force ERRORS_RO behavior when system is rebooting. Otherwise we
 	 * could panic during 'reboot -f' as the underlying device got already
@@ -685,7 +692,38 @@ static void flush_stashed_error_work(struct work_struct *work)
 {
 	struct ext4_sb_info *sbi = container_of(work, struct ext4_sb_info,
 						s_error_work);
+	journal_t *journal = sbi->s_journal;
+	handle_t *handle;
 
+	/*
+	 * If the journal is still running, we have to write out superblock
+	 * through the journal to avoid collisions of other journalled sb
+	 * updates.
+	 *
+	 * We use directly jbd2 functions here to avoid recursing back into
+	 * ext4 error handling code during handling of previous errors.
+	 */
+	if (!sb_rdonly(sbi->s_sb) && journal) {
+		handle = jbd2_journal_start(journal, 1);
+		if (IS_ERR(handle))
+			goto write_directly;
+		if (jbd2_journal_get_write_access(handle, sbi->s_sbh)) {
+			jbd2_journal_stop(handle);
+			goto write_directly;
+		}
+		ext4_update_super(sbi->s_sb);
+		if (jbd2_journal_dirty_metadata(handle, sbi->s_sbh)) {
+			jbd2_journal_stop(handle);
+			goto write_directly;
+		}
+		jbd2_journal_stop(handle);
+		return;
+	}
+write_directly:
+	/*
+	 * Write through journal failed. Write sb directly to get error info
+	 * out and hope for the best.
+	 */
 	ext4_commit_super(sbi->s_sb);
 }
 
@@ -944,9 +982,11 @@ __acquires(bitlock)
 		if (test_opt(sb, WARN_ON_ERROR))
 			WARN_ON_ONCE(1);
 		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
-		__save_error_info(sb, EFSCORRUPTED, ino, block, function, line);
-		if (!bdev_read_only(sb->s_bdev))
+		if (!bdev_read_only(sb->s_bdev)) {
+			save_error_info(sb, EFSCORRUPTED, ino, block, function,
+					line);
 			schedule_work(&EXT4_SB(sb)->s_error_work);
+		}
 		return;
 	}
 	ext4_unlock_group(sb, grp);
@@ -5426,15 +5466,12 @@ static int ext4_load_journal(struct super_block *sb,
 	return err;
 }
 
-static int ext4_commit_super(struct super_block *sb)
+/* Copy state of EXT4_SB(sb) into buffer for on-disk superblock */
+static void ext4_update_super(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
 	struct buffer_head *sbh = EXT4_SB(sb)->s_sbh;
-	int error = 0;
-
-	if (!sbh || block_device_ejected(sb))
-		return error;
 
 	lock_buffer(sbh);
 	/*
@@ -5502,8 +5539,20 @@ static int ext4_commit_super(struct super_block *sb)
 	}
 	spin_unlock(&sbi->s_error_lock);
 
-	BUFFER_TRACE(sbh, "marking dirty");
 	ext4_superblock_csum_set(sb);
+	unlock_buffer(sbh);
+}
+
+static int ext4_commit_super(struct super_block *sb)
+{
+	struct buffer_head *sbh = EXT4_SB(sb)->s_sbh;
+	int error = 0;
+
+	if (!sbh || block_device_ejected(sb))
+		return error;
+
+	ext4_update_super(sb);
+
 	if (buffer_write_io_error(sbh) || !buffer_uptodate(sbh)) {
 		/*
 		 * Oh, dear.  A previous attempt to write the
@@ -5518,8 +5567,8 @@ static int ext4_commit_super(struct super_block *sb)
 		clear_buffer_write_io_error(sbh);
 		set_buffer_uptodate(sbh);
 	}
+	BUFFER_TRACE(sbh, "marking dirty");
 	mark_buffer_dirty(sbh);
-	unlock_buffer(sbh);
 	error = __sync_dirty_buffer(sbh,
 		REQ_SYNC | (test_opt(sb, BARRIER) ? REQ_FUA : 0));
 	if (buffer_write_io_error(sbh)) {
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/8] ext4: Use sbi instead of EXT4_SB(sb) in ext4_update_super()
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
                   ` (3 preceding siblings ...)
  2020-12-16 10:18 ` [PATCH 4/8] ext4: Save error info to sb through journal if available Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 15:59   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 6/8] ext4: Fix deadlock with fs freezing and EA inodes Jan Kara
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

No behavioral change.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3f6ea48e7b4b..502ae491d07d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5470,8 +5470,8 @@ static int ext4_load_journal(struct super_block *sb,
 static void ext4_update_super(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
-	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
-	struct buffer_head *sbh = EXT4_SB(sb)->s_sbh;
+	struct ext4_super_block *es = sbi->s_es;
+	struct buffer_head *sbh = sbi->s_sbh;
 
 	lock_buffer(sbh);
 	/*
@@ -5488,21 +5488,20 @@ static void ext4_update_super(struct super_block *sb)
 		ext4_update_tstamp(es, s_wtime);
 	if (sb->s_bdev->bd_part)
 		es->s_kbytes_written =
-			cpu_to_le64(EXT4_SB(sb)->s_kbytes_written +
+			cpu_to_le64(sbi->s_kbytes_written +
 			    ((part_stat_read(sb->s_bdev->bd_part,
 					     sectors[STAT_WRITE]) -
-			      EXT4_SB(sb)->s_sectors_written_start) >> 1));
+			      sbi->s_sectors_written_start) >> 1));
 	else
-		es->s_kbytes_written =
-			cpu_to_le64(EXT4_SB(sb)->s_kbytes_written);
-	if (percpu_counter_initialized(&EXT4_SB(sb)->s_freeclusters_counter))
+		es->s_kbytes_written = cpu_to_le64(sbi->s_kbytes_written);
+	if (percpu_counter_initialized(&sbi->s_freeclusters_counter))
 		ext4_free_blocks_count_set(es,
-			EXT4_C2B(EXT4_SB(sb), percpu_counter_sum_positive(
-				&EXT4_SB(sb)->s_freeclusters_counter)));
-	if (percpu_counter_initialized(&EXT4_SB(sb)->s_freeinodes_counter))
+			EXT4_C2B(sbi, percpu_counter_sum_positive(
+				&sbi->s_freeclusters_counter)));
+	if (percpu_counter_initialized(&sbi->s_freeinodes_counter))
 		es->s_free_inodes_count =
 			cpu_to_le32(percpu_counter_sum_positive(
-				&EXT4_SB(sb)->s_freeinodes_counter));
+				&sbi->s_freeinodes_counter));
 	/* Copy error information to the on-disk superblock */
 	spin_lock(&sbi->s_error_lock);
 	if (sbi->s_add_error_count > 0) {
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/8] ext4: Fix deadlock with fs freezing and EA inodes
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
                   ` (4 preceding siblings ...)
  2020-12-16 10:18 ` [PATCH 5/8] ext4: Use sbi instead of EXT4_SB(sb) in ext4_update_super() Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 16:01   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 7/8] ext4: Fix superblock checksum failure when setting password salt Jan Kara
  2020-12-16 10:18 ` [PATCH 8/8] ext4: Drop ext4_handle_dirty_super() Jan Kara
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara, stable, Tahsin Erdogan

Xattr code using inodes with large xattr data can end up dropping last
inode reference (and thus deleting the inode) from places like
ext4_xattr_set_entry(). That function is called with transaction started
and so ext4_evict_inode() can deadlock against fs freezing like:

CPU1					CPU2

removexattr()				freeze_super()
  vfs_removexattr()
    ext4_xattr_set()
      handle = ext4_journal_start()
      ...
      ext4_xattr_set_entry()
        iput(old_ea_inode)
          ext4_evict_inode(old_ea_inode)
					  sb->s_writers.frozen = SB_FREEZE_FS;
					  sb_wait_write(sb, SB_FREEZE_FS);
					  ext4_freeze()
					    jbd2_journal_lock_updates()
					      -> blocks waiting for all
					         handles to stop
            sb_start_intwrite()
	      -> blocks as sb is already in SB_FREEZE_FS state

Generally it is advisable to delete inodes from a separate transaction
as it can consume quite some credits however in this case it would be
quite clumsy and furthermore the credits for inode deletion are quite
limited and already accounted for. So just tweak ext4_evict_inode() to
avoid freeze protection if we have transaction already started and thus
it is not really needed anyway.

CC: stable@vger.kernel.org
Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
CC: Tahsin Erdogan <tahsin@google.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 72534319fae5..777eb08b29cd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -175,6 +175,7 @@ void ext4_evict_inode(struct inode *inode)
 	 */
 	int extra_credits = 6;
 	struct ext4_xattr_inode_array *ea_inode_array = NULL;
+	bool freeze_protected = false;
 
 	trace_ext4_evict_inode(inode);
 
@@ -232,9 +233,14 @@ void ext4_evict_inode(struct inode *inode)
 
 	/*
 	 * Protect us against freezing - iput() caller didn't have to have any
-	 * protection against it
+	 * protection against it. When we are in a running transaction though,
+	 * we are already protected against freezing and we cannot grab further
+	 * protection due to lock ordering constraints.
 	 */
-	sb_start_intwrite(inode->i_sb);
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(inode->i_sb);
+		freeze_protected = true;
+	}
 
 	if (!IS_NOQUOTA(inode))
 		extra_credits += EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb);
@@ -253,7 +259,8 @@ void ext4_evict_inode(struct inode *inode)
 		 * cleaned up.
 		 */
 		ext4_orphan_del(NULL, inode);
-		sb_end_intwrite(inode->i_sb);
+		if (freeze_protected)
+			sb_end_intwrite(inode->i_sb);
 		goto no_delete;
 	}
 
@@ -294,7 +301,8 @@ void ext4_evict_inode(struct inode *inode)
 stop_handle:
 		ext4_journal_stop(handle);
 		ext4_orphan_del(NULL, inode);
-		sb_end_intwrite(inode->i_sb);
+		if (freeze_protected)
+			sb_end_intwrite(inode->i_sb);
 		ext4_xattr_inode_array_free(ea_inode_array);
 		goto no_delete;
 	}
@@ -323,7 +331,8 @@ void ext4_evict_inode(struct inode *inode)
 	else
 		ext4_free_inode(handle, inode);
 	ext4_journal_stop(handle);
-	sb_end_intwrite(inode->i_sb);
+	if (freeze_protected)
+		sb_end_intwrite(inode->i_sb);
 	ext4_xattr_inode_array_free(ea_inode_array);
 	return;
 no_delete:
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 7/8] ext4: Fix superblock checksum failure when setting password salt
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
                   ` (5 preceding siblings ...)
  2020-12-16 10:18 ` [PATCH 6/8] ext4: Fix deadlock with fs freezing and EA inodes Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 16:02   ` Theodore Y. Ts'o
  2020-12-16 10:18 ` [PATCH 8/8] ext4: Drop ext4_handle_dirty_super() Jan Kara
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara, Michael Halcrow

When setting password salt in the superblock, we forget to recompute the
superblock checksum so it will not match until the next superblock
modification which recomputes the checksum. Fix it.

CC: Michael Halcrow <mhalcrow@google.com>
Reported-by: Andreas Dilger <adilger@dilger.ca>
Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt support")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index f0381876a7e5..106bf149e8ca 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1157,7 +1157,10 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			err = ext4_journal_get_write_access(handle, sbi->s_sbh);
 			if (err)
 				goto pwsalt_err_journal;
+			lock_buffer(sbi->s_sbh);
 			generate_random_uuid(sbi->s_es->s_encrypt_pw_salt);
+			ext4_superblock_csum_set(sb);
+			unlock_buffer(sbi->s_sbh);
 			err = ext4_handle_dirty_metadata(handle, NULL,
 							 sbi->s_sbh);
 		pwsalt_err_journal:
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 8/8] ext4: Drop ext4_handle_dirty_super()
  2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
                   ` (6 preceding siblings ...)
  2020-12-16 10:18 ` [PATCH 7/8] ext4: Fix superblock checksum failure when setting password salt Jan Kara
@ 2020-12-16 10:18 ` Jan Kara
  2020-12-17 16:08   ` Theodore Y. Ts'o
  7 siblings, 1 reply; 17+ messages in thread
From: Jan Kara @ 2020-12-16 10:18 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, harshad shirwadkar, Jan Kara

The wrapper is now useless since it does what
ext4_handle_dirty_metadata() does. Just remove it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.c | 16 ----------------
 fs/ext4/ext4_jbd2.h |  5 -----
 fs/ext4/file.c      |  2 +-
 fs/ext4/inode.c     |  3 ++-
 fs/ext4/namei.c     |  4 ++--
 fs/ext4/resize.c    |  8 ++++----
 fs/ext4/xattr.c     |  2 +-
 7 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index c7e410c4ab7d..be799040a415 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -372,19 +372,3 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 	}
 	return err;
 }
-
-int __ext4_handle_dirty_super(const char *where, unsigned int line,
-			      handle_t *handle, struct super_block *sb)
-{
-	struct buffer_head *bh = EXT4_SB(sb)->s_sbh;
-	int err = 0;
-
-	if (ext4_handle_valid(handle)) {
-		err = jbd2_journal_dirty_metadata(handle, bh);
-		if (err)
-			ext4_journal_abort_handle(where, line, __func__,
-						  bh, handle, err);
-	} else
-		mark_buffer_dirty(bh);
-	return err;
-}
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 00dc668e052b..b9881ee1bf93 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -247,9 +247,6 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 				 handle_t *handle, struct inode *inode,
 				 struct buffer_head *bh);
 
-int __ext4_handle_dirty_super(const char *where, unsigned int line,
-			      handle_t *handle, struct super_block *sb);
-
 #define ext4_journal_get_write_access(handle, bh) \
 	__ext4_journal_get_write_access(__func__, __LINE__, (handle), (bh))
 #define ext4_forget(handle, is_metadata, inode, bh, block_nr) \
@@ -260,8 +257,6 @@ int __ext4_handle_dirty_super(const char *where, unsigned int line,
 #define ext4_handle_dirty_metadata(handle, inode, bh) \
 	__ext4_handle_dirty_metadata(__func__, __LINE__, (handle), (inode), \
 				     (bh))
-#define ext4_handle_dirty_super(handle, sb) \
-	__ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb))
 
 handle_t *__ext4_journal_start_sb(struct super_block *sb, unsigned int line,
 				  int type, int blocks, int rsv_blocks,
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 26907d5835d0..1cd3d26e3217 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -814,7 +814,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
 		sizeof(sbi->s_es->s_last_mounted));
 	ext4_superblock_csum_set(sb);
 	unlock_buffer(sbi->s_sbh);
-	ext4_handle_dirty_super(handle, sb);
+	ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
 out_journal:
 	ext4_journal_stop(handle);
 out:
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 777eb08b29cd..a1baa6084db3 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5155,7 +5155,8 @@ static int ext4_do_update_inode(handle_t *handle,
 		ext4_superblock_csum_set(sb);
 		unlock_buffer(EXT4_SB(sb)->s_sbh);
 		ext4_handle_sync(handle);
-		err = ext4_handle_dirty_super(handle, sb);
+		err = ext4_handle_dirty_metadata(handle, NULL,
+						 EXT4_SB(sb)->s_sbh);
 	}
 	ext4_update_inode_fsync_trans(handle, inode, need_datasync);
 out_brelse:
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index d804505e1a32..c1ec073551ba 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2992,7 +2992,7 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode)
 	mutex_unlock(&sbi->s_orphan_lock);
 
 	if (dirty) {
-		err = ext4_handle_dirty_super(handle, sb);
+		err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
 		rc = ext4_mark_iloc_dirty(handle, inode, &iloc);
 		if (!err)
 			err = rc;
@@ -3073,7 +3073,7 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode)
 		ext4_superblock_csum_set(inode->i_sb);
 		unlock_buffer(sbi->s_sbh);
 		mutex_unlock(&sbi->s_orphan_lock);
-		err = ext4_handle_dirty_super(handle, inode->i_sb);
+		err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
 	} else {
 		struct ext4_iloc iloc2;
 		struct inode *i_prev =
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 6155f2b9538c..bd0d185654f3 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -903,7 +903,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	le16_add_cpu(&es->s_reserved_gdt_blocks, -1);
 	ext4_superblock_csum_set(sb);
 	unlock_buffer(EXT4_SB(sb)->s_sbh);
-	err = ext4_handle_dirty_super(handle, sb);
+	err = ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
 	if (err)
 		ext4_std_error(sb, err);
 	return err;
@@ -1521,7 +1521,7 @@ static int ext4_flex_group_add(struct super_block *sb,
 
 	ext4_update_super(sb, flex_gd);
 
-	err = ext4_handle_dirty_super(handle, sb);
+	err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
 
 exit_journal:
 	err2 = ext4_journal_stop(handle);
@@ -1734,7 +1734,7 @@ static int ext4_group_extend_no_check(struct super_block *sb,
 	err = ext4_group_add_blocks(handle, sb, o_blocks_count, add);
 	if (err)
 		goto errout;
-	ext4_handle_dirty_super(handle, sb);
+	ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
 	ext4_debug("freed blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
 errout:
@@ -1891,7 +1891,7 @@ static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
 	ext4_superblock_csum_set(sb);
 	unlock_buffer(sbi->s_sbh);
 
-	err = ext4_handle_dirty_super(handle, sb);
+	err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
 	if (err) {
 		ext4_std_error(sb, err);
 		goto errout;
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index ab46aa447baa..a236e606fa95 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -796,7 +796,7 @@ static void ext4_xattr_update_super_block(handle_t *handle,
 		ext4_set_feature_xattr(sb);
 		ext4_superblock_csum_set(sb);
 		unlock_buffer(EXT4_SB(sb)->s_sbh);
-		ext4_handle_dirty_super(handle, sb);
+		ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
 	}
 }
 
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info()
  2020-12-16 10:18 ` [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info() Jan Kara
@ 2020-12-17 15:50   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 15:50 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar

On Wed, Dec 16, 2020 at 11:18:37AM +0100, Jan Kara wrote:
> save_error_info() is always called together with ext4_handle_error().
> Combine them into a single call and move unconditional bits out of
> save_error_info() into ext4_handle_error().
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/8] ext4: Drop sync argument of ext4_commit_super()
  2020-12-16 10:18 ` [PATCH 2/8] ext4: Drop sync argument of ext4_commit_super() Jan Kara
@ 2020-12-17 15:51   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 15:51 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar

On Wed, Dec 16, 2020 at 11:18:38AM +0100, Jan Kara wrote:
> Everybody passes 1 as sync argument of ext4_commit_super(). Just drop
> it.
> 
> Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/8] ext4: Protect superblock modifications with a buffer lock
  2020-12-16 10:18 ` [PATCH 3/8] ext4: Protect superblock modifications with a buffer lock Jan Kara
@ 2020-12-17 15:56   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 15:56 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar

On Wed, Dec 16, 2020 at 11:18:39AM +0100, Jan Kara wrote:
> Protect all superblock modifications (including checksum computation)
> with a superblock buffer lock. That way we are sure computed checksum
> matches current superblock contents (a mismatch could cause checksum
> failures in nojournal mode or if an unjournalled superblock update races
> with a journalled one). Also we avoid modifying superblock contents
> while it is being written out (which can cause DIF/DIX failures if we
> are running in nojournal mode).
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/8] ext4: Save error info to sb through journal if available
  2020-12-16 10:18 ` [PATCH 4/8] ext4: Save error info to sb through journal if available Jan Kara
@ 2020-12-17 15:58   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 15:58 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar

On Wed, Dec 16, 2020 at 11:18:40AM +0100, Jan Kara wrote:
> If journalling is still working at the moment we get to writing error
> information to the superblock we cannot write directly to the superblock
> as such write could race with journalled update of the superblock and
> cause journal checksum failures, writing inconsistent information to the
> journal or other problems. We cannot journal the superblock directly
> from the error handling functions as we are running in uncertain context
> and could deadlock so just punt journalled superblock update to a
> workqueue.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/8] ext4: Use sbi instead of EXT4_SB(sb) in ext4_update_super()
  2020-12-16 10:18 ` [PATCH 5/8] ext4: Use sbi instead of EXT4_SB(sb) in ext4_update_super() Jan Kara
@ 2020-12-17 15:59   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 15:59 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar

On Wed, Dec 16, 2020 at 11:18:41AM +0100, Jan Kara wrote:
> No behavioral change.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 6/8] ext4: Fix deadlock with fs freezing and EA inodes
  2020-12-16 10:18 ` [PATCH 6/8] ext4: Fix deadlock with fs freezing and EA inodes Jan Kara
@ 2020-12-17 16:01   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 16:01 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar, stable, Tahsin Erdogan

On Wed, Dec 16, 2020 at 11:18:42AM +0100, Jan Kara wrote:
> Xattr code using inodes with large xattr data can end up dropping last
> inode reference (and thus deleting the inode) from places like
> ext4_xattr_set_entry(). That function is called with transaction started
> and so ext4_evict_inode() can deadlock against fs freezing like:
> 
> CPU1					CPU2
> 
> removexattr()				freeze_super()
>   vfs_removexattr()
>     ext4_xattr_set()
>       handle = ext4_journal_start()
>       ...
>       ext4_xattr_set_entry()
>         iput(old_ea_inode)
>           ext4_evict_inode(old_ea_inode)
> 					  sb->s_writers.frozen = SB_FREEZE_FS;
> 					  sb_wait_write(sb, SB_FREEZE_FS);
> 					  ext4_freeze()
> 					    jbd2_journal_lock_updates()
> 					      -> blocks waiting for all
> 					         handles to stop
>             sb_start_intwrite()
> 	      -> blocks as sb is already in SB_FREEZE_FS state
> 
> Generally it is advisable to delete inodes from a separate transaction
> as it can consume quite some credits however in this case it would be
> quite clumsy and furthermore the credits for inode deletion are quite
> limited and already accounted for. So just tweak ext4_evict_inode() to
> avoid freeze protection if we have transaction already started and thus
> it is not really needed anyway.
> 
> CC: stable@vger.kernel.org
> Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
> CC: Tahsin Erdogan <tahsin@google.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Already applied.

						- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 7/8] ext4: Fix superblock checksum failure when setting password salt
  2020-12-16 10:18 ` [PATCH 7/8] ext4: Fix superblock checksum failure when setting password salt Jan Kara
@ 2020-12-17 16:02   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 16:02 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar, Michael Halcrow

On Wed, Dec 16, 2020 at 11:18:43AM +0100, Jan Kara wrote:
> When setting password salt in the superblock, we forget to recompute the
> superblock checksum so it will not match until the next superblock
> modification which recomputes the checksum. Fix it.
> 
> CC: Michael Halcrow <mhalcrow@google.com>
> Reported-by: Andreas Dilger <adilger@dilger.ca>
> Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt support")
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

						- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 8/8] ext4: Drop ext4_handle_dirty_super()
  2020-12-16 10:18 ` [PATCH 8/8] ext4: Drop ext4_handle_dirty_super() Jan Kara
@ 2020-12-17 16:08   ` Theodore Y. Ts'o
  0 siblings, 0 replies; 17+ messages in thread
From: Theodore Y. Ts'o @ 2020-12-17 16:08 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, harshad shirwadkar

On Wed, Dec 16, 2020 at 11:18:44AM +0100, Jan Kara wrote:
> The wrapper is now useless since it does what
> ext4_handle_dirty_metadata() does. Just remove it.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-12-17 16:09 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-16 10:18 [PATCH 0/8 v2 PARTIAL] ext4: Fix error handling Jan Kara
2020-12-16 10:18 ` [PATCH 1/8] ext4: Combine ext4_handle_error() and save_error_info() Jan Kara
2020-12-17 15:50   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 2/8] ext4: Drop sync argument of ext4_commit_super() Jan Kara
2020-12-17 15:51   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 3/8] ext4: Protect superblock modifications with a buffer lock Jan Kara
2020-12-17 15:56   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 4/8] ext4: Save error info to sb through journal if available Jan Kara
2020-12-17 15:58   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 5/8] ext4: Use sbi instead of EXT4_SB(sb) in ext4_update_super() Jan Kara
2020-12-17 15:59   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 6/8] ext4: Fix deadlock with fs freezing and EA inodes Jan Kara
2020-12-17 16:01   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 7/8] ext4: Fix superblock checksum failure when setting password salt Jan Kara
2020-12-17 16:02   ` Theodore Y. Ts'o
2020-12-16 10:18 ` [PATCH 8/8] ext4: Drop ext4_handle_dirty_super() Jan Kara
2020-12-17 16:08   ` Theodore Y. Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).