* [PATCH 0/3] xfs: random fixes for 5.11
@ 2020-12-04 1:12 Darrick J. Wong
2020-12-04 1:12 ` [PATCH 1/3] xfs: detect overflows in bmbt records Darrick J. Wong
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Darrick J. Wong @ 2020-12-04 1:12 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
Hi all,
Here's a few fixes for 5.11.
If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.
This is an extraordinary way to destroy everything. Enjoy!
Comments and questions are, as always, welcome.
--D
kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes-5.11
---
fs/xfs/libxfs/xfs_bmap.c | 5 +++++
fs/xfs/scrub/dir.c | 21 ++++++++++++++++++---
fs/xfs/scrub/parent.c | 10 +++++-----
3 files changed, 28 insertions(+), 8 deletions(-)
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/3] xfs: detect overflows in bmbt records 2020-12-04 1:12 [PATCH 0/3] xfs: random fixes for 5.11 Darrick J. Wong @ 2020-12-04 1:12 ` Darrick J. Wong 2020-12-04 10:09 ` Christoph Hellwig 2020-12-04 1:12 ` [PATCH 2/3] xfs: fix parent pointer scrubber bailing out on unallocated inodes Darrick J. Wong 2020-12-04 1:13 ` [PATCH 3/3] xfs: scrub should mark a directory corrupt if any entries cannot be iget'd Darrick J. Wong 2 siblings, 1 reply; 7+ messages in thread From: Darrick J. Wong @ 2020-12-04 1:12 UTC (permalink / raw) To: darrick.wong; +Cc: linux-xfs From: Darrick J. Wong <darrick.wong@oracle.com> Detect file block mappings with a blockcount that's either so large that integer overflows occur or are zero, because neither are valid in the filesystem. Worse yet, attempting directory modifications causes the iext code to trip over the bmbt key handling and takes the filesystem down. We can fix most of this by preventing the bad metadata from entering the incore structures in the first place. Found by setting blockcount=0 in a directory data fork mapping and watching the fireworks. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/libxfs/xfs_bmap.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index d9a692484eae..de9c27ef68d8 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -6229,6 +6229,11 @@ xfs_bmap_validate_extent( xfs_fsblock_t endfsb; bool isrt; + if (irec->br_startblock + irec->br_blockcount <= irec->br_startblock) + return __this_address; + if (irec->br_startoff + irec->br_blockcount <= irec->br_startoff) + return __this_address; + isrt = XFS_IS_REALTIME_INODE(ip); endfsb = irec->br_startblock + irec->br_blockcount - 1; if (isrt && whichfork == XFS_DATA_FORK) { ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] xfs: detect overflows in bmbt records 2020-12-04 1:12 ` [PATCH 1/3] xfs: detect overflows in bmbt records Darrick J. Wong @ 2020-12-04 10:09 ` Christoph Hellwig 0 siblings, 0 replies; 7+ messages in thread From: Christoph Hellwig @ 2020-12-04 10:09 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] xfs: fix parent pointer scrubber bailing out on unallocated inodes 2020-12-04 1:12 [PATCH 0/3] xfs: random fixes for 5.11 Darrick J. Wong 2020-12-04 1:12 ` [PATCH 1/3] xfs: detect overflows in bmbt records Darrick J. Wong @ 2020-12-04 1:12 ` Darrick J. Wong 2020-12-04 10:09 ` Christoph Hellwig 2020-12-04 1:13 ` [PATCH 3/3] xfs: scrub should mark a directory corrupt if any entries cannot be iget'd Darrick J. Wong 2 siblings, 1 reply; 7+ messages in thread From: Darrick J. Wong @ 2020-12-04 1:12 UTC (permalink / raw) To: darrick.wong; +Cc: linux-xfs From: Darrick J. Wong <darrick.wong@oracle.com> xfs_iget can return -ENOENT for a file that the inobt thinks is allocated but has zeroed mode. This currently causes scrub to exit with an operational error instead of flagging this as a corruption. The end result is that scrub mistakenly reports the ENOENT to the user instead of "directory parent pointer corrupt" like we do for EINVAL. Fixes: 5927268f5a04 ("xfs: flag inode corruption if parent ptr doesn't get us a real inode") Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/scrub/parent.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c index 855aa8bcab64..66c35f6dfc24 100644 --- a/fs/xfs/scrub/parent.c +++ b/fs/xfs/scrub/parent.c @@ -164,13 +164,13 @@ xchk_parent_validate( * can't use DONTCACHE here because DONTCACHE inodes can trigger * immediate inactive cleanup of the inode. * - * If _iget returns -EINVAL then the parent inode number is garbage - * and the directory is corrupt. If the _iget returns -EFSCORRUPTED - * or -EFSBADCRC then the parent is corrupt which is a cross - * referencing error. Any other error is an operational error. + * If _iget returns -EINVAL or -ENOENT then the parent inode number is + * garbage and the directory is corrupt. If the _iget returns + * -EFSCORRUPTED or -EFSBADCRC then the parent is corrupt which is a + * cross referencing error. Any other error is an operational error. */ error = xfs_iget(mp, sc->tp, dnum, XFS_IGET_UNTRUSTED, 0, &dp); - if (error == -EINVAL) { + if (error == -EINVAL || error == -ENOENT) { error = -EFSCORRUPTED; xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error); goto out; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] xfs: fix parent pointer scrubber bailing out on unallocated inodes 2020-12-04 1:12 ` [PATCH 2/3] xfs: fix parent pointer scrubber bailing out on unallocated inodes Darrick J. Wong @ 2020-12-04 10:09 ` Christoph Hellwig 0 siblings, 0 replies; 7+ messages in thread From: Christoph Hellwig @ 2020-12-04 10:09 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs On Thu, Dec 03, 2020 at 05:12:56PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > xfs_iget can return -ENOENT for a file that the inobt thinks is > allocated but has zeroed mode. This currently causes scrub to exit > with an operational error instead of flagging this as a corruption. The > end result is that scrub mistakenly reports the ENOENT to the user > instead of "directory parent pointer corrupt" like we do for EINVAL. Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] xfs: scrub should mark a directory corrupt if any entries cannot be iget'd 2020-12-04 1:12 [PATCH 0/3] xfs: random fixes for 5.11 Darrick J. Wong 2020-12-04 1:12 ` [PATCH 1/3] xfs: detect overflows in bmbt records Darrick J. Wong 2020-12-04 1:12 ` [PATCH 2/3] xfs: fix parent pointer scrubber bailing out on unallocated inodes Darrick J. Wong @ 2020-12-04 1:13 ` Darrick J. Wong 2020-12-04 10:11 ` Christoph Hellwig 2 siblings, 1 reply; 7+ messages in thread From: Darrick J. Wong @ 2020-12-04 1:13 UTC (permalink / raw) To: darrick.wong; +Cc: linux-xfs From: Darrick J. Wong <darrick.wong@oracle.com> It's possible that xfs_iget can return EINVAL for inodes that the inobt thinks are free, or ENOENT for inodes that look free. If this is the case, mark the directory corrupt immediately when we check ftype. Note that we already check the ftype of the '.' and '..' entries, so we can skip the iget part since we already know the inode type for '.' and we have a separate parent pointer scrubber for '..'. Fixes: a5c46e5e8912 ("xfs: scrub directory metadata") Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/xfs/scrub/dir.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c index b045e95c2ea7..178b3455a170 100644 --- a/fs/xfs/scrub/dir.c +++ b/fs/xfs/scrub/dir.c @@ -66,8 +66,18 @@ xchk_dir_check_ftype( * eofblocks cleanup (which allocates what would be a nested * transaction), we can't use DONTCACHE here because DONTCACHE * inodes can trigger immediate inactive cleanup of the inode. + * + * If _iget returns -EINVAL or -ENOENT then the child inode number is + * garbage and the directory is corrupt. If the _iget returns + * -EFSCORRUPTED or -EFSBADCRC then the child is corrupt which is a + * cross referencing error. Any other error is an operational error. */ error = xfs_iget(mp, sdc->sc->tp, inum, 0, 0, &ip); + if (error == -EINVAL || error == -ENOENT) { + error = -EFSCORRUPTED; + xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, 0, &error); + goto out; + } if (!xchk_fblock_xref_process_error(sdc->sc, XFS_DATA_FORK, offset, &error)) goto out; @@ -105,6 +115,7 @@ xchk_dir_actor( struct xfs_name xname; xfs_ino_t lookup_ino; xfs_dablk_t offset; + bool checked_ftype = false; int error = 0; sdc = container_of(dir_iter, struct xchk_dir_ctx, dir_iter); @@ -133,6 +144,7 @@ xchk_dir_actor( if (xfs_sb_version_hasftype(&mp->m_sb) && type != DT_DIR) xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset); + checked_ftype = true; if (ino != ip->i_ino) xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset); @@ -144,6 +156,7 @@ xchk_dir_actor( if (xfs_sb_version_hasftype(&mp->m_sb) && type != DT_DIR) xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset); + checked_ftype = true; if (ip->i_ino == mp->m_sb.sb_rootino && ino != ip->i_ino) xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset); @@ -167,9 +180,11 @@ xchk_dir_actor( } /* Verify the file type. This function absorbs error codes. */ - error = xchk_dir_check_ftype(sdc, offset, lookup_ino, type); - if (error) - goto out; + if (!checked_ftype) { + error = xchk_dir_check_ftype(sdc, offset, lookup_ino, type); + if (error) + goto out; + } out: /* * A negative error code returned here is supposed to cause the ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] xfs: scrub should mark a directory corrupt if any entries cannot be iget'd 2020-12-04 1:13 ` [PATCH 3/3] xfs: scrub should mark a directory corrupt if any entries cannot be iget'd Darrick J. Wong @ 2020-12-04 10:11 ` Christoph Hellwig 0 siblings, 0 replies; 7+ messages in thread From: Christoph Hellwig @ 2020-12-04 10:11 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs On Thu, Dec 03, 2020 at 05:13:02PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > It's possible that xfs_iget can return EINVAL for inodes that the inobt > thinks are free, or ENOENT for inodes that look free. If this is the > case, mark the directory corrupt immediately when we check ftype. Note > that we already check the ftype of the '.' and '..' entries, so we > can skip the iget part since we already know the inode type for '.' and > we have a separate parent pointer scrubber for '..'. Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> But I wonder if we need the EINVAL vs ENOENT distinction to start with or if we could return a single coherent error code from iget. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-04 10:11 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-04 1:12 [PATCH 0/3] xfs: random fixes for 5.11 Darrick J. Wong 2020-12-04 1:12 ` [PATCH 1/3] xfs: detect overflows in bmbt records Darrick J. Wong 2020-12-04 10:09 ` Christoph Hellwig 2020-12-04 1:12 ` [PATCH 2/3] xfs: fix parent pointer scrubber bailing out on unallocated inodes Darrick J. Wong 2020-12-04 10:09 ` Christoph Hellwig 2020-12-04 1:13 ` [PATCH 3/3] xfs: scrub should mark a directory corrupt if any entries cannot be iget'd Darrick J. Wong 2020-12-04 10:11 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).