[PATCH 0/2] xfs: avoid overflow of attr3 leaf block headers with 64k blocks

[PATCH 0/2] xfs: avoid overflow of attr3 leaf block headers with 64k blocks

[Brian Foster]

Hi all,

As it turns out, some of the leaf block codepaths other than xattr
addition look like they wouldn't work properly with an unaligned
firstused value as demonstrated in my previous patch:

http://oss.sgi.com/archives/xfs/2015-02/msg00479.html

Here's an alternate approach that handles the overflow in the header
conversion functions. It passes the basic tests so far, but more testing
is needed to make sure I've made valid assumptions. Thoughts?

FWIW, another approach could be to leak the last few bytes of the blocks
(e.g., max firstused at the last valid aligned offset and update freemap
size accordingly)...

Brian

Brian Foster (2):
  xfs: pass attr geometry to attr leaf header conversion functions
  xfs: use larger in-core attr firstused field and detect overflow

 fs/xfs/libxfs/xfs_attr_leaf.c | 99 +++++++++++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_attr_leaf.h |  6 ++-
 fs/xfs/libxfs/xfs_da_format.h |  8 +++-
 fs/xfs/xfs_attr_inactive.c    |  3 +-
 fs/xfs/xfs_attr_list.c        |  9 ++--
 5 files changed, 86 insertions(+), 39 deletions(-)

-- 
1.9.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs: pass attr geometry to attr leaf header conversion functions

[Brian Foster]

