[PATCH] ocfs2: fix ABBA deadlock in suballocator reclaim

[PATCH] ocfs2: fix ABBA deadlock in suballocator reclaim

[ThangNN99]

_ocfs2_reclaim_suballoc_to_main() runs inside an already-started
transaction (j_trans_barrier held) and locks the global bitmap inode
to return clusters to it. Every other allocation path takes that
inode lock *before* starting a transaction, so this reverses the
order and can deadlock:

  CPU0                          CPU1
  rlock(j_trans_barrier)
                                lock(sb_internal)
                                lock(j_trans_barrier)
  lock(GLOBAL_BITMAP inode)

Since reclaim is best-effort (its caller already ignores the return
value), fix it by acquiring the global bitmap inode with trylock,
before mutating anything, and bailing out to skip reclaim on a busy
lock instead of blocking in the wrong order.

Reproduced with the syzbot C repro under QEMU: the unpatched kernel
hits the lockdep splat on the very first mount+mkdir+rmdir cycle
(~10s in); the patched kernel ran the same cycle 169 times with zero
splats, and instrumentation confirmed reclaim keeps running (trylock
succeeds) rather than being silently skipped.

Reported-by: syzbot+73d1166b94ed9875af54@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=73d1166b94ed9875af54
Fixes: 4a54331616b3 ("ocfs2: give ocfs2 the ability to reclaim suballocator free bg")
Cc: stable@vger.kernel.org
Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
---
 fs/ocfs2/suballoc.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index 20c3aec6b987..085af9ba38ab 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -2716,18 +2716,39 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 	idx = le16_to_cpu(group->bg_chain);
 	rec = &(cl->cl_recs[idx]);
 
+	/*
+	 * We already hold j_trans_barrier, while everywhere else locks the
+	 * allocator inode before starting a transaction. Trylock here, before
+	 * mutating anything, so a busy lock skips this best-effort reclaim
+	 * instead of inverting that order into an ABBA deadlock.
+	 */
+	main_bm_inode = ocfs2_get_system_file_inode(osb,
+						    GLOBAL_BITMAP_SYSTEM_INODE,
+						    OCFS2_INVALID_SLOT);
+	if (!main_bm_inode)
+		goto bail; /* ignore the error in reclaim path */
+
+	if (!inode_trylock(main_bm_inode)) {
+		iput(main_bm_inode);
+		goto bail; /* ignore the error in reclaim path */
+	}
+
+	status = ocfs2_try_inode_lock(main_bm_inode, &main_bm_bh, 1);
+	if (status < 0)
+		goto free_bm_inode; /* ignore the error in reclaim path */
+
 	status = ocfs2_extend_trans(handle,
 				ocfs2_calc_group_alloc_credits(osb->sb,
 						 le16_to_cpu(cl->cl_cpg)));
 	if (status) {
 		mlog_errno(status);
-		goto bail;
+		goto free_bm_bh;
 	}
 	status = ocfs2_journal_access_di(handle, INODE_CACHE(alloc_inode),
 					 alloc_bh, OCFS2_JOURNAL_ACCESS_WRITE);
 	if (status < 0) {
 		mlog_errno(status);
-		goto bail;
+		goto free_bm_bh;
 	}
 
 	/*
@@ -2795,18 +2816,6 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
 	memset(group, 0, sizeof(struct ocfs2_group_desc));
 
 	/* prepare job for reclaim clusters */
-	main_bm_inode = ocfs2_get_system_file_inode(osb,
-						    GLOBAL_BITMAP_SYSTEM_INODE,
-						    OCFS2_INVALID_SLOT);
-	if (!main_bm_inode)
-		goto bail; /* ignore the error in reclaim path */
-
-	inode_lock(main_bm_inode);
-
-	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
-	if (status < 0)
-		goto free_bm_inode; /* ignore the error in reclaim path */
-
 	ocfs2_block_to_cluster_group(main_bm_inode, start_blk, &bg_blkno,
 				     &start_bit);
 	fe = (struct ocfs2_dinode *) main_bm_bh->b_data;
-- 
2.43.0

Re: [PATCH] ocfs2: fix ABBA deadlock in suballocator reclaim

[Joseph Qi]

Seems you don't rebase the latest linux-next.
Actually this is already fixed in:
107451a4d732 ocfs2: defer suballocator block group reclaim to workqueue

Thanks,
Joseph

