Commit 115ea07b authored by Darrick J. Wong's avatar Darrick J. Wong Committed by Carlos Maiolino
Browse files

xfs: don't report half-built inodes to fserror 



Sam Sun apparently found a syzbot way to fuzz a filesystem such that
xfs_iget_cache_miss would free the inode before the fserror code could
catch up.  Frustratingly he doesn't use the syzbot dashboard so there's
no C reproducer and not even a full error report, so I'm guessing that:

Inodes that are being constructed or torn down inside XFS are not
visible to the VFS.  They should never be reported to fserror.
Also, any inode that has been freshly allocated in _cache_miss should be
marked INEW immediately because, well, it's an incompletely constructed
inode that isn't yet visible to the VFS.

Reported-by: default avatarSam Sun <samsun1006219@gmail.com>
Fixes: 5eb4cb18 ("xfs: convey metadata health events to the health monitor")
Signed-off-by: default avatar"Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Reviewed-by: default avatarCarlos Maiolino <cmaiolino@redhat.com>
Signed-off-by: default avatarCarlos Maiolino <cem@kernel.org>
parent 75690e5f

fs/xfs/xfs_health.c

+6 −2
Original line number Diff line number Diff line
@@ -316,8 +316,12 @@ xfs_rgno_mark_sick( 


static inline void xfs_inode_report_fserror(struct xfs_inode *ip)

{

	/* Report metadata inodes as general filesystem corruption */

	if (xfs_is_internal_inode(ip)) {

	/*

	 * Do not report inodes being constructed or freed, or metadata inodes,

	 * to fsnotify.

	 */

	if (xfs_iflags_test(ip, XFS_INEW | XFS_IRECLAIM) ||

	    xfs_is_internal_inode(ip)) {

		fserror_report_metadata(ip->i_mount->m_super, -EFSCORRUPTED,

				GFP_NOFS);

		return;

fs/xfs/xfs_icache.c

+8 −1
Original line number Diff line number Diff line
@@ -639,6 +639,14 @@ xfs_iget_cache_miss( 
	if (!ip)

		return -ENOMEM;



	/*

	 * Set XFS_INEW as early as possible so that the health code won't pass

	 * the inode to the fserror code if the ondisk inode cannot be loaded.

	 * We're going to free the xfs_inode immediately if that happens, which

	 * would lead to UAF problems.

	 */

	xfs_iflags_set(ip, XFS_INEW);



	error = xfs_imap(pag, tp, ip->i_ino, &ip->i_imap, flags);

	if (error)

		goto out_destroy;
@@ -716,7 +724,6 @@ xfs_iget_cache_miss( 
	ip->i_udquot = NULL;

	ip->i_gdquot = NULL;

	ip->i_pdquot = NULL;

	xfs_iflags_set(ip, XFS_INEW);



	/* insert the new inode */

	spin_lock(&pag->pag_ici_lock);