[PATCH] xfs: fix inode number overflow in ifree cluster helper

[PATCH] xfs: fix inode number overflow in ifree cluster helper

[Brian Foster]

Qian Cai reports seemingly random buffer read verifier errors during
filesystem writeback. This was isolated to a recent patch that
factored out some inode cluster freeing code and happened to cast an
unsigned inode number type to a signed value. If the inode number
value overflows, we can skip marking in-core inodes associated with
the underlying buffer stale at the time the physical inodes are
freed. If such an inode happens to be dirty, xfsaild will eventually
attempt to write it back over non-inode blocks. The invalidation of
the underlying inode buffer causes writeback to read the buffer from
disk. This fails the read verifier (preventing eventual corruption)
if the buffer no longer looks like an inode cluster. Analysis by
Dave Chinner.

Fix up the helper to use the proper type for inode number values.

Fixes: 5806165a6663 ("xfs: factor inode lookup from xfs_ifree_cluster")
Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---

Fixes the problem described here[1]. I wasn't sure if we planned on
fixing the original patch in for-next or wanted a separate patch. Feel
free to commit standalone or fold into the original...

Brian

[1] https://lore.kernel.org/linux-xfs/990EDC4E-1A4E-4AC3-84D9-078ACF5EB9CC@lca.pw/

 fs/xfs/xfs_inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 0cac0d37e3ae..ae86c870da92 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2511,7 +2511,7 @@ static struct xfs_inode *
 xfs_ifree_get_one_inode(
 	struct xfs_perag	*pag,
 	struct xfs_inode	*free_ip,
-	int			inum)
+	xfs_ino_t		inum)
 {
 	struct xfs_mount	*mp = pag->pag_mount;
 	struct xfs_inode	*ip;
-- 
2.21.1

Re: [PATCH] xfs: fix inode number overflow in ifree cluster helper

[Christoph Hellwig]

On Thu, Apr 02, 2020 at 06:57:18AM -0400, Brian Foster wrote:
> Qian Cai reports seemingly random buffer read verifier errors during
> filesystem writeback. This was isolated to a recent patch that
> factored out some inode cluster freeing code and happened to cast an
> unsigned inode number type to a signed value. If the inode number
> value overflows, we can skip marking in-core inodes associated with
> the underlying buffer stale at the time the physical inodes are
> freed. If such an inode happens to be dirty, xfsaild will eventually
> attempt to write it back over non-inode blocks. The invalidation of
> the underlying inode buffer causes writeback to read the buffer from
> disk. This fails the read verifier (preventing eventual corruption)
> if the buffer no longer looks like an inode cluster. Analysis by
> Dave Chinner.
> 
> Fix up the helper to use the proper type for inode number values.
> 
> Fixes: 5806165a6663 ("xfs: factor inode lookup from xfs_ifree_cluster")
> Reported-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Fixes the problem described here[1]. I wasn't sure if we planned on
> fixing the original patch in for-next or wanted a separate patch. Feel
> free to commit standalone or fold into the original...

Looks good:

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

Re: [PATCH] xfs: fix inode number overflow in ifree cluster helper

[Darrick J. Wong]

On Thu, Apr 02, 2020 at 06:57:18AM -0400, Brian Foster wrote:
> Qian Cai reports seemingly random buffer read verifier errors during
> filesystem writeback. This was isolated to a recent patch that
> factored out some inode cluster freeing code and happened to cast an
> unsigned inode number type to a signed value. If the inode number
> value overflows, we can skip marking in-core inodes associated with
> the underlying buffer stale at the time the physical inodes are
> freed. If such an inode happens to be dirty, xfsaild will eventually
> attempt to write it back over non-inode blocks. The invalidation of
> the underlying inode buffer causes writeback to read the buffer from
> disk. This fails the read verifier (preventing eventual corruption)
> if the buffer no longer looks like an inode cluster. Analysis by
> Dave Chinner.
> 
> Fix up the helper to use the proper type for inode number values.
> 
> Fixes: 5806165a6663 ("xfs: factor inode lookup from xfs_ifree_cluster")
> Reported-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Fixes the problem described here[1]. I wasn't sure if we planned on
> fixing the original patch in for-next or wanted a separate patch. Feel
> free to commit standalone or fold into the original...
> 
> Brian
> 
> [1] https://lore.kernel.org/linux-xfs/990EDC4E-1A4E-4AC3-84D9-078ACF5EB9CC@lca.pw/

Looks ok, I'll probably just attach it to the end rather than rebase
for-next at this super late point...

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
>  fs/xfs/xfs_inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 0cac0d37e3ae..ae86c870da92 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2511,7 +2511,7 @@ static struct xfs_inode *
>  xfs_ifree_get_one_inode(
>  	struct xfs_perag	*pag,
>  	struct xfs_inode	*free_ip,
> -	int			inum)
> +	xfs_ino_t		inum)
>  {
>  	struct xfs_mount	*mp = pag->pag_mount;
>  	struct xfs_inode	*ip;
> -- 
> 2.21.1
>