by Thilo Fromm

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

Hello Honza,

> Yeah, I was pondering about this for some time but still I have no clue who
> could be holding the buffer lock (which blocks the task holding the
> transaction open) or how this could related to the commit you have
> identified. I have two things to try:
>
> 1) Can you please check whether the deadlock reproduces also with 6.0
> kernel? The thing is that xattr handling code in ext4 has there some
> additional changes, commit 307af6c8793 ("mbcache: automatically delete
> entries from cache on freeing") in particular.

This would be complex; we currently do not integrate 6.0 with Flatcar
and would need to spend quite some effort ingesting it first (mostly,
make sure the new kernel does not break something unrelated). Flatcar is
an image-based distro, so kernel updates imply full distro updates.

> 2) I have created a debug patch (against 5.15.x stable kernel). Can you
> please reproduce the failure with it and post the output of "echo w
>> /proc/sysrq-trigger" and also the output the debug patch will put into the
> kernel log? It will dump the information about buffer lock owner if we > cannot get the lock for more than 32 seconds.

This would be more straightforward - I can reach out to one of our users
suffering from the issue; they can reliably reproduce it and don't shy
away from patching their kernel. Where can I find the patch?

Best,
Thilo

2022-10-26 10:27:54

by Jan Kara

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

On Mon 21-11-22 16:18:09, Thorsten Leemhuis wrote:
> On 21.11.22 16:00, Jan Kara wrote:
> > OK, attached patch fixes the deadlock for me. Can you test whether it fixes
> > the problem for you as well? Thanks!
>
> Jan, many thx for taking care of this. There is one small thing to
> improve, please add the following tags to your patch:
>
> Link:
> https://lore.kernel.org/r/[email protected]/

Good point. Thanks, added!

> Not sure, maybe a reported-by for Syzbot would be good as well, see:
> https://lore.kernel.org/linux-ext4/[email protected]/

The syzbot report is actually unrelated. So far I think that the bisection
that landed guys on the same commit as syzbot was just luck or maybe change
in timing that made the problem easier to reproduce.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2022-11-21 18:20:43

by Jeremi Piotrowski

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

