[PATCH v2] ocfs2: fix deadlock in inline-data truncate transactions

[PATCH v2] ocfs2: fix deadlock in inline-data truncate transactions

[ZhengYuan Huang]

[BUG]
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)

[CAUSE]
The xattr set path starts its merged transaction before the helpers which
modify inode-body xattrs acquire ip_alloc_sem. This creates the reverse of
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 for ordinary xattr updates.

The create-time transaction -> ip_xattr_sem dependency is a separate case.
The fix in 
commit c13024342c82 ("ocfs2: fix circular locking dependency in ocfs2_init_acl()")
prepares the parent ACL before the create transaction and avoids the xattr
semaphore for initial xattrs on an unpublished inode. That change does not
cover ordinary ocfs2_xattr_set().

[FIX]
Acquire ip_alloc_sem in ocfs2_xattr_set() before xattr preparation,
allocation reservations, and transaction start.  Tell the inode-body and
indexed-block helpers when the semaphore is already held, while preserving
their local locking for callers that do not provide outer protection.

Keep the existing ip_alloc_sem -> transaction order in allocation and
truncate paths; fixing the xattr side avoids changing that established
ordering.

Fixes: 85db90e77806 ("ocfs2/xattr: Merge xattr set transaction.")
Signed-off-by: ZhengYuan Huang <gality369@gmail.com>
---
v2:
- Fix the xattr side of the lock ordering based on Joseph's feedback.
- Explain why the remaining fix belongs on the ordinary xattr side.
---
 fs/ocfs2/xattr.c | 49 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 35bcbb0ff607..27bea1f360a1 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -74,6 +74,7 @@ struct ocfs2_xattr_set_ctxt {
 	struct ocfs2_alloc_context *data_ac;
 	struct ocfs2_cached_dealloc_ctxt dealloc;
 	int set_abort;
+	int alloc_sem_protected;
 };
 
 #define OCFS2_XATTR_ROOT_SIZE	(sizeof(struct ocfs2_xattr_def_value_root))
@@ -2916,7 +2917,8 @@ static int ocfs2_xattr_has_space_inline(struct inode *inode,
 static int ocfs2_xattr_ibody_find(struct inode *inode,
 				  int name_index,
 				  const char *name,
-				  struct ocfs2_xattr_search *xs)
+				  struct ocfs2_xattr_search *xs,
+				  int lock_alloc_sem)
 {
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
@@ -2927,9 +2929,11 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
 		return 0;
 
 	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
-		down_read(&oi->ip_alloc_sem);
+		if (lock_alloc_sem)
+			down_read(&oi->ip_alloc_sem);
 		has_space = ocfs2_xattr_has_space_inline(inode, di);
-		up_read(&oi->ip_alloc_sem);
+		if (lock_alloc_sem)
+			up_read(&oi->ip_alloc_sem);
 		if (!has_space)
 			return 0;
 	}
@@ -3016,14 +3020,17 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
 				 struct ocfs2_xattr_search *xs,
 				 struct ocfs2_xattr_set_ctxt *ctxt)
 {
-	int ret;
+	int ret, took_alloc_sem = 0;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_xa_loc loc;
 
 	if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
 		return -ENOSPC;
 
-	down_write(&oi->ip_alloc_sem);
+	if (!ctxt->alloc_sem_protected) {
+		down_write(&oi->ip_alloc_sem);
+		took_alloc_sem = 1;
+	}
 	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
 		ret = ocfs2_xattr_ibody_init(inode, xs->inode_bh, ctxt);
 		if (ret) {
@@ -3044,7 +3051,8 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
 	xs->here = loc.xl_entry;
 
 out:
-	up_write(&oi->ip_alloc_sem);
+	if (took_alloc_sem)
+		up_write(&oi->ip_alloc_sem);
 
 	return ret;
 }
@@ -3729,6 +3737,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
 		.handle = handle,
 		.meta_ac = meta_ac,
 		.data_ac = data_ac,
+		.alloc_sem_protected = 1,
 	};
 
 	if (!ocfs2_supports_xattr(OCFS2_SB(inode->i_sb)))
@@ -3750,7 +3759,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
 	xis.inode_bh = xbs.inode_bh = di_bh;
 	di = (struct ocfs2_dinode *)di_bh->b_data;
 
