[PATCH v3] ocfs2: fix deadlock in inline-data truncate transactions
From: Joseph Qi
Date: Wed Aug 26 2026 - 07:30:54 EST
Updating an inode xattr can cause an ABBA deadlock with inline file
truncation:
ocfs2_truncate_file()
down_write(&oi->ip_alloc_sem)
ocfs2_truncate_inline()
ocfs2_start_trans()
ocfs2_xattr_set()
ocfs2_start_trans()
ocfs2_xattr_ibody_set()
down_write(&oi->ip_alloc_sem)
The xattr set path starts the merged transaction before the
inode-body xattr helper acquires ip_alloc_sem, reversing the
ip_alloc_sem -> transaction order used by the allocation and truncate
paths. The transaction merge in commit 85db90e77806 ("ocfs2/xattr:
Merge xattr set transaction.") introduced this ordering.
Fix it by acquiring ip_alloc_sem once in ocfs2_xattr_set(), before
xattr preparation, allocation reservations and ocfs2_start_trans(),
and removing the per-helper acquisition from ocfs2_xattr_ibody_find(),
ocfs2_xattr_ibody_set() and ocfs2_xattr_create_index_block(). These
helpers now assert via lockdep that the caller holds ip_alloc_sem.
ocfs2_xattr_set_handle(), which only sets initial ACL or security
xattrs on unpublished inodes inside the create transaction, takes
ip_alloc_sem under a dedicated lockdep subclass so that the assertions
hold without creating a transaction -> ip_alloc_sem cycle against the
ip_alloc_sem -> transaction order. The inode is unpublished, so the
acquisition can never contend.
This keeps the established ip_alloc_sem -> transaction order and makes
the locking unconditional, so lockdep can verify a single plain
ordering instead of conditional acquisitions.
Fixes: 85db90e77806 ("ocfs2/xattr: Merge xattr set transaction.")
Cc: ZhengYuan Huang <gality369@xxxxxxxxx>
Signed-off-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
---
fs/ocfs2/xattr.c | 79 ++++++++++++++++++++++++++++++++----------------
1 file changed, 53 insertions(+), 26 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index e955126e4d7d..143d6f75f9c9 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -2941,6 +2941,9 @@ static int ocfs2_xattr_has_space_inline(struct inode *inode,
*
* Find extended attribute in inode block and
* fill search info into struct ocfs2_xattr_search.
+ *
+ * The inline free-space check races with truncate and allocation, so
+ * callers must hold ip_alloc_sem for writing.
*/
static int ocfs2_xattr_ibody_find(struct inode *inode,
int name_index,
@@ -2952,13 +2955,13 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
int ret;
int has_space = 0;
+ lockdep_assert_held_write(&oi->ip_alloc_sem);
+
if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
return 0;
if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
- down_read(&oi->ip_alloc_sem);
has_space = ocfs2_xattr_has_space_inline(inode, di);
- up_read(&oi->ip_alloc_sem);
if (!has_space)
return 0;
}
@@ -3039,6 +3042,7 @@ static int ocfs2_xattr_ibody_init(struct inode *inode,
*
* Set, replace or remove an extended attribute into inode block.
*
+ * Callers must hold ip_alloc_sem for writing.
*/
static int ocfs2_xattr_ibody_set(struct inode *inode,
struct ocfs2_xattr_info *xi,
@@ -3049,16 +3053,17 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
struct ocfs2_inode_info *oi = OCFS2_I(inode);
struct ocfs2_xa_loc loc;
+ lockdep_assert_held_write(&oi->ip_alloc_sem);
+
if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
return -ENOSPC;
- down_write(&oi->ip_alloc_sem);
if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
ret = ocfs2_xattr_ibody_init(inode, xs->inode_bh, ctxt);
if (ret) {
if (ret != -ENOSPC)
mlog_errno(ret);
- goto out;
+ return ret;
}
}
@@ -3068,13 +3073,10 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
if (ret) {
if (ret != -ENOSPC)
mlog_errno(ret);
- goto out;
+ return ret;
}
xs->here = loc.xl_entry;
-out:
- up_write(&oi->ip_alloc_sem);
-
return ret;
}
@@ -3718,6 +3720,18 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
return ret;
}
+/*
+ * ip_alloc_sem subclass for inodes being initialized before publication.
+ * ocfs2_xattr_set_handle() runs inside the create transaction, so taking
+ * ip_alloc_sem there adds a transaction -> ip_alloc_sem order that would
+ * form a lockdep cycle with the ip_alloc_sem -> transaction order used
+ * elsewhere, if not for this separate subclass. The inode is unpublished
+ * so the acquisition can never contend.
+ */
+enum {
+ OCFS2_IP_ALLOC_SEM_UNPUBLISHED = 1,
+};
+
/*
* This helper is only for setting initial ACL or security xattrs on an inode
* that is still unpublished, unhashed, and unattached to a dentry.
@@ -3779,6 +3793,13 @@ int ocfs2_xattr_set_handle(handle_t *handle,
xis.inode_bh = xbs.inode_bh = di_bh;
di = (struct ocfs2_dinode *)di_bh->b_data;
+ /*
+ * The inode is unpublished and cannot contend, but take the
+ * semaphore anyway so the helpers' lockdep assertions hold.
+ */
+ down_write_nested(&OCFS2_I(inode)->ip_alloc_sem,
+ OCFS2_IP_ALLOC_SEM_UNPUBLISHED);
+
ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis);
if (ret)
goto cleanup;
@@ -3791,6 +3812,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
cleanup:
+ up_write(&OCFS2_I(inode)->ip_alloc_sem);
brelse(xbs.xattr_bh);
ocfs2_xattr_bucket_free(xbs.bucket);
@@ -3859,30 +3881,38 @@ int ocfs2_xattr_set(struct inode *inode,
di = (struct ocfs2_dinode *)di_bh->b_data;
down_write(&OCFS2_I(inode)->ip_xattr_sem);
+ /*
+ * The allocation and truncate paths take ip_alloc_sem before
+ * starting a transaction, so take it here before xattr
+ * preparation, allocation reservations and ocfs2_start_trans()
+ * to keep that order. The xattr helpers below no longer take
+ * it themselves.
+ */
+ down_write(&OCFS2_I(inode)->ip_alloc_sem);
/*
* Scan inode and external block to find the same name
* extended attribute and collect search information.
*/
ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis);
if (ret)
- goto cleanup;
+ goto out_free_ac;
if (xis.not_found) {
ret = ocfs2_xattr_block_find(inode, name_index, name, &xbs);
if (ret)
- goto cleanup;
+ goto out_free_ac;
}
if (xis.not_found && xbs.not_found) {
ret = -ENODATA;
if (flags & XATTR_REPLACE)
- goto cleanup;
+ goto out_free_ac;
ret = 0;
if (!value)
- goto cleanup;
+ goto out_free_ac;
} else {
ret = -EEXIST;
if (flags & XATTR_CREATE)
- goto cleanup;
+ goto out_free_ac;
}
/* Check whether the value is refcounted and do some preparation. */
@@ -3893,7 +3923,7 @@ int ocfs2_xattr_set(struct inode *inode,
&ref_meta, &ref_credits);
if (ret) {
mlog_errno(ret);
- goto cleanup;
+ goto out_free_ac;
}
}
@@ -3904,7 +3934,7 @@ int ocfs2_xattr_set(struct inode *inode,
if (ret < 0) {
inode_unlock(tl_inode);
mlog_errno(ret);
- goto cleanup;
+ goto out_free_ac;
}
}
inode_unlock(tl_inode);
@@ -3913,7 +3943,7 @@ int ocfs2_xattr_set(struct inode *inode,
&xbs, &ctxt, ref_meta, &credits);
if (ret) {
mlog_errno(ret);
- goto cleanup;
+ goto out_free_ac;
}
/* we need to update inode's ctime field, so add credit for it. */
@@ -3931,6 +3961,7 @@ int ocfs2_xattr_set(struct inode *inode,
ocfs2_commit_trans(osb, ctxt.handle);
out_free_ac:
+ up_write(&OCFS2_I(inode)->ip_alloc_sem);
if (ctxt.data_ac)
ocfs2_free_alloc_context(ctxt.data_ac);
if (ctxt.meta_ac)
@@ -3939,7 +3970,6 @@ int ocfs2_xattr_set(struct inode *inode,
ocfs2_schedule_truncate_log_flush(osb, 1);
ocfs2_run_deallocs(osb, &ctxt.dealloc);
-cleanup:
if (ref_tree)
ocfs2_unlock_refcount_tree(osb, ref_tree, 1);
up_write(&OCFS2_I(inode)->ip_xattr_sem);
@@ -4544,6 +4574,10 @@ static void ocfs2_xattr_update_xattr_search(struct inode *inode,
xs->here = &xs->header->xh_entries[i];
}
+/*
+ * Caller must hold ip_alloc_sem for writing, since a new xattr block
+ * is allocated and the xattr block header is rewritten.
+ */
static int ocfs2_xattr_create_index_block(struct inode *inode,
struct ocfs2_xattr_search *xs,
struct ocfs2_xattr_set_ctxt *ctxt)
@@ -4559,19 +4593,14 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
struct ocfs2_xattr_tree_root *xr;
u16 xb_flags = le16_to_cpu(xb->xb_flags);
+ lockdep_assert_held_write(&oi->ip_alloc_sem);
+
trace_ocfs2_xattr_create_index_block_begin(
(unsigned long long)xb_bh->b_blocknr);
BUG_ON(xb_flags & OCFS2_XATTR_INDEXED);
BUG_ON(!xs->bucket);
- /*
- * XXX:
- * We can use this lock for now, and maybe move to a dedicated mutex
- * if performance becomes a problem later.
- */
- down_write(&oi->ip_alloc_sem);
-
ret = ocfs2_journal_access_xb(handle, INODE_CACHE(inode), xb_bh,
OCFS2_JOURNAL_ACCESS_WRITE);
if (ret) {
@@ -4633,8 +4662,6 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
ocfs2_journal_dirty(handle, xb_bh);
out:
- up_write(&oi->ip_alloc_sem);
-
return ret;
}
--
2.39.3