[PATCH] xfs: annotate data race on li_lsn in CIL formatting vs AIL insertion

[PATCH] xfs: annotate data race on li_lsn in CIL formatting vs AIL insertion

[Cen Zhang]

xfs_inode_item_format_core() reads lip->li_lsn without holding any lock
to embed the last on-disk LSN into the log dinode during CIL commit:

    xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);

Concurrently, xfs_trans_ail_update_bulk() writes lip->li_lsn under
ail_lock when inserting items into the AIL after log IO completion:

    lip->li_lsn = lsn;

The CIL context lock (xc_ctx_lock) and the AIL lock (ail_lock) are
independent and provide no mutual exclusion between these paths.

Under simple interleaving on 64-bit architectures this is benign since
li_lsn monotonically increases and both old/new values are valid
checkpoint LSNs.  However, on 32-bit architectures the 64-bit xfs_lsn_t
can be torn into two 32-bit loads, producing a bogus LSN that could
cause log recovery to make incorrect replay decisions.  XFS already
acknowledges this concern via the xfs_trans_ail_copy_lsn() helper which
takes ail_lock on 32-bit.

Annotate with READ_ONCE()/WRITE_ONCE() to prevent compiler-level
tearing on all architectures.

Fixes: 93f958f9c41f ("xfs: cull unnecessary icdinode fields")
Cc: stable@vger.kernel.org
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 fs/xfs/xfs_inode_item.c | 2 +-
 fs/xfs/xfs_trans_ail.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 8913036b8024..ef0a0889c580 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -624,7 +624,7 @@ xfs_inode_item_format_core(
 	struct xfs_log_dinode	*dic;
 
 	dic = xlog_format_start(lfb, XLOG_REG_TYPE_ICORE);
-	xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
+	xfs_inode_to_log_dinode(ip, dic, READ_ONCE(ip->i_itemp->ili_item.li_lsn));
 	xlog_format_commit(lfb, xfs_log_dinode_size(ip->i_mount));
 }
 
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 923729af4206..3a0e0c65ebc5 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -831,7 +831,7 @@ xfs_trans_ail_update_bulk(
 		} else {
 			trace_xfs_ail_insert(lip, 0, lsn);
 		}
-		lip->li_lsn = lsn;
+		WRITE_ONCE(lip->li_lsn, lsn);
 		list_add_tail(&lip->li_ail, &tmp);
 	}
 
-- 
2.34.1

Re: [PATCH] xfs: annotate data race on li_lsn in CIL formatting vs AIL insertion

[Christoph Hellwig]

On Fri, Mar 20, 2026 at 10:55:07AM +0800, Cen Zhang wrote:
> Under simple interleaving on 64-bit architectures this is benign since
> li_lsn monotonically increases and both old/new values are valid
> checkpoint LSNs.  However, on 32-bit architectures the 64-bit xfs_lsn_t
> can be torn into two 32-bit loads, producing a bogus LSN that could
> cause log recovery to make incorrect replay decisions.  XFS already
> acknowledges this concern via the xfs_trans_ail_copy_lsn() helper which
> takes ail_lock on 32-bit.

Yes.

> Annotate with READ_ONCE()/WRITE_ONCE() to prevent compiler-level
> tearing on all architectures.

Well, xfs_trans_ail_copy_lsn pretty clearly documents that we actually
need a locak for the 32-bit case.  Assuming we don't have lock ordering
issues, using xfs_trans_ail_copy_lsn would be the right thing here.

> -	xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
> +	xfs_inode_to_log_dinode(ip, dic, READ_ONCE(ip->i_itemp->ili_item.li_lsn));

.. and either way please avoid the overly long lines.

Re: [PATCH] xfs: annotate data race on li_lsn in CIL formatting vs AIL insertion

[Cen Zhang]

Hi Christoph,

> Well, xfs_trans_ail_copy_lsn pretty clearly documents that we actually
> need a locak for the 32-bit case.  Assuming we don't have lock ordering
> issues, using xfs_trans_ail_copy_lsn would be the right thing here.

Thanks for the clarification.

> .. and either way please avoid the overly long lines.

I'll update the patch accordingly and also fix the long line in v2.

Best regards,
Cen Zhang