linux-kernel - Re: [PATCH] ocfs2: fix circular locking dependency in ocfs2_acquire

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <25d90176-743e-4638-9643-861bd5792eed@linux.alibaba.com>
Date: Fri, 9 Jan 2026 09:07:53 +0800
From: Joseph Qi <joseph.qi@...ux.alibaba.com>
To: Szymon Wilczek <swilczek.lx@...il.com>
Cc: ocfs2-devel@...ts.linux.dev, mark@...heh.com, jlbec@...lplan.org,
 linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
 syzbot+51244a05705883616c95@...kaller.appspotmail.com
Subject: Re: [PATCH] ocfs2: fix circular locking dependency in
 ocfs2_acquire_dquot



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 

> 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@...ux.alibaba.com> 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@...kaller.appspotmail.com
>>> Tested-by: syzbot+51244a05705883616c95@...kaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=51244a05705883616c95
>>> Signed-off-by: Szymon Wilczek <swilczek.lx@...il.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));
>>
>> BTW, even if your analysis is right, here also calls ocfs2_start_trans().
>>
>> Thanks,
>> Joseph
>>