[PATCH] ocfs2: fix circular locking dependency in ocfs2_acquire_dquot

[Szymon Wilczek]

Move ocfs2_extend_no_holes() to execute before ocfs2_lock_global_qf() to
fix a circular locking dependency reported by syzbot.

The issue occurs because ocfs2_extend_no_holes() internally calls
ocfs2_extend_allocation() which starts a transaction (acquiring
sb_start_intwrite). When called while holding the global quota file
lock, this conflicts with mount-time operations that acquire
sb_internal first, creating the following circular dependency:

sb_internal -> ocfs2_sysfile_lock_key -> ocfs2_quota_ip_alloc_sem_key

By moving the quota file extension before acquiring the global quota
file lock, we ensure that any internal transactions complete before
quota locks are held, breaking the circular dependency.

Reported-by: syzbot+51244a...@syzkaller.appspotmail.com
Tested-by: syzbot+51244a...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=51244a05705883616c95
Signed-off-by: Szymon Wilczek <swilc...@gmail.com>
---
fs/ocfs2/quota_global.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index e85b1ccf81be..136aaaae27f3 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -821,6 +821,19 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)
trace_ocfs2_acquire_dquot(from_kqid(&init_user_ns, dquot->dq_id),
type);
mutex_lock(&dquot->dq_lock);
+ /*
+ * Extend global quota file before acquiring global qf lock to avoid
+ * lock inversion with sb_internal (via ocfs2_start_trans).
+ */
+ if (need_alloc) {
+ WARN_ON(journal_current_handle());
+ status = ocfs2_extend_no_holes(gqinode, NULL,
+ i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits),
+ i_size_read(gqinode));
+ if (status < 0)
+ goto out;
+ }
+
/*
* We need an exclusive lock, because we're going to update use count
* and instantiate possibly new dquot structure
@@ -843,19 +856,8 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)
OCFS2_DQUOT(dquot)->dq_use_count++;
OCFS2_DQUOT(dquot)->dq_origspace = dquot->dq_dqb.dqb_curspace;
OCFS2_DQUOT(dquot)->dq_originodes = dquot->dq_dqb.dqb_curinodes;
- if (!dquot->dq_off) { /* No real quota entry? */
+ if (!dquot->dq_off) /* No real quota entry? */
ex = 1;
- /*
- * Add blocks to quota file before we start a transaction since
- * locking allocators ranks above a transaction start
- */
- WARN_ON(journal_current_handle());
- status = ocfs2_extend_no_holes(gqinode, NULL,
- i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits),
- i_size_read(gqinode));
- if (status < 0)
- goto out_dq;
- }

handle = ocfs2_start_trans(osb,
ocfs2_calc_global_qinit_credits(sb, type));
--
2.52.0

[Andrew Morton]

On Sat, 27 Dec 2025 18:42:51 +0100 Szymon Wilczek <swilc...@gmail.com> wrote:

> Move ocfs2_extend_no_holes() to execute before ocfs2_lock_global_qf() to
> fix a circular locking dependency reported by syzbot.
>
> The issue occurs because ocfs2_extend_no_holes() internally calls
> ocfs2_extend_allocation() which starts a transaction (acquiring
> sb_start_intwrite). When called while holding the global quota file
> lock, this conflicts with mount-time operations that acquire
> sb_internal first, creating the following circular dependency:
>
> sb_internal -> ocfs2_sysfile_lock_key -> ocfs2_quota_ip_alloc_sem_key
>
> By moving the quota file extension before acquiring the global quota
> file lock, we ensure that any internal transactions complete before
> quota locks are held, breaking the circular dependency.

Thanks. This might have escaped reviewer attention due to the holiday
season. I'll add it to mm.git's mm-nonmm-unstable branch for runtime
testing while we await review (please!).

[Joseph Qi]

On 2025/12/28 01:42, Szymon Wilczek wrote:
> Move ocfs2_extend_no_holes() to execute before ocfs2_lock_global_qf() to
> fix a circular locking dependency reported by syzbot.
>
> The issue occurs because ocfs2_extend_no_holes() internally calls
> ocfs2_extend_allocation() which starts a transaction (acquiring
> sb_start_intwrite). When called while holding the global quota file
> lock, this conflicts with mount-time operations that acquire