On 9/6/26 9:31 PM, ThangNN99 wrote:
> _ocfs2_reclaim_suballoc_to_main() runs inside an already-started
> transaction (j_trans_barrier held) and locks the global bitmap inode
> to return clusters to it. Every other allocation path takes that
> inode lock *before* starting a transaction, so this reverses the
> order and can deadlock:
> 
>   CPU0                          CPU1
>   rlock(j_trans_barrier)
>                                 lock(sb_internal)
>                                 lock(j_trans_barrier)
>   lock(GLOBAL_BITMAP inode)
> 
> Since reclaim is best-effort (its caller already ignores the return
> value), fix it by acquiring the global bitmap inode with trylock,
> before mutating anything, and bailing out to skip reclaim on a busy
> lock instead of blocking in the wrong order.
> 
> Reproduced with the syzbot C repro under QEMU: the unpatched kernel
> hits the lockdep splat on the very first mount+mkdir+rmdir cycle
> (~10s in); the patched kernel ran the same cycle 169 times with zero
> splats, and instrumentation confirmed reclaim keeps running (trylock
> succeeds) rather than being silently skipped.
> 
> Reported-by: syzbot+73d1166b94ed9875af54@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=73d1166b94ed9875af54
> Fixes: 4a54331616b3 ("ocfs2: give ocfs2 the ability to reclaim suballocator free bg")
> Cc: stable@vger.kernel.org
> Signed-off-by: ThangNN99 <ngocthang2710.1999@gmail.com>
> ---
>  fs/ocfs2/suballoc.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index 20c3aec6b987..085af9ba38ab 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -2716,18 +2716,39 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  	idx = le16_to_cpu(group->bg_chain);
>  	rec = &(cl->cl_recs[idx]);
>  
> +	/*
> +	 * We already hold j_trans_barrier, while everywhere else locks the
> +	 * allocator inode before starting a transaction. Trylock here, before
> +	 * mutating anything, so a busy lock skips this best-effort reclaim
> +	 * instead of inverting that order into an ABBA deadlock.
> +	 */
> +	main_bm_inode = ocfs2_get_system_file_inode(osb,
> +						    GLOBAL_BITMAP_SYSTEM_INODE,
> +						    OCFS2_INVALID_SLOT);
> +	if (!main_bm_inode)
> +		goto bail; /* ignore the error in reclaim path */
> +
> +	if (!inode_trylock(main_bm_inode)) {
> +		iput(main_bm_inode);
> +		goto bail; /* ignore the error in reclaim path */
> +	}
> +
> +	status = ocfs2_try_inode_lock(main_bm_inode, &main_bm_bh, 1);
> +	if (status < 0)
> +		goto free_bm_inode; /* ignore the error in reclaim path */
> +
>  	status = ocfs2_extend_trans(handle,
>  				ocfs2_calc_group_alloc_credits(osb->sb,
>  						 le16_to_cpu(cl->cl_cpg)));
>  	if (status) {
>  		mlog_errno(status);
> -		goto bail;
> +		goto free_bm_bh;
>  	}
>  	status = ocfs2_journal_access_di(handle, INODE_CACHE(alloc_inode),
>  					 alloc_bh, OCFS2_JOURNAL_ACCESS_WRITE);
>  	if (status < 0) {
>  		mlog_errno(status);
> -		goto bail;
> +		goto free_bm_bh;
>  	}
>  
>  	/*
> @@ -2795,18 +2816,6 @@ static int _ocfs2_reclaim_suballoc_to_main(handle_t *handle,
>  	memset(group, 0, sizeof(struct ocfs2_group_desc));
>  
>  	/* prepare job for reclaim clusters */
> -	main_bm_inode = ocfs2_get_system_file_inode(osb,
> -						    GLOBAL_BITMAP_SYSTEM_INODE,
> -						    OCFS2_INVALID_SLOT);
> -	if (!main_bm_inode)
> -		goto bail; /* ignore the error in reclaim path */
> -
> -	inode_lock(main_bm_inode);
> -
> -	status = ocfs2_inode_lock(main_bm_inode, &main_bm_bh, 1);
> -	if (status < 0)
> -		goto free_bm_inode; /* ignore the error in reclaim path */
> -
>  	ocfs2_block_to_cluster_group(main_bm_inode, start_blk, &bg_blkno,
>  				     &start_bit);
>  	fe = (struct ocfs2_dinode *) main_bm_bh->b_data;