Re: [PATCH 2/2] xfs: fix finding a last resort AG in xfs_filestream_pick_ag

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

On Tue, Oct 22, 2024 at 02:13:38PM +0200, Christoph Hellwig wrote:
> When the main loop in xfs_filestream_pick_ag fails to find a suitable
> AG it tries to just pick the online AG.  But the loop for that uses
> args->pag as loop iterator while the later code expects pag to be
> set.  Fix this by reusing the max_pag case for this last resort, and
> also add a check for impossible case of no AG just to make sure that
> the uninitialized pag doesn't even escape in theory.
> 
> Reported-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com

Well, that bug was pretty subtle. :(
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Nit: should trace_xfs_filestream_pick() lose its third argument?

--D

> ---
>  fs/xfs/xfs_filestream.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index f523027cc32586..ecf8a0f6c1362e 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -64,7 +64,7 @@ xfs_filestream_pick_ag(
>  	struct xfs_perag	*pag;
>  	struct xfs_perag	*max_pag = NULL;
>  	xfs_extlen_t		minlen = *longest;
> -	xfs_extlen_t		free = 0, minfree, maxfree = 0;
> +	xfs_extlen_t		minfree, maxfree = 0;
>  	xfs_agnumber_t		agno;
>  	bool			first_pass = true;
>  
> @@ -113,7 +113,6 @@ xfs_filestream_pick_ag(
>  			     !(flags & XFS_PICK_USERDATA) ||
>  			     (flags & XFS_PICK_LOWSPACE))) {
>  				/* Break out, retaining the reference on the AG. */
> -				free = pag->pagf_freeblks;
>  				if (max_pag)
>  					xfs_perag_rele(max_pag);
>  				goto done;
> @@ -149,18 +148,19 @@ xfs_filestream_pick_ag(
>  	 * filestream. It none suit, just use whatever AG we can grab.
>  	 */
>  	if (!max_pag) {
> -		for_each_perag_wrap(args->mp, 0, start_agno, args->pag)
> +		for_each_perag_wrap(args->mp, 0, start_agno, pag) {
> +			max_pag = pag;
>  			break;
> -		atomic_inc(&args->pag->pagf_fstrms);
> -		*longest = 0;
> -	} else {
> -		pag = max_pag;
> -		free = maxfree;
> -		atomic_inc(&pag->pagf_fstrms);
> +		}
> +		/* Bail if there are no AGs at all to select from. */
> +		if (!max_pag)
> +			return -ENOSPC;
>  	}
>  
> +	pag = max_pag;
> +	atomic_inc(&pag->pagf_fstrms);
>  done:
> -	trace_xfs_filestream_pick(pag, pino, free);
> +	trace_xfs_filestream_pick(pag, pino, pag->pagf_freeblks);
>  	args->pag = pag;
>  	return 0;
>  
> -- 
> 2.45.2
> 
>