filestreams syzbot fix

filestreams syzbot fix

[Christoph Hellwig]

Hi all,

this series fixes the recently reported crash exposed by syzbot in the
filestreams code.

[PATCH 1/2] xfs: streamline xfs_filestream_pick_ag

[Christoph Hellwig]

Directly return the error from xfs_bmap_longest_free_extent instead
of breaking from the loop and handling it there, and use a done
label to directly jump to the exist when we found a suitable perag
structure to reduce the indentation level and pag/max_pag check
complexity in the tail of the function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_filestream.c | 95 ++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 49 deletions(-)

diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
index e3aaa055559781..f523027cc32586 100644
--- a/fs/xfs/xfs_filestream.c
+++ b/fs/xfs/xfs_filestream.c
@@ -67,22 +67,28 @@ xfs_filestream_pick_ag(
 	xfs_extlen_t		free = 0, minfree, maxfree = 0;
 	xfs_agnumber_t		agno;
 	bool			first_pass = true;
-	int			err;
 
 	/* 2% of an AG's blocks must be free for it to be chosen. */
 	minfree = mp->m_sb.sb_agblocks / 50;
 
 restart:
 	for_each_perag_wrap(mp, start_agno, agno, pag) {
+		int		err;
+
 		trace_xfs_filestream_scan(pag, pino);
+
 		*longest = 0;
 		err = xfs_bmap_longest_free_extent(pag, NULL, longest);
 		if (err) {
-			if (err != -EAGAIN)
-				break;
-			/* Couldn't lock the AGF, skip this AG. */
-			err = 0;
-			continue;
+			if (err == -EAGAIN) {
+				/* Couldn't lock the AGF, skip this AG. */
+				err = 0;
+				continue;
+			}
+			xfs_perag_rele(pag);
+			if (max_pag)
+				xfs_perag_rele(max_pag);
+			return err;
 		}
 
 		/* Keep track of the AG with the most free blocks. */
@@ -108,7 +114,9 @@ xfs_filestream_pick_ag(
 			     (flags & XFS_PICK_LOWSPACE))) {
 				/* Break out, retaining the reference on the AG. */
 				free = pag->pagf_freeblks;
-				break;
+				if (max_pag)
+					xfs_perag_rele(max_pag);
+				goto done;
 			}
 		}
 
@@ -116,53 +124,42 @@ xfs_filestream_pick_ag(
 		atomic_dec(&pag->pagf_fstrms);
 	}
 
