Skip to contentSkip to conversation

xfs: scrub extended attribute leaf space

PATCH Superseded Darrick J. Wong 2 participants 5 messages Oct 31, 2017 – Nov 1, 2017 XFS filesystem

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xfs: scrub extended attribute leaf space

From: Darrick J. Wong <[email protected]>

As we walk the attribute btree, explicitly check the structure of the attribute leaves to make sure the pointers make sense and the freemap is sensible.

---
 fs/xfs/scrub/attr.c    |  233 ++++++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/scrub/dabtree.c |    4 +
 fs/xfs/scrub/dabtree.h |    3 -
 fs/xfs/scrub/dir.c     |    2 
 4 files changed, 218 insertions(+), 24 deletions(-)
fs/xfs/scrub/attr.c +212 -21
index a70cd9b..5d624ca 100644
@@ -50,8 +50,17 @@ xfs_scrub_setup_xattr(
 	struct xfs_scrub_context	*sc,
 	struct xfs_inode		*ip)
 {
-	/* Allocate the buffer without the inode lock held. */
-	sc->buf = kmem_zalloc_large(XATTR_SIZE_MAX, KM_SLEEP);
+	size_t				sz;
+
+	/*
+	 * Allocate the buffer without the inode lock held.  We need enough
+	 * space to read every xattr value in the file and enough space to
+	 * hold three copies of the xattr free space bitmap.  (Not both at
+	 * the same time.)
+	 */
+	sz = max_t(size_t, XATTR_SIZE_MAX, 3 * sizeof(long) *
+			BITS_TO_LONGS(sc->mp->m_attr_geo->blksize));
+	sc->buf = kmem_zalloc_large(sz, KM_SLEEP);
 	if (!sc->buf)
 		return -ENOMEM;
 
@@ -122,6 +131,197 @@ xfs_scrub_xattr_listent(
 	return;
 }
 
+/*
+ * Mark a range [start, start+len) in this map.  Returns true if the
+ * region was free, and false if there's a conflict or a problem.
+ *
+ * Within a char, the lowest bit of the char represents the byte with
+ * the smallest address
+ */
+STATIC bool
+xfs_scrub_xattr_set_map(
+	struct xfs_scrub_context	*sc,
+	unsigned long			*map,
+	unsigned int			start,
+	unsigned int			len)
+{
+	unsigned int			mapsize = sc->mp->m_attr_geo->blksize;
+	bool				ret = true;
+
+	if (start >= mapsize)
+		return false;
+	if (start + len > mapsize) {
+		len = mapsize - start;
+		ret = false;
+	}
+
+	ret &= find_next_bit(map, mapsize, start) >= (start + len);
+	bitmap_set(map, start, len);
+
+	return ret;
+}
+
+/*
+ * Check the leaf freemap from the usage bitmap.  Returns false if the
+ * attr freemap has problems or points to used space.
+ */
+STATIC bool
+xfs_scrub_xattr_check_freemap(
+	struct xfs_scrub_context	*sc,
+	unsigned long			*map,
+	struct xfs_attr3_icleaf_hdr	*leafhdr)
+{
+	unsigned long			*freemap;
+	unsigned long			*dstmap;
+	unsigned int			mapsize = sc->mp->m_attr_geo->blksize;
+	int				i;
+
+	/* Construct bitmap of freemap contents. */
+	freemap = (unsigned long *)sc->buf + BITS_TO_LONGS(mapsize);
+	bitmap_zero(freemap, mapsize);
+	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
+		if (!xfs_scrub_xattr_set_map(sc, freemap,
+				leafhdr->freemap[i].base,
+				leafhdr->freemap[i].size))
+			return false;
+	}
+
+	/* Look for bits that are set in freemap and are marked in use. */
+	dstmap = freemap + BITS_TO_LONGS(mapsize);
+	return bitmap_and(dstmap, freemap, map, mapsize) == 0;
+}
+
+/* Scrub an attribute leaf. */
+STATIC int
+xfs_scrub_xattr_block(
+	struct xfs_scrub_da_btree	*ds,
+	int				level)
+{
+	struct xfs_attr3_icleaf_hdr	leafhdr;
+	struct xfs_mount		*mp = ds->state->mp;
+	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
+	struct xfs_buf			*bp = blk->bp;
+	xfs_dablk_t			*last_checked = ds->private;
+	struct xfs_attr_leafblock	*leaf = bp->b_addr;
+	struct xfs_attr_leaf_entry	*ent;
+	struct xfs_attr_leaf_entry	*entries;
+	struct xfs_attr_leaf_name_local	*lentry;
+	struct xfs_attr_leaf_name_remote	*rentry;
+	unsigned long			*usedmap = ds->sc->buf;
+	char				*name_end;
+	char				*buf_end;
+	__u32				last_hashval = 0;
+	unsigned int			usedbytes = 0;
+	unsigned int			nameidx;
+	unsigned int			hdrsize;
+	unsigned int			namesize;
+	int				i;
+
+	if (*last_checked == blk->blkno)
+		return 0;
+	*last_checked = blk->blkno;
+	bitmap_zero(usedmap, mp->m_attr_geo->blksize);
+
+	/* Check all the padding. */
+	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb)) {
+		struct xfs_attr3_leafblock	*leaf = bp->b_addr;
+
+		if (leaf->hdr.pad1 != 0 ||
+		    leaf->hdr.pad2 != cpu_to_be32(0) ||
+		    leaf->hdr.info.hdr.pad != cpu_to_be16(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+	} else {
+		if (leaf->hdr.pad1 != 0 ||
+		    leaf->hdr.info.pad != cpu_to_be16(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+	}
+
+	/* Check the leaf header */
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
+
+	if (leafhdr.usedbytes > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (leafhdr.firstused > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	hdrsize = xfs_attr3_leaf_hdr_size(leaf);
+	if (leafhdr.firstused < hdrsize)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, 0, hdrsize)) {
+		xfs_scrub_da_set_corrupt(ds, level);
+		goto out;
+	}
+
+	entries = xfs_attr3_leaf_entryp(leaf);
+	if ((char *)&entries[leafhdr.count] > (char *)leaf + leafhdr.firstused)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	/* Check the entries' relations to each other */
+	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
+	for (i = 0, ent = entries; i < leafhdr.count; ent++, i++) {
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap,
+				(char *)ent - (char *)leaf,
+				sizeof(xfs_attr_leaf_entry_t))) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+
+		if (ent->pad2 != cpu_to_be32(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+
+		if (be32_to_cpu(ent->hashval) < last_hashval)
+			xfs_scrub_da_set_corrupt(ds, level);
+		last_hashval = be32_to_cpu(ent->hashval);
+
+		nameidx = be16_to_cpu(ent->nameidx);
+		if (nameidx < hdrsize ||
+		    nameidx < leafhdr.firstused ||
+		    nameidx >= mp->m_attr_geo->blksize) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+
+		if (ent->flags & XFS_ATTR_LOCAL) {
+			lentry = xfs_attr3_leaf_name_local(leaf, i);
+			if (lentry->namelen <= 0)
+				xfs_scrub_da_set_corrupt(ds, level);
+			name_end = (char *)lentry->nameval + lentry->namelen +
+					be16_to_cpu(lentry->valuelen);
+			if (name_end > buf_end)
+				xfs_scrub_da_set_corrupt(ds, level);
+			namesize = xfs_attr_leaf_entsize_local(
+					lentry->namelen,
+					be16_to_cpu(lentry->valuelen));
+		} else {
+			rentry = xfs_attr3_leaf_name_remote(leaf, i);
+			if (rentry->namelen <= 0)
+				xfs_scrub_da_set_corrupt(ds, level);
+			name_end = (char *)rentry->name + rentry->namelen;
+			if (name_end > buf_end)
+				xfs_scrub_da_set_corrupt(ds, level);
+			namesize = xfs_attr_leaf_entsize_remote(
+						rentry->namelen);
+			if (rentry->valueblk == cpu_to_be32(0))
+				xfs_scrub_da_set_corrupt(ds, level);
+		}
+		usedbytes += namesize;
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, nameidx,
+				namesize)) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+	}
+
+	if (!xfs_scrub_xattr_check_freemap(ds->sc, usedmap, &leafhdr))
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (leafhdr.usedbytes != usedbytes)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+out:
+	return 0;
+}
+
 /* Scrub a attribute btree record. */
 STATIC int
 xfs_scrub_xattr_rec(
@@ -144,6 +344,13 @@ xfs_scrub_xattr_rec(
 
 	blk = &ds->state->path.blk[level];
 
+	/* Check the whole block, if necessary. */
+	error = xfs_scrub_xattr_block(ds, level);
+	if (error)
+		goto out;
+	if (ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+		goto out;
+
 	/* Check the hash of the entry. */
 	error = xfs_scrub_da_btree_hash(ds, level, &ent->hashval);
 	if (error)
@@ -158,24 +365,6 @@ xfs_scrub_xattr_rec(
 		goto out;
 	}
 
-	/* Check all the padding. */
-	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb)) {
-		struct xfs_attr3_leafblock	*leaf = bp->b_addr;
-
-		if (leaf->hdr.pad1 != 0 ||
-		    leaf->hdr.pad2 != cpu_to_be32(0) ||
-		    leaf->hdr.info.hdr.pad != cpu_to_be16(0))
-			xfs_scrub_da_set_corrupt(ds, level);
-	} else {
-		struct xfs_attr_leafblock	*leaf = bp->b_addr;
-
-		if (leaf->hdr.pad1 != 0 ||
-		    leaf->hdr.info.pad != cpu_to_be16(0))
-			xfs_scrub_da_set_corrupt(ds, level);
-	}
-	if (ent->pad2 != 0)
-		xfs_scrub_da_set_corrupt(ds, level);
-
 	/* Retrieve the entry and check it. */
 	hash = be32_to_cpu(ent->hashval);
 	badflags = ~(XFS_ATTR_LOCAL | XFS_ATTR_ROOT | XFS_ATTR_SECURE |
@@ -213,6 +402,7 @@ xfs_scrub_xattr(
 {
 	struct xfs_scrub_xattr		sx = { 0 };
 	struct attrlist_cursor_kern	cursor = { 0 };
+	xfs_dablk_t			last_checked = -1U;
 	int				error = 0;
 
 	if (!xfs_inode_hasattr(sc->ip))
@@ -220,7 +410,8 @@ xfs_scrub_xattr(
 
 	memset(&sx, 0, sizeof(sx));
 	/* Check attribute tree structure */
-	error = xfs_scrub_da_btree(sc, XFS_ATTR_FORK, xfs_scrub_xattr_rec);
+	error = xfs_scrub_da_btree(sc, XFS_ATTR_FORK, xfs_scrub_xattr_rec,
+			&last_checked);
 	if (error)
 		goto out;
 
fs/xfs/scrub/dabtree.c +3 -1
index 4a93cf1..c21c528 100644
@@ -467,7 +467,8 @@ int
 xfs_scrub_da_btree(
 	struct xfs_scrub_context	*sc,
 	int				whichfork,
-	xfs_scrub_da_btree_rec_fn	scrub_fn)
+	xfs_scrub_da_btree_rec_fn	scrub_fn,
+	void				*private)
 {
 	struct xfs_scrub_da_btree	ds = {};
 	struct xfs_mount		*mp = sc->mp;
@@ -492,6 +493,7 @@ xfs_scrub_da_btree(
 	ds.state->args = &ds.dargs;
 	ds.state->mp = mp;
 	ds.sc = sc;
+	ds.private = private;
 	if (whichfork == XFS_ATTR_FORK) {
 		ds.dargs.geo = mp->m_attr_geo;
 		ds.lowest = 0;
fs/xfs/scrub/dabtree.h +2 -1
index 2a766de1f..d31468d 100644
@@ -28,6 +28,7 @@ struct xfs_scrub_da_btree {
 	int				maxrecs[XFS_DA_NODE_MAXDEPTH];
 	struct xfs_da_state		*state;
 	struct xfs_scrub_context	*sc;
+	void				*private;
 
 	/*
 	 * Lowest and highest directory block address in which we expect
@@ -53,6 +54,6 @@ void xfs_scrub_da_set_corrupt(struct xfs_scrub_da_btree *ds, int level);
 int xfs_scrub_da_btree_hash(struct xfs_scrub_da_btree *ds, int level,
 			    __be32 *hashp);
 int xfs_scrub_da_btree(struct xfs_scrub_context *sc, int whichfork,
-		       xfs_scrub_da_btree_rec_fn scrub_fn);
+		       xfs_scrub_da_btree_rec_fn scrub_fn, void *private);
 
 #endif /* __XFS_SCRUB_DABTREE_H__ */
fs/xfs/scrub/dir.c +1 -1
index 169fb10..c61362f 100644
@@ -770,7 +770,7 @@ xfs_scrub_directory(
 	}
 
 	/* Check directory tree structure */
-	error = xfs_scrub_da_btree(sc, XFS_DATA_FORK, xfs_scrub_dir_rec);
+	error = xfs_scrub_da_btree(sc, XFS_DATA_FORK, xfs_scrub_dir_rec, NULL);
 	if (error)
 		return error;
 
#1 lore raw
Quoted 19 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

As we walk the attribute btree, explicitly check the structure of the attribute leaves to make sure the pointers make sense and the freemap is sensible.

---
 fs/xfs/scrub/attr.c    |  233 ++++++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/scrub/dabtree.c |    4 +
 fs/xfs/scrub/dabtree.h |    3 -
 fs/xfs/scrub/dir.c     |    2 
 4 files changed, 218 insertions(+), 24 deletions(-)
fs/xfs/scrub/attr.c +0 -0
index a70cd9b..5d624ca 100644

fs/xfs/scrub/attr.c xfs_scrub_setup_xattr( · line 57

Quoted 10 lines
fs/xfs/scrub/attr.c +5 -2
@@ -50,8 +50,17 @@ xfs_scrub_setup_xattr(
 	struct xfs_scrub_context	*sc,
 	struct xfs_inode		*ip)
 {
-	/* Allocate the buffer without the inode lock held. */
-	sc->buf = kmem_zalloc_large(XATTR_SIZE_MAX, KM_SLEEP);
+	size_t				sz;
+
+	/*
+	 * Allocate the buffer without the inode lock held.  We need enough
+	 * space to read every xattr value in the file and enough space to
						       ^^^

nit: or

Quoted 28 lines
fs/xfs/scrub/attr.c +21 -0
+	 * hold three copies of the xattr free space bitmap.  (Not both at
+	 * the same time.)
+	 */
+	sz = max_t(size_t, XATTR_SIZE_MAX, 3 * sizeof(long) *
+			BITS_TO_LONGS(sc->mp->m_attr_geo->blksize));
+	sc->buf = kmem_zalloc_large(sz, KM_SLEEP);
 	if (!sc->buf)
 		return -ENOMEM;
 
@@ -122,6 +131,197 @@ xfs_scrub_xattr_listent(
 	return;
 }
 
+/*
+ * Mark a range [start, start+len) in this map.  Returns true if the
+ * region was free, and false if there's a conflict or a problem.
+ *
+ * Within a char, the lowest bit of the char represents the byte with
+ * the smallest address
+ */
+STATIC bool
+xfs_scrub_xattr_set_map(
+	struct xfs_scrub_context	*sc,
+	unsigned long			*map,
+	unsigned int			start,
+	unsigned int			len)
+{
+	unsigned int			mapsize = sc->mp->m_attr_geo->blksize;

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 158

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+	bool				ret = true;
+
+	if (start >= mapsize)
+		return false;
+	if (start + len > mapsize) {
+		len = mapsize - start;
+		ret = false;
+	}
+
+	ret &= find_next_bit(map, mapsize, start) >= (start + len);

That's a bit convoluted. It's a conditional boolean bit masking operation. AFAICT it clears ret if the next bit is < (start
+ len), but if ret is already false it can never be set. I think.

Much simpler to write:

	if (find_next_bit(map, mapsize, start) < start + len)
		ret = false;
Quoted 27 lines
fs/xfs/scrub/attr.c +27 -0
+/* Scrub an attribute leaf. */
+STATIC int
+xfs_scrub_xattr_block(
+	struct xfs_scrub_da_btree	*ds,
+	int				level)
+{
+	struct xfs_attr3_icleaf_hdr	leafhdr;
+	struct xfs_mount		*mp = ds->state->mp;
+	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
+	struct xfs_buf			*bp = blk->bp;
+	xfs_dablk_t			*last_checked = ds->private;
+	struct xfs_attr_leafblock	*leaf = bp->b_addr;
+	struct xfs_attr_leaf_entry	*ent;
+	struct xfs_attr_leaf_entry	*entries;
+	struct xfs_attr_leaf_name_local	*lentry;
+	struct xfs_attr_leaf_name_remote	*rentry;
+	unsigned long			*usedmap = ds->sc->buf;
+	char				*name_end;
+	char				*buf_end;
+	__u32				last_hashval = 0;
+	unsigned int			usedbytes = 0;
+	unsigned int			nameidx;
+	unsigned int			hdrsize;
+	unsigned int			namesize;
+	int				i;
+
+	if (*last_checked == blk->blkno)

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 230

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+		return 0;
+	*last_checked = blk->blkno;
+	bitmap_zero(usedmap, mp->m_attr_geo->blksize);
+
+	/* Check all the padding. */
+	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb)) {
+		struct xfs_attr3_leafblock	*leaf = bp->b_addr;
+
+		if (leaf->hdr.pad1 != 0 ||
+		    leaf->hdr.pad2 != cpu_to_be32(0) ||

cpu_to_be32(0) = 0.

So there's no need for endian conversion of 0.

Quoted 17 lines
fs/xfs/scrub/attr.c +17 -0
+		    leaf->hdr.info.hdr.pad != cpu_to_be16(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+	} else {
+		if (leaf->hdr.pad1 != 0 ||
+		    leaf->hdr.info.pad != cpu_to_be16(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+	}
+
+	/* Check the leaf header */
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
+
+	if (leafhdr.usedbytes > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (leafhdr.firstused > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	hdrsize = xfs_attr3_leaf_hdr_size(leaf);
+	if (leafhdr.firstused < hdrsize)

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 257

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, 0, hdrsize)) {
+		xfs_scrub_da_set_corrupt(ds, level);
+		goto out;
+	}
+
+	entries = xfs_attr3_leaf_entryp(leaf);
+	if ((char *)&entries[leafhdr.count] > (char *)leaf + leafhdr.firstused)
+		xfs_scrub_da_set_corrupt(ds, level);

All the casts are a bit ugly, but I guess we've got to check offsets somehow.

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 267

Quoted 9 lines
fs/xfs/scrub/attr.c +9 -0
+	/* Check the entries' relations to each other */
+	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
+	for (i = 0, ent = entries; i < leafhdr.count; ent++, i++) {
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap,
+				(char *)ent - (char *)leaf,
+				sizeof(xfs_attr_leaf_entry_t))) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}

The amount of indentation and broken lines inside this loop makes it reall hard to read. Can you factor it at all? ...

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 270

Quoted 3 lines
fs/xfs/scrub/attr.c +3 -0
+
+		if (ent->pad2 != cpu_to_be32(0))
+			xfs_scrub_da_set_corrupt(ds, level);

compare against 0

Quoted 9 lines
fs/xfs/scrub/attr.c +9 -0
+
+		if (be32_to_cpu(ent->hashval) < last_hashval)
+			xfs_scrub_da_set_corrupt(ds, level);
+		last_hashval = be32_to_cpu(ent->hashval);
+
+		nameidx = be16_to_cpu(ent->nameidx);
+		if (nameidx < hdrsize ||
+		    nameidx < leafhdr.firstused ||
+		    nameidx >= mp->m_attr_geo->blksize) {

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 289

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+
+		if (ent->flags & XFS_ATTR_LOCAL) {
+			lentry = xfs_attr3_leaf_name_local(leaf, i);
+			if (lentry->namelen <= 0)
+				xfs_scrub_da_set_corrupt(ds, level);
+			name_end = (char *)lentry->nameval + lentry->namelen +
+					be16_to_cpu(lentry->valuelen);

Isn't there padding that has to be taken into account here?

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 294

Quoted 5 lines
fs/xfs/scrub/attr.c +5 -0
+			if (name_end > buf_end)
+				xfs_scrub_da_set_corrupt(ds, level);
+			namesize = xfs_attr_leaf_entsize_local(
+					lentry->namelen,
+					be16_to_cpu(lentry->valuelen));

Because this pads the size of the entry...

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 299

Quoted 5 lines
fs/xfs/scrub/attr.c +5 -0
+		} else {
+			rentry = xfs_attr3_leaf_name_remote(leaf, i);
+			if (rentry->namelen <= 0)
+				xfs_scrub_da_set_corrupt(ds, level);
+			name_end = (char *)rentry->name + rentry->namelen;

Same here.

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 306

Quoted 7 lines
fs/xfs/scrub/attr.c +7 -0
+			if (name_end > buf_end)
+				xfs_scrub_da_set_corrupt(ds, level);
+			namesize = xfs_attr_leaf_entsize_remote(
+						rentry->namelen);
+			if (rentry->valueblk == cpu_to_be32(0))
+				xfs_scrub_da_set_corrupt(ds, level);
+		}

Maybe just factoring the local/remote attr name checks will clean this all up - this is the part that's really hard to read.

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 319

Quoted 13 lines
fs/xfs/scrub/attr.c +13 -0
+		usedbytes += namesize;
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, nameidx,
+				namesize)) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+	}
+
+	if (!xfs_scrub_xattr_check_freemap(ds->sc, usedmap, &leafhdr))
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (leafhdr.usedbytes != usedbytes)
+		xfs_scrub_da_set_corrupt(ds, level);

And so this validates all the padded entries....

Cheers,

Dave.

Signature · 2 lines
Dave Chinner
[email protected]
#2 lore raw
Quoted 31 lines · Dave Chinner
Quoted 19 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

As we walk the attribute btree, explicitly check the structure of the attribute leaves to make sure the pointers make sense and the freemap is sensible.

---
 fs/xfs/scrub/attr.c    |  233 ++++++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/scrub/dabtree.c |    4 +
 fs/xfs/scrub/dabtree.h |    3 -
 fs/xfs/scrub/dir.c     |    2 
 4 files changed, 218 insertions(+), 24 deletions(-)
fs/xfs/scrub/attr.c +0 -0
index a70cd9b..5d624ca 100644

fs/xfs/scrub/attr.c xfs_scrub_setup_xattr( · line 57

Quoted 10 lines
fs/xfs/scrub/attr.c +5 -2
@@ -50,8 +50,17 @@ xfs_scrub_setup_xattr(
 	struct xfs_scrub_context	*sc,
 	struct xfs_inode		*ip)
 {
-	/* Allocate the buffer without the inode lock held. */
-	sc->buf = kmem_zalloc_large(XATTR_SIZE_MAX, KM_SLEEP);
+	size_t				sz;
+
+	/*
+	 * Allocate the buffer without the inode lock held.  We need enough
+	 * space to read every xattr value in the file and enough space to
						       ^^^
Quoted 1 line

nit: or

Fixed.

Quoted 43 lines
Quoted 28 lines
fs/xfs/scrub/attr.c +21 -0
+	 * hold three copies of the xattr free space bitmap.  (Not both at
+	 * the same time.)
+	 */
+	sz = max_t(size_t, XATTR_SIZE_MAX, 3 * sizeof(long) *
+			BITS_TO_LONGS(sc->mp->m_attr_geo->blksize));
+	sc->buf = kmem_zalloc_large(sz, KM_SLEEP);
 	if (!sc->buf)
 		return -ENOMEM;
 
@@ -122,6 +131,197 @@ xfs_scrub_xattr_listent(
 	return;
 }
 
+/*
+ * Mark a range [start, start+len) in this map.  Returns true if the
+ * region was free, and false if there's a conflict or a problem.
+ *
+ * Within a char, the lowest bit of the char represents the byte with
+ * the smallest address
+ */
+STATIC bool
+xfs_scrub_xattr_set_map(
+	struct xfs_scrub_context	*sc,
+	unsigned long			*map,
+	unsigned int			start,
+	unsigned int			len)
+{
+	unsigned int			mapsize = sc->mp->m_attr_geo->blksize;

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 158

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+	bool				ret = true;
+
+	if (start >= mapsize)
+		return false;
+	if (start + len > mapsize) {
+		len = mapsize - start;
+		ret = false;
+	}
+
+	ret &= find_next_bit(map, mapsize, start) >= (start + len);

That's a bit convoluted. It's a conditional boolean bit masking operation. AFAICT it clears ret if the next bit is < (start
+ len), but if ret is already false it can never be set. I think.

Quoted 4 lines

Much simpler to write:

	if (find_next_bit(map, mapsize, start) < start + len)
		ret = false;

Fixed.

Quoted 40 lines
Quoted 27 lines
fs/xfs/scrub/attr.c +27 -0
+/* Scrub an attribute leaf. */
+STATIC int
+xfs_scrub_xattr_block(
+	struct xfs_scrub_da_btree	*ds,
+	int				level)
+{
+	struct xfs_attr3_icleaf_hdr	leafhdr;
+	struct xfs_mount		*mp = ds->state->mp;
+	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
+	struct xfs_buf			*bp = blk->bp;
+	xfs_dablk_t			*last_checked = ds->private;
+	struct xfs_attr_leafblock	*leaf = bp->b_addr;
+	struct xfs_attr_leaf_entry	*ent;
+	struct xfs_attr_leaf_entry	*entries;
+	struct xfs_attr_leaf_name_local	*lentry;
+	struct xfs_attr_leaf_name_remote	*rentry;
+	unsigned long			*usedmap = ds->sc->buf;
+	char				*name_end;
+	char				*buf_end;
+	__u32				last_hashval = 0;
+	unsigned int			usedbytes = 0;
+	unsigned int			nameidx;
+	unsigned int			hdrsize;
+	unsigned int			namesize;
+	int				i;
+
+	if (*last_checked == blk->blkno)

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 230

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+		return 0;
+	*last_checked = blk->blkno;
+	bitmap_zero(usedmap, mp->m_attr_geo->blksize);
+
+	/* Check all the padding. */
+	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb)) {
+		struct xfs_attr3_leafblock	*leaf = bp->b_addr;
+
+		if (leaf->hdr.pad1 != 0 ||
+		    leaf->hdr.pad2 != cpu_to_be32(0) ||

cpu_to_be32(0) = 0.

Quoted 1 line

So there's no need for endian conversion of 0.

Fixed.

Quoted 31 lines
Quoted 17 lines
fs/xfs/scrub/attr.c +17 -0
+		    leaf->hdr.info.hdr.pad != cpu_to_be16(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+	} else {
+		if (leaf->hdr.pad1 != 0 ||
+		    leaf->hdr.info.pad != cpu_to_be16(0))
+			xfs_scrub_da_set_corrupt(ds, level);
+	}
+
+	/* Check the leaf header */
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
+
+	if (leafhdr.usedbytes > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (leafhdr.firstused > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	hdrsize = xfs_attr3_leaf_hdr_size(leaf);
+	if (leafhdr.firstused < hdrsize)

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 257

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, 0, hdrsize)) {
+		xfs_scrub_da_set_corrupt(ds, level);
+		goto out;
+	}
+
+	entries = xfs_attr3_leaf_entryp(leaf);
+	if ((char *)&entries[leafhdr.count] > (char *)leaf + leafhdr.firstused)
+		xfs_scrub_da_set_corrupt(ds, level);

All the casts are a bit ugly, but I guess we've got to check offsets somehow.

Quoted 12 lines

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 267

Quoted 9 lines
fs/xfs/scrub/attr.c +9 -0
+	/* Check the entries' relations to each other */
+	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
+	for (i = 0, ent = entries; i < leafhdr.count; ent++, i++) {
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap,
+				(char *)ent - (char *)leaf,
+				sizeof(xfs_attr_leaf_entry_t))) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}

The amount of indentation and broken lines inside this loop makes it reall hard to read. Can you factor it at all? ...

Done.

Quoted 5 lines

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 270

Quoted 3 lines
fs/xfs/scrub/attr.c +3 -0
+
+		if (ent->pad2 != cpu_to_be32(0))
+			xfs_scrub_da_set_corrupt(ds, level);

compare against 0

Done.

Quoted 13 lines
Quoted 13 lines
fs/xfs/scrub/attr.c +13 -0
+
+		if (be32_to_cpu(ent->hashval) < last_hashval)
+			xfs_scrub_da_set_corrupt(ds, level);
+		last_hashval = be32_to_cpu(ent->hashval);
+
+		nameidx = be16_to_cpu(ent->nameidx);
+		if (nameidx < hdrsize ||
+		    nameidx < leafhdr.firstused ||
+		    nameidx >= mp->m_attr_geo->blksize) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+
Quoted 8 lines

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 289

Quoted 6 lines
fs/xfs/scrub/attr.c +6 -0
+		if (ent->flags & XFS_ATTR_LOCAL) {
+			lentry = xfs_attr3_leaf_name_local(leaf, i);
+			if (lentry->namelen <= 0)
+				xfs_scrub_da_set_corrupt(ds, level);
+			name_end = (char *)lentry->nameval + lentry->namelen +
+					be16_to_cpu(lentry->valuelen);

Isn't there padding that has to be taken into account here?

Yeah, I think there is. All this crap here can be simplified to:

	lentry = xfs_attr3_leaf_name_local(leaf, idx);
	namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
			be16_to_cpu(lentry->valuelen));
	name_end = (char *)lentry + namesize;
	if (lentry->namelen == 0)
		xfs_scrub_da_set_corrupt(ds, level);
	if (name_end > buf_end)
		xfs_scrub_da_set_corrupt(ds, level);
Quoted 17 lines

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 294

Quoted 6 lines
fs/xfs/scrub/attr.c +5 -0
+			if (name_end > buf_end)
+				xfs_scrub_da_set_corrupt(ds, level);
+			namesize = xfs_attr_leaf_entsize_local(
+					lentry->namelen,
+					be16_to_cpu(lentry->valuelen));

Because this pads the size of the entry...

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 299

Quoted 5 lines
fs/xfs/scrub/attr.c +5 -0
+		} else {
+			rentry = xfs_attr3_leaf_name_remote(leaf, i);
+			if (rentry->namelen <= 0)
+				xfs_scrub_da_set_corrupt(ds, level);
+			name_end = (char *)rentry->name + rentry->namelen;

Same here.

Quoted 10 lines

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 306

Quoted 7 lines
fs/xfs/scrub/attr.c +7 -0
+			if (name_end > buf_end)
+				xfs_scrub_da_set_corrupt(ds, level);
+			namesize = xfs_attr_leaf_entsize_remote(
+						rentry->namelen);
+			if (rentry->valueblk == cpu_to_be32(0))
+				xfs_scrub_da_set_corrupt(ds, level);
+		}

Maybe just factoring the local/remote attr name checks will clean this all up - this is the part that's really hard to read.

yeah.

Quoted 16 lines
Quoted 4 lines
fs/xfs/scrub/attr.c +3 -0
+		usedbytes += namesize;
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, nameidx,
+				namesize)) {

fs/xfs/scrub/attr.c xfs_scrub_xattr_listent( · line 319

Quoted 10 lines
fs/xfs/scrub/attr.c +10 -0
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+	}
+
+	if (!xfs_scrub_xattr_check_freemap(ds->sc, usedmap, &leafhdr))
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (leafhdr.usedbytes != usedbytes)
+		xfs_scrub_da_set_corrupt(ds, level);

And so this validates all the padded entries....

<nod>

--D

Quoted 10 lines

Cheers,

Dave.

Signature · 6 lines
Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw

PATCH v2 1/1 xfs: scrub extended attribute leaf space

In reply to Darrick J. Wong

From: Darrick J. Wong <[email protected]>

As we walk the attribute btree, explicitly check the structure of the attribute leaves to make sure the pointers make sense and the freemap is sensible.

---
 fs/xfs/scrub/attr.c    |  253 ++++++++++++++++++++++++++++++++++++++++++++----
 fs/xfs/scrub/dabtree.c |    4 +
 fs/xfs/scrub/dabtree.h |    3 -
 fs/xfs/scrub/dir.c     |    2 
 4 files changed, 238 insertions(+), 24 deletions(-)
fs/xfs/scrub/attr.c +232 -21
index a70cd9b..51a5533 100644
@@ -50,8 +50,17 @@ xfs_scrub_setup_xattr(
 	struct xfs_scrub_context	*sc,
 	struct xfs_inode		*ip)
 {
-	/* Allocate the buffer without the inode lock held. */
-	sc->buf = kmem_zalloc_large(XATTR_SIZE_MAX, KM_SLEEP);
+	size_t				sz;
+
+	/*
+	 * Allocate the buffer without the inode lock held.  We need enough
+	 * space to read every xattr value in the file or enough space to
+	 * hold three copies of the xattr free space bitmap.  (Not both at
+	 * the same time.)
+	 */
+	sz = max_t(size_t, XATTR_SIZE_MAX, 3 * sizeof(long) *
+			BITS_TO_LONGS(sc->mp->m_attr_geo->blksize));
+	sc->buf = kmem_zalloc_large(sz, KM_SLEEP);
 	if (!sc->buf)
 		return -ENOMEM;
 
@@ -122,6 +131,217 @@ xfs_scrub_xattr_listent(
 	return;
 }
 
+/*
+ * Mark a range [start, start+len) in this map.  Returns true if the
+ * region was free, and false if there's a conflict or a problem.
+ *
+ * Within a char, the lowest bit of the char represents the byte with
+ * the smallest address
+ */
+STATIC bool
+xfs_scrub_xattr_set_map(
+	struct xfs_scrub_context	*sc,
+	unsigned long			*map,
+	unsigned int			start,
+	unsigned int			len)
+{
+	unsigned int			mapsize = sc->mp->m_attr_geo->blksize;
+	bool				ret = true;
+
+	if (start >= mapsize)
+		return false;
+	if (start + len > mapsize) {
+		len = mapsize - start;
+		ret = false;
+	}
+
+	if (find_next_bit(map, mapsize, start) < start + len)
+		ret = false;
+	bitmap_set(map, start, len);
+
+	return ret;
+}
+
+/*
+ * Check the leaf freemap from the usage bitmap.  Returns false if the
+ * attr freemap has problems or points to used space.
+ */
+STATIC bool
+xfs_scrub_xattr_check_freemap(
+	struct xfs_scrub_context	*sc,
+	unsigned long			*map,
+	struct xfs_attr3_icleaf_hdr	*leafhdr)
+{
+	unsigned long			*freemap;
+	unsigned long			*dstmap;
+	unsigned int			mapsize = sc->mp->m_attr_geo->blksize;
+	int				i;
+
+	/* Construct bitmap of freemap contents. */
+	freemap = (unsigned long *)sc->buf + BITS_TO_LONGS(mapsize);
+	bitmap_zero(freemap, mapsize);
+	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
+		if (!xfs_scrub_xattr_set_map(sc, freemap,
+				leafhdr->freemap[i].base,
+				leafhdr->freemap[i].size))
+			return false;
+	}
+
+	/* Look for bits that are set in freemap and are marked in use. */
+	dstmap = freemap + BITS_TO_LONGS(mapsize);
+	return bitmap_and(dstmap, freemap, map, mapsize) == 0;
+}
+
+/*
+ * Check this leaf entry's relations to everything else.
+ * Returns the number of bytes used for the name/value data.
+ */
+STATIC void
+xfs_scrub_xattr_entry(
+	struct xfs_scrub_da_btree	*ds,
+	int				level,
+	char				*buf_end,
+	struct xfs_attr_leafblock	*leaf,
+	struct xfs_attr3_icleaf_hdr	*leafhdr,
+	unsigned long			*usedmap,
+	struct xfs_attr_leaf_entry	*ent,
+	int				idx,
+	unsigned int			*usedbytes,
+	__u32				*last_hashval)
+{
+	struct xfs_mount		*mp = ds->state->mp;
+	char				*name_end;
+	struct xfs_attr_leaf_name_local	*lentry;
+	struct xfs_attr_leaf_name_remote *rentry;
+	unsigned int			nameidx;
+	unsigned int			namesize;
+
+	if (ent->pad2 != 0)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	/* Hash values in order? */
+	if (be32_to_cpu(ent->hashval) < *last_hashval)
+		xfs_scrub_da_set_corrupt(ds, level);
+	*last_hashval = be32_to_cpu(ent->hashval);
+
+	nameidx = be16_to_cpu(ent->nameidx);
+	if (nameidx < leafhdr->firstused ||
+	    nameidx >= mp->m_attr_geo->blksize) {
+		xfs_scrub_da_set_corrupt(ds, level);
+		return;
+	}
+
+	/* Check the name information. */
+	if (ent->flags & XFS_ATTR_LOCAL) {
+		lentry = xfs_attr3_leaf_name_local(leaf, idx);
+		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
+				be16_to_cpu(lentry->valuelen));
+		name_end = (char *)lentry + namesize;
+		if (lentry->namelen == 0)
+			xfs_scrub_da_set_corrupt(ds, level);
+	} else {
+		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
+		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
+		name_end = (char *)rentry + namesize;
+		if (rentry->namelen == 0 || rentry->valueblk == 0)
+			xfs_scrub_da_set_corrupt(ds, level);
+	}
+	if (name_end > buf_end)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, nameidx, namesize))
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (!(ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+		*usedbytes += namesize;
+}
+
+/* Scrub an attribute leaf. */
+STATIC int
+xfs_scrub_xattr_block(
+	struct xfs_scrub_da_btree	*ds,
+	int				level)
+{
+	struct xfs_attr3_icleaf_hdr	leafhdr;
+	struct xfs_mount		*mp = ds->state->mp;
+	struct xfs_da_state_blk		*blk = &ds->state->path.blk[level];
+	struct xfs_buf			*bp = blk->bp;
+	xfs_dablk_t			*last_checked = ds->private;
+	struct xfs_attr_leafblock	*leaf = bp->b_addr;
+	struct xfs_attr_leaf_entry	*ent;
+	struct xfs_attr_leaf_entry	*entries;
+	unsigned long			*usedmap = ds->sc->buf;
+	char				*buf_end;
+	size_t				off;
+	__u32				last_hashval = 0;
+	unsigned int			usedbytes = 0;
+	unsigned int			hdrsize;
+	int				i;
+
+	if (*last_checked == blk->blkno)
+		return 0;
+	*last_checked = blk->blkno;
+	bitmap_zero(usedmap, mp->m_attr_geo->blksize);
+
+	/* Check all the padding. */
+	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb)) {
+		struct xfs_attr3_leafblock	*leaf = bp->b_addr;
+
+		if (leaf->hdr.pad1 != 0 || leaf->hdr.pad2 != 0 ||
+		    leaf->hdr.info.hdr.pad != 0)
+			xfs_scrub_da_set_corrupt(ds, level);
+	} else {
+		if (leaf->hdr.pad1 != 0 || leaf->hdr.info.pad != 0)
+			xfs_scrub_da_set_corrupt(ds, level);
+	}
+
+	/* Check the leaf header */
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
+	hdrsize = xfs_attr3_leaf_hdr_size(leaf);
+
+	if (leafhdr.usedbytes > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (leafhdr.firstused > mp->m_attr_geo->blksize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (leafhdr.firstused < hdrsize)
+		xfs_scrub_da_set_corrupt(ds, level);
+	if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, 0, hdrsize))
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+		goto out;
+
+	entries = xfs_attr3_leaf_entryp(leaf);
+	if ((char *)&entries[leafhdr.count] > (char *)leaf + leafhdr.firstused)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
+	for (i = 0, ent = entries; i < leafhdr.count; ent++, i++) {
+		/* Mark the leaf entry itself. */
+		off = (char *)ent - (char *)leaf;
+		if (!xfs_scrub_xattr_set_map(ds->sc, usedmap, off,
+				sizeof(xfs_attr_leaf_entry_t))) {
+			xfs_scrub_da_set_corrupt(ds, level);
+			goto out;
+		}
+
+		/* Check the entry and nameval. */
+		xfs_scrub_xattr_entry(ds, level, buf_end, leaf, &leafhdr,
+				usedmap, ent, i, &usedbytes, &last_hashval);
+
+		if (ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+			goto out;
+	}
+
+	if (!xfs_scrub_xattr_check_freemap(ds->sc, usedmap, &leafhdr))
+		xfs_scrub_da_set_corrupt(ds, level);
+
+	if (leafhdr.usedbytes != usedbytes)
+		xfs_scrub_da_set_corrupt(ds, level);
+
+out:
+	return 0;
+}
+
 /* Scrub a attribute btree record. */
 STATIC int
 xfs_scrub_xattr_rec(
@@ -144,6 +364,13 @@ xfs_scrub_xattr_rec(
 
 	blk = &ds->state->path.blk[level];
 
+	/* Check the whole block, if necessary. */
+	error = xfs_scrub_xattr_block(ds, level);
+	if (error)
+		goto out;
+	if (ds->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+		goto out;
+
 	/* Check the hash of the entry. */
 	error = xfs_scrub_da_btree_hash(ds, level, &ent->hashval);
 	if (error)
@@ -158,24 +385,6 @@ xfs_scrub_xattr_rec(
 		goto out;
 	}
 
-	/* Check all the padding. */
-	if (xfs_sb_version_hascrc(&ds->sc->mp->m_sb)) {
-		struct xfs_attr3_leafblock	*leaf = bp->b_addr;
-
-		if (leaf->hdr.pad1 != 0 ||
-		    leaf->hdr.pad2 != cpu_to_be32(0) ||
-		    leaf->hdr.info.hdr.pad != cpu_to_be16(0))
-			xfs_scrub_da_set_corrupt(ds, level);
-	} else {
-		struct xfs_attr_leafblock	*leaf = bp->b_addr;
-
-		if (leaf->hdr.pad1 != 0 ||
-		    leaf->hdr.info.pad != cpu_to_be16(0))
-			xfs_scrub_da_set_corrupt(ds, level);
-	}
-	if (ent->pad2 != 0)
-		xfs_scrub_da_set_corrupt(ds, level);
-
 	/* Retrieve the entry and check it. */
 	hash = be32_to_cpu(ent->hashval);
 	badflags = ~(XFS_ATTR_LOCAL | XFS_ATTR_ROOT | XFS_ATTR_SECURE |
@@ -213,6 +422,7 @@ xfs_scrub_xattr(
 {
 	struct xfs_scrub_xattr		sx = { 0 };
 	struct attrlist_cursor_kern	cursor = { 0 };
+	xfs_dablk_t			last_checked = -1U;
 	int				error = 0;
 
 	if (!xfs_inode_hasattr(sc->ip))
@@ -220,7 +430,8 @@ xfs_scrub_xattr(
 
 	memset(&sx, 0, sizeof(sx));
 	/* Check attribute tree structure */
-	error = xfs_scrub_da_btree(sc, XFS_ATTR_FORK, xfs_scrub_xattr_rec);
+	error = xfs_scrub_da_btree(sc, XFS_ATTR_FORK, xfs_scrub_xattr_rec,
+			&last_checked);
 	if (error)
 		goto out;
 
fs/xfs/scrub/dabtree.c +3 -1
index 4a93cf1..c21c528 100644
@@ -467,7 +467,8 @@ int
 xfs_scrub_da_btree(
 	struct xfs_scrub_context	*sc,
 	int				whichfork,
-	xfs_scrub_da_btree_rec_fn	scrub_fn)
+	xfs_scrub_da_btree_rec_fn	scrub_fn,
+	void				*private)
 {
 	struct xfs_scrub_da_btree	ds = {};
 	struct xfs_mount		*mp = sc->mp;
@@ -492,6 +493,7 @@ xfs_scrub_da_btree(
 	ds.state->args = &ds.dargs;
 	ds.state->mp = mp;
 	ds.sc = sc;
+	ds.private = private;
 	if (whichfork == XFS_ATTR_FORK) {
 		ds.dargs.geo = mp->m_attr_geo;
 		ds.lowest = 0;
fs/xfs/scrub/dabtree.h +2 -1
index 2a766de1f..d31468d 100644
@@ -28,6 +28,7 @@ struct xfs_scrub_da_btree {
 	int				maxrecs[XFS_DA_NODE_MAXDEPTH];
 	struct xfs_da_state		*state;
 	struct xfs_scrub_context	*sc;
+	void				*private;
 
 	/*
 	 * Lowest and highest directory block address in which we expect
@@ -53,6 +54,6 @@ void xfs_scrub_da_set_corrupt(struct xfs_scrub_da_btree *ds, int level);
 int xfs_scrub_da_btree_hash(struct xfs_scrub_da_btree *ds, int level,
 			    __be32 *hashp);
 int xfs_scrub_da_btree(struct xfs_scrub_context *sc, int whichfork,
-		       xfs_scrub_da_btree_rec_fn scrub_fn);
+		       xfs_scrub_da_btree_rec_fn scrub_fn, void *private);
 
 #endif /* __XFS_SCRUB_DABTREE_H__ */
fs/xfs/scrub/dir.c +1 -1
index 169fb10..c61362f 100644
@@ -770,7 +770,7 @@ xfs_scrub_directory(
 	}
 
 	/* Check directory tree structure */
-	error = xfs_scrub_da_btree(sc, XFS_DATA_FORK, xfs_scrub_dir_rec);
+	error = xfs_scrub_da_btree(sc, XFS_DATA_FORK, xfs_scrub_dir_rec, NULL);
 	if (error)
 		return error;