On Wed, Oct 23, 2024 at 03:37:22PM +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
Cc: <stable@xxxxxxxxxxxxxxx> # v6.3
Fixes: f8f1ed1ab3baba ("xfs: return a referenced perag from filestreams allocator")
Perhaps?
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_filestream.c | 23 ++++++++++++-----------
> fs/xfs/xfs_trace.h | 15 +++++----------
> 2 files changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index e3aaa0555597..88bd23ce74cd 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;
> int err;
> @@ -107,7 +107,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;
> break;
> }
> }
> @@ -150,23 +149,25 @@ xfs_filestream_pick_ag(
> * 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);
> } else if (max_pag) {
> xfs_perag_rele(max_pag);
> }
>
> - trace_xfs_filestream_pick(pag, pino, free);
> + trace_xfs_filestream_pick(pag, pino);
> args->pag = pag;
> return 0;
> -
> }
>
> static struct xfs_inode *
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index ee9f0b1f548d..fcb2bad4f76e 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -691,8 +691,8 @@ DEFINE_FILESTREAM_EVENT(xfs_filestream_lookup);
> DEFINE_FILESTREAM_EVENT(xfs_filestream_scan);
>
> TRACE_EVENT(xfs_filestream_pick,
> - TP_PROTO(struct xfs_perag *pag, xfs_ino_t ino, xfs_extlen_t free),
> - TP_ARGS(pag, ino, free),
> + TP_PROTO(struct xfs_perag *pag, xfs_ino_t ino),
> + TP_ARGS(pag, ino),
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(xfs_ino_t, ino)
> @@ -703,14 +703,9 @@ TRACE_EVENT(xfs_filestream_pick,
> TP_fast_assign(
> __entry->dev = pag->pag_mount->m_super->s_dev;
> __entry->ino = ino;
> - if (pag) {
> - __entry->agno = pag->pag_agno;
> - __entry->streams = atomic_read(&pag->pagf_fstrms);
> - } else {
> - __entry->agno = NULLAGNUMBER;
> - __entry->streams = 0;
> - }
> - __entry->free = free;
> + __entry->agno = pag->pag_agno;
> + __entry->streams = atomic_read(&pag->pagf_fstrms);
> + __entry->free = pag->pagf_freeblks;
> ),
> TP_printk("dev %d:%d ino 0x%llx agno 0x%x streams %d free %d",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> --
> 2.45.2
>
>
