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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Tested-by: syzbot+4125a3c514e3436a02e6@xxxxxxxxxxxxxxxxxxxxxxxxx
Well, that bug was pretty subtle. :(
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
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
>
>