-	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis);
+	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis, 0);
 	if (ret)
 		goto cleanup;
 	if (xis.not_found) {
@@ -3834,7 +3843,7 @@ int ocfs2_xattr_set(struct inode *inode,
 	 * 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);
+	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis, 1);
 	if (ret)
 		goto cleanup;
 	if (xis.not_found) {
@@ -3856,6 +3865,8 @@ int ocfs2_xattr_set(struct inode *inode,
 			goto cleanup;
 	}
 
+	down_write(&OCFS2_I(inode)->ip_alloc_sem);
+
 	/* Check whether the value is refcounted and do some preparation. */
 	if (ocfs2_is_refcount_inode(inode) &&
 	    (!xis.not_found || !xbs.not_found)) {
@@ -3864,7 +3875,7 @@ int ocfs2_xattr_set(struct inode *inode,
 						   &ref_meta, &ref_credits);
 		if (ret) {
 			mlog_errno(ret);
-			goto cleanup;
+			goto out_unlock_alloc;
 		}
 	}
 
@@ -3875,7 +3886,7 @@ int ocfs2_xattr_set(struct inode *inode,
 		if (ret < 0) {
 			inode_unlock(tl_inode);
 			mlog_errno(ret);
-			goto cleanup;
+			goto out_unlock_alloc;
 		}
 	}
 	inode_unlock(tl_inode);
@@ -3884,8 +3895,9 @@ int ocfs2_xattr_set(struct inode *inode,
 					&xbs, &ctxt, ref_meta, &credits);
 	if (ret) {
 		mlog_errno(ret);
-		goto cleanup;
+		goto out_unlock_alloc;
 	}
+	ctxt.alloc_sem_protected = 1;
 
 	/* we need to update inode's ctime field, so add credit for it. */
 	credits += OCFS2_INODE_UPDATE_CREDITS;
@@ -3893,15 +3905,15 @@ int ocfs2_xattr_set(struct inode *inode,
 	if (IS_ERR(ctxt.handle)) {
 		ret = PTR_ERR(ctxt.handle);
 		mlog_errno(ret);
-		goto out_free_ac;
+		goto out_unlock_alloc;
 	}
 
 	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
 	ocfs2_update_inode_fsync_trans(ctxt.handle, inode, 0);
 
 	ocfs2_commit_trans(osb, ctxt.handle);
-
-out_free_ac:
+out_unlock_alloc:
+	up_write(&OCFS2_I(inode)->ip_alloc_sem);
 	if (ctxt.data_ac)
 		ocfs2_free_alloc_context(ctxt.data_ac);
 	if (ctxt.meta_ac)
@@ -4520,6 +4532,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 	u64 blkno;
 	handle_t *handle = ctxt->handle;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
+	int took_alloc_sem = 0;
 	struct buffer_head *xb_bh = xs->xattr_bh;
 	struct ocfs2_xattr_block *xb =
 			(struct ocfs2_xattr_block *)xb_bh->b_data;
@@ -4537,7 +4550,10 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 	 * 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);
+	if (!ctxt->alloc_sem_protected) {
+		down_write(&oi->ip_alloc_sem);
+		took_alloc_sem = 1;
+	}
 
 	ret = ocfs2_journal_access_xb(handle, INODE_CACHE(inode), xb_bh,
 				      OCFS2_JOURNAL_ACCESS_WRITE);
@@ -4600,7 +4616,8 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
 	ocfs2_journal_dirty(handle, xb_bh);
 
 out:
-	up_write(&oi->ip_alloc_sem);
+	if (took_alloc_sem)
+		up_write(&oi->ip_alloc_sem);
 
 	return ret;
 }

Re: [PATCH v2] ocfs2: fix deadlock in inline-data truncate transactions

[Joseph Qi]