It seems the following locking sequence is fine:
ocfs2_lock_global_qf -> start_trans

So could you please elaborate more?

BTW, even if your analysis is right, here also calls ocfs2_start_trans().

Thanks,
Joseph

[syzbot]

> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

This crash does not have a reproducer. I cannot test it.

> master
>
> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> index e85b1ccf81be..95bb901820cf 100644
> --- a/fs/ocfs2/quota_global.c
> +++ b/fs/ocfs2/quota_global.c
> @@ -821,13 +821,35 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)

> trace_ocfs2_acquire_dquot(from_kqid(&init_user_ns, dquot->dq_id),
> type);
> mutex_lock(&dquot->dq_lock);
> +

> + /*
> + * Speculatively extend quota file before acquiring any locks or
> + * starting transaction to avoid lock inversion. sb_start_intwrite
> + * (via ocfs2_start_trans) ranks above quota file locks.

> + */
> + if (need_alloc) {
> + WARN_ON(journal_current_handle());
> + status = ocfs2_extend_no_holes(gqinode, NULL,
> + i_size_read(gqinode) + (need_alloc <<
> sb->s_blocksize_bits),
> + i_size_read(gqinode));
> + if (status < 0)
> + goto out;
> + }
> +

> + handle = ocfs2_start_trans(osb,
> + ocfs2_calc_global_qinit_credits(sb, type));
> + if (IS_ERR(handle)) {
> + status = PTR_ERR(handle);

> + goto out;
> + }
> +
> /*
> * We need an exclusive lock, because we're going to update use count
> * and instantiate possibly new dquot structure

> */
> status = ocfs2_lock_global_qf(info, 1);
> if (status < 0)
> - goto out;
> + goto out_trans;
> status = ocfs2_qinfo_lock(info, 0);
> if (status < 0)
> goto out_dq;
> @@ -843,29 +865,12 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)

> OCFS2_DQUOT(dquot)->dq_use_count++;
> OCFS2_DQUOT(dquot)->dq_origspace = dquot->dq_dqb.dqb_curspace;
> OCFS2_DQUOT(dquot)->dq_originodes = dquot->dq_dqb.dqb_curinodes;
> - if (!dquot->dq_off) { /* No real quota entry? */
> + if (!dquot->dq_off) /* No real quota entry? */
> ex = 1;
> - /*
> - * Add blocks to quota file before we start a transaction since
> - * locking allocators ranks above a transaction start
> - */
> - WARN_ON(journal_current_handle());
> - status = ocfs2_extend_no_holes(gqinode, NULL,
> - i_size_read(gqinode) + (need_alloc <<
> sb->s_blocksize_bits),
> - i_size_read(gqinode));
> - if (status < 0)
> - goto out_dq;
> - }
>

> - handle = ocfs2_start_trans(osb,
> - ocfs2_calc_global_qinit_credits(sb, type));
> - if (IS_ERR(handle)) {
> - status = PTR_ERR(handle);
> - goto out_dq;
> - }
> status = ocfs2_qinfo_lock(info, ex);
> if (status < 0)
> - goto out_trans;
> + goto out_dq;
> status = qtree_write_dquot(&info->dqi_gi, dquot);
> if (ex && info_dirty(sb_dqinfo(sb, type))) {
> err = __ocfs2_global_write_info(sb, type);
> @@ -873,10 +878,10 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)
> status = err;
> }
> ocfs2_qinfo_unlock(info, ex);
> -out_trans:
> - ocfs2_commit_trans(osb, handle);
> out_dq:
> ocfs2_unlock_global_qf(info, 1);
> +out_trans:
> + ocfs2_commit_trans(osb, handle);
> if (status < 0)
> goto out;

[Szymon Wilczek]

(Resending to mailing lists - apologies for the private reply)

Hi Joseph,

Thank you for the review.

You are right - my v1 patch was incomplete. Looking at the lockdep
trace again, the issue is that ocfs2_start_trans() acquires sb_internal
while holding ip_alloc_sem (taken by ocfs2_lock_global_qf). This
conflicts with freeze/dismount paths that acquire sb_internal first.

The chain reported by lockdep:
sb_internal -> sysfile_lock_key -> ip_alloc_sem

Problematic sequence was:
ip_alloc_sem (via lock_global_qf) -> sb_internal (via start_trans)

