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

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

[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;

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

[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+51244a05705883616c95@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug\?extid\=51244a05705883616c95
Signed-off-by: Szymon Wilczek <swilczek.lx@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