Re: [PATCH] ocfs2: fix circular locking dependency in ocfs2_acquire_dquot

[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

On Thu, Jan 8, 2026 at 9:43 AM Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> 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?
>
> > 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+51244a05705883616c95@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Tested-by: syzbot+51244a05705883616c95@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=51244a05705883616c95
> > Signed-off-by: Szymon Wilczek <swilczek.lx@xxxxxxxxx>
> > ---
> >  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));
>
> BTW, even if your analysis is right, here also calls ocfs2_start_trans().
>
> Thanks,
> Joseph
>