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] 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+51244a05705883616c95@syzkaller.appspotmail.com
Tested-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 | 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

[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