On 8/6/26 11:59 AM, ZhengYuan Huang wrote:
> [BUG]
> 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)
> 
> [CAUSE]
> The xattr set path starts its merged transaction before the helpers which
> modify inode-body xattrs acquire ip_alloc_sem. This creates the reverse of
> 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 for ordinary xattr updates.
> 
> The create-time transaction -> ip_xattr_sem dependency is a separate case.
> The fix in 
> commit c13024342c82 ("ocfs2: fix circular locking dependency in ocfs2_init_acl()")
> prepares the parent ACL before the create transaction and avoids the xattr
> semaphore for initial xattrs on an unpublished inode. That change does not
> cover ordinary ocfs2_xattr_set().
> 
> [FIX]
> Acquire ip_alloc_sem in ocfs2_xattr_set() before xattr preparation,
> allocation reservations, and transaction start.  Tell the inode-body and
> indexed-block helpers when the semaphore is already held, while preserving
> their local locking for callers that do not provide outer protection.
> 
> Keep the existing ip_alloc_sem -> transaction order in allocation and
> truncate paths; fixing the xattr side avoids changing that established
> ordering.
> 

I'd rather fix it in an alternative way by pushing the semaphore fully
up into ocfs2_xattr_set() and strip it from the helpers entirely, which
seems more cleaner.

I have a local prototype and will send out when pass regression tests.

Thanks,
Joseph