This is a lock inversion.

You correctly identified that my v1 patch left the direct
ocfs2_start_trans() call after ocfs2_lock_global_qf(), which doesn't
fix the problem.

I'm preparing a v2 patch that also moves ocfs2_start_trans() before
ocfs2_lock_global_qf() to properly fix the lock ordering.

Thanks,
Szymon

[Szymon Wilczek]

Move ocfs2_extend_no_holes() and ocfs2_start_trans() to execute before

ocfs2_lock_global_qf() to fix a circular locking dependency reported
by syzbot.

The issue occurs because ocfs2_start_trans() acquires sb_internal (via
sb_start_intwrite) while already holding quota file locks (ip_alloc_sem
and sysfile_lock_key). This conflicts with freeze/dismount paths that

acquire sb_internal first, creating the following circular dependency:

sb_internal -> sysfile_lock_key -> ip_alloc_sem (freeze/dismount)
ip_alloc_sem -> sysfile_lock_key -> sb_internal (quota acquire)

Fix this by reordering the operations:
1. Speculatively extend quota file (uses need_alloc estimate)
2. Start transaction
3. Acquire global quota file lock
4. Perform quota operations
5. Release lock
6. Commit transaction

This ensures sb_internal is acquired before quota file locks, matching
the expected kernel lock order.

Reported-by: syzbot+51244a...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug\?extid\=51244a05705883616c95

Signed-off-by: Szymon Wilczek <swilc...@gmail.com>
---

fs/ocfs2/quota_global.c | 49 +++++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index e85b1ccf81be..95bb901820cf 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -821,13 +821,35 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)

trace_ocfs2_acquire_dquot(from_kqid(&init_user_ns, dquot->dq_id),
type);
mutex_lock(&dquot->dq_lock);
+

+ /*
+ * Speculatively extend quota file before acquiring any locks or
+ * starting transaction to avoid lock inversion. sb_start_intwrite
+ * (via ocfs2_start_trans) ranks above quota file locks.

+ */
+ if (need_alloc) {
+ WARN_ON(journal_current_handle());
+ status = ocfs2_extend_no_holes(gqinode, NULL,
+ i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits),
+ i_size_read(gqinode));
+ if (status < 0)
+ goto out;
+ }
+

+ handle = ocfs2_start_trans(osb,
+ ocfs2_calc_global_qinit_credits(sb, type));
+ if (IS_ERR(handle)) {
+ status = PTR_ERR(handle);

+ goto out;
+ }
+
/*
* We need an exclusive lock, because we're going to update use count
* and instantiate possibly new dquot structure

*/
status = ocfs2_lock_global_qf(info, 1);

if (status < 0)

- goto out;
+ goto out_trans;
status = ocfs2_qinfo_lock(info, 0);
if (status < 0)
goto out_dq;

@@ -843,29 +865,12 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)

OCFS2_DQUOT(dquot)->dq_use_count++;
OCFS2_DQUOT(dquot)->dq_origspace = dquot->dq_dqb.dqb_curspace;
OCFS2_DQUOT(dquot)->dq_originodes = dquot->dq_dqb.dqb_curinodes;
- if (!dquot->dq_off) { /* No real quota entry? */
+ if (!dquot->dq_off) /* No real quota entry? */
ex = 1;
- /*
- * Add blocks to quota file before we start a transaction since
- * locking allocators ranks above a transaction start
- */
- WARN_ON(journal_current_handle());
- status = ocfs2_extend_no_holes(gqinode, NULL,
- i_size_read(gqinode) + (need_alloc << sb->s_blocksize_bits),
- i_size_read(gqinode));
- if (status < 0)
- goto out_dq;
- }

- handle = ocfs2_start_trans(osb,
- ocfs2_calc_global_qinit_credits(sb, type));
- if (IS_ERR(handle)) {
- status = PTR_ERR(handle);
- goto out_dq;
- }
status = ocfs2_qinfo_lock(info, ex);

if (status < 0)

