[PATCH 1/2] xfs: don't allow UAF in xlog_recover_iget

[PATCH 1/2] xfs: don't allow UAF in xlog_recover_iget

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix this function to avoid touching the passed in ipp argument until
we're 100% certain that we're returning zero.  This avoids creating a
dangling pointer in the caller.  The xfs_irele call in
xfs_attri_recover_work was never correct and should go away.

Cc: <stable@vger.kernel.org> # v5.15
Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_log_recover.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 09e6678ca4878e..0e91a62348eb79 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1745,21 +1745,23 @@ xlog_recover_iget(
 	xfs_ino_t		ino,
 	struct xfs_inode	**ipp)
 {
+	struct xfs_inode	*ip;
 	int			error;
 
-	error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
+	error = xfs_iget(mp, NULL, ino, 0, 0, &ip);
 	if (error)
 		return error;
 
-	error = xfs_qm_dqattach(*ipp);
+	error = xfs_qm_dqattach(ip);
 	if (error) {
-		xfs_irele(*ipp);
+		xfs_irele(ip);
 		return error;
 	}
 
-	if (VFS_I(*ipp)->i_nlink == 0)
-		xfs_iflags_set(*ipp, XFS_IRECOVERY);
+	if (VFS_I(ip)->i_nlink == 0)
+		xfs_iflags_set(ip, XFS_IRECOVERY);
 
+	*ipp = ip;
 	return 0;
 }
 

[PATCH 2/2] xfs: don't irele after failing to iget in xfs_attri_recover_work

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xlog_recovery_iget* never set @ip to a valid pointer if they return
zero, so this irele will walk off a dangling pointer.  Fix that.

Cc: <stable@vger.kernel.org> # v6.10
Fixes: ae673f534a3097 ("xfs: record inode generation in xattr update log intent items")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 354472bf45f145..fe909bc44c3a79 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -653,7 +653,6 @@ xfs_attri_recover_work(
 		break;
 	}
 	if (error) {
-		xfs_irele(ip);
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
 				sizeof(*attrp));
 		return ERR_PTR(-EFSCORRUPTED);

Re: [PATCH 1/2] xfs: don't allow UAF in xlog_recover_iget

[Long Li]

On Thu, Mar 19, 2026 at 10:09:38AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix this function to avoid touching the passed in ipp argument until
> we're 100% certain that we're returning zero.  This avoids creating a
> dangling pointer in the caller.  The xfs_irele call in
> xfs_attri_recover_work was never correct and should go away.
> 
> Cc: <stable@vger.kernel.org> # v5.15
> Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_log_recover.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 09e6678ca4878e..0e91a62348eb79 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1745,21 +1745,23 @@ xlog_recover_iget(
>  	xfs_ino_t		ino,
>  	struct xfs_inode	**ipp)
>  {
> +	struct xfs_inode	*ip;
>  	int			error;
>  
> -	error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
> +	error = xfs_iget(mp, NULL, ino, 0, 0, &ip);
>  	if (error)
>  		return error;
>  
> -	error = xfs_qm_dqattach(*ipp);
> +	error = xfs_qm_dqattach(ip);
>  	if (error) {
> -		xfs_irele(*ipp);
> +		xfs_irele(ip);
>  		return error;
>  	}
>  
> -	if (VFS_I(*ipp)->i_nlink == 0)
> -		xfs_iflags_set(*ipp, XFS_IRECOVERY);
> +	if (VFS_I(ip)->i_nlink == 0)
> +		xfs_iflags_set(ip, XFS_IRECOVERY);
>  
> +	*ipp = ip;
>  	return 0;
>  }
>  
> 
Looks good
Reviewed-by: Long Li <leo.lilong@huawei.com>

Re: [PATCH 2/2] xfs: don't irele after failing to iget in xfs_attri_recover_work

[Long Li]

On Thu, Mar 19, 2026 at 10:10:44AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xlog_recovery_iget* never set @ip to a valid pointer if they return
> zero, so this irele will walk off a dangling pointer.  Fix that.
> 
> Cc: <stable@vger.kernel.org> # v6.10
> Fixes: ae673f534a3097 ("xfs: record inode generation in xattr update log intent items")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_attr_item.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 354472bf45f145..fe909bc44c3a79 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -653,7 +653,6 @@ xfs_attri_recover_work(
>  		break;
>  	}
>  	if (error) {
> -		xfs_irele(ip);
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
>  				sizeof(*attrp));
>  		return ERR_PTR(-EFSCORRUPTED);
> 

Looks good
Reviewed-by: Long Li <leo.lilong@huawei.com>

Re: [PATCH 1/2] xfs: don't allow UAF in xlog_recover_iget

[Christoph Hellwig]

"Don't allow UAF" reads rally giberrish.

On Thu, Mar 19, 2026 at 10:09:38AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix this function to avoid touching the passed in ipp argument until
> we're 100% certain that we're returning zero.  This avoids creating a
> dangling pointer in the caller.

Why does this matter?  The caller need to check the return value.

> The xfs_irele call in
> xfs_attri_recover_work was never correct and should go away.

No idea what this is supposed to say, seem unrelated to this patch.

Re: [PATCH 2/2] xfs: don't irele after failing to iget in xfs_attri_recover_work

[Christoph Hellwig]

On Thu, Mar 19, 2026 at 10:10:44AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xlog_recovery_iget* never set @ip to a valid pointer if they return
> zero, so this irele will walk off a dangling pointer.  Fix that.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: don't allow UAF in xlog_recover_iget

[Darrick J. Wong]

On Fri, Mar 20, 2026 at 12:19:19AM -0700, Christoph Hellwig wrote:
> "Don't allow UAF" reads rally giberrish.
> 
> On Thu, Mar 19, 2026 at 10:09:38AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Fix this function to avoid touching the passed in ipp argument until
> > we're 100% certain that we're returning zero.  This avoids creating a
> > dangling pointer in the caller.
> 
> Why does this matter?  The caller need to check the return value.

I think the fundamental problem here is that Long Li ran a static
checker, and the static checker suggested that the callsite in
xfs_attri_recover_work should be fixed by guarding the xfs_irele call in
the error handler with a null pointer check.  That's completely wrong,
the xfs_irele should never have been put in the error path at all.

So the proper fix is the one in the next patch that removes the
xfs_irele.  At the same time, xlog_recover_iget should get fixed so that
it never exposes stale pointers in the first place.

"Fix this function to avoid exposing a stale pointer to the caller when
returning an error code after dqattach fails." ?

> > The xfs_irele call in
> > xfs_attri_recover_work was never correct and should go away.
> 
> No idea what this is supposed to say, seem unrelated to this patch.

Yeah, I'll just drop that.

--D

Re: [PATCH 1/2] xfs: don't allow UAF in xlog_recover_iget

[Christoph Hellwig]

On Fri, Mar 20, 2026 at 07:37:19AM -0700, Darrick J. Wong wrote:
> > Why does this matter?  The caller need to check the return value.
> 
> I think the fundamental problem here is that Long Li ran a static
> checker, and the static checker suggested that the callsite in
> xfs_attri_recover_work should be fixed by guarding the xfs_irele call in
> the error handler with a null pointer check.  That's completely wrong,
> the xfs_irele should never have been put in the error path at all.

Yes.

> So the proper fix is the one in the next patch that removes the
> xfs_irele.  At the same time, xlog_recover_iget should get fixed so that
> it never exposes stale pointers in the first place.

Agreed.

> "Fix this function to avoid exposing a stale pointer to the caller when
> returning an error code after dqattach fails." ?

I'd just drop this patch.