> Fixes: 85db90e77806 ("ocfs2/xattr: Merge xattr set transaction.")
> Signed-off-by: ZhengYuan Huang <gality369@gmail.com>
> ---
> v2:
> - Fix the xattr side of the lock ordering based on Joseph's feedback.
> - Explain why the remaining fix belongs on the ordinary xattr side.
> ---
>  fs/ocfs2/xattr.c | 49 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 35bcbb0ff607..27bea1f360a1 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -74,6 +74,7 @@ struct ocfs2_xattr_set_ctxt {
>  	struct ocfs2_alloc_context *data_ac;
>  	struct ocfs2_cached_dealloc_ctxt dealloc;
>  	int set_abort;
> +	int alloc_sem_protected;
>  };
>  
>  #define OCFS2_XATTR_ROOT_SIZE	(sizeof(struct ocfs2_xattr_def_value_root))
> @@ -2916,7 +2917,8 @@ static int ocfs2_xattr_has_space_inline(struct inode *inode,
>  static int ocfs2_xattr_ibody_find(struct inode *inode,
>  				  int name_index,
>  				  const char *name,
> -				  struct ocfs2_xattr_search *xs)
> +				  struct ocfs2_xattr_search *xs,
> +				  int lock_alloc_sem)
>  {
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
> @@ -2927,9 +2929,11 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
>  		return 0;
>  
>  	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
> -		down_read(&oi->ip_alloc_sem);
> +		if (lock_alloc_sem)
> +			down_read(&oi->ip_alloc_sem);
>  		has_space = ocfs2_xattr_has_space_inline(inode, di);
> -		up_read(&oi->ip_alloc_sem);
> +		if (lock_alloc_sem)
> +			up_read(&oi->ip_alloc_sem);
>  		if (!has_space)
>  			return 0;
>  	}
> @@ -3016,14 +3020,17 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
>  				 struct ocfs2_xattr_search *xs,
>  				 struct ocfs2_xattr_set_ctxt *ctxt)
>  {
> -	int ret;
> +	int ret, took_alloc_sem = 0;
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
>  	struct ocfs2_xa_loc loc;
>  
>  	if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
>  		return -ENOSPC;
>  
> -	down_write(&oi->ip_alloc_sem);
> +	if (!ctxt->alloc_sem_protected) {
> +		down_write(&oi->ip_alloc_sem);
> +		took_alloc_sem = 1;
> +	}
>  	if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
>  		ret = ocfs2_xattr_ibody_init(inode, xs->inode_bh, ctxt);
>  		if (ret) {
> @@ -3044,7 +3051,8 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
>  	xs->here = loc.xl_entry;
>  
>  out:
> -	up_write(&oi->ip_alloc_sem);
> +	if (took_alloc_sem)
> +		up_write(&oi->ip_alloc_sem);
>  
>  	return ret;
>  }
> @@ -3729,6 +3737,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
>  		.handle = handle,
>  		.meta_ac = meta_ac,
>  		.data_ac = data_ac,
> +		.alloc_sem_protected = 1,
>  	};
>  
>  	if (!ocfs2_supports_xattr(OCFS2_SB(inode->i_sb)))
> @@ -3750,7 +3759,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
>  	xis.inode_bh = xbs.inode_bh = di_bh;
>  	di = (struct ocfs2_dinode *)di_bh->b_data;
>  
> -	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis);
> +	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis, 0);
>  	if (ret)
>  		goto cleanup;
>  	if (xis.not_found) {
> @@ -3834,7 +3843,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  	 * 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);
> +	ret = ocfs2_xattr_ibody_find(inode, name_index, name, &xis, 1);
>  	if (ret)
>  		goto cleanup;
>  	if (xis.not_found) {
> @@ -3856,6 +3865,8 @@ int ocfs2_xattr_set(struct inode *inode,
>  			goto cleanup;
>  	}
>  
> +	down_write(&OCFS2_I(inode)->ip_alloc_sem);
> +
>  	/* Check whether the value is refcounted and do some preparation. */
>  	if (ocfs2_is_refcount_inode(inode) &&
>  	    (!xis.not_found || !xbs.not_found)) {
> @@ -3864,7 +3875,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  						   &ref_meta, &ref_credits);
>  		if (ret) {
>  			mlog_errno(ret);
> -			goto cleanup;
> +			goto out_unlock_alloc;
>  		}
>  	}
>  
> @@ -3875,7 +3886,7 @@ int ocfs2_xattr_set(struct inode *inode,
>  		if (ret < 0) {
>  			inode_unlock(tl_inode);
>  			mlog_errno(ret);
> -			goto cleanup;
> +			goto out_unlock_alloc;
>  		}
>  	}
>  	inode_unlock(tl_inode);
> @@ -3884,8 +3895,9 @@ int ocfs2_xattr_set(struct inode *inode,
>  					&xbs, &ctxt, ref_meta, &credits);
>  	if (ret) {
>  		mlog_errno(ret);
> -		goto cleanup;
> +		goto out_unlock_alloc;
>  	}
> +	ctxt.alloc_sem_protected = 1;
>  
>  	/* we need to update inode's ctime field, so add credit for it. */
>  	credits += OCFS2_INODE_UPDATE_CREDITS;
> @@ -3893,15 +3905,15 @@ int ocfs2_xattr_set(struct inode *inode,
>  	if (IS_ERR(ctxt.handle)) {
>  		ret = PTR_ERR(ctxt.handle);
>  		mlog_errno(ret);
> -		goto out_free_ac;
> +		goto out_unlock_alloc;
>  	}
>  
>  	ret = __ocfs2_xattr_set_handle(inode, di, &xi, &xis, &xbs, &ctxt);
>  	ocfs2_update_inode_fsync_trans(ctxt.handle, inode, 0);
>  
>  	ocfs2_commit_trans(osb, ctxt.handle);
> -
> -out_free_ac:
> +out_unlock_alloc:
> +	up_write(&OCFS2_I(inode)->ip_alloc_sem);
>  	if (ctxt.data_ac)
>  		ocfs2_free_alloc_context(ctxt.data_ac);
>  	if (ctxt.meta_ac)
> @@ -4520,6 +4532,7 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	u64 blkno;
>  	handle_t *handle = ctxt->handle;
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +	int took_alloc_sem = 0;
>  	struct buffer_head *xb_bh = xs->xattr_bh;
>  	struct ocfs2_xattr_block *xb =
>  			(struct ocfs2_xattr_block *)xb_bh->b_data;
> @@ -4537,7 +4550,10 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	 * 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);
> +	if (!ctxt->alloc_sem_protected) {
> +		down_write(&oi->ip_alloc_sem);
> +		took_alloc_sem = 1;
> +	}
>  
>  	ret = ocfs2_journal_access_xb(handle, INODE_CACHE(inode), xb_bh,
>  				      OCFS2_JOURNAL_ACCESS_WRITE);
> @@ -4600,7 +4616,8 @@ static int ocfs2_xattr_create_index_block(struct inode *inode,
>  	ocfs2_journal_dirty(handle, xb_bh);
>  
>  out:
> -	up_write(&oi->ip_alloc_sem);
> +	if (took_alloc_sem)
> +		up_write(&oi->ip_alloc_sem);
>  
>  	return ret;
>  }