Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5e13ad94 authored by Darrick J. Wong's avatar Darrick J. Wong Committed by Greg Kroah-Hartman
Browse files

xfs: fix memory corruption during remote attr value buffer invalidation



commit e8db2aafcedb7d88320ab83f1000f1606b26d4d7 upstream.

[Replaced XFS_IS_CORRUPT() calls with ASSERT() for 5.4.y backport]

While running generic/103, I observed what looks like memory corruption
and (with slub debugging turned on) a slub redzone warning on i386 when
inactivating an inode with a 64k remote attr value.

On a v5 filesystem, maximally sized remote attr values require one block
more than 64k worth of space to hold both the remote attribute value
header (64 bytes).  On a 4k block filesystem this results in a 68k
buffer; on a 64k block filesystem, this would be a 128k buffer.  Note
that even though we'll never use more than 65,600 bytes of this buffer,
XFS_MAX_BLOCKSIZE is 64k.

This is a problem because the definition of struct xfs_buf_log_format
allows for XFS_MAX_BLOCKSIZE worth of dirty bitmap (64k).  On i386 when we
invalidate a remote attribute, xfs_trans_binval zeroes all 68k worth of
the dirty map, writing right off the end of the log item and corrupting
memory.  We've gotten away with this on x86_64 for years because the
compiler inserts a u32 padding on the end of struct xfs_buf_log_format.

Fortunately for us, remote attribute values are written to disk with
xfs_bwrite(), which is to say that they are not logged.  Fix the problem
by removing all places where we could end up creating a buffer log item
for a remote attribute value and leave a note explaining why.  Next,
replace the open-coded buffer invalidation with a call to the helper we
created in the previous patch that does better checking for bad metadata
before marking the buffer stale.

Signed-off-by: default avatarDarrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Acked-by: default avatarDarrick J. Wong <djwong@kernel.org>
Signed-off-by: default avatarChandan Babu R <chandan.babu@oracle.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 821e0951
Loading
Loading
Loading
Loading
+31 −6
Original line number Original line Diff line number Diff line
@@ -24,6 +24,23 @@


#define ATTR_RMTVALUE_MAPSIZE	1	/* # of map entries at once */
#define ATTR_RMTVALUE_MAPSIZE	1	/* # of map entries at once */


/*
 * Remote Attribute Values
 * =======================
 *
 * Remote extended attribute values are conceptually simple -- they're written
 * to data blocks mapped by an inode's attribute fork, and they have an upper
 * size limit of 64k.  Setting a value does not involve the XFS log.
 *
 * However, on a v5 filesystem, maximally sized remote attr values require one
 * block more than 64k worth of space to hold both the remote attribute value
 * header (64 bytes).  On a 4k block filesystem this results in a 68k buffer;
 * on a 64k block filesystem, this would be a 128k buffer.  Note that the log
 * format can only handle a dirty buffer of XFS_MAX_BLOCKSIZE length (64k).
 * Therefore, we /must/ ensure that remote attribute value buffers never touch
 * the logging system and therefore never have a log item.
 */

/*
/*
 * Each contiguous block has a header, so it is not just a simple attribute
 * Each contiguous block has a header, so it is not just a simple attribute
 * length to FSB conversion.
 * length to FSB conversion.
@@ -400,17 +417,25 @@ xfs_attr_rmtval_get(
			       (map[i].br_startblock != HOLESTARTBLOCK));
			       (map[i].br_startblock != HOLESTARTBLOCK));
			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
			dblkno = XFS_FSB_TO_DADDR(mp, map[i].br_startblock);
			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
			dblkcnt = XFS_FSB_TO_BB(mp, map[i].br_blockcount);
			error = xfs_trans_read_buf(mp, args->trans,
			bp = xfs_buf_read(mp->m_ddev_targp, dblkno, dblkcnt, 0,
						   mp->m_ddev_targp,
						   dblkno, dblkcnt, 0, &bp,
					&xfs_attr3_rmt_buf_ops);
					&xfs_attr3_rmt_buf_ops);
			if (error)
			if (!bp)
				return -ENOMEM;
			error = bp->b_error;
			if (error) {
				xfs_buf_ioerror_alert(bp, __func__);
				xfs_buf_relse(bp);

				/* bad CRC means corrupted metadata */
				if (error == -EFSBADCRC)
					error = -EFSCORRUPTED;
				return error;
				return error;
			}


			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
			error = xfs_attr_rmtval_copyout(mp, bp, args->dp->i_ino,
							&offset, &valuelen,
							&offset, &valuelen,
							&dst);
							&dst);
			xfs_trans_brelse(args->trans, bp);
			xfs_buf_relse(bp);
			if (error)
			if (error)
				return error;
				return error;


