[PATCH 0/2] xfs: fix inode leak and clean up attr intent recovery

[PATCH 0/2] xfs: fix inode leak and clean up attr intent recovery

[Yingjie Gao]

Hi all,

The first patch fixes a leaked inode reference in
xfs_attr_recover_work() when transaction allocation fails after the
recovered attr work item has been attached to the defer pending list.

The second patch cleans up the same function's exit paths so they follow
the same linear cleanup style as the neighboring bmap and exchmaps
recovery helpers, and initializes the local error variable in the same
style.

Thanks,
Yingjie

Yingjie Gao (2):
  xfs: fix inode ref leak in attr intent recovery
  xfs: clean up attr intent recovery error paths

 fs/xfs/xfs_attr_item.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 1/2] xfs: fix inode ref leak in attr intent recovery

[Yingjie Gao]

xfs_attri_recover_work() grabs the target inode, attaches it to the
reconstructed attr work item, and adds that work item to the defer
pending list.

If xfs_attr_recover_work() fails to allocate the recovery transaction,
it returns immediately without dropping the inode reference.  The later
cancel path only frees the attr work item state, so the inode reference
leaks.

Release the inode before returning the transaction allocation failure.

Fixes: e70fb328d527 ("xfs: recreate work items when recovering intent items")
Cc: <stable@vger.kernel.org> # v6.8
Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
---
 fs/xfs/xfs_attr_item.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index deab14f31b38..c3d96c7a5bca 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -773,8 +773,10 @@ xfs_attr_recover_work(
 	}
 	resv = xlog_recover_resv(&resv);
 	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
-	if (error)
+	if (error) {
+		xfs_irele(ip);
 		return error;
+	}
 	args->trans = tp;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
-- 
2.20.1

Re: [PATCH 1/2] xfs: fix inode ref leak in attr intent recovery

[Darrick J. Wong]

On Tue, Jun 09, 2026 at 07:16:18PM +0800, Yingjie Gao wrote:
> xfs_attri_recover_work() grabs the target inode, attaches it to the
> reconstructed attr work item, and adds that work item to the defer
> pending list.
> 
> If xfs_attr_recover_work() fails to allocate the recovery transaction,
> it returns immediately without dropping the inode reference.  The later
> cancel path only frees the attr work item state, so the inode reference
> leaks.
> 
> Release the inode before returning the transaction allocation failure.
> 
> Fixes: e70fb328d527 ("xfs: recreate work items when recovering intent items")
> Cc: <stable@vger.kernel.org> # v6.8
> Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
> ---
>  fs/xfs/xfs_attr_item.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index deab14f31b38..c3d96c7a5bca 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -773,8 +773,10 @@ xfs_attr_recover_work(
>  	}
>  	resv = xlog_recover_resv(&resv);
>  	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
> -	if (error)
> +	if (error) {
> +		xfs_irele(ip);

Seems fine but I wonder why you don't just add an out_rele label on the
line above the existing xfs_irele() call and make this goto there?

--D

>  		return error;
> +	}
>  	args->trans = tp;
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> -- 
> 2.20.1
> 
> 

Re: [PATCH 1/2] xfs: fix inode ref leak in attr intent recovery

[Yingjie Gao]

在 2026/6/9 22:57, Darrick J. Wong 写道:
> On Tue, Jun 09, 2026 at 07:16:18PM +0800, Yingjie Gao wrote:
>> xfs_attri_recover_work() grabs the target inode, attaches it to the
>> reconstructed attr work item, and adds that work item to the defer
>> pending list.
>>
>> If xfs_attr_recover_work() fails to allocate the recovery transaction,
>> it returns immediately without dropping the inode reference.  The later
>> cancel path only frees the attr work item state, so the inode reference
>> leaks.
>>
>> Release the inode before returning the transaction allocation failure.
>>
>> Fixes: e70fb328d527 ("xfs: recreate work items when recovering intent items")
>> Cc: <stable@vger.kernel.org> # v6.8
>> Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
>> ---
>>  fs/xfs/xfs_attr_item.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
>> index deab14f31b38..c3d96c7a5bca 100644
>> --- a/fs/xfs/xfs_attr_item.c
>> +++ b/fs/xfs/xfs_attr_item.c
>> @@ -773,8 +773,10 @@ xfs_attr_recover_work(
>>  	}
>>  	resv = xlog_recover_resv(&resv);
>>  	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
>> -	if (error)
>> +	if (error) {
>> +		xfs_irele(ip);
> 
> Seems fine but I wonder why you don't just add an out_rele label on the
> line above the existing xfs_irele() call and make this goto there?
> 
> --D
> 
Good point, I'll update the patch and send a v2.
Thanks for the review.

-- 
Yingjie
>>  		return error;
>> +	}
>>  	args->trans = tp;
>>  
>>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>> -- 
>> 2.20.1
>>
>>
> 

[PATCH 2/2] xfs: clean up attr intent recovery error paths

[Yingjie Gao]

xfs_attr_recover_work() still uses a backward goto from out_cancel to
out_unlock, and the transaction allocation failure path returns from the
middle of the function.

Restructure the cleanup labels into the same linear fallthrough style
used by the neighboring bmap and exchmaps recovery helpers.  Initialize
the local error variable to zero as those helpers do as well, which
makes the shared return path easier to follow.

No functional change intended.

Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
---
 fs/xfs/xfs_attr_item.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index c3d96c7a5bca..f8aa9dd80bb9 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -739,7 +739,7 @@ xfs_attr_recover_work(
 	struct xfs_trans_res		resv;
 	struct xfs_attri_log_format	*attrp;
 	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
-	int				error;
+	int				error = 0;
 	unsigned int			total = 0;
 
 	/*
@@ -773,10 +773,8 @@ xfs_attr_recover_work(
 	}
 	resv = xlog_recover_resv(&resv);
 	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
-	if (error) {
-		xfs_irele(ip);
-		return error;
-	}
+	if (error)
+		goto out_rele;
 	args->trans = tp;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
@@ -791,13 +789,20 @@ xfs_attr_recover_work(
 		goto out_cancel;
 
 	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
-out_unlock:
+	if (error)
+		goto out_unlock;
+
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_irele(ip);
-	return error;
+	return 0;
+
 out_cancel:
 	xfs_trans_cancel(tp);
-	goto out_unlock;
+out_unlock:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+out_rele:
+	xfs_irele(ip);
+	return error;
 }
 
 /* Re-log an intent item to push the log tail forward. */
-- 
2.20.1

[PATCH v2 0/2] xfs: fix inode leak and clean up attr intent recovery

[Yingjie Gao]

Hi all,

The first patch fixes a leaked inode reference in
xfs_attr_recover_work() when transaction allocation fails after the
recovered attr work item has been attached to the defer pending list.

The second patch cleans up the same function's exit paths so they follow
the same linear cleanup style as the neighboring bmap and exchmaps
recovery helpers, and initializes the local error variable in the same
style.

Changes since v1:
- route the transaction allocation failure through out_rele in patch 1,
  as Darrick suggested;
- keep patch 2 focused on the remaining cleanup-path reshaping and the
  local error initialization.

Thanks,
Yingjie

Yingjie Gao (2):
  xfs: fix inode ref leak in attr intent recovery
  xfs: clean up attr intent recovery error paths

 fs/xfs/xfs_attr_item.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] xfs: fix inode ref leak in attr intent recovery

[Yingjie Gao]

xfs_attri_recover_work() grabs the target inode, attaches it to the
reconstructed attr work item, and adds that work item to the defer
pending list.

If xfs_attr_recover_work() fails to allocate the recovery transaction,
it returns immediately without dropping the inode reference.  The later
cancel path only frees the attr work item state, so the inode reference
leaks.

Send the failure through the existing cleanup path so the inode
reference is dropped before the function returns the error.