The firstused field of the xfs_attr3_leaf_hdr structure is subject to an
overflow when fs blocksize is 64k. In preparation to handle this
overflow in the header conversion functions, pass the attribute geometry
to the functions that convert the in-core structure to and from the
on-disk structure.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 63 +++++++++++++++++++++++--------------------
 fs/xfs/libxfs/xfs_attr_leaf.h |  6 +++--
 fs/xfs/xfs_attr_inactive.c    |  3 ++-
 fs/xfs/xfs_attr_list.c        |  9 ++++---
 4 files changed, 46 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 15105db..3337516 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -88,6 +88,7 @@ STATIC int xfs_attr_leaf_entsize(xfs_attr_leafblock_t *leaf, int index);
 
 void
 xfs_attr3_leaf_hdr_from_disk(
+	struct xfs_da_geometry		*geo,
 	struct xfs_attr3_icleaf_hdr	*to,
 	struct xfs_attr_leafblock	*from)
 {
@@ -129,6 +130,7 @@ xfs_attr3_leaf_hdr_from_disk(
 
 void
 xfs_attr3_leaf_hdr_to_disk(
+	struct xfs_da_geometry		*geo,
 	struct xfs_attr_leafblock	*to,
 	struct xfs_attr3_icleaf_hdr	*from)
 {
@@ -178,7 +180,7 @@ xfs_attr3_leaf_verify(
 	struct xfs_attr_leafblock *leaf = bp->b_addr;
 	struct xfs_attr3_icleaf_hdr ichdr;
 
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
@@ -757,9 +759,10 @@ xfs_attr_shortform_allfit(
 	struct xfs_attr3_icleaf_hdr leafhdr;
 	int			bytes;
 	int			i;
+	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&leafhdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
 	entry = xfs_attr3_leaf_entryp(leaf);
 
 	bytes = sizeof(struct xfs_attr_sf_hdr);
@@ -812,7 +815,7 @@ xfs_attr3_leaf_to_shortform(
 	memcpy(tmpbuffer, bp->b_addr, args->geo->blksize);
 
 	leaf = (xfs_attr_leafblock_t *)tmpbuffer;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	entry = xfs_attr3_leaf_entryp(leaf);
 
 	/* XXX (dgc): buffer is about to be marked stale - why zero it? */
@@ -923,7 +926,7 @@ xfs_attr3_leaf_to_node(
 	btree = dp->d_ops->node_tree_p(node);
 
 	leaf = bp2->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&icleafhdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &icleafhdr, leaf);
 	entries = xfs_attr3_leaf_entryp(leaf);
 
 	/* both on-disk, don't endian-flip twice */
@@ -988,7 +991,7 @@ xfs_attr3_leaf_create(
 	}
 	ichdr.freemap[0].size = ichdr.firstused - ichdr.freemap[0].base;
 
-	xfs_attr3_leaf_hdr_to_disk(leaf, &ichdr);
+	xfs_attr3_leaf_hdr_to_disk(args->geo, leaf, &ichdr);
 	xfs_trans_log_buf(args->trans, bp, 0, args->geo->blksize - 1);
 
 	*bpp = bp;
@@ -1073,7 +1076,7 @@ xfs_attr3_leaf_add(
 	trace_xfs_attr_leaf_add(args);
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	ASSERT(args->index >= 0 && args->index <= ichdr.count);
 	entsize = xfs_attr_leaf_newentsize(args, NULL);
 
@@ -1126,7 +1129,7 @@ xfs_attr3_leaf_add(
 	tmp = xfs_attr3_leaf_add_work(bp, &ichdr, args, 0);
 
 out_log_hdr:
-	xfs_attr3_leaf_hdr_to_disk(leaf, &ichdr);
+	xfs_attr3_leaf_hdr_to_disk(args->geo, leaf, &ichdr);
 	xfs_trans_log_buf(args->trans, bp,
 		XFS_DA_LOGRANGE(leaf, &leaf->hdr,
 				xfs_attr3_leaf_hdr_size(leaf)));
@@ -1294,7 +1297,7 @@ xfs_attr3_leaf_compact(
 						ichdr_dst->freemap[0].base;
 
 	/* write the header back to initialise the underlying buffer */
-	xfs_attr3_leaf_hdr_to_disk(leaf_dst, ichdr_dst);
+	xfs_attr3_leaf_hdr_to_disk(args->geo, leaf_dst, ichdr_dst);
 
 	/*
 	 * Copy all entry's in the same (sorted) order,
@@ -1344,9 +1347,10 @@ xfs_attr_leaf_order(
 {
 	struct xfs_attr3_icleaf_hdr ichdr1;
 	struct xfs_attr3_icleaf_hdr ichdr2;
+	struct xfs_mount *mp = leaf1_bp->b_target->bt_mount;
 
-	xfs_attr3_leaf_hdr_from_disk(&ichdr1, leaf1_bp->b_addr);
-	xfs_attr3_leaf_hdr_from_disk(&ichdr2, leaf2_bp->b_addr);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr1, leaf1_bp->b_addr);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr2, leaf2_bp->b_addr);
 	return xfs_attr3_leaf_order(leaf1_bp, &ichdr1, leaf2_bp, &ichdr2);
 }
 
@@ -1388,8 +1392,8 @@ xfs_attr3_leaf_rebalance(
 	ASSERT(blk2->magic == XFS_ATTR_LEAF_MAGIC);
 	leaf1 = blk1->bp->b_addr;
 	leaf2 = blk2->bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr1, leaf1);
-	xfs_attr3_leaf_hdr_from_disk(&ichdr2, leaf2);
+	xfs_attr3_leaf_hdr_from_disk(state->args->geo, &ichdr1, leaf1);
+	xfs_attr3_leaf_hdr_from_disk(state->args->geo, &ichdr2, leaf2);
 	ASSERT(ichdr2.count == 0);
 	args = state->args;
 
@@ -1490,8 +1494,8 @@ xfs_attr3_leaf_rebalance(
 					ichdr1.count, count);
 	}
 
-	xfs_attr3_leaf_hdr_to_disk(leaf1, &ichdr1);
-	xfs_attr3_leaf_hdr_to_disk(leaf2, &ichdr2);
+	xfs_attr3_leaf_hdr_to_disk(state->args->geo, leaf1, &ichdr1);
+	xfs_attr3_leaf_hdr_to_disk(state->args->geo, leaf2, &ichdr2);
 	xfs_trans_log_buf(args->trans, blk1->bp, 0, args->geo->blksize - 1);
 	xfs_trans_log_buf(args->trans, blk2->bp, 0, args->geo->blksize - 1);
 
@@ -1684,7 +1688,7 @@ xfs_attr3_leaf_toosmall(
 	 */
 	blk = &state->path.blk[ state->path.active-1 ];
 	leaf = blk->bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(state->args->geo, &ichdr, leaf);
 	bytes = xfs_attr3_leaf_hdr_size(leaf) +
 		ichdr.count * sizeof(xfs_attr_leaf_entry_t) +
 		ichdr.usedbytes;
@@ -1740,7 +1744,7 @@ xfs_attr3_leaf_toosmall(
 		if (error)
 			return error;
 
-		xfs_attr3_leaf_hdr_from_disk(&ichdr2, bp->b_addr);
+		xfs_attr3_leaf_hdr_from_disk(state->args->geo, &ichdr2, bp->b_addr);
 
 		bytes = state->args->geo->blksize -
 			(state->args->geo->blksize >> 2) -
@@ -1805,7 +1809,7 @@ xfs_attr3_leaf_remove(
 	trace_xfs_attr_leaf_remove(args);
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 
 	ASSERT(ichdr.count > 0 && ichdr.count < args->geo->blksize / 8);
 	ASSERT(args->index >= 0 && args->index < ichdr.count);
@@ -1923,7 +1927,7 @@ xfs_attr3_leaf_remove(
 	} else {
 		ichdr.holes = 1;	/* mark as needing compaction */
 	}
-	xfs_attr3_leaf_hdr_to_disk(leaf, &ichdr);
+	xfs_attr3_leaf_hdr_to_disk(args->geo, leaf, &ichdr);
 	xfs_trans_log_buf(args->trans, bp,
 			  XFS_DA_LOGRANGE(leaf, &leaf->hdr,
 					  xfs_attr3_leaf_hdr_size(leaf)));
@@ -1957,8 +1961,8 @@ xfs_attr3_leaf_unbalance(
 
 	drop_leaf = drop_blk->bp->b_addr;
 	save_leaf = save_blk->bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&drophdr, drop_leaf);
-	xfs_attr3_leaf_hdr_from_disk(&savehdr, save_leaf);
+	xfs_attr3_leaf_hdr_from_disk(state->args->geo, &drophdr, drop_leaf);
+	xfs_attr3_leaf_hdr_from_disk(state->args->geo, &savehdr, save_leaf);
 	entry = xfs_attr3_leaf_entryp(drop_leaf);
 
 	/*
@@ -2012,7 +2016,7 @@ xfs_attr3_leaf_unbalance(
 		tmphdr.firstused = state->args->geo->blksize;
 
 		/* write the header to the temp buffer to initialise it */
-		xfs_attr3_leaf_hdr_to_disk(tmp_leaf, &tmphdr);
+		xfs_attr3_leaf_hdr_to_disk(state->args->geo, tmp_leaf, &tmphdr);
 
 		if (xfs_attr3_leaf_order(save_blk->bp, &savehdr,
 					 drop_blk->bp, &drophdr)) {
@@ -2039,7 +2043,7 @@ xfs_attr3_leaf_unbalance(
 		kmem_free(tmp_leaf);
 	}
 
-	xfs_attr3_leaf_hdr_to_disk(save_leaf, &savehdr);
+	xfs_attr3_leaf_hdr_to_disk(state->args->geo, save_leaf, &savehdr);
 	xfs_trans_log_buf(state->args->trans, save_blk->bp, 0,
 					   state->args->geo->blksize - 1);
 
@@ -2085,7 +2089,7 @@ xfs_attr3_leaf_lookup_int(
 	trace_xfs_attr_leaf_lookup(args);
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	entries = xfs_attr3_leaf_entryp(leaf);
 	ASSERT(ichdr.count < args->geo->blksize / 8);
 
@@ -2190,7 +2194,7 @@ xfs_attr3_leaf_getvalue(
 	int			valuelen;
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	ASSERT(ichdr.count < args->geo->blksize / 8);
 	ASSERT(args->index < ichdr.count);
 
@@ -2391,8 +2395,9 @@ xfs_attr_leaf_lasthash(
 {
 	struct xfs_attr3_icleaf_hdr ichdr;
 	struct xfs_attr_leaf_entry *entries;
+	struct xfs_mount *mp = bp->b_target->bt_mount;
 
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, bp->b_addr);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, bp->b_addr);
 	entries = xfs_attr3_leaf_entryp(bp->b_addr);
 	if (count)
 		*count = ichdr.count;
@@ -2486,7 +2491,7 @@ xfs_attr3_leaf_clearflag(
 	ASSERT(entry->flags & XFS_ATTR_INCOMPLETE);
 
 #ifdef DEBUG
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	ASSERT(args->index < ichdr.count);
 	ASSERT(args->index >= 0);
 
@@ -2550,7 +2555,7 @@ xfs_attr3_leaf_setflag(
 
 	leaf = bp->b_addr;
 #ifdef DEBUG
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr, leaf);
 	ASSERT(args->index < ichdr.count);
 	ASSERT(args->index >= 0);
 #endif
@@ -2629,11 +2634,11 @@ xfs_attr3_leaf_flipflags(
 	entry2 = &xfs_attr3_leaf_entryp(leaf2)[args->index2];
 
 #ifdef DEBUG
-	xfs_attr3_leaf_hdr_from_disk(&ichdr1, leaf1);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr1, leaf1);
 	ASSERT(args->index < ichdr1.count);
 	ASSERT(args->index >= 0);
 
-	xfs_attr3_leaf_hdr_from_disk(&ichdr2, leaf2);
+	xfs_attr3_leaf_hdr_from_disk(args->geo, &ichdr2, leaf2);
 	ASSERT(args->index2 < ichdr2.count);
 	ASSERT(args->index2 >= 0);
 
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index e2929da..025c4b8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -100,9 +100,11 @@ int	xfs_attr_leaf_newentsize(struct xfs_da_args *args, int *local);
 int	xfs_attr3_leaf_read(struct xfs_trans *tp, struct xfs_inode *dp,
 			xfs_dablk_t bno, xfs_daddr_t mappedbno,
 			struct xfs_buf **bpp);
-void	xfs_attr3_leaf_hdr_from_disk(struct xfs_attr3_icleaf_hdr *to,
+void	xfs_attr3_leaf_hdr_from_disk(struct xfs_da_geometry *geo,
+				     struct xfs_attr3_icleaf_hdr *to,
 				     struct xfs_attr_leafblock *from);
-void	xfs_attr3_leaf_hdr_to_disk(struct xfs_attr_leafblock *to,
+void	xfs_attr3_leaf_hdr_to_disk(struct xfs_da_geometry *geo,
+				   struct xfs_attr_leafblock *to,
 				   struct xfs_attr3_icleaf_hdr *from);
 
 #endif	/* __XFS_ATTR_LEAF_H__ */
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index 83af4c1..f9c1c64 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -132,9 +132,10 @@ xfs_attr3_leaf_inactive(
 	int			size;
 	int			tmp;
 	int			i;
+	struct xfs_mount	*mp = bp->b_target->bt_mount;
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
 	/*
 	 * Count the number of "remote" value extents.
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index a43d370..65fb37a 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -225,6 +225,7 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
 	int error, i;
 	struct xfs_buf *bp;
 	struct xfs_inode	*dp = context->dp;
+	struct xfs_mount	*mp = dp->i_mount;
 
 	trace_xfs_attr_node_list(context);
 
@@ -256,7 +257,8 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
 			case XFS_ATTR_LEAF_MAGIC:
 			case XFS_ATTR3_LEAF_MAGIC:
 				leaf = bp->b_addr;
-				xfs_attr3_leaf_hdr_from_disk(&leafhdr, leaf);
+				xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo,
+							     &leafhdr, leaf);
 				entries = xfs_attr3_leaf_entryp(leaf);
 				if (cursor->hashval > be32_to_cpu(
 						entries[leafhdr.count - 1].hashval)) {
@@ -340,7 +342,7 @@ xfs_attr_node_list(xfs_attr_list_context_t *context)
 			xfs_trans_brelse(NULL, bp);
 			return error;
 		}
-		xfs_attr3_leaf_hdr_from_disk(&leafhdr, leaf);
+		xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
 		if (context->seen_enough || leafhdr.forw == 0)
 			break;
 		cursor->blkno = leafhdr.forw;
@@ -368,11 +370,12 @@ xfs_attr3_leaf_list_int(
 	struct xfs_attr_leaf_entry	*entry;
 	int				retval;
 	int				i;
+	struct xfs_mount		*mp = context->dp->i_mount;
 
 	trace_xfs_attr_list_leaf(context);
 
 	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(&ichdr, leaf);
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 	entries = xfs_attr3_leaf_entryp(leaf);
 
 	cursor = context->cursor;
-- 
1.9.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: use larger in-core attr firstused field and detect overflow

[Brian Foster]

The on-disk xfs_attr3_leaf_hdr structure firstused field is 16-bit and
subject to overflow when fs block size is 64k. The field is typically
initialized to block size when an attr leaf block is initialized. This
problem is demonstrated by assert failures when running xfstests
generic/117 on an fs with 64k blocks.

To support the existing attr leaf block algorithms for insertion,
rebalance and entry movement, increase the size of the in-core firstused
field to 32-bit and handle the potential overflow on conversion to/from
the on-disk structure. If the overflow condition occurs, set a special
value in the firstused field that is translated back on header read. The
special value is only required in the case of an empty 64k attr block. A
value of zero is used because firstused is initialized to the block size
and grows backwards from there. Furthermore, the attribute block header
occupies the first bytes of the block. Thus, a value of zero has no
other legitimate meaning for this structure.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 36 +++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_da_format.h |  8 +++++++-
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 3337516..3277b40 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -106,6 +106,12 @@ xfs_attr3_leaf_hdr_from_disk(
 		to->count = be16_to_cpu(hdr3->count);
 		to->usedbytes = be16_to_cpu(hdr3->usedbytes);
 		to->firstused = be16_to_cpu(hdr3->firstused);
+		if (to->firstused == XFS_ATTR3_LEAF_NULLOFF) {
+			/* only empty blocks when size overflows firstused! */
+			ASSERT(!to->count && !to->usedbytes &&
+			       geo->blksize > USHRT_MAX);
+			to->firstused = geo->blksize;
+		}
 		to->holes = hdr3->holes;
 
 		for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
@@ -120,6 +126,12 @@ xfs_attr3_leaf_hdr_from_disk(
 	to->count = be16_to_cpu(from->hdr.count);
 	to->usedbytes = be16_to_cpu(from->hdr.usedbytes);
 	to->firstused = be16_to_cpu(from->hdr.firstused);
+	if (to->firstused == XFS_ATTR3_LEAF_NULLOFF) {
+		/* only empty blocks when size overflows firstused! */
+		ASSERT(!to->count && !to->usedbytes &&
+		       geo->blksize > USHRT_MAX);
+		to->firstused = geo->blksize;
+	}
 	to->holes = from->hdr.holes;
 
 	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
@@ -134,11 +146,29 @@ xfs_attr3_leaf_hdr_to_disk(
 	struct xfs_attr_leafblock	*to,
 	struct xfs_attr3_icleaf_hdr	*from)
 {
-	int	i;
+	int				i;
+	uint16_t			firstused;
 
 	ASSERT(from->magic == XFS_ATTR_LEAF_MAGIC ||
 	       from->magic == XFS_ATTR3_LEAF_MAGIC);
 
+	/*
+	 * Handle overflow of the on-disk firstused field. firstused is
+	 * typically initialized to block size, but we only have 2-bytes in the
+	 * on-disk structure. This means a 64k block size overflows the field.
+	 *
+	 * firstused should only match block size for an empty attr block so set
+	 * a special value that the from_disk() variant can convert back to
+	 * blocksize in the in-core structure.
+	 */
+	if (from->firstused > USHRT_MAX) {
+		ASSERT(from->firstused == geo->blksize);
+		firstused = XFS_ATTR3_LEAF_NULLOFF;
+	} else {
+		ASSERT(from->firstused != 0);
+		firstused = from->firstused;
+	}
+
 	if (from->magic == XFS_ATTR3_LEAF_MAGIC) {
 		struct xfs_attr3_leaf_hdr *hdr3 = (struct xfs_attr3_leaf_hdr *)to;
 
@@ -147,7 +177,7 @@ xfs_attr3_leaf_hdr_to_disk(
 		hdr3->info.hdr.magic = cpu_to_be16(from->magic);
 		hdr3->count = cpu_to_be16(from->count);
 		hdr3->usedbytes = cpu_to_be16(from->usedbytes);
-		hdr3->firstused = cpu_to_be16(from->firstused);
+		hdr3->firstused = cpu_to_be16(firstused);
 		hdr3->holes = from->holes;
 		hdr3->pad1 = 0;
 
@@ -162,7 +192,7 @@ xfs_attr3_leaf_hdr_to_disk(
 	to->hdr.info.magic = cpu_to_be16(from->magic);
 	to->hdr.count = cpu_to_be16(from->count);
 	to->hdr.usedbytes = cpu_to_be16(from->usedbytes);
-	to->hdr.firstused = cpu_to_be16(from->firstused);
+	to->hdr.firstused = cpu_to_be16(firstused);
 	to->hdr.holes = from->holes;
 	to->hdr.pad1 = 0;
 
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 0a49b02..d2d0498 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -725,7 +725,7 @@ struct xfs_attr3_icleaf_hdr {
 	__uint16_t	magic;
 	__uint16_t	count;
 	__uint16_t	usedbytes;
-	__uint16_t	firstused;
+	__uint32_t	firstused;
 	__u8		holes;
 	struct {
 		__uint16_t	base;
@@ -734,6 +734,12 @@ struct xfs_attr3_icleaf_hdr {
 };
 
 /*
+ * Special value to represent fs block size in the leaf header firstused field.
+ * Only used when block size overflows the 2-bytes available on disk.
+ */
+#define XFS_ATTR3_LEAF_NULLOFF	0
+
+/*
  * Flags used in the leaf_entry[i].flags field.
  * NOTE: the INCOMPLETE bit must not collide with the flags bits specified
  * on the system call, they are "or"ed together for various operations.
-- 
1.9.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] xfs: avoid overflow of attr3 leaf block headers with 64k blocks

[Brian Foster]

On Tue, Feb 24, 2015 at 03:50:22PM -0500, Brian Foster wrote:
> Hi all,
> 
> As it turns out, some of the leaf block codepaths other than xattr
> addition look like they wouldn't work properly with an unaligned
> firstused value as demonstrated in my previous patch:
> 
> http://oss.sgi.com/archives/xfs/2015-02/msg00479.html
> 
> Here's an alternate approach that handles the overflow in the header
> conversion functions. It passes the basic tests so far, but more testing
> is needed to make sure I've made valid assumptions. Thoughts?
> 
> FWIW, another approach could be to leak the last few bytes of the blocks
> (e.g., max firstused at the last valid aligned offset and update freemap
> size accordingly)...
> 

ping?

> Brian
> 
> Brian Foster (2):
>   xfs: pass attr geometry to attr leaf header conversion functions
>   xfs: use larger in-core attr firstused field and detect overflow
> 
>  fs/xfs/libxfs/xfs_attr_leaf.c | 99 +++++++++++++++++++++++++++++--------------
>  fs/xfs/libxfs/xfs_attr_leaf.h |  6 ++-
>  fs/xfs/libxfs/xfs_da_format.h |  8 +++-
>  fs/xfs/xfs_attr_inactive.c    |  3 +-
>  fs/xfs/xfs_attr_list.c        |  9 ++--
>  5 files changed, 86 insertions(+), 39 deletions(-)
> 
> -- 
> 1.9.3
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] xfs: avoid overflow of attr3 leaf block headers with 64k blocks

[Darrick J. Wong]

On Wed, Mar 25, 2015 at 12:56:36PM -0400, Brian Foster wrote:
> On Tue, Feb 24, 2015 at 03:50:22PM -0500, Brian Foster wrote:
> > Hi all,
> > 
> > As it turns out, some of the leaf block codepaths other than xattr
> > addition look like they wouldn't work properly with an unaligned
> > firstused value as demonstrated in my previous patch:
> > 
> > http://oss.sgi.com/archives/xfs/2015-02/msg00479.html
> > 
> > Here's an alternate approach that handles the overflow in the header
> > conversion functions. It passes the basic tests so far, but more testing
> > is needed to make sure I've made valid assumptions. Thoughts?
> > 
> > FWIW, another approach could be to leak the last few bytes of the blocks
> > (e.g., max firstused at the last valid aligned offset and update freemap
> > size accordingly)...
> > 
> 
> ping?

Looks reasonable enough to me...

--D

> 
> > Brian
> > 
> > Brian Foster (2):
> >   xfs: pass attr geometry to attr leaf header conversion functions
> >   xfs: use larger in-core attr firstused field and detect overflow
> > 
> >  fs/xfs/libxfs/xfs_attr_leaf.c | 99 +++++++++++++++++++++++++++++--------------
> >  fs/xfs/libxfs/xfs_attr_leaf.h |  6 ++-
> >  fs/xfs/libxfs/xfs_da_format.h |  8 +++-
> >  fs/xfs/xfs_attr_inactive.c    |  3 +-
> >  fs/xfs/xfs_attr_list.c        |  9 ++--
> >  5 files changed, 86 insertions(+), 39 deletions(-)
> > 
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: pass attr geometry to attr leaf header conversion functions

[Dave Chinner]

On Tue, Feb 24, 2015 at 03:50:23PM -0500, Brian Foster wrote:
> The firstused field of the xfs_attr3_leaf_hdr structure is subject to an
> overflow when fs blocksize is 64k. In preparation to handle this
> overflow in the header conversion functions, pass the attribute geometry
> to the functions that convert the in-core structure to and from the
> on-disk structure.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Hmmm. not sure I'm a great fan of this, but let's see.

Ok, this just a conversion patch, moving on to the second patch...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: use larger in-core attr firstused field and detect overflow

[Dave Chinner]

On Tue, Feb 24, 2015 at 03:50:24PM -0500, Brian Foster wrote:
> The on-disk xfs_attr3_leaf_hdr structure firstused field is 16-bit and
> subject to overflow when fs block size is 64k. The field is typically
> initialized to block size when an attr leaf block is initialized. This
> problem is demonstrated by assert failures when running xfstests
> generic/117 on an fs with 64k blocks.
> 
> To support the existing attr leaf block algorithms for insertion,
> rebalance and entry movement, increase the size of the in-core firstused
> field to 32-bit and handle the potential overflow on conversion to/from
> the on-disk structure. If the overflow condition occurs, set a special
> value in the firstused field that is translated back on header read. The
> special value is only required in the case of an empty 64k attr block. A
> value of zero is used because firstused is initialized to the block size
> and grows backwards from there. Furthermore, the attribute block header
> occupies the first bytes of the block. Thus, a value of zero has no
> other legitimate meaning for this structure.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 36 +++++++++++++++++++++++++++++++++---
>  fs/xfs/libxfs/xfs_da_format.h |  8 +++++++-
>  2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 3337516..3277b40 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -106,6 +106,12 @@ xfs_attr3_leaf_hdr_from_disk(
>  		to->count = be16_to_cpu(hdr3->count);
>  		to->usedbytes = be16_to_cpu(hdr3->usedbytes);
>  		to->firstused = be16_to_cpu(hdr3->firstused);
> +		if (to->firstused == XFS_ATTR3_LEAF_NULLOFF) {
> +			/* only empty blocks when size overflows firstused! */
> +			ASSERT(!to->count && !to->usedbytes &&
> +			       geo->blksize > USHRT_MAX);
> +			to->firstused = geo->blksize;
> +		}
>  		to->holes = hdr3->holes;

So if it's already zero on disk, we set it to the block size....

>  		for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
> @@ -120,6 +126,12 @@ xfs_attr3_leaf_hdr_from_disk(
>  	to->count = be16_to_cpu(from->hdr.count);
>  	to->usedbytes = be16_to_cpu(from->hdr.usedbytes);
>  	to->firstused = be16_to_cpu(from->hdr.firstused);
> +	if (to->firstused == XFS_ATTR3_LEAF_NULLOFF) {
> +		/* only empty blocks when size overflows firstused! */
> +		ASSERT(!to->count && !to->usedbytes &&
> +		       geo->blksize > USHRT_MAX);
> +		to->firstused = geo->blksize;
> +	}
>  	to->holes = from->hdr.holes;

And duplicate the code here as well.

>  	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
> @@ -134,11 +146,29 @@ xfs_attr3_leaf_hdr_to_disk(
>  	struct xfs_attr_leafblock	*to,
>  	struct xfs_attr3_icleaf_hdr	*from)
>  {
> -	int	i;
> +	int				i;
> +	uint16_t			firstused;
>  
>  	ASSERT(from->magic == XFS_ATTR_LEAF_MAGIC ||
>  	       from->magic == XFS_ATTR3_LEAF_MAGIC);
>  
> +	/*
> +	 * Handle overflow of the on-disk firstused field. firstused is
> +	 * typically initialized to block size, but we only have 2-bytes in the
> +	 * on-disk structure. This means a 64k block size overflows the field.
> +	 *
> +	 * firstused should only match block size for an empty attr block so set
> +	 * a special value that the from_disk() variant can convert back to
> +	 * blocksize in the in-core structure.
> +	 */
> +	if (from->firstused > USHRT_MAX) {
> +		ASSERT(from->firstused == geo->blksize);
> +		firstused = XFS_ATTR3_LEAF_NULLOFF;
> +	} else {
> +		ASSERT(from->firstused != 0);
> +		firstused = from->firstused;
> +	}

Ok, that makes this a non-trivial sized function now :P

I think helpers might be appropriate...

> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 0a49b02..d2d0498 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -725,7 +725,7 @@ struct xfs_attr3_icleaf_hdr {
>  	__uint16_t	magic;
>  	__uint16_t	count;
>  	__uint16_t	usedbytes;
> -	__uint16_t	firstused;
> +	__uint32_t	firstused;

Needs a comment explaining why it's different to the on-disk
structure.

>  	__u8		holes;
>  	struct {
>  		__uint16_t	base;
> @@ -734,6 +734,12 @@ struct xfs_attr3_icleaf_hdr {
>  };
>  
>  /*
> + * Special value to represent fs block size in the leaf header firstused field.
> + * Only used when block size overflows the 2-bytes available on disk.
> + */
> +#define XFS_ATTR3_LEAF_NULLOFF	0
> +

I think declaring a pair of helper functions (e.g.
xfs_attr3_leaf_firstused_to/from_disk) here would be a good idea.
That way all the coversions, magic numbers and comments documenting
the behaviour are in the one place in the on-disk format
specification....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs