[PATCH] xfs: fix double xfs_perag_rele() in xfs_filestream_pick_ag()

[PATCH] xfs: fix double xfs_perag_rele() in xfs_filestream_pick_ag()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

xfs_bmap_longest_free_extent() can return an error when accessing
the AGF fails. In this case, the behaviour of
xfs_filestream_pick_ag() is conditional on the error. We may
continue the loop, or break out of it. The error handling after the
loop cleans up the perag reference held when the break occurs. If we
continue, the next loop iteration handles cleaning up the perag
reference.

EIther way, we don't need to release the active perag reference when
xfs_bmap_longest_free_extent() fails. Doing so means we do a double
decrement on the active reference count, and this causes tha active
reference count to fall to zero. At this point, new active
references will fail.

This leads to unmount hanging because it tries to grab active
references to that perag, only for it to fail. This happens inside a
loop that retries until a inode tree radix tree tag is cleared,
which cannot happen because we can't get an active reference to the
perag.

The unmount livelocks in this path:

  xfs_reclaim_inodes+0x80/0xc0
  xfs_unmount_flush_inodes+0x5b/0x70
  xfs_unmountfs+0x5b/0x1a0
  xfs_fs_put_super+0x49/0x110
  generic_shutdown_super+0x7c/0x1a0
  kill_block_super+0x27/0x50
  deactivate_locked_super+0x30/0x90
  deactivate_super+0x3c/0x50
  cleanup_mnt+0xc2/0x160
  __cleanup_mnt+0x12/0x20
  task_work_run+0x5e/0xa0
  exit_to_user_mode_prepare+0x1bc/0x1c0
  syscall_exit_to_user_mode+0x16/0x40
  do_syscall_64+0x40/0x80
  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Reported-by: Pengfei Xu <pengfei.xu@intel.com>
Fixes: eb70aa2d8ed9 ("xfs: use for_each_perag_wrap in xfs_filestream_pick_ag")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_filestream.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
index 22c13933c8f8..2fc98d313708 100644
--- a/fs/xfs/xfs_filestream.c
+++ b/fs/xfs/xfs_filestream.c
@@ -78,7 +78,6 @@ xfs_filestream_pick_ag(
 		*longest = 0;
 		err = xfs_bmap_longest_free_extent(pag, NULL, longest);
 		if (err) {
-			xfs_perag_rele(pag);
 			if (err != -EAGAIN)
 				break;
 			/* Couldn't lock the AGF, skip this AG. */
-- 
2.40.1

Re: [PATCH] xfs: fix double xfs_perag_rele() in xfs_filestream_pick_ag()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] xfs: fix double xfs_perag_rele() in xfs_filestream_pick_ag()

[Darrick J. Wong]

On Mon, May 29, 2023 at 12:59:50PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_bmap_longest_free_extent() can return an error when accessing
> the AGF fails. In this case, the behaviour of
> xfs_filestream_pick_ag() is conditional on the error. We may
> continue the loop, or break out of it. The error handling after the
> loop cleans up the perag reference held when the break occurs. If we
> continue, the next loop iteration handles cleaning up the perag
> reference.
> 
> EIther way, we don't need to release the active perag reference when
> xfs_bmap_longest_free_extent() fails. Doing so means we do a double
> decrement on the active reference count, and this causes tha active
> reference count to fall to zero. At this point, new active
> references will fail.
> 
> This leads to unmount hanging because it tries to grab active
> references to that perag, only for it to fail. This happens inside a
> loop that retries until a inode tree radix tree tag is cleared,
> which cannot happen because we can't get an active reference to the
> perag.

Wouldn't it be nice if C had^W^Wthe kernel coding rules allowed
iterators that managed these active refs for us...

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

--D


> The unmount livelocks in this path:
> 
>   xfs_reclaim_inodes+0x80/0xc0
>   xfs_unmount_flush_inodes+0x5b/0x70
>   xfs_unmountfs+0x5b/0x1a0
>   xfs_fs_put_super+0x49/0x110
>   generic_shutdown_super+0x7c/0x1a0
>   kill_block_super+0x27/0x50
>   deactivate_locked_super+0x30/0x90
>   deactivate_super+0x3c/0x50
>   cleanup_mnt+0xc2/0x160
>   __cleanup_mnt+0x12/0x20
>   task_work_run+0x5e/0xa0
>   exit_to_user_mode_prepare+0x1bc/0x1c0
>   syscall_exit_to_user_mode+0x16/0x40
>   do_syscall_64+0x40/0x80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Fixes: eb70aa2d8ed9 ("xfs: use for_each_perag_wrap in xfs_filestream_pick_ag")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_filestream.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index 22c13933c8f8..2fc98d313708 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -78,7 +78,6 @@ xfs_filestream_pick_ag(
>  		*longest = 0;
>  		err = xfs_bmap_longest_free_extent(pag, NULL, longest);
>  		if (err) {
> -			xfs_perag_rele(pag);
>  			if (err != -EAGAIN)
>  				break;
>  			/* Couldn't lock the AGF, skip this AG. */
> -- 
> 2.40.1
>