Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Jan Kara <jack@suse.cz>
To:     Ted Tso <tytso@mit.edu>
Cc:     <linux-ext4@vger.kernel.org>,
        Ritesh Harjani <ritesh.list@gmail.com>,
        Jan Kara <jack@suse.cz>, stable@vger.kernel.org
Subject: [PATCH 2/2] ext4: Fix race when reusing xattr blocks
Date:   Mon,  6 Jun 2022 16:28:47 +0200
Message-Id: <20220606142850.15489-2-jack@suse.cz>
In-Reply-To: <20220606142215.17962-1-jack@suse.cz>
References: <20220606142215.17962-1-jack@suse.cz>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

When ext4_xattr_block_set() decides to remove xattr block the following
race can happen:

CPU1					CPU2
ext4_xattr_block_set()			ext4_xattr_release_block()
  new_bh = ext4_xattr_block_cache_find()

					  lock_buffer(bh);
					  ref = le32_to_cpu(BHDR(bh)->h_refcount);
					  if (ref == 1) {
					    ...
					    mb_cache_entry_delete();
					    unlock_buffer(bh);
					    ext4_free_blocks();
					      ...
					      ext4_forget(..., bh, ...);
						jbd2_journal_revoke(..., bh);

  ext4_journal_get_write_access(..., new_bh, ...)
    do_get_write_access()
      jbd2_journal_cancel_revoke(..., new_bh);

Later the code in ext4_xattr_block_set() finds out the block got freed
and cancels reusal of the block but the revoke stays canceled and so in
case of block reuse and journal replay the filesystem can get corrupted.
If the race works out slightly differently, we can also hit assertions
in the jbd2 code. Fix the problem by waiting for users of the mbcache
entry (so xattr block reuse gets canceled) before we free xattr block to
prevent the issue with racing ext4_journal_get_write_access() call.

CC: stable@vger.kernel.org
Fixes: 82939d7999df ("ext4: convert to mbcache2")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/xattr.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 042325349098..4eeeb3db618f 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1241,17 +1241,29 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
 	hash = le32_to_cpu(BHDR(bh)->h_hash);
 	ref = le32_to_cpu(BHDR(bh)->h_refcount);
 	if (ref == 1) {
+		struct mb_cache_entry *ce = NULL;
+
 		ea_bdebug(bh, "refcount now=0; freeing");
 		/*
 		 * This must happen under buffer lock for
 		 * ext4_xattr_block_set() to reliably detect freed block
 		 */
 		if (ea_block_cache)
-			mb_cache_entry_delete(ea_block_cache, hash,
-					      bh->b_blocknr);
+			ce = mb_cache_entry_invalidate(ea_block_cache, hash,
+						       bh->b_blocknr);
 		get_bh(bh);
 		unlock_buffer(bh);
 
+		if (ce) {
+			/*
+			 * Wait for outstanding users of xattr entry so that we
+			 * know they don't try to reuse xattr block before we
+			 * free it - that revokes the block from the journal
+			 * which upsets jbd2_journal_get_write_access()
+			 */
+			mb_cache_entry_wait_unused(ce);
+			mb_cache_entry_put(ea_block_cache, ce);
+		}
 		if (ext4_has_feature_ea_inode(inode->i_sb))
 			ext4_xattr_inode_dec_ref_all(handle, inode, bh,
 						     BFIRST(bh),
@@ -1847,7 +1859,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 	struct buffer_head *new_bh = NULL;
 	struct ext4_xattr_search s_copy = bs->s;
 	struct ext4_xattr_search *s = &s_copy;
-	struct mb_cache_entry *ce = NULL;
+	struct mb_cache_entry *ce = NULL, *old_ce = NULL;
 	int error = 0;
 	struct mb_cache *ea_block_cache = EA_BLOCK_CACHE(inode);
 	struct inode *ea_inode = NULL, *tmp_inode;
@@ -1871,11 +1883,15 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 			/*
 			 * This must happen under buffer lock for
 			 * ext4_xattr_block_set() to reliably detect modified
-			 * block
+			 * block. We keep ourselves entry reference so that
+			 * we can wait for outstanding users of the entry
+			 * before freeing the xattr block.
 			 */
-			if (ea_block_cache)
-				mb_cache_entry_delete(ea_block_cache, hash,
-						      bs->bh->b_blocknr);
+			if (ea_block_cache) {
+				old_ce = mb_cache_entry_invalidate(
+						ea_block_cache, hash,
+						bs->bh->b_blocknr);
+			}
 			ea_bdebug(bs->bh, "modifying in-place");
 			error = ext4_xattr_set_entry(i, s, handle, inode,
 						     true /* is_block */);
@@ -2127,6 +2143,14 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 	if (bs->bh && bs->bh != new_bh) {
 		struct ext4_xattr_inode_array *ea_inode_array = NULL;
 
+		/*
+		 * Wait for outstanding users of xattr entry so that we know
+		 * they don't try to reuse xattr block before we free it - that
+		 * revokes the block from the journal which upsets
+		 * jbd2_journal_get_write_access()
+		 */
+		if (old_ce)
+			mb_cache_entry_wait_unused(old_ce);
 		ext4_xattr_release_block(handle, inode, bs->bh,
 					 &ea_inode_array,
 					 0 /* extra_credits */);
@@ -2151,6 +2175,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 	}
 	if (ce)
 		mb_cache_entry_put(ea_block_cache, ce);
+	if (old_ce)
+		mb_cache_entry_put(ea_block_cache, old_ce);
 	brelse(new_bh);
 	if (!(bs->bh && s->base == bs->bh->b_data))
 		kfree(s->base);
-- 
2.35.3