Fixes: e70fb328d527 ("xfs: recreate work items when recovering intent items")
Cc: <stable@vger.kernel.org> # v6.8
Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
---
 fs/xfs/xfs_attr_item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index deab14f31b38..841838bc1d0f 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -774,7 +774,7 @@ xfs_attr_recover_work(
 	resv = xlog_recover_resv(&resv);
 	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
 	if (error)
-		return error;
+		goto out_rele;
 	args->trans = tp;
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
@@ -791,6 +791,7 @@ xfs_attr_recover_work(
 	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+out_rele:
 	xfs_irele(ip);
 	return error;
 out_cancel:
-- 
2.20.1

Re: [PATCH v2 1/2] xfs: fix inode ref leak in attr intent recovery

[Darrick J. Wong]

On Wed, Jun 10, 2026 at 10:20:27AM +0800, Yingjie Gao wrote:
> xfs_attri_recover_work() grabs the target inode, attaches it to the
> reconstructed attr work item, and adds that work item to the defer
> pending list.
> 
> If xfs_attr_recover_work() fails to allocate the recovery transaction,
> it returns immediately without dropping the inode reference.  The later
> cancel path only frees the attr work item state, so the inode reference
> leaks.
> 
> Send the failure through the existing cleanup path so the inode
> reference is dropped before the function returns the error.
> 
> Fixes: e70fb328d527 ("xfs: recreate work items when recovering intent items")
> Cc: <stable@vger.kernel.org> # v6.8
> Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>

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

--D

> ---
>  fs/xfs/xfs_attr_item.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index deab14f31b38..841838bc1d0f 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -774,7 +774,7 @@ xfs_attr_recover_work(
>  	resv = xlog_recover_resv(&resv);
>  	error = xfs_trans_alloc(mp, &resv, total, 0, XFS_TRANS_RESERVE, &tp);
>  	if (error)
> -		return error;
> +		goto out_rele;
>  	args->trans = tp;
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> @@ -791,6 +791,7 @@ xfs_attr_recover_work(
>  	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +out_rele:
>  	xfs_irele(ip);
>  	return error;
>  out_cancel:
> -- 
> 2.20.1
> 
> 

[PATCH v2 2/2] xfs: clean up attr intent recovery error paths

[Yingjie Gao]

xfs_attr_recover_work() still uses a backward goto from out_cancel to
out_unlock.

Restructure the cleanup labels into the same linear fallthrough style
used by the neighboring bmap and exchmaps recovery helpers.  Initialize
the local error variable to zero as those helpers do as well, which
makes the shared return path easier to follow.

No functional change intended.

Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
---
 fs/xfs/xfs_attr_item.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 841838bc1d0f..f8aa9dd80bb9 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -739,7 +739,7 @@ xfs_attr_recover_work(
 	struct xfs_trans_res		resv;
 	struct xfs_attri_log_format	*attrp;
 	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
-	int				error;
+	int				error = 0;
 	unsigned int			total = 0;
 
 	/*
@@ -789,14 +789,20 @@ xfs_attr_recover_work(
 		goto out_cancel;
 
 	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
+	if (error)
+		goto out_unlock;
+
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	xfs_irele(ip);
+	return 0;
+
+out_cancel:
+	xfs_trans_cancel(tp);
 out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out_rele:
 	xfs_irele(ip);
 	return error;
-out_cancel:
-	xfs_trans_cancel(tp);
-	goto out_unlock;
 }
 
 /* Re-log an intent item to push the log tail forward. */
-- 
2.20.1

Re: [PATCH v2 2/2] xfs: clean up attr intent recovery error paths

[Darrick J. Wong]

On Wed, Jun 10, 2026 at 10:20:28AM +0800, Yingjie Gao wrote:
> xfs_attr_recover_work() still uses a backward goto from out_cancel to
> out_unlock.

What's so bad about backwards gotos?

--D

> Restructure the cleanup labels into the same linear fallthrough style
> used by the neighboring bmap and exchmaps recovery helpers.  Initialize
> the local error variable to zero as those helpers do as well, which
> makes the shared return path easier to follow.
> 
> No functional change intended.
> 
> Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
> ---
>  fs/xfs/xfs_attr_item.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 841838bc1d0f..f8aa9dd80bb9 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -739,7 +739,7 @@ xfs_attr_recover_work(
>  	struct xfs_trans_res		resv;
>  	struct xfs_attri_log_format	*attrp;
>  	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
> -	int				error;
> +	int				error = 0;
>  	unsigned int			total = 0;
>  
>  	/*
> @@ -789,14 +789,20 @@ xfs_attr_recover_work(
>  		goto out_cancel;
>  
>  	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
> +	if (error)
> +		goto out_unlock;
> +
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	xfs_irele(ip);
> +	return 0;
> +
> +out_cancel:
> +	xfs_trans_cancel(tp);
>  out_unlock:
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out_rele:
>  	xfs_irele(ip);
>  	return error;
> -out_cancel:
> -	xfs_trans_cancel(tp);
> -	goto out_unlock;
>  }
>  
>  /* Re-log an intent item to push the log tail forward. */
> -- 
> 2.20.1
> 
> 

Re: [PATCH v2 2/2] xfs: clean up attr intent recovery error paths

[Yingjie Gao]

在 2026/6/10 13:12, Darrick J. Wong 写道:
> On Wed, Jun 10, 2026 at 10:20:28AM +0800, Yingjie Gao wrote:
>> xfs_attr_recover_work() still uses a backward goto from out_cancel to
>> out_unlock.
> 
> What's so bad about backwards gotos?
> 
> --D
> 
There is nothing wrong with backward gotos. They are perfectly clear
and correct as-is. My only thought was that since the bmap and exchmaps
recovery helpers next door both use the linear fallthrough style, it
might be nice to align this function for consistency. That said, I do
not feel strongly about it -- if you would prefer to leave the backward
goto in place, I am happy to drop patch 2 and proceed with just patch
1.

Either way, thank you for taking the time to review this.

Best regards,
Yingjie
```
>> Restructure the cleanup labels into the same linear fallthrough style
>> used by the neighboring bmap and exchmaps recovery helpers.  Initialize
>> the local error variable to zero as those helpers do as well, which
>> makes the shared return path easier to follow.
>>
>> No functional change intended.
>>
>> Signed-off-by: Yingjie Gao <gaoyingjie@uniontech.com>
>> ---
>>  fs/xfs/xfs_attr_item.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
>> index 841838bc1d0f..f8aa9dd80bb9 100644
>> --- a/fs/xfs/xfs_attr_item.c
>> +++ b/fs/xfs/xfs_attr_item.c
>> @@ -739,7 +739,7 @@ xfs_attr_recover_work(
>>  	struct xfs_trans_res		resv;
>>  	struct xfs_attri_log_format	*attrp;
>>  	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
>> -	int				error;
>> +	int				error = 0;
>>  	unsigned int			total = 0;
>>  
>>  	/*
>> @@ -789,14 +789,20 @@ xfs_attr_recover_work(
>>  		goto out_cancel;
>>  
>>  	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
>> +	if (error)
>> +		goto out_unlock;
>> +
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +	xfs_irele(ip);
>> +	return 0;
>> +
>> +out_cancel:
>> +	xfs_trans_cancel(tp);
>>  out_unlock:
>>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>  out_rele:
>>  	xfs_irele(ip);
>>  	return error;
>> -out_cancel:
>> -	xfs_trans_cancel(tp);
>> -	goto out_unlock;
>>  }
>>  
>>  /* Re-log an intent item to push the log tail forward. */
>> -- 
>> 2.20.1
>>
>>
> 

Re: [PATCH v2 0/2] xfs: fix inode leak and clean up attr intent recovery

[Carlos Maiolino]

On Wed, Jun 10, 2026 at 10:20:26AM +0800, Yingjie Gao wrote:
> Hi all,
> 
> The first patch fixes a leaked inode reference in
> xfs_attr_recover_work() when transaction allocation fails after the
> recovered attr work item has been attached to the defer pending list.
> 
> The second patch cleans up the same function's exit paths so they follow
> the same linear cleanup style as the neighboring bmap and exchmaps
> recovery helpers, and initializes the local error variable in the same
> style.
> 
> Changes since v1:
> - route the transaction allocation failure through out_rele in patch 1,
>   as Darrick suggested;
> - keep patch 2 focused on the remaining cleanup-path reshaping and the
>   local error initialization.

Please avoid sending new versions in-reply-to the previous version. Send
them as a new thread.

> 
> Thanks,
> Yingjie
> 
> Yingjie Gao (2):
>   xfs: fix inode ref leak in attr intent recovery
>   xfs: clean up attr intent recovery error paths
> 
>  fs/xfs/xfs_attr_item.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> -- 
> 2.20.1
> 

Re: [PATCH v2 0/2] xfs: fix inode leak and clean up attr intent recovery

[Yingjie Gao]

在 2026/6/10 14:30, Carlos Maiolino 写道:
> On Wed, Jun 10, 2026 at 10:20:26AM +0800, Yingjie Gao wrote:
>> Hi all,
>>
>> The first patch fixes a leaked inode reference in
>> xfs_attr_recover_work() when transaction allocation fails after the
>> recovered attr work item has been attached to the defer pending list.
>>
>> The second patch cleans up the same function's exit paths so they follow
>> the same linear cleanup style as the neighboring bmap and exchmaps
>> recovery helpers, and initializes the local error variable in the same
>> style.
>>
>> Changes since v1:
>> - route the transaction allocation failure through out_rele in patch 1,
>>   as Darrick suggested;
>> - keep patch 2 focused on the remaining cleanup-path reshaping and the
>>   local error initialization.
> 
> Please avoid sending new versions in-reply-to the previous version. Send
> them as a new thread.
> 
Understood, sorry about that.  I will make sure new versions are sent
as separate threads going forward.

Thanks,
Yingjie
```
>>
>> Thanks,
>> Yingjie
>>
>> Yingjie Gao (2):
>>   xfs: fix inode ref leak in attr intent recovery
>>   xfs: clean up attr intent recovery error paths
>>
>>  fs/xfs/xfs_attr_item.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> -- 
>> 2.20.1
>>
>