[PATCH] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf

[PATCH] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf

[Yun Zhou]

xfs_sync_sb_buf() holds sb/rtsb buffer locks across a synchronous
xfs_trans_commit(), which flushes the CIL push workqueue internally.
If shutdown occurs during the CIL push, xfs_buf_item_unpin() needs to
lock these buffers to fail them, causing a deadlock:

  setlabel:          holds buf lock -> flush_workqueue(xfs-cil)
  CIL push worker:   xfs_buf_item_unpin -> xfs_buf_lock(same buf)

Remove the xfs_trans_bhold() calls so that commit releases the buffer
locks normally.  After the sync commit, re-acquire the buffers via
mp->m_sb_bp / mp->m_rtsb_bp for the on-disk writeback.

Fixes: f7664b31975b ("xfs: implement online get/set fs label")
Reported-by: syzbot+837bcd54843dd6262f2f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=837bcd54843dd6262f2f
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
 fs/xfs/libxfs/xfs_sb.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 47322adb7690..fbb4505b1e9e 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1471,7 +1471,6 @@ xfs_sync_sb_buf(
 {
 	struct xfs_trans	*tp;
 	struct xfs_buf		*bp;
-	struct xfs_buf		*rtsb_bp = NULL;
 	int			error;
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0, 0, &tp);
@@ -1480,26 +1479,28 @@ xfs_sync_sb_buf(
 
 	bp = xfs_trans_getsb(tp);
 	xfs_log_sb(tp);
-	xfs_trans_bhold(tp, bp);
-	if (update_rtsb) {
-		rtsb_bp = xfs_log_rtsb(tp, bp);
-		if (rtsb_bp)
-			xfs_trans_bhold(tp, rtsb_bp);
-	}
+	if (update_rtsb)
+		xfs_log_rtsb(tp, bp);
 	xfs_trans_set_sync(tp);
 	error = xfs_trans_commit(tp);
 	if (error)
-		goto out;
-	/*
-	 * write out the sb buffer to get the changes to disk
-	 */
-	error = xfs_bwrite(bp);
-	if (!error && rtsb_bp)
-		error = xfs_bwrite(rtsb_bp);
-out:
-	if (rtsb_bp)
-		xfs_buf_relse(rtsb_bp);
-	xfs_buf_relse(bp);
+		return error;
+
+	/* Re-acquire and write the sb and rtsb to disk. */
+	xfs_buf_lock(mp->m_sb_bp);
+	xfs_buf_hold(mp->m_sb_bp);
+	error = xfs_bwrite(mp->m_sb_bp);
+	xfs_buf_relse(mp->m_sb_bp);
+	if (error)
+		return error;
+
+	if (update_rtsb && mp->m_rtsb_bp) {
+		xfs_buf_lock(mp->m_rtsb_bp);
+		xfs_buf_hold(mp->m_rtsb_bp);
+		error = xfs_bwrite(mp->m_rtsb_bp);
+		xfs_buf_relse(mp->m_rtsb_bp);
+	}
+
 	return error;
 }
 
-- 
2.43.0

Re: [PATCH] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf

[Darrick J. Wong]

On Mon, Jul 13, 2026 at 10:50:02PM +0800, Yun Zhou wrote:
> xfs_sync_sb_buf() holds sb/rtsb buffer locks across a synchronous
> xfs_trans_commit(), which flushes the CIL push workqueue internally.
> If shutdown occurs during the CIL push, xfs_buf_item_unpin() needs to
> lock these buffers to fail them, causing a deadlock:
> 
>   setlabel:          holds buf lock -> flush_workqueue(xfs-cil)
>   CIL push worker:   xfs_buf_item_unpin -> xfs_buf_lock(same buf)
> 
> Remove the xfs_trans_bhold() calls so that commit releases the buffer
> locks normally.  After the sync commit, re-acquire the buffers via
> mp->m_sb_bp / mp->m_rtsb_bp for the on-disk writeback.

Is there a risk of writing out the wrong superblock contents if
something else grabs the buffer lock(s) between the xfs_trans_commit and
the xfs_buf_lock calls?  Can we walk off a garbage
xfs_mount::m_{rt,}sb_bp pointer if the filesystem is being torn down, or
does something prevent that?

--D

> Fixes: f7664b31975b ("xfs: implement online get/set fs label")
> Reported-by: syzbot+837bcd54843dd6262f2f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=837bcd54843dd6262f2f
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 47322adb7690..fbb4505b1e9e 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -1471,7 +1471,6 @@ xfs_sync_sb_buf(
>  {
>  	struct xfs_trans	*tp;
>  	struct xfs_buf		*bp;
> -	struct xfs_buf		*rtsb_bp = NULL;
>  	int			error;
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0, 0, &tp);
> @@ -1480,26 +1479,28 @@ xfs_sync_sb_buf(
>  
>  	bp = xfs_trans_getsb(tp);
>  	xfs_log_sb(tp);
> -	xfs_trans_bhold(tp, bp);
> -	if (update_rtsb) {
> -		rtsb_bp = xfs_log_rtsb(tp, bp);
> -		if (rtsb_bp)
> -			xfs_trans_bhold(tp, rtsb_bp);
> -	}
> +	if (update_rtsb)
> +		xfs_log_rtsb(tp, bp);
>  	xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp);
>  	if (error)
> -		goto out;
> -	/*
> -	 * write out the sb buffer to get the changes to disk
> -	 */
> -	error = xfs_bwrite(bp);
> -	if (!error && rtsb_bp)
> -		error = xfs_bwrite(rtsb_bp);
> -out:
> -	if (rtsb_bp)
> -		xfs_buf_relse(rtsb_bp);
> -	xfs_buf_relse(bp);
> +		return error;
> +
> +	/* Re-acquire and write the sb and rtsb to disk. */
> +	xfs_buf_lock(mp->m_sb_bp);
> +	xfs_buf_hold(mp->m_sb_bp);
> +	error = xfs_bwrite(mp->m_sb_bp);
> +	xfs_buf_relse(mp->m_sb_bp);
> +	if (error)
> +		return error;
> +
> +	if (update_rtsb && mp->m_rtsb_bp) {
> +		xfs_buf_lock(mp->m_rtsb_bp);
> +		xfs_buf_hold(mp->m_rtsb_bp);
> +		error = xfs_bwrite(mp->m_rtsb_bp);
> +		xfs_buf_relse(mp->m_rtsb_bp);
> +	}
> +
>  	return error;
>  }
>  
> -- 
> 2.43.0
> 
> 

Re: [PATCH] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf

[Zhou, Yun]

Thanks a lot for your review.

On 7/14/26 06:04, Darrick J. Wong wrote:
> 
> On Mon, Jul 13, 2026 at 10:50:02PM +0800, Yun Zhou wrote:
>> xfs_sync_sb_buf() holds sb/rtsb buffer locks across a synchronous
>> xfs_trans_commit(), which flushes the CIL push workqueue internally.
>> If shutdown occurs during the CIL push, xfs_buf_item_unpin() needs to
>> lock these buffers to fail them, causing a deadlock:
>>
>>    setlabel:          holds buf lock -> flush_workqueue(xfs-cil)
>>    CIL push worker:   xfs_buf_item_unpin -> xfs_buf_lock(same buf)
>>
>> Remove the xfs_trans_bhold() calls so that commit releases the buffer
>> locks normally.  After the sync commit, re-acquire the buffers via
>> mp->m_sb_bp / mp->m_rtsb_bp for the on-disk writeback.
> 
> Is there a risk of writing out the wrong superblock contents if
> something else grabs the buffer lock(s) between the xfs_trans_commit and
> the xfs_buf_lock calls?  Can we walk off a garbage
> xfs_mount::m_{rt,}sb_bp pointer if the filesystem is being torn down, or
> does something prevent that?
> 

There shouldn't be a risk of inconsistent contents. Each update to the 
sb buffer is a full overwrite of mp->m_sb via xfs_sb_to_disk(), 
performed under the buffer lock. So when we re-lock after commit, the 
buffer  always contains a complete, consistent snapshot — we may see a 
newer version if someone else updated it in between, but never a 
partially written one.

For the pointer lifetime: mp->m_sb_bp is allocated at mount and freed in 
xfs_unmountfs(). The caller holds mnt_want_write_file(), which prevents 
unmount from making progress, so the pointer should remain valid 
throughout xfs_sync_sb_buf().

BR,
Yun

Re: [PATCH] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf

[Darrick J. Wong]

On Tue, Jul 14, 2026 at 09:41:11AM +0800, Zhou, Yun wrote:
> 
> Thanks a lot for your review.
> 
> On 7/14/26 06:04, Darrick J. Wong wrote:
> > 
> > On Mon, Jul 13, 2026 at 10:50:02PM +0800, Yun Zhou wrote:
> > > xfs_sync_sb_buf() holds sb/rtsb buffer locks across a synchronous
> > > xfs_trans_commit(), which flushes the CIL push workqueue internally.
> > > If shutdown occurs during the CIL push, xfs_buf_item_unpin() needs to
> > > lock these buffers to fail them, causing a deadlock:
> > > 
> > >    setlabel:          holds buf lock -> flush_workqueue(xfs-cil)
> > >    CIL push worker:   xfs_buf_item_unpin -> xfs_buf_lock(same buf)
> > > 
> > > Remove the xfs_trans_bhold() calls so that commit releases the buffer
> > > locks normally.  After the sync commit, re-acquire the buffers via
> > > mp->m_sb_bp / mp->m_rtsb_bp for the on-disk writeback.
> > 
> > Is there a risk of writing out the wrong superblock contents if
> > something else grabs the buffer lock(s) between the xfs_trans_commit and
> > the xfs_buf_lock calls?  Can we walk off a garbage
> > xfs_mount::m_{rt,}sb_bp pointer if the filesystem is being torn down, or
> > does something prevent that?
> > 
> 
> There shouldn't be a risk of inconsistent contents. Each update to the sb
> buffer is a full overwrite of mp->m_sb via xfs_sb_to_disk(), performed under
> the buffer lock. So when we re-lock after commit, the buffer  always
> contains a complete, consistent snapshot — we may see a newer version if
> someone else updated it in between, but never a partially written one.
> 
> For the pointer lifetime: mp->m_sb_bp is allocated at mount and freed in
> xfs_unmountfs(). The caller holds mnt_want_write_file(), which prevents
> unmount from making progress, so the pointer should remain valid throughout
> xfs_sync_sb_buf().

<nod> Ok then.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


> BR,
> Yun
>