- goto out_trans;
+ goto out_dq;
status = qtree_write_dquot(&info->dqi_gi, dquot);
if (ex && info_dirty(sb_dqinfo(sb, type))) {
err = __ocfs2_global_write_info(sb, type);
@@ -873,10 +878,10 @@ static int ocfs2_acquire_dquot(struct dquot *dquot)
status = err;
}
ocfs2_qinfo_unlock(info, ex);
-out_trans:
- ocfs2_commit_trans(osb, handle);
out_dq:
ocfs2_unlock_global_qf(info, 1);
+out_trans:
+ ocfs2_commit_trans(osb, handle);
if (status < 0)
goto out;

--
2.52.0

[Joseph Qi]

On 2026/1/8 22:15, Szymon Wilczek wrote:
> (Resending to mailing lists - apologies for the private reply)
>
> Hi Joseph,
>
> Thank you for the review.
>
> You are right - my v1 patch was incomplete. Looking at the lockdep
> trace again, the issue is that ocfs2_start_trans() acquires sb_internal
> while holding ip_alloc_sem (taken by ocfs2_lock_global_qf). This
> conflicts with freeze/dismount paths that acquire sb_internal first.
>
> The chain reported by lockdep:
> sb_internal -> sysfile_lock_key -> ip_alloc_sem
>
> Problematic sequence was:
> ip_alloc_sem (via lock_global_qf) -> sb_internal (via start_trans)
>

According to description of locking dependencies in quota_global.c,
this is the designed order.

Thanks,
Joseph

[Szymon Wilczek]

Hi Joseph,

Thank you for pointing to the designed order in quota_global.c.

I looked at the locking documentation. I see that
"Acquire dquot for the first time" specifies:
ocfs2_lock_global_qf -> start_trans

However, I believe the v2 patch is still necessary because lockdep
detected a real circular dependency:

sb_internal -> sysfile_lock_key -> ip_alloc_sem

Since ocfs2_lock_global_qf takes ip_alloc_sem, and start_trans takes
sb_internal (via sb_start_intwrite), the documented order inverts the
chain above, creating an ABBA deadlock with freeze/dismount paths.

The designed order may have been correct before sb_start_intwrite
became part of ocfs2_start_trans, but now this sequence conflicts
with the VFS freeze mechanism.

My patch ensures sb_internal is acquired before ip_alloc_sem, which
matches the expected freeze ordering. Perhaps the documentation in
quota_global.c should be updated to reflect this change?

If you see a problem with this approach, I'm happy to adjust.
What would you suggest?

Thanks,
Szymon

[Joseph Qi]

On 1/9/26 10:02 AM, Szymon Wilczek wrote:
> Hi Joseph,
>
> Thank you for pointing to the designed order in quota_global.c.
>
> I looked at the locking documentation. I see that
> "Acquire dquot for the first time" specifies:
> ocfs2_lock_global_qf -> start_trans
>

Ummm... When sync and release also is

ocfs2_lock_global_qf -> start_trans

> However, I believe the v2 patch is still necessary because lockdep
> detected a real circular dependency:
>
> sb_internal -> sysfile_lock_key -> ip_alloc_sem
>
> Since ocfs2_lock_global_qf takes ip_alloc_sem, and start_trans takes
> sb_internal (via sb_start_intwrite), the documented order inverts the
> chain above, creating an ABBA deadlock with freeze/dismount paths.
>

From the report link, start_trans is called during
ocfs2_shutdown_local_alloc(), but ocfs2_disable_quotas() is fnished now.
So how it happens?

Thanks,
Joseph

[Szymon Wilczek]

Hi Joseph,

You are absolutely correct. Since ocfs2_disable_quotas() completes before
ocfs2_shutdown_local_alloc(), these two specific paths likely cannot race
at runtime.

However, the Lockdep report is based on lock classes, not just runtime
concurrency.

1. The shutdown path (ocfs2_shutdown_local_alloc) teaches Lockdep:
sb_internal -> ocfs2_sysfile_lock_key (on Local Alloc inode)

2. The acquire path (ocfs2_acquire_dquot) teaches Lockdep:
ocfs2_sysfile_lock_key (on Quota inode) -> sb_internal

Because both inodes use the same lock class (ocfs2_sysfile_lock_key),
Lockdep sees a contradiction:
sb_internal -> sysfile -> sb_internal

Even if they don't race, this inconsistent ordering is a violation
that Lockdep flags. Fixing the order in acquire_dquot resolves this
contradiction and ensures sb_internal (freeze protection) is always
taken outermost.

Thanks,
Szymon