On Mon, Nov 21, 2022 at 04:00:18PM +0100, Jan Kara wrote:
> On Mon 21-11-22 14:35:59, Jan Kara wrote:
> > On Fri 11-11-22 07:52:38, Jeremi Piotrowski wrote:
> > > On Fri, Nov 11, 2022 at 07:10:29AM -0800, Jeremi Piotrowski wrote:
> > > > On Fri, Nov 11, 2022 at 03:24:24PM +0100, Jan Kara wrote:
> > > > > On Thu 10-11-22 11:27:01, Jeremi Piotrowski wrote:
> > > > > > On Thu, Nov 10, 2022 at 04:26:37PM +0100, Jan Kara wrote:
> > > > > > > On Thu 10-11-22 04:57:58, Jeremi Piotrowski wrote:
> > > > > > > > On Wed, Oct 26, 2022 at 12:18:54PM +0200, Jan Kara wrote:
> > > > > > > > > On Mon 24-10-22 18:32:51, Thilo Fromm wrote:
> > > > > > > > > > Hello Honza,
> > > > > > > > > >
> > > > > > > > > > > Yeah, I was pondering about this for some time but still I have no clue who
> > > > > > > > > > > could be holding the buffer lock (which blocks the task holding the
> > > > > > > > > > > transaction open) or how this could related to the commit you have
> > > > > > > > > > > identified. I have two things to try:
> > > > > > > > > > >
> > > > > > > > > > > 1) Can you please check whether the deadlock reproduces also with 6.0
> > > > > > > > > > > kernel? The thing is that xattr handling code in ext4 has there some
> > > > > > > > > > > additional changes, commit 307af6c8793 ("mbcache: automatically delete
> > > > > > > > > > > entries from cache on freeing") in particular.
> > > > > > > > > >
> > > > > > > > > > This would be complex; we currently do not integrate 6.0 with Flatcar and
> > > > > > > > > > would need to spend quite some effort ingesting it first (mostly, make sure
> > > > > > > > > > the new kernel does not break something unrelated). Flatcar is an
> > > > > > > > > > image-based distro, so kernel updates imply full distro updates.
> > > > > > > > >
> > > > > > > > > OK, understood.
> > > > > > > > >
> > > > > > > > > > > 2) I have created a debug patch (against 5.15.x stable kernel). Can you
> > > > > > > > > > > please reproduce the failure with it and post the output of "echo w
> > > > > > > > > > > > /proc/sysrq-trigger" and also the output the debug patch will put into the
> > > > > > > > > > > kernel log? It will dump the information about buffer lock owner if we > cannot get the lock for more than 32 seconds.
> > > > > > > > > >
> > > > > > > > > > This would be more straightforward - I can reach out to one of our users
> > > > > > > > > > suffering from the issue; they can reliably reproduce it and don't shy away
> > > > > > > > > > from patching their kernel. Where can I find the patch?
> > > > > > > > >
> > > > > > > > > Ha, my bad. I forgot to attach it. Here it is.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Unfortunately this patch produced no output, but I have been able to repro so I
> > > > > > > > understand why: except for the hung tasks, we have 1+ tasks busy-looping through
> > > > > > > > the following code in ext4_xattr_block_set():
> > > > > > > >
> > > > > > > > inserted:
> > > > > > > > if (!IS_LAST_ENTRY(s->first)) {
> > > > > > > > new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
> > > > > > > > &ce);
> > > > > > > > if (new_bh) {
> > > > > > > > /* We found an identical block in the cache. */
> > > > > > > > if (new_bh == bs->bh)
> > > > > > > > ea_bdebug(new_bh, "keeping");
> > > > > > > > else {
> > > > > > > > u32 ref;
> > > > > > > >
> > > > > > > > WARN_ON_ONCE(dquot_initialize_needed(inode));
> > > > > > > >
> > > > > > > > /* The old block is released after updating
> > > > > > > > the inode. */
> > > > > > > > error = dquot_alloc_block(inode,
> > > > > > > > EXT4_C2B(EXT4_SB(sb), 1));
> > > > > > > > if (error)
> > > > > > > > goto cleanup;
> > > > > > > > BUFFER_TRACE(new_bh, "get_write_access");
> > > > > > > > error = ext4_journal_get_write_access(
> > > > > > > > handle, sb, new_bh,
> > > > > > > > EXT4_JTR_NONE);
> > > > > > > > if (error)
> > > > > > > > goto cleanup_dquot;
> > > > > > > > lock_buffer(new_bh);
> > > > > > > > /*
> > > > > > > > * We have to be careful about races with
> > > > > > > > * adding references to xattr block. Once we
> > > > > > > > * hold buffer lock xattr block's state is
> > > > > > > > * stable so we can check the additional
> > > > > > > > * reference fits.
> > > > > > > > */
> > > > > > > > ref = le32_to_cpu(BHDR(new_bh)->h_refcount) + 1;
> > > > > > > > if (ref > EXT4_XATTR_REFCOUNT_MAX) {
> > > > > > > > /*
> > > > > > > > * Undo everything and check mbcache
> > > > > > > > * again.
> > > > > > > > */
> > > > > > > > unlock_buffer(new_bh);
> > > > > > > > dquot_free_block(inode,
> > > > > > > > EXT4_C2B(EXT4_SB(sb),
> > > > > > > > 1));
> > > > > > > > brelse(new_bh);
> > > > > > > > mb_cache_entry_put(ea_block_cache, ce);
> > > > > > > > ce = NULL;
> > > > > > > > new_bh = NULL;
> > > > > > > > goto inserted;
> > > > > > > > }
> > > > > > > >
> > > > > > > > The tasks keep taking the 'goto inserted' branch, and never finish. I've been
> > > > > > > > able to repro with kernel v6.0.7 as well.
> > > > > > >
> > > > > > > Interesting! That makes is much clearer (and also makes my debug patch
> > > > > > > unnecessary). So clearly the e_reusable variable in the mb_cache_entry got
> > > > > > > out of sync with the number of references really in the xattr block - in
> > > > > > > particular the block likely has h_refcount >= EXT4_XATTR_REFCOUNT_MAX but
> > > > > > > e_reusable is set to true. Now I can see how e_reusable can stay at false due
> > > > > > > to a race when refcount is actually smaller but I don't see how it could
> > > > > > > stay at true when refcount is big enough - that part seems to be locked
> > > > > > > properly. If you can reproduce reasonably easily, can you try reproducing
> > > > > > > with attached patch? Thanks!
> > > > > > >
> > > > > >
> > > > > > Sure, with that patch I'm getting the following output, reusable is false on
> > > > > > most items until we hit something with reusable true and then that loops
> > > > > > indefinitely:
> > > > >
> > > > > Thanks. So that is what I've suspected. I'm still not 100% clear on how
> > > > > this inconsistency can happen although I have a suspicion - does attached
> > > > > patch fix the problem for you?
> > > > >
> > > > > Also is it possible to share the reproducer or it needs some special
> > > > > infrastructure?
> > > > >
> > > > > Honza
> > > >
> > > > I'll test the patch and report back.
> > > >
> > > > Attached you'll find the reproducer, for me it reproduces within a few minutes.
> > > > It brings up a k8s node and then runs 3 instances of the application which
> > > > creates a lot of small files in a loop. The OS we run it on has selinux enabled
> > > > in permissive mode, that might play a role.
> > > >
> > >
> > > I can still reproduce it with the patch.
> >
> > Thanks for the answer! So I was trying to make your reproducer work on my
> > system but it was not so easy on openSUSE ;). Anyway, when working on this
> > I've realized there may be a simpler way to tickle the bug and indeed, I
> > can now trigger it with a simple C program. So thanks for your help, I'm
> > now debugging the issue on my system.
>
> OK, attached patch fixes the deadlock for me. Can you test whether it fixes
> the problem for you as well? Thanks!

I'll test the fix tomorrow, but I've noticed it doesn't apply cleanly to
5.15.78, which seems to be missing:

- 5fc4cbd9fde5d4630494fd6ffc884148fb618087 mbcache: Avoid nesting of cache->c_list_lock under bit locks
(this one is marked for stable but not in 5.15?)
- 307af6c879377c1c63e71cbdd978201f9c7ee8df mbcache: automatically delete entries from cache on freeing
(this one is not marked for stable)

So either a special backport is needed or these two would need to be applied as
well.

>
> Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR

> >From 513b8ebc1df41937b2d522e21668584d8b5a48a1 Mon Sep 17 00:00:00 2001
> From: Jan Kara <[email protected]>
> Date: Mon, 21 Nov 2022 15:44:10 +0100
> Subject: [PATCH] ext4: Fix deadlock due to mbcache entry corruption
>
> When manipulating xattr blocks, we can deadlock infinitely looping
> inside ext4_xattr_block_set() where we constantly keep finding xattr
> block for reuse in mbcache but we are unable to reuse it because its
> reference count is too big. This happens because cache entry for the
> xattr block is marked as reusable (e_reusable set) although its
> reference count is too big. When this inconsistency happens, this
> inconsistent state is kept indefinitely and so ext4_xattr_block_set()
> keeps retrying indefinitely.
>
> The inconsistent state is caused by non-atomic update of e_reusable bit.
> e_reusable is part of a bitfield and e_reusable update can race with
> update of e_referenced bit in the same bitfield resulting in loss of one
> of the updates. Fix the problem by using atomic bitops instead.
>
> CC: [email protected]
> Fixes: 6048c64b2609 ("mbcache: add reusable flag to cache entries")
> Reported-by: Jeremi Piotrowski <[email protected]>
> Reported-by: Thilo Fromm <[email protected]>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/ext4/xattr.c | 4 ++--
> fs/mbcache.c | 14 ++++++++------
> include/linux/mbcache.h | 9 +++++++--
> 3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 800ce5cdb9d2..08043aa72cf1 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1281,7 +1281,7 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
> ce = mb_cache_entry_get(ea_block_cache, hash,
> bh->b_blocknr);
> if (ce) {
> - ce->e_reusable = 1;
> + set_bit(MBE_REUSABLE_B, &ce->e_flags);
> mb_cache_entry_put(ea_block_cache, ce);
> }
> }
> @@ -2042,7 +2042,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
> }
> BHDR(new_bh)->h_refcount = cpu_to_le32(ref);
> if (ref == EXT4_XATTR_REFCOUNT_MAX)
> - ce->e_reusable = 0;
> + clear_bit(MBE_REUSABLE_B, &ce->e_flags);
> ea_bdebug(new_bh, "reusing; refcount now=%d",
> ref);
> ext4_xattr_block_csum_set(inode, new_bh);
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index e272ad738faf..2a4b8b549e93 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -100,8 +100,9 @@ int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
> atomic_set(&entry->e_refcnt, 2);
> entry->e_key = key;
> entry->e_value = value;
> - entry->e_reusable = reusable;
> - entry->e_referenced = 0;
> + entry->e_flags = 0;
> + if (reusable)
> + set_bit(MBE_REUSABLE_B, &entry->e_flags);
> head = mb_cache_entry_head(cache, key);
> hlist_bl_lock(head);
> hlist_bl_for_each_entry(dup, dup_node, head, e_hash_list) {
> @@ -165,7 +166,8 @@ static struct mb_cache_entry *__entry_find(struct mb_cache *cache,
> while (node) {
> entry = hlist_bl_entry(node, struct mb_cache_entry,
> e_hash_list);
> - if (entry->e_key == key && entry->e_reusable &&
> + if (entry->e_key == key &&
> + test_bit(MBE_REUSABLE_B, &entry->e_flags) &&
> atomic_inc_not_zero(&entry->e_refcnt))
> goto out;
> node = node->next;
> @@ -284,7 +286,7 @@ EXPORT_SYMBOL(mb_cache_entry_delete_or_get);
> void mb_cache_entry_touch(struct mb_cache *cache,
> struct mb_cache_entry *entry)
> {
> - entry->e_referenced = 1;
> + set_bit(MBE_REFERENCED_B, &entry->e_flags);
> }
> EXPORT_SYMBOL(mb_cache_entry_touch);
>
> @@ -309,9 +311,9 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
> entry = list_first_entry(&cache->c_list,
> struct mb_cache_entry, e_list);
> /* Drop initial hash reference if there is no user */
> - if (entry->e_referenced ||
> + if (test_bit(MBE_REFERENCED_B, &entry->e_flags) ||
> atomic_cmpxchg(&entry->e_refcnt, 1, 0) != 1) {
> - entry->e_referenced = 0;
> + clear_bit(MBE_REFERENCED_B, &entry->e_flags);
> list_move_tail(&entry->e_list, &cache->c_list);
> continue;
> }
> diff --git a/include/linux/mbcache.h b/include/linux/mbcache.h
> index 2da63fd7b98f..97e64184767d 100644
> --- a/include/linux/mbcache.h
> +++ b/include/linux/mbcache.h
> @@ -10,6 +10,12 @@
>
> struct mb_cache;
>
> +/* Cache entry flags */
> +enum {
> + MBE_REFERENCED_B = 0,
> + MBE_REUSABLE_B
> +};
> +
> struct mb_cache_entry {
> /* List of entries in cache - protected by cache->c_list_lock */
> struct list_head e_list;
> @@ -26,8 +32,7 @@ struct mb_cache_entry {
> atomic_t e_refcnt;
> /* Key in hash - stable during lifetime of the entry */
> u32 e_key;
> - u32 e_referenced:1;
> - u32 e_reusable:1;
> + unsigned long e_flags;
> /* User provided value - stable during lifetime of the entry */
> u64 e_value;
> };
> --
> 2.35.3
>

2022-11-22 12:11:48

by Jan Kara

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

On Mon 21-11-22 10:15:58, Jeremi Piotrowski wrote:
> On Mon, Nov 21, 2022 at 04:00:18PM +0100, Jan Kara wrote:
> > On Mon 21-11-22 14:35:59, Jan Kara wrote:
> > > On Fri 11-11-22 07:52:38, Jeremi Piotrowski wrote:
> > > > On Fri, Nov 11, 2022 at 07:10:29AM -0800, Jeremi Piotrowski wrote:
> > > > > On Fri, Nov 11, 2022 at 03:24:24PM +0100, Jan Kara wrote:
> > > > > > On Thu 10-11-22 11:27:01, Jeremi Piotrowski wrote:
> > > > > > > On Thu, Nov 10, 2022 at 04:26:37PM +0100, Jan Kara wrote:
> > > > > > > > On Thu 10-11-22 04:57:58, Jeremi Piotrowski wrote:
> > > > > > > > > On Wed, Oct 26, 2022 at 12:18:54PM +0200, Jan Kara wrote:
> > > > > > > > > > On Mon 24-10-22 18:32:51, Thilo Fromm wrote:
> > > > > > > > > > > Hello Honza,
> > > > > > > > > > >
> > > > > > > > > > > > Yeah, I was pondering about this for some time but still I have no clue who
> > > > > > > > > > > > could be holding the buffer lock (which blocks the task holding the
> > > > > > > > > > > > transaction open) or how this could related to the commit you have
> > > > > > > > > > > > identified. I have two things to try:
> > > > > > > > > > > >
> > > > > > > > > > > > 1) Can you please check whether the deadlock reproduces also with 6.0
> > > > > > > > > > > > kernel? The thing is that xattr handling code in ext4 has there some
> > > > > > > > > > > > additional changes, commit 307af6c8793 ("mbcache: automatically delete
> > > > > > > > > > > > entries from cache on freeing") in particular.
> > > > > > > > > > >
> > > > > > > > > > > This would be complex; we currently do not integrate 6.0 with Flatcar and
> > > > > > > > > > > would need to spend quite some effort ingesting it first (mostly, make sure
> > > > > > > > > > > the new kernel does not break something unrelated). Flatcar is an
> > > > > > > > > > > image-based distro, so kernel updates imply full distro updates.
> > > > > > > > > >
> > > > > > > > > > OK, understood.
> > > > > > > > > >
> > > > > > > > > > > > 2) I have created a debug patch (against 5.15.x stable kernel). Can you
> > > > > > > > > > > > please reproduce the failure with it and post the output of "echo w
> > > > > > > > > > > > > /proc/sysrq-trigger" and also the output the debug patch will put into the
> > > > > > > > > > > > kernel log? It will dump the information about buffer lock owner if we > cannot get the lock for more than 32 seconds.
> > > > > > > > > > >
> > > > > > > > > > > This would be more straightforward - I can reach out to one of our users
> > > > > > > > > > > suffering from the issue; they can reliably reproduce it and don't shy away
> > > > > > > > > > > from patching their kernel. Where can I find the patch?
> > > > > > > > > >
> > > > > > > > > > Ha, my bad. I forgot to attach it. Here it is.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Unfortunately this patch produced no output, but I have been able to repro so I
> > > > > > > > > understand why: except for the hung tasks, we have 1+ tasks busy-looping through
> > > > > > > > > the following code in ext4_xattr_block_set():
> > > > > > > > >
> > > > > > > > > inserted:
> > > > > > > > > if (!IS_LAST_ENTRY(s->first)) {
> > > > > > > > > new_bh = ext4_xattr_block_cache_find(inode, header(s->base),
> > > > > > > > > &ce);
> > > > > > > > > if (new_bh) {
> > > > > > > > > /* We found an identical block in the cache. */
> > > > > > > > > if (new_bh == bs->bh)
> > > > > > > > > ea_bdebug(new_bh, "keeping");
> > > > > > > > > else {
> > > > > > > > > u32 ref;
> > > > > > > > >
> > > > > > > > > WARN_ON_ONCE(dquot_initialize_needed(inode));
> > > > > > > > >
> > > > > > > > > /* The old block is released after updating
> > > > > > > > > the inode. */
> > > > > > > > > error = dquot_alloc_block(inode,
> > > > > > > > > EXT4_C2B(EXT4_SB(sb), 1));
> > > > > > > > > if (error)
> > > > > > > > > goto cleanup;
> > > > > > > > > BUFFER_TRACE(new_bh, "get_write_access");
> > > > > > > > > error = ext4_journal_get_write_access(
> > > > > > > > > handle, sb, new_bh,
> > > > > > > > > EXT4_JTR_NONE);
> > > > > > > > > if (error)
> > > > > > > > > goto cleanup_dquot;
> > > > > > > > > lock_buffer(new_bh);
> > > > > > > > > /*
> > > > > > > > > * We have to be careful about races with
> > > > > > > > > * adding references to xattr block. Once we
> > > > > > > > > * hold buffer lock xattr block's state is
> > > > > > > > > * stable so we can check the additional
> > > > > > > > > * reference fits.
> > > > > > > > > */
> > > > > > > > > ref = le32_to_cpu(BHDR(new_bh)->h_refcount) + 1;
> > > > > > > > > if (ref > EXT4_XATTR_REFCOUNT_MAX) {
> > > > > > > > > /*
> > > > > > > > > * Undo everything and check mbcache
> > > > > > > > > * again.
> > > > > > > > > */
> > > > > > > > > unlock_buffer(new_bh);
> > > > > > > > > dquot_free_block(inode,
> > > > > > > > > EXT4_C2B(EXT4_SB(sb),
> > > > > > > > > 1));
> > > > > > > > > brelse(new_bh);
> > > > > > > > > mb_cache_entry_put(ea_block_cache, ce);
> > > > > > > > > ce = NULL;
> > > > > > > > > new_bh = NULL;
> > > > > > > > > goto inserted;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > The tasks keep taking the 'goto inserted' branch, and never finish. I've been
> > > > > > > > > able to repro with kernel v6.0.7 as well.
> > > > > > > >
> > > > > > > > Interesting! That makes is much clearer (and also makes my debug patch
> > > > > > > > unnecessary). So clearly the e_reusable variable in the mb_cache_entry got
> > > > > > > > out of sync with the number of references really in the xattr block - in
> > > > > > > > particular the block likely has h_refcount >= EXT4_XATTR_REFCOUNT_MAX but
> > > > > > > > e_reusable is set to true. Now I can see how e_reusable can stay at false due
> > > > > > > > to a race when refcount is actually smaller but I don't see how it could
> > > > > > > > stay at true when refcount is big enough - that part seems to be locked
> > > > > > > > properly. If you can reproduce reasonably easily, can you try reproducing
> > > > > > > > with attached patch? Thanks!
> > > > > > > >
> > > > > > >
> > > > > > > Sure, with that patch I'm getting the following output, reusable is false on
> > > > > > > most items until we hit something with reusable true and then that loops
> > > > > > > indefinitely:
> > > > > >
> > > > > > Thanks. So that is what I've suspected. I'm still not 100% clear on how
> > > > > > this inconsistency can happen although I have a suspicion - does attached
> > > > > > patch fix the problem for you?
> > > > > >
> > > > > > Also is it possible to share the reproducer or it needs some special
> > > > > > infrastructure?
> > > > > >
> > > > > > Honza
> > > > >
> > > > > I'll test the patch and report back.
> > > > >
> > > > > Attached you'll find the reproducer, for me it reproduces within a few minutes.
> > > > > It brings up a k8s node and then runs 3 instances of the application which
> > > > > creates a lot of small files in a loop. The OS we run it on has selinux enabled
> > > > > in permissive mode, that might play a role.
> > > > >
> > > >
> > > > I can still reproduce it with the patch.
> > >
> > > Thanks for the answer! So I was trying to make your reproducer work on my
> > > system but it was not so easy on openSUSE ;). Anyway, when working on this
> > > I've realized there may be a simpler way to tickle the bug and indeed, I
> > > can now trigger it with a simple C program. So thanks for your help, I'm
> > > now debugging the issue on my system.
> >
> > OK, attached patch fixes the deadlock for me. Can you test whether it fixes
> > the problem for you as well? Thanks!
>
> I'll test the fix tomorrow, but I've noticed it doesn't apply cleanly to
> 5.15.78, which seems to be missing:
>
> - 5fc4cbd9fde5d4630494fd6ffc884148fb618087 mbcache: Avoid nesting of cache->c_list_lock under bit locks
> (this one is marked for stable but not in 5.15?)
> - 307af6c879377c1c63e71cbdd978201f9c7ee8df mbcache: automatically delete entries from cache on freeing
> (this one is not marked for stable)
>
> So either a special backport is needed or these two would need to be applied as
> well.

Right. The fix is against current mainline kernel. Stable tree backports
are a second step once the fix is confirmed to work :). Let me know in case
you need help with porting to the kernel version you need for testing.

Honza

--
Jan Kara <[email protected]>
SUSE Labs, CR

2022-11-22 17:59:41

by Jeremi Piotrowski

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

From e7ec42e181c6213d1fd71b946196f05af601ba5c Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Mon, 21 Nov 2022 15:44:10 +0100
Subject: [PATCH] ext4: Fix deadlock due to mbcache entry corruption

When manipulating xattr blocks, we can deadlock infinitely looping
inside ext4_xattr_block_set() where we constantly keep finding xattr
block for reuse in mbcache but we are unable to reuse it because its
reference count is too big. This happens because cache entry for the
xattr block is marked as reusable (e_reusable set) although its
reference count is too big. When this inconsistency happens, this
inconsistent state is kept indefinitely and so ext4_xattr_block_set()
keeps retrying indefinitely.

The inconsistent state is caused by non-atomic update of e_reusable bit.
e_reusable is part of a bitfield and e_reusable update can race with
update of e_referenced bit in the same bitfield resulting in loss of one
of the updates. Fix the problem by using atomic bitops instead.

CC: [email protected]
Fixes: 6048c64b2609 ("mbcache: add reusable flag to cache entries")
Reported-by: Jeremi Piotrowski <[email protected]>
Reported-by: Thilo Fromm <[email protected]>
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/xattr.c | 4 ++--
fs/mbcache.c | 14 ++++++++------
include/linux/mbcache.h | 9 +++++++--
3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 533216e80fa2..22700812a4d3 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1281,7 +1281,7 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
ce = mb_cache_entry_get(ea_block_cache, hash,
bh->b_blocknr);
if (ce) {
- ce->e_reusable = 1;
+ set_bit(MBE_REUSABLE_B, &ce->e_flags);
mb_cache_entry_put(ea_block_cache, ce);
}
}
@@ -2042,7 +2042,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
}
BHDR(new_bh)->h_refcount = cpu_to_le32(ref);
if (ref == EXT4_XATTR_REFCOUNT_MAX)
- ce->e_reusable = 0;
+ clear_bit(MBE_REUSABLE_B, &ce->e_flags);
ea_bdebug(new_bh, "reusing; refcount now=%d",
ref);
ext4_xattr_block_csum_set(inode, new_bh);
diff --git a/fs/mbcache.c b/fs/mbcache.c
index 2010bc80a3f2..ac07b50ea3df 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -94,8 +94,9 @@ int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
atomic_set(&entry->e_refcnt, 1);
entry->e_key = key;
entry->e_value = value;
- entry->e_reusable = reusable;
- entry->e_referenced = 0;
+ entry->e_flags = 0;
+ if (reusable)
+ set_bit(MBE_REUSABLE_B, &entry->e_flags);
head = mb_cache_entry_head(cache, key);
hlist_bl_lock(head);
hlist_bl_for_each_entry(dup, dup_node, head, e_hash_list) {
@@ -155,7 +156,8 @@ static struct mb_cache_entry *__entry_find(struct mb_cache *cache,
while (node) {
entry = hlist_bl_entry(node, struct mb_cache_entry,
e_hash_list);
- if (entry->e_key == key && entry->e_reusable) {
+ if (entry->e_key == key &&
+ test_bit(MBE_REUSABLE_B, &entry->e_flags)) {
atomic_inc(&entry->e_refcnt);
goto out;
}
@@ -325,7 +327,7 @@ EXPORT_SYMBOL(mb_cache_entry_delete_or_get);
void mb_cache_entry_touch(struct mb_cache *cache,
struct mb_cache_entry *entry)
{
- entry->e_referenced = 1;
+ set_bit(MBE_REFERENCED_B, &entry->e_flags);
}
EXPORT_SYMBOL(mb_cache_entry_touch);

@@ -350,8 +352,8 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
while (nr_to_scan-- && !list_empty(&cache->c_list)) {
entry = list_first_entry(&cache->c_list,
struct mb_cache_entry, e_list);
- if (entry->e_referenced || atomic_read(&entry->e_refcnt) > 2) {
- entry->e_referenced = 0;
+ if (test_bit(MBE_REFERENCED_B, &entry->e_flags) || atomic_read(&entry->e_refcnt) > 2) {
+ clear_bit(MBE_REFERENCED_B, &entry->e_flags);
list_move_tail(&entry->e_list, &cache->c_list);
continue;
}
diff --git a/include/linux/mbcache.h b/include/linux/mbcache.h
index 8eca7f25c432..62927f7e2588 100644
--- a/include/linux/mbcache.h
+++ b/include/linux/mbcache.h
@@ -10,6 +10,12 @@

struct mb_cache;

+/* Cache entry flags */
+enum {
+ MBE_REFERENCED_B = 0,
+ MBE_REUSABLE_B
+};
+
struct mb_cache_entry {
/* List of entries in cache - protected by cache->c_list_lock */
struct list_head e_list;
@@ -18,8 +24,7 @@ struct mb_cache_entry {
atomic_t e_refcnt;
/* Key in hash - stable during lifetime of the entry */
u32 e_key;
- u32 e_referenced:1;
- u32 e_reusable:1;
+ unsigned long e_flags;
/* User provided value - stable during lifetime of the entry */
u64 e_value;
};
--
2.25.1

Attachments:

(No filename) (1.22 kB)
0001-ext4-Fix-deadlock-due-to-mbcache-en.patch (4.68 kB)
Download all attachments

2022-11-23 10:09:01

by Linux regression tracking (Thorsten Leemhuis)

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates #forregzbot

2022-11-23 19:51:56

by Jan Kara

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

On Tue 22-11-22 09:48:07, Jeremi Piotrowski wrote:
> On Tue, Nov 22, 2022 at 12:57:15PM +0100, Jan Kara wrote:
> > On Mon 21-11-22 10:15:58, Jeremi Piotrowski wrote:
> > > On Mon, Nov 21, 2022 at 04:00:18PM +0100, Jan Kara wrote:
> > > >
> > > > OK, attached patch fixes the deadlock for me. Can you test whether it fixes
> > > > the problem for you as well? Thanks!
> > >
> > > I'll test the fix tomorrow, but I've noticed it doesn't apply cleanly to
> > > 5.15.78, which seems to be missing:
> > >
> > > - 5fc4cbd9fde5d4630494fd6ffc884148fb618087 mbcache: Avoid nesting of cache->c_list_lock under bit locks
> > > (this one is marked for stable but not in 5.15?)
> > > - 307af6c879377c1c63e71cbdd978201f9c7ee8df mbcache: automatically delete entries from cache on freeing
> > > (this one is not marked for stable)
> > >
> > > So either a special backport is needed or these two would need to be applied as
> > > well.
> >
> > Right. The fix is against current mainline kernel. Stable tree backports
> > are a second step once the fix is confirmed to work :). Let me know in case
> > you need help with porting to the kernel version you need for testing.
> >
>
> I confirm that this patch fixes things with the more complicated reproducer :)
> Attached is my tweaked version of the patch that applies against 5.15.

Thanks for testing! I've added your Tested-by tag and submitted patch for
inclusion.

Honza

> From e7ec42e181c6213d1fd71b946196f05af601ba5c Mon Sep 17 00:00:00 2001
> From: Jan Kara <[email protected]>
> Date: Mon, 21 Nov 2022 15:44:10 +0100
> Subject: [PATCH] ext4: Fix deadlock due to mbcache entry corruption
>
> When manipulating xattr blocks, we can deadlock infinitely looping
> inside ext4_xattr_block_set() where we constantly keep finding xattr
> block for reuse in mbcache but we are unable to reuse it because its
> reference count is too big. This happens because cache entry for the
> xattr block is marked as reusable (e_reusable set) although its
> reference count is too big. When this inconsistency happens, this
> inconsistent state is kept indefinitely and so ext4_xattr_block_set()
> keeps retrying indefinitely.
>
> The inconsistent state is caused by non-atomic update of e_reusable bit.
> e_reusable is part of a bitfield and e_reusable update can race with
> update of e_referenced bit in the same bitfield resulting in loss of one
> of the updates. Fix the problem by using atomic bitops instead.
>
> CC: [email protected]
> Fixes: 6048c64b2609 ("mbcache: add reusable flag to cache entries")
> Reported-by: Jeremi Piotrowski <[email protected]>
> Reported-by: Thilo Fromm <[email protected]>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/ext4/xattr.c | 4 ++--
> fs/mbcache.c | 14 ++++++++------
> include/linux/mbcache.h | 9 +++++++--
> 3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 533216e80fa2..22700812a4d3 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1281,7 +1281,7 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
> ce = mb_cache_entry_get(ea_block_cache, hash,
> bh->b_blocknr);
> if (ce) {
> - ce->e_reusable = 1;
> + set_bit(MBE_REUSABLE_B, &ce->e_flags);
> mb_cache_entry_put(ea_block_cache, ce);
> }
> }
> @@ -2042,7 +2042,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
> }
> BHDR(new_bh)->h_refcount = cpu_to_le32(ref);
> if (ref == EXT4_XATTR_REFCOUNT_MAX)
> - ce->e_reusable = 0;
> + clear_bit(MBE_REUSABLE_B, &ce->e_flags);
> ea_bdebug(new_bh, "reusing; refcount now=%d",
> ref);
> ext4_xattr_block_csum_set(inode, new_bh);
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index 2010bc80a3f2..ac07b50ea3df 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -94,8 +94,9 @@ int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key,
> atomic_set(&entry->e_refcnt, 1);
> entry->e_key = key;
> entry->e_value = value;
> - entry->e_reusable = reusable;
> - entry->e_referenced = 0;
> + entry->e_flags = 0;
> + if (reusable)
> + set_bit(MBE_REUSABLE_B, &entry->e_flags);
> head = mb_cache_entry_head(cache, key);
> hlist_bl_lock(head);
> hlist_bl_for_each_entry(dup, dup_node, head, e_hash_list) {
> @@ -155,7 +156,8 @@ static struct mb_cache_entry *__entry_find(struct mb_cache *cache,
> while (node) {
> entry = hlist_bl_entry(node, struct mb_cache_entry,
> e_hash_list);
> - if (entry->e_key == key && entry->e_reusable) {
> + if (entry->e_key == key &&
> + test_bit(MBE_REUSABLE_B, &entry->e_flags)) {
> atomic_inc(&entry->e_refcnt);
> goto out;
> }
> @@ -325,7 +327,7 @@ EXPORT_SYMBOL(mb_cache_entry_delete_or_get);
> void mb_cache_entry_touch(struct mb_cache *cache,
> struct mb_cache_entry *entry)
> {
> - entry->e_referenced = 1;
> + set_bit(MBE_REFERENCED_B, &entry->e_flags);
> }
> EXPORT_SYMBOL(mb_cache_entry_touch);
>
> @@ -350,8 +352,8 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
> while (nr_to_scan-- && !list_empty(&cache->c_list)) {
> entry = list_first_entry(&cache->c_list,
> struct mb_cache_entry, e_list);
> - if (entry->e_referenced || atomic_read(&entry->e_refcnt) > 2) {
> - entry->e_referenced = 0;
> + if (test_bit(MBE_REFERENCED_B, &entry->e_flags) || atomic_read(&entry->e_refcnt) > 2) {
> + clear_bit(MBE_REFERENCED_B, &entry->e_flags);
> list_move_tail(&entry->e_list, &cache->c_list);
> continue;
> }
> diff --git a/include/linux/mbcache.h b/include/linux/mbcache.h
> index 8eca7f25c432..62927f7e2588 100644
> --- a/include/linux/mbcache.h
> +++ b/include/linux/mbcache.h
> @@ -10,6 +10,12 @@
>
> struct mb_cache;
>
> +/* Cache entry flags */
> +enum {
> + MBE_REFERENCED_B = 0,
> + MBE_REUSABLE_B
> +};
> +
> struct mb_cache_entry {
> /* List of entries in cache - protected by cache->c_list_lock */
> struct list_head e_list;
> @@ -18,8 +24,7 @@ struct mb_cache_entry {
> atomic_t e_refcnt;
> /* Key in hash - stable during lifetime of the entry */
> u32 e_key;
> - u32 e_referenced:1;
> - u32 e_reusable:1;
> + unsigned long e_flags;
> /* User provided value - stable during lifetime of the entry */
> u64 e_value;
> };
> --
> 2.25.1
>

--
Jan Kara <[email protected]>
SUSE Labs, CR

2023-04-30 23:41:18

by Theodore Ts'o

[permalink] [raw]

Subject: Re: [syzbot] possible deadlock in jbd2_journal_lock_updates

#syz fix: ext4, jbd2: add an optimized bmap for the journal inode