stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mbcache: Add functions to invalidate entries and wait for entry users
       [not found] <20220606142215.17962-1-jack@suse.cz>
@ 2022-06-06 14:28 ` Jan Kara
  2022-06-06 14:28 ` [PATCH 2/2] ext4: Fix race when reusing xattr blocks Jan Kara
  1 sibling, 0 replies; 2+ messages in thread
From: Jan Kara @ 2022-06-06 14:28 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, Ritesh Harjani, Jan Kara, stable

Add functions which allow for invalidating entry in the cache but
keeping the last reference and for waiting on remaining users of the
cache entry. These functions will be used by ext4 to fix races with
xattr block sharing.

CC: stable@vger.kernel.org
Fixes: 82939d7999df ("ext4: convert to mbcache2")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/mbcache.c            | 47 ++++++++++++++++++++++++++++++++++++-----
 include/linux/mbcache.h | 11 +++++++++-
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/fs/mbcache.c b/fs/mbcache.c
index 97c54d3a2227..b81ea7c6fa21 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -125,6 +125,21 @@ void __mb_cache_entry_free(struct mb_cache_entry *entry)
 }
 EXPORT_SYMBOL(__mb_cache_entry_free);
 
+/*
+ * mb_cache_entry_wait_unused - wait to be the last user of the entry
+ *
+ * @entry - entry to work on
+ *
+ * Wait to be the last user of the entry.
+ */
+void mb_cache_entry_wait_unused(struct mb_cache_entry *entry)
+{
+	WARN_ON_ONCE(!list_empty(&entry->e_list));
+	WARN_ON_ONCE(!hlist_bl_unhashed(&entry->e_hash_list));
+	wait_var_event(&entry->e_refcnt, atomic_read(&entry->e_refcnt) == 1);
+}
+EXPORT_SYMBOL(mb_cache_entry_wait_unused);
+
 static struct mb_cache_entry *__entry_find(struct mb_cache *cache,
 					   struct mb_cache_entry *entry,
 					   u32 key)
@@ -217,14 +232,18 @@ struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key,
 }
 EXPORT_SYMBOL(mb_cache_entry_get);
 
-/* mb_cache_entry_delete - remove a cache entry
+/* mb_cache_entry_invalidate - invalidate mbcache entry
  * @cache - cache we work with
  * @key - key
  * @value - value
  *
- * Remove entry from cache @cache with key @key and value @value.
+ * Invalidate entry in cache @cache with key @key and value @value. The entry
+ * is removed from the hash and LRU so there can be no new users of it. The
+ * invalidated entry is returned so that the caller can drop the last
+ * reference (perhaps after waiting for remaining users).
  */
-void mb_cache_entry_delete(struct mb_cache *cache, u32 key, u64 value)
+struct mb_cache_entry *mb_cache_entry_invalidate(struct mb_cache *cache,
+						 u32 key, u64 value)
 {
 	struct hlist_bl_node *node;
 	struct hlist_bl_head *head;
@@ -246,11 +265,29 @@ void mb_cache_entry_delete(struct mb_cache *cache, u32 key, u64 value)
 				atomic_dec(&entry->e_refcnt);
 			}
 			spin_unlock(&cache->c_list_lock);
-			mb_cache_entry_put(cache, entry);
-			return;
+			return entry;
 		}
 	}
 	hlist_bl_unlock(head);
+
+	return NULL;
+}
+EXPORT_SYMBOL(mb_cache_entry_invalidate);
+
+/* mb_cache_entry_delete - delete mbcache entry
+ * @cache - cache we work with
+ * @key - key
+ * @value - value
+ *
+ * Delete entry in cache @cache with key @key and value @value.
+ */
+void mb_cache_entry_delete(struct mb_cache *cache, u32 key, u64 value)
+{
+	struct mb_cache_entry *entry;
+
+	entry = mb_cache_entry_invalidate(cache, key, value);
+	if (entry)
+		mb_cache_entry_put(cache, entry);
 }
 EXPORT_SYMBOL(mb_cache_entry_delete);
 
diff --git a/include/linux/mbcache.h b/include/linux/mbcache.h
index 20f1e3ff6013..4e6a5e05e78b 100644
--- a/include/linux/mbcache.h
+++ b/include/linux/mbcache.h
@@ -30,15 +30,24 @@ void mb_cache_destroy(struct mb_cache *cache);
 int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
 			  u64 value, bool reusable);
 void __mb_cache_entry_free(struct mb_cache_entry *entry);
+void mb_cache_entry_wait_unused(struct mb_cache_entry *entry);
 static inline int mb_cache_entry_put(struct mb_cache *cache,
 				     struct mb_cache_entry *entry)
 {
-	if (!atomic_dec_and_test(&entry->e_refcnt))
+	unsigned int cnt;
+
+	cnt = atomic_dec_return(&entry->e_refcnt);
+	if (cnt > 0) {
+		if (cnt == 1)
+			wake_up_var(&entry->e_refcnt);
 		return 0;
+	}
 	__mb_cache_entry_free(entry);
 	return 1;
 }
 
+struct mb_cache_entry *mb_cache_entry_invalidate(struct mb_cache *cache,
+						 u32 key, u64 value);
 void mb_cache_entry_delete(struct mb_cache *cache, u32 key, u64 value);
 struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key,
 					  u64 value);
-- 
2.35.3


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

* [PATCH 2/2] ext4: Fix race when reusing xattr blocks
       [not found] <20220606142215.17962-1-jack@suse.cz>
  2022-06-06 14:28 ` [PATCH 1/2] mbcache: Add functions to invalidate entries and wait for entry users Jan Kara
@ 2022-06-06 14:28 ` Jan Kara
  1 sibling, 0 replies; 2+ messages in thread
From: Jan Kara @ 2022-06-06 14:28 UTC (permalink / raw)
  To: Ted Tso; +Cc: linux-ext4, Ritesh Harjani, Jan Kara, stable

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


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

end of thread, other threads:[~2022-06-06 14:28 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220606142215.17962-1-jack@suse.cz>
2022-06-06 14:28 ` [PATCH 1/2] mbcache: Add functions to invalidate entries and wait for entry users Jan Kara
2022-06-06 14:28 ` [PATCH 2/2] ext4: Fix race when reusing xattr blocks Jan Kara

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).