diff mbox series

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

Message ID 20241023133755.524345-2-hch@lst.de (mailing list archive)
State Under Review
Headers show
Series [1/2] xfs: fix finding a last resort AG in xfs_filestream_pick_ag | expand

Commit Message

Christoph Hellwig Oct. 23, 2024, 1:37 p.m. UTC 
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
---
 fs/xfs/xfs_filestream.c | 23 ++++++++++++-----------
 fs/xfs/xfs_trace.h      | 15 +++++----------
 2 files changed, 17 insertions(+), 21 deletions(-)

Comments

Darrick J. Wong Oct. 24, 2024, 5:58 p.m. UTC | #1 
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@syzkaller.appspotmail.com
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com

Cc: <stable@vger.kernel.org> # v6.3
Fixes: f8f1ed1ab3baba ("xfs: return a referenced perag from filestreams allocator")
Perhaps?

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--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
> 
>
diff mbox series

Patch

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),