+13 −34
Original line number Original line Diff line number Diff line
@@ -25,22 +25,20 @@
#include "xfs_error.h"
#include "xfs_error.h"


/*
/*
 * Look at all the extents for this logical region,
 * Invalidate any incore buffers associated with this remote attribute value
 * invalidate any buffers that are incore/in transactions.
 * extent.   We never log remote attribute value buffers, which means that they
 * won't be attached to a transaction and are therefore safe to mark stale.
 * The actual bunmapi will be taken care of later.
 */
 */
STATIC int
STATIC int
xfs_attr3_leaf_freextent(
xfs_attr3_rmt_stale(
	struct xfs_trans	**trans,
	struct xfs_inode	*dp,
	struct xfs_inode	*dp,
	xfs_dablk_t		blkno,
	xfs_dablk_t		blkno,
	int			blkcnt)
	int			blkcnt)
{
{
	struct xfs_bmbt_irec	map;
	struct xfs_bmbt_irec	map;
	struct xfs_buf		*bp;
	xfs_dablk_t		tblkno;
	xfs_dablk_t		tblkno;
	xfs_daddr_t		dblkno;
	int			tblkcnt;
	int			tblkcnt;
	int			dblkcnt;
	int			nmap;
	int			nmap;
	int			error;
	int			error;


@@ -57,35 +55,18 @@ xfs_attr3_leaf_freextent(
		nmap = 1;
		nmap = 1;
		error = xfs_bmapi_read(dp, (xfs_fileoff_t)tblkno, tblkcnt,
		error = xfs_bmapi_read(dp, (xfs_fileoff_t)tblkno, tblkcnt,
				       &map, &nmap, XFS_BMAPI_ATTRFORK);
				       &map, &nmap, XFS_BMAPI_ATTRFORK);
		if (error) {
		if (error)
			return error;
			return error;
		}
		ASSERT(nmap == 1);
		ASSERT(nmap == 1);
		ASSERT(map.br_startblock != DELAYSTARTBLOCK);


		/*
		/*
		 * If it's a hole, these are already unmapped
		 * Mark any incore buffers for the remote value as stale.  We
		 * so there's nothing to invalidate.
		 * never log remote attr value buffers, so the buffer should be
		 * easy to kill.
		 */
		 */
		if (map.br_startblock != HOLESTARTBLOCK) {
		error = xfs_attr_rmtval_stale(dp, &map, 0);

			dblkno = XFS_FSB_TO_DADDR(dp->i_mount,
						  map.br_startblock);
			dblkcnt = XFS_FSB_TO_BB(dp->i_mount,
						map.br_blockcount);
			bp = xfs_trans_get_buf(*trans,
					dp->i_mount->m_ddev_targp,
					dblkno, dblkcnt, 0);
			if (!bp)
				return -ENOMEM;
			xfs_trans_binval(*trans, bp);
			/*
			 * Roll to next transaction.
			 */
			error = xfs_trans_roll_inode(trans, dp);
		if (error)
		if (error)
			return error;
			return error;
		}


		tblkno += map.br_blockcount;
		tblkno += map.br_blockcount;
		tblkcnt -= map.br_blockcount;
		tblkcnt -= map.br_blockcount;
@@ -174,9 +155,7 @@ xfs_attr3_leaf_inactive(
	 */
	 */
	error = 0;
	error = 0;
	for (lp = list, i = 0; i < count; i++, lp++) {
	for (lp = list, i = 0; i < count; i++, lp++) {
		tmp = xfs_attr3_leaf_freextent(trans, dp,
		tmp = xfs_attr3_rmt_stale(dp, lp->valueblk, lp->valuelen);
				lp->valueblk, lp->valuelen);

		if (error == 0)
		if (error == 0)
			error = tmp;	/* save only the 1st errno */
			error = tmp;	/* save only the 1st errno */
	}
	}