-	if (err) {
-		xfs_perag_rele(pag);
-		if (max_pag)
-			xfs_perag_rele(max_pag);
-		return err;
+	/*
+	 * Allow a second pass to give xfs_bmap_longest_free_extent() another
+	 * attempt at locking AGFs that it might have skipped over before we
+	 * fail.
+	 */
+	if (first_pass) {
+		first_pass = false;
+		goto restart;
 	}
 
-	if (!pag) {
-		/*
-		 * Allow a second pass to give xfs_bmap_longest_free_extent()
-		 * another attempt at locking AGFs that it might have skipped
-		 * over before we fail.
-		 */
-		if (first_pass) {
-			first_pass = false;
-			goto restart;
-		}
-
-		/*
-		 * We must be low on data space, so run a final lowspace
-		 * optimised selection pass if we haven't already.
-		 */
-		if (!(flags & XFS_PICK_LOWSPACE)) {
-			flags |= XFS_PICK_LOWSPACE;
-			goto restart;
-		}
+	/*
+	 * We must be low on data space, so run a final lowspace optimised
+	 * selection pass if we haven't already.
+	 */
+	if (!(flags & XFS_PICK_LOWSPACE)) {
+		flags |= XFS_PICK_LOWSPACE;
+		goto restart;
+	}
 
-		/*
-		 * No unassociated AGs are available, so select the AG with the
-		 * most free space, regardless of whether it's already in use by
-		 * another 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)
-				break;
-			atomic_inc(&args->pag->pagf_fstrms);
-			*longest = 0;
-		} else {
-			pag = max_pag;
-			free = maxfree;
-			atomic_inc(&pag->pagf_fstrms);
-		}
-	} else if (max_pag) {
-		xfs_perag_rele(max_pag);
+	/*
+	 * No unassociated AGs are available, so select the AG with the most
+	 * free space, regardless of whether it's already in use by another
+	 * 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)
+			break;
+		atomic_inc(&args->pag->pagf_fstrms);
+		*longest = 0;
+	} else {
+		pag = max_pag;
+		free = maxfree;
+		atomic_inc(&pag->pagf_fstrms);
 	}
 
+done:
 	trace_xfs_filestream_pick(pag, pino, free);
 	args->pag = pag;
 	return 0;
-- 
2.45.2

Re: [PATCH 1/2] xfs: streamline xfs_filestream_pick_ag

[Darrick J. Wong]

On Tue, Oct 22, 2024 at 02:13:37PM +0200, Christoph Hellwig wrote:
> Directly return the error from xfs_bmap_longest_free_extent instead
> of breaking from the loop and handling it there, and use a done
> label to directly jump to the exist when we found a suitable perag
> structure to reduce the indentation level and pag/max_pag check
> complexity in the tail of the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

So the key change here is that now the function can exit directly from
the for_each_perag_wrap loop if it finds a suitable perag, and that the
rest of the function has less indentation?

Ok, sounds good to me though the bugfix probably should've come first.

Don't really care either way, so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_filestream.c | 95 ++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index e3aaa055559781..f523027cc32586 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -67,22 +67,28 @@ xfs_filestream_pick_ag(
>  	xfs_extlen_t		free = 0, minfree, maxfree = 0;
>  	xfs_agnumber_t		agno;
>  	bool			first_pass = true;
> -	int			err;
>  
>  	/* 2% of an AG's blocks must be free for it to be chosen. */
>  	minfree = mp->m_sb.sb_agblocks / 50;
>  
>  restart:
>  	for_each_perag_wrap(mp, start_agno, agno, pag) {
> +		int		err;
> +
>  		trace_xfs_filestream_scan(pag, pino);
> +
>  		*longest = 0;
>  		err = xfs_bmap_longest_free_extent(pag, NULL, longest);
>  		if (err) {
> -			if (err != -EAGAIN)
> -				break;
> -			/* Couldn't lock the AGF, skip this AG. */
> -			err = 0;
> -			continue;
> +			if (err == -EAGAIN) {
> +				/* Couldn't lock the AGF, skip this AG. */
> +				err = 0;
> +				continue;
> +			}
> +			xfs_perag_rele(pag);
> +			if (max_pag)
> +				xfs_perag_rele(max_pag);
> +			return err;
>  		}
>  
>  		/* Keep track of the AG with the most free blocks. */
> @@ -108,7 +114,9 @@ xfs_filestream_pick_ag(
>  			     (flags & XFS_PICK_LOWSPACE))) {
>  				/* Break out, retaining the reference on the AG. */
>  				free = pag->pagf_freeblks;
> -				break;
> +				if (max_pag)
> +					xfs_perag_rele(max_pag);
> +				goto done;
>  			}
>  		}
>  
> @@ -116,53 +124,42 @@ xfs_filestream_pick_ag(
>  		atomic_dec(&pag->pagf_fstrms);
>  	}
>  
> -	if (err) {
> -		xfs_perag_rele(pag);
> -		if (max_pag)
> -			xfs_perag_rele(max_pag);
> -		return err;
> +	/*
> +	 * Allow a second pass to give xfs_bmap_longest_free_extent() another
> +	 * attempt at locking AGFs that it might have skipped over before we
> +	 * fail.
> +	 */
> +	if (first_pass) {
> +		first_pass = false;
> +		goto restart;
>  	}
>  
> -	if (!pag) {
> -		/*
> -		 * Allow a second pass to give xfs_bmap_longest_free_extent()
> -		 * another attempt at locking AGFs that it might have skipped
> -		 * over before we fail.
> -		 */
> -		if (first_pass) {
> -			first_pass = false;
> -			goto restart;
> -		}
> -
> -		/*
> -		 * We must be low on data space, so run a final lowspace
> -		 * optimised selection pass if we haven't already.
> -		 */
> -		if (!(flags & XFS_PICK_LOWSPACE)) {
> -			flags |= XFS_PICK_LOWSPACE;
> -			goto restart;
> -		}
> +	/*
> +	 * We must be low on data space, so run a final lowspace optimised
> +	 * selection pass if we haven't already.
> +	 */
> +	if (!(flags & XFS_PICK_LOWSPACE)) {
> +		flags |= XFS_PICK_LOWSPACE;
> +		goto restart;
> +	}
>  
> -		/*
> -		 * No unassociated AGs are available, so select the AG with the
> -		 * most free space, regardless of whether it's already in use by
> -		 * another 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)
> -				break;
> -			atomic_inc(&args->pag->pagf_fstrms);
> -			*longest = 0;
> -		} else {
> -			pag = max_pag;
> -			free = maxfree;
> -			atomic_inc(&pag->pagf_fstrms);
> -		}
> -	} else if (max_pag) {
> -		xfs_perag_rele(max_pag);
> +	/*
> +	 * No unassociated AGs are available, so select the AG with the most
> +	 * free space, regardless of whether it's already in use by another
> +	 * 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)
> +			break;
> +		atomic_inc(&args->pag->pagf_fstrms);
> +		*longest = 0;
> +	} else {
> +		pag = max_pag;
> +		free = maxfree;
> +		atomic_inc(&pag->pagf_fstrms);
>  	}
>  
> +done:
>  	trace_xfs_filestream_pick(pag, pino, free);
>  	args->pag = pag;
>  	return 0;
> -- 
> 2.45.2
> 
> 

Re: [PATCH 1/2] xfs: streamline xfs_filestream_pick_ag

[Christoph Hellwig]

On Tue, Oct 22, 2024 at 11:05:35AM -0700, Darrick J. Wong wrote:
> On Tue, Oct 22, 2024 at 02:13:37PM +0200, Christoph Hellwig wrote:
> > Directly return the error from xfs_bmap_longest_free_extent instead
> > of breaking from the loop and handling it there, and use a done
> > label to directly jump to the exist when we found a suitable perag
> > structure to reduce the indentation level and pag/max_pag check
> > complexity in the tail of the function.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> So the key change here is that now the function can exit directly from
> the for_each_perag_wrap loop if it finds a suitable perag, and that the
> rest of the function has less indentation?

Yes.

> Ok, sounds good to me though the bugfix probably should've come first.

I needed the refactor to understand the mess in the function :)
But I'll reorder it.

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

[Christoph Hellwig]

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

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

[Darrick J. Wong]

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

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

[Christoph Hellwig]

On Tue, Oct 22, 2024 at 10:59:28AM -0700, Darrick J. Wong wrote:
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> Nit: should trace_xfs_filestream_pick() lose its third argument?

Yes.