Re: [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Mar 26, 2026 at 02:04:19PM +0100, Carlos Maiolino wrote:
> On Mon, Mar 23, 2026 at 02:03:51PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Fix this function to avoid exposing a stale pointer to the caller when
> > returning an error code after dqattach fails.
> > 
> > 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>
> > Reviewed-by: Long Li <leo.lilong@huawei.com>
> > ---
> 
> Hi.
> 
> 
> >  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;
> 
> Honestly I tend to agree with Christoph here. I don't really understand
> what bug, real or theoretical, this is trying to fix, other than
> somebody not aborting whatever operation is being done if an error
> is returned.
> 
> I don't see any issue with an extra inode pointer being passed on the
> stack here, but I don't see any benefit for this either.
> 
> Did you fall into this somehow? Have you seen this into the wild? I'll
> pull the other patches meanwhile, but I think this one deserves more
> discussion.

Yeah, look who wrote the function and the bad error handling.  You could
say that the *author* didn't understand his own function's outparam
conventions and wants to prevent himself or anyone else from making
further mistakes.  ;)

--D