* [PATCH v3 0/9] xfs: fix [f]inobt magic value verification
@ 2019-02-04 14:52 Brian Foster
2019-02-04 14:52 ` [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type Brian Foster
` (9 more replies)
0 siblings, 10 replies; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
Hi all,
Here's v3 of the verifier magic value fixup series. This version adds a
couple more patches to separate and convert the allocation btree buffer
verifier to the new magic value verification scheme. The allocbt is
similar to the inobt verifier in that it currently uses the same
verifier code for multiple magic values. This change allows the verifier
to ensure that bnobt/cntbt blocks have the appropriate magic value for
v4 and v5 filesytems. The only other change from v2 is a fixup to
include parameter names in function declarations.
Thoughts, reviews, flames appreciated.
Brian
v3:
- Fix function declarations to include parameter names.
- Convert allocbt buffer verifier.
v2: https://marc.info/?l=linux-xfs&m=154894958207167&w=2
- Include djwong's ->b_ops patch w/ modifications.
- Added patch to fix up existing cpu endian magic checks, fold in typo
fix.
- Replace static inline magic verifier helper with out of line variant,
kill macro.
- Store on-disk byte order magics in ->b_ops.
- Added patch to refactor common xfs_da3_blkinfo checks.
v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
- Remove endian conversion from helper.
- Drop finobt bad magic mitigation patch.
- Additional verifier magic fixups.
- Add verifier name typo fixup.
rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
- Split off finobt verifier into separate patch, assign it
appropriately.
- Created helpers for xfs_buf_ops magic value verification.
- Added error mitigation patch for problematic finobt blocks.
rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
Brian Foster (8):
xfs: always check magic values in on-disk byte order
xfs: create a separate finobt verifier
xfs: distinguish between inobt and finobt magic values
xfs: split up allocation btree verifier
xfs: distinguish between bnobt and cntbt magic values
xfs: use verifier magic field in dir2 leaf verifiers
xfs: miscellaneous verifier magic value fixups
xfs: factor xfs_da3_blkinfo verification into common helper
Darrick J. Wong (1):
xfs: set buffer ops when repair probes for btree type
fs/xfs/libxfs/xfs_ag.c | 6 +-
fs/xfs/libxfs/xfs_alloc.c | 12 ++--
fs/xfs/libxfs/xfs_alloc_btree.c | 74 ++++++++++-----------
fs/xfs/libxfs/xfs_attr_leaf.c | 21 ++----
fs/xfs/libxfs/xfs_attr_remote.c | 8 ++-
fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++--
fs/xfs/libxfs/xfs_da_btree.c | 50 ++++++++++-----
fs/xfs/libxfs/xfs_da_format.h | 3 +
fs/xfs/libxfs/xfs_dir2_block.c | 10 +--
fs/xfs/libxfs/xfs_dir2_data.c | 12 ++--
fs/xfs/libxfs/xfs_dir2_leaf.c | 100 ++++++-----------------------
fs/xfs/libxfs/xfs_dir2_node.c | 11 ++--
fs/xfs/libxfs/xfs_ialloc.c | 3 +-
fs/xfs/libxfs/xfs_ialloc_btree.c | 25 +++++---
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/libxfs/xfs_refcount_btree.c | 3 +-
fs/xfs/libxfs/xfs_rmap_btree.c | 3 +-
fs/xfs/libxfs/xfs_sb.c | 5 +-
fs/xfs/libxfs/xfs_shared.h | 4 +-
fs/xfs/libxfs/xfs_symlink_remote.c | 3 +-
fs/xfs/scrub/agheader_repair.c | 6 +-
fs/xfs/scrub/repair.c | 11 +++-
fs/xfs/xfs_buf.c | 41 ++++++++++--
fs/xfs/xfs_buf.h | 4 +-
fs/xfs/xfs_log_recover.c | 12 ++--
fs/xfs/xfs_trans_buf.c | 2 +-
26 files changed, 229 insertions(+), 215 deletions(-)
--
2.17.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 18:08 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 2/9] xfs: always check magic values in on-disk byte order Brian Foster
` (8 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
From: "Darrick J. Wong" <darrick.wong@oracle.com>
In xrep_findroot_block, we work out the btree type and correctness of a
given block by calling different btree verifiers on root block
candidates. However, we leave the NULL b_ops while ->verify_read
validates the block, which means that if the verifier calls
xfs_buf_verifier_error it'll crash on the null b_ops. Fix it to set
b_ops before calling the verifier and unsetting it if the verifier
fails.
Furthermore, improve the documentation around xfs_buf_ensure_ops, which
is the function that is responsible for cleaning up the b_ops state of
buffers that go through xrep_findroot_block but don't match anything.
[bfoster: Renamed xfs_buf_ensure_ops() and modified comment.]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/scrub/repair.c | 11 ++++++++---
fs/xfs/xfs_buf.c | 22 ++++++++++++++++------
fs/xfs/xfs_buf.h | 2 +-
fs/xfs/xfs_trans_buf.c | 2 +-
4 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 1c8eecfe52b8..6acf1bfa0bfe 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -768,18 +768,23 @@ xrep_findroot_block(
if (!uuid_equal(&btblock->bb_u.s.bb_uuid,
&mp->m_sb.sb_meta_uuid))
goto out;
+ /*
+ * Read verifiers can reference b_ops, so we set the pointer
+ * here. If the verifier fails we'll reset the buffer state
+ * to what it was before we touched the buffer.
+ */
+ bp->b_ops = fab->buf_ops;
fab->buf_ops->verify_read(bp);
if (bp->b_error) {
+ bp->b_ops = NULL;
bp->b_error = 0;
goto out;
}
/*
* Some read verifiers will (re)set b_ops, so we must be
- * careful not to blow away any such assignment.
+ * careful not to change b_ops after running the verifier.
*/
- if (!bp->b_ops)
- bp->b_ops = fab->buf_ops;
}
/*
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index eedc5e0156ff..222b5260ed35 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -776,13 +776,23 @@ _xfs_buf_read(
}
/*
- * If the caller passed in an ops structure and the buffer doesn't have ops
- * assigned, set the ops and use them to verify the contents. If the contents
- * cannot be verified, we'll clear XBF_DONE. We assume the buffer has no
- * recorded errors and is already in XBF_DONE state.
+ * Reverify a buffer found in cache without an attached ->b_ops.
+ *
+ * If the caller passed an ops structure and the buffer doesn't have ops
+ * assigned, set the ops and use it to verify the contents. If verification
+ * fails, clear XBF_DONE. We assume the buffer has no recorded errors and is
+ * already in XBF_DONE state on entry.
+ *
+ * Under normal operations, every in-core buffer is verified on read I/O
+ * completion. There are two scenarios that can lead to in-core buffers without
+ * an assigned ->b_ops. The first is during log recovery of buffers on a V4
+ * filesystem, though these buffers are purged at the end of recovery. The
+ * other is online repair, which intentionally reads with a NULL buffer ops to
+ * run several verifiers across an in-core buffer in order to establish buffer
+ * type.
*/
int
-xfs_buf_ensure_ops(
+xfs_buf_reverify(
struct xfs_buf *bp,
const struct xfs_buf_ops *ops)
{
@@ -824,7 +834,7 @@ xfs_buf_read_map(
return bp;
}
- xfs_buf_ensure_ops(bp, ops);
+ xfs_buf_reverify(bp, ops);
if (flags & XBF_ASYNC) {
/*
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index b9f5511ea998..1c436a85b71d 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -385,6 +385,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
#define xfs_getsize_buftarg(buftarg) block_size((buftarg)->bt_bdev)
#define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
-int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
+int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
#endif /* __XFS_BUF_H__ */
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 629f1479c9d2..7d65ebf1e847 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -277,7 +277,7 @@ xfs_trans_read_buf_map(
* release this buffer when it kills the tranaction.
*/
ASSERT(bp->b_ops != NULL);
- error = xfs_buf_ensure_ops(bp, ops);
+ error = xfs_buf_reverify(bp, ops);
if (error) {
xfs_buf_ioerror_alert(bp, __func__);
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 2/9] xfs: always check magic values in on-disk byte order
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
2019-02-04 14:52 ` [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 17:08 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 3/9] xfs: create a separate finobt verifier Brian Foster
` (7 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
Most verifiers that check on-disk magic values convert the CPU
endian magic value constant to disk endian to facilitate compile
time optimization of the byte swap and reduce the need for runtime
byte swaps in buffer verifiers. Several buffer verifiers do not
follow this pattern. Update those verifiers for consistency.
Also fix up a random typo in the inode readahead verifier name.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_alloc.c | 2 +-
fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
fs/xfs/libxfs/xfs_da_btree.c | 4 ++--
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/libxfs/xfs_sb.c | 3 ++-
5 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index b715668886a4..48aab07e7138 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -570,7 +570,7 @@ xfs_agfl_verify(
if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
- if (be32_to_cpu(agfl->agfl_magicnum) != XFS_AGFL_MAGIC)
+ if (agfl->agfl_magicnum != cpu_to_be32(XFS_AGFL_MAGIC))
return __this_address;
/*
* during growfs operations, the perag is not fully initialised,
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 2652d00842d6..e60eba7f129c 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -251,7 +251,7 @@ xfs_attr3_leaf_verify(
if (xfs_sb_version_hascrc(&mp->m_sb)) {
struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
- if (ichdr.magic != XFS_ATTR3_LEAF_MAGIC)
+ if (hdr3->info.hdr.magic != cpu_to_be16(XFS_ATTR3_LEAF_MAGIC))
return __this_address;
if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
@@ -261,7 +261,7 @@ xfs_attr3_leaf_verify(
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
return __this_address;
} else {
- if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
+ if (leaf->hdr.info.magic != cpu_to_be16(XFS_ATTR_LEAF_MAGIC))
return __this_address;
}
/*
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 376bee94b5dd..355322688c9f 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -132,7 +132,7 @@ xfs_da3_node_verify(
if (xfs_sb_version_hascrc(&mp->m_sb)) {
struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
- if (ichdr.magic != XFS_DA3_NODE_MAGIC)
+ if (hdr3->info.hdr.magic != cpu_to_be16(XFS_DA3_NODE_MAGIC))
return __this_address;
if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
@@ -142,7 +142,7 @@ xfs_da3_node_verify(
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
return __this_address;
} else {
- if (ichdr.magic != XFS_DA_NODE_MAGIC)
+ if (hdr->hdr.info.magic != cpu_to_be16(XFS_DA_NODE_MAGIC))
return __this_address;
}
if (ichdr.level == 0)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 09d9c8cfa4a0..fd2df5747a3a 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -152,7 +152,7 @@ const struct xfs_buf_ops xfs_inode_buf_ops = {
};
const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
- .name = "xxfs_inode_ra",
+ .name = "xfs_inode_ra",
.verify_read = xfs_inode_buf_readahead_verify,
.verify_write = xfs_inode_buf_write_verify,
};
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b5a82acd7dfe..a2f52a958091 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -225,10 +225,11 @@ xfs_validate_sb_common(
struct xfs_buf *bp,
struct xfs_sb *sbp)
{
+ struct xfs_dsb *dsb = XFS_BUF_TO_SBP(bp);
uint32_t agcount = 0;
uint32_t rem;
- if (sbp->sb_magicnum != XFS_SB_MAGIC) {
+ if (dsb->sb_magicnum != cpu_to_be32(XFS_SB_MAGIC)) {
xfs_warn(mp, "bad magic number");
return -EWRONGFS;
}
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 3/9] xfs: create a separate finobt verifier
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
2019-02-04 14:52 ` [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type Brian Foster
2019-02-04 14:52 ` [PATCH v3 2/9] xfs: always check magic values in on-disk byte order Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 18:09 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 4/9] xfs: distinguish between inobt and finobt magic values Brian Foster
` (6 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
The inobt verifier is reused for the inobt and finobt, which
prevents the ability to distinguish between magic values on a
per-tree basis. Create a separate finobt structure in preparation
for changes to enforce the appropriate magic value for the
associated tree. This patch has no functional change.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_ag.c | 2 +-
fs/xfs/libxfs/xfs_ialloc_btree.c | 9 ++++++++-
fs/xfs/libxfs/xfs_shared.h | 1 +
| 2 +-
fs/xfs/xfs_log_recover.c | 6 ++++--
5 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 999ad8d00d43..bde67ef3ff43 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -361,7 +361,7 @@ xfs_ag_init_headers(
{ /* FINO root block */
.daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_FIBT_BLOCK(mp)),
.numblks = BTOBB(mp->m_sb.sb_blocksize),
- .ops = &xfs_inobt_buf_ops,
+ .ops = &xfs_finobt_buf_ops,
.work = &xfs_btroot_init,
.type = XFS_BTNUM_FINO,
.need_init = xfs_sb_version_hasfinobt(&mp->m_sb)
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 9b25e7a0df47..798269eb4767 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -333,6 +333,13 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
.verify_struct = xfs_inobt_verify,
};
+const struct xfs_buf_ops xfs_finobt_buf_ops = {
+ .name = "xfs_finobt",
+ .verify_read = xfs_inobt_read_verify,
+ .verify_write = xfs_inobt_write_verify,
+ .verify_struct = xfs_inobt_verify,
+};
+
STATIC int
xfs_inobt_keys_inorder(
struct xfs_btree_cur *cur,
@@ -389,7 +396,7 @@ static const struct xfs_btree_ops xfs_finobt_ops = {
.init_rec_from_cur = xfs_inobt_init_rec_from_cur,
.init_ptr_from_cur = xfs_finobt_init_ptr_from_cur,
.key_diff = xfs_inobt_key_diff,
- .buf_ops = &xfs_inobt_buf_ops,
+ .buf_ops = &xfs_finobt_buf_ops,
.diff_two_keys = xfs_inobt_diff_two_keys,
.keys_inorder = xfs_inobt_keys_inorder,
.recs_inorder = xfs_inobt_recs_inorder,
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 1c5debe748f0..9855f4d2f98f 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -36,6 +36,7 @@ extern const struct xfs_buf_ops xfs_dquot_buf_ops;
extern const struct xfs_buf_ops xfs_symlink_buf_ops;
extern const struct xfs_buf_ops xfs_agi_buf_ops;
extern const struct xfs_buf_ops xfs_inobt_buf_ops;
+extern const struct xfs_buf_ops xfs_finobt_buf_ops;
extern const struct xfs_buf_ops xfs_inode_buf_ops;
extern const struct xfs_buf_ops xfs_inode_buf_ra_ops;
extern const struct xfs_buf_ops xfs_dquot_buf_ops;
--git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 03d1e15cceba..673be3cf7b8d 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -879,7 +879,7 @@ xrep_agi(
},
[XREP_AGI_FINOBT] = {
.rmap_owner = XFS_RMAP_OWN_INOBT,
- .buf_ops = &xfs_inobt_buf_ops,
+ .buf_ops = &xfs_finobt_buf_ops,
.magic = XFS_FIBT_CRC_MAGIC,
},
[XREP_AGI_END] = {
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 9fe88d125f0a..228c754bb137 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2445,11 +2445,13 @@ xlog_recover_validate_buf_type(
bp->b_ops = &xfs_allocbt_buf_ops;
break;
case XFS_IBT_CRC_MAGIC:
- case XFS_FIBT_CRC_MAGIC:
case XFS_IBT_MAGIC:
- case XFS_FIBT_MAGIC:
bp->b_ops = &xfs_inobt_buf_ops;
break;
+ case XFS_FIBT_CRC_MAGIC:
+ case XFS_FIBT_MAGIC:
+ bp->b_ops = &xfs_finobt_buf_ops;
+ break;
case XFS_BMAP_CRC_MAGIC:
case XFS_BMAP_MAGIC:
bp->b_ops = &xfs_bmbt_buf_ops;
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 4/9] xfs: distinguish between inobt and finobt magic values
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (2 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 3/9] xfs: create a separate finobt verifier Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 18:12 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 5/9] xfs: split up allocation btree verifier Brian Foster
` (5 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
The inode btree verifier code is shared between the inode btree and
free inode btree because the underlying metadata formats are
essentially equivalent. A side effect of this is that the verifier
cannot determine whether a particular btree block should have an
inobt or finobt magic value.
This logic allows an unfortunate xfs_repair bug to escape detection
where certain level > 0 nodes of the finobt are stamped with inobt
magic by xfs_repair finobt reconstruction. This is fortunately not a
severe problem since the inode btree magic values do not contribute
to any changes in kernel behavior, but we do need a means to detect
and prevent this problem in the future.
Add a field to xfs_buf_ops to store the v4 and v5 superblock magic
values expected by a particular verifier. Add a helper to check an
on-disk magic value against the value expected by the verifier. Call
the helper from the shared [f]inobt verifier code for magic value
verification. This ensures that the inode btree blocks each have the
appropriate magic value based on specific tree type and superblock
version.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc_btree.c | 16 +++++++---------
fs/xfs/xfs_buf.c | 19 +++++++++++++++++++
fs/xfs/xfs_buf.h | 2 ++
3 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 798269eb4767..c2df1f89eec8 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -260,6 +260,9 @@ xfs_inobt_verify(
xfs_failaddr_t fa;
unsigned int level;
+ if (!xfs_verify_magic(bp, block->bb_magic))
+ return __this_address;
+
/*
* During growfs operations, we can't verify the exact owner as the
* perag is not fully initialised and hence not attached to the buffer.
@@ -270,18 +273,10 @@ xfs_inobt_verify(
* but beware of the landmine (i.e. need to check pag->pagi_init) if we
* ever do.
*/
- switch (block->bb_magic) {
- case cpu_to_be32(XFS_IBT_CRC_MAGIC):
- case cpu_to_be32(XFS_FIBT_CRC_MAGIC):
+ if (xfs_sb_version_hascrc(&mp->m_sb)) {
fa = xfs_btree_sblock_v5hdr_verify(bp);
if (fa)
return fa;
- /* fall through */
- case cpu_to_be32(XFS_IBT_MAGIC):
- case cpu_to_be32(XFS_FIBT_MAGIC):
- break;
- default:
- return __this_address;
}
/* level verification */
@@ -328,6 +323,7 @@ xfs_inobt_write_verify(
const struct xfs_buf_ops xfs_inobt_buf_ops = {
.name = "xfs_inobt",
+ .magic = { cpu_to_be32(XFS_IBT_MAGIC), cpu_to_be32(XFS_IBT_CRC_MAGIC) },
.verify_read = xfs_inobt_read_verify,
.verify_write = xfs_inobt_write_verify,
.verify_struct = xfs_inobt_verify,
@@ -335,6 +331,8 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
const struct xfs_buf_ops xfs_finobt_buf_ops = {
.name = "xfs_finobt",
+ .magic = { cpu_to_be32(XFS_FIBT_MAGIC),
+ cpu_to_be32(XFS_FIBT_CRC_MAGIC) },
.verify_read = xfs_inobt_read_verify,
.verify_write = xfs_inobt_write_verify,
.verify_struct = xfs_inobt_verify,
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 222b5260ed35..0481c19fe5ae 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2204,3 +2204,22 @@ void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
atomic_set(&bp->b_lru_ref, lru_ref);
}
+
+/*
+ * Verify an on-disk magic value against the magic value specified in the
+ * verifier structure. The verifier magic is in disk byte order so the caller is
+ * expected to pass the value directly from disk.
+ */
+bool
+xfs_verify_magic(
+ struct xfs_buf *bp,
+ uint32_t dmagic)
+{
+ struct xfs_mount *mp = bp->b_target->bt_mount;
+ int idx;
+
+ idx = xfs_sb_version_hascrc(&mp->m_sb);
+ if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
+ return false;
+ return dmagic == bp->b_ops->magic[idx];
+}
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 1c436a85b71d..44f9423a1861 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -125,6 +125,7 @@ struct xfs_buf_map {
struct xfs_buf_ops {
char *name;
+ uint32_t magic[2]; /* v4 and v5 on disk magic values */
void (*verify_read)(struct xfs_buf *);
void (*verify_write)(struct xfs_buf *);
xfs_failaddr_t (*verify_struct)(struct xfs_buf *bp);
@@ -386,5 +387,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
#define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
+bool xfs_verify_magic(struct xfs_buf *bp, uint32_t dmagic);
#endif /* __XFS_BUF_H__ */
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 5/9] xfs: split up allocation btree verifier
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (3 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 4/9] xfs: distinguish between inobt and finobt magic values Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 18:15 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 6/9] xfs: distinguish between bnobt and cntbt magic values Brian Foster
` (4 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
Similar to the inode btree verifier, the same allocation btree
verifier structure is shared between the by-bno (bnobt) and by-size
(cntbt) btrees. This prevents the ability to distinguish magic
values between them. Separate the verifier into two, one for each
tree, and assign them appropriately. No functional changes.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_ag.c | 4 ++--
fs/xfs/libxfs/xfs_alloc_btree.c | 14 ++++++++++----
fs/xfs/libxfs/xfs_shared.h | 3 ++-
| 4 ++--
fs/xfs/xfs_log_recover.c | 6 ++++--
5 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index bde67ef3ff43..1ef8acf35e7d 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -339,14 +339,14 @@ xfs_ag_init_headers(
{ /* BNO root block */
.daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_BNO_BLOCK(mp)),
.numblks = BTOBB(mp->m_sb.sb_blocksize),
- .ops = &xfs_allocbt_buf_ops,
+ .ops = &xfs_bnobt_buf_ops,
.work = &xfs_bnoroot_init,
.need_init = true
},
{ /* CNT root block */
.daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_CNT_BLOCK(mp)),
.numblks = BTOBB(mp->m_sb.sb_blocksize),
- .ops = &xfs_allocbt_buf_ops,
+ .ops = &xfs_cntbt_buf_ops,
.work = &xfs_cntroot_init,
.need_init = true
},
diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 4e59cc8a2802..480d0f52da64 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -377,13 +377,19 @@ xfs_allocbt_write_verify(
}
-const struct xfs_buf_ops xfs_allocbt_buf_ops = {
- .name = "xfs_allocbt",
+const struct xfs_buf_ops xfs_bnobt_buf_ops = {
+ .name = "xfs_bnobt",
.verify_read = xfs_allocbt_read_verify,
.verify_write = xfs_allocbt_write_verify,
.verify_struct = xfs_allocbt_verify,
};
+const struct xfs_buf_ops xfs_cntbt_buf_ops = {
+ .name = "xfs_cntbt",
+ .verify_read = xfs_allocbt_read_verify,
+ .verify_write = xfs_allocbt_write_verify,
+ .verify_struct = xfs_allocbt_verify,
+};
STATIC int
xfs_bnobt_keys_inorder(
@@ -448,7 +454,7 @@ static const struct xfs_btree_ops xfs_bnobt_ops = {
.init_rec_from_cur = xfs_allocbt_init_rec_from_cur,
.init_ptr_from_cur = xfs_allocbt_init_ptr_from_cur,
.key_diff = xfs_bnobt_key_diff,
- .buf_ops = &xfs_allocbt_buf_ops,
+ .buf_ops = &xfs_bnobt_buf_ops,
.diff_two_keys = xfs_bnobt_diff_two_keys,
.keys_inorder = xfs_bnobt_keys_inorder,
.recs_inorder = xfs_bnobt_recs_inorder,
@@ -470,7 +476,7 @@ static const struct xfs_btree_ops xfs_cntbt_ops = {
.init_rec_from_cur = xfs_allocbt_init_rec_from_cur,
.init_ptr_from_cur = xfs_allocbt_init_ptr_from_cur,
.key_diff = xfs_cntbt_key_diff,
- .buf_ops = &xfs_allocbt_buf_ops,
+ .buf_ops = &xfs_cntbt_buf_ops,
.diff_two_keys = xfs_cntbt_diff_two_keys,
.keys_inorder = xfs_cntbt_keys_inorder,
.recs_inorder = xfs_cntbt_recs_inorder,
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 9855f4d2f98f..4e909791aeac 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -25,7 +25,8 @@ extern const struct xfs_buf_ops xfs_agf_buf_ops;
extern const struct xfs_buf_ops xfs_agi_buf_ops;
extern const struct xfs_buf_ops xfs_agf_buf_ops;
extern const struct xfs_buf_ops xfs_agfl_buf_ops;
-extern const struct xfs_buf_ops xfs_allocbt_buf_ops;
+extern const struct xfs_buf_ops xfs_bnobt_buf_ops;
+extern const struct xfs_buf_ops xfs_cntbt_buf_ops;
extern const struct xfs_buf_ops xfs_rmapbt_buf_ops;
extern const struct xfs_buf_ops xfs_refcountbt_buf_ops;
extern const struct xfs_buf_ops xfs_attr3_leaf_buf_ops;
--git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 673be3cf7b8d..2799d1531639 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -341,12 +341,12 @@ xrep_agf(
struct xrep_find_ag_btree fab[XREP_AGF_MAX] = {
[XREP_AGF_BNOBT] = {
.rmap_owner = XFS_RMAP_OWN_AG,
- .buf_ops = &xfs_allocbt_buf_ops,
+ .buf_ops = &xfs_bnobt_buf_ops,
.magic = XFS_ABTB_CRC_MAGIC,
},
[XREP_AGF_CNTBT] = {
.rmap_owner = XFS_RMAP_OWN_AG,
- .buf_ops = &xfs_allocbt_buf_ops,
+ .buf_ops = &xfs_cntbt_buf_ops,
.magic = XFS_ABTC_CRC_MAGIC,
},
[XREP_AGF_RMAPBT] = {
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 228c754bb137..5ad42d598333 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2439,10 +2439,12 @@ xlog_recover_validate_buf_type(
case XFS_BLFT_BTREE_BUF:
switch (magic32) {
case XFS_ABTB_CRC_MAGIC:
- case XFS_ABTC_CRC_MAGIC:
case XFS_ABTB_MAGIC:
+ bp->b_ops = &xfs_bnobt_buf_ops;
+ break;
+ case XFS_ABTC_CRC_MAGIC:
case XFS_ABTC_MAGIC:
- bp->b_ops = &xfs_allocbt_buf_ops;
+ bp->b_ops = &xfs_cntbt_buf_ops;
break;
case XFS_IBT_CRC_MAGIC:
case XFS_IBT_MAGIC:
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 6/9] xfs: distinguish between bnobt and cntbt magic values
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (4 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 5/9] xfs: split up allocation btree verifier Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 18:15 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers Brian Foster
` (3 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
The allocation btree verifiers share code that is unable to detect
cross-tree magic value corruptions such as a bnobt block with a
cntbt magic value. Populate the b_ops->magic field of the associated
verifier structures such that the structure verifier can check the
magic value against the expected value based on tree type.
The btree level check requires knowledge of the tree type to
determine the appropriate maximum value. This was previously part of
the hardcoded magic value checks. With that code removed, peek at
the first magic value in the verifier to determine the expected tree
type of the current block.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_alloc_btree.c | 60 ++++++++++++++-------------------
1 file changed, 25 insertions(+), 35 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 480d0f52da64..9fe949f6055e 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -297,48 +297,34 @@ xfs_allocbt_verify(
struct xfs_perag *pag = bp->b_pag;
xfs_failaddr_t fa;
unsigned int level;
+ xfs_btnum_t btnum = XFS_BTNUM_BNOi;
+
+ if (!xfs_verify_magic(bp, block->bb_magic))
+ return __this_address;
+
+ if (xfs_sb_version_hascrc(&mp->m_sb)) {
+ fa = xfs_btree_sblock_v5hdr_verify(bp);
+ if (fa)
+ return fa;
+ }
/*
- * magic number and level verification
- *
- * During growfs operations, we can't verify the exact level or owner as
- * the perag is not fully initialised and hence not attached to the
- * buffer. In this case, check against the maximum tree depth.
+ * The perag may not be attached during grow operations or fully
+ * initialized from the AGF during log recovery. Therefore we can only
+ * check against maximum tree depth from those contexts.
*
- * Similarly, during log recovery we will have a perag structure
- * attached, but the agf information will not yet have been initialised
- * from the on disk AGF. Again, we can only check against maximum limits
- * in this case.
+ * Otherwise check against the per-tree limit. Peek at one of the
+ * verifier magic values to determine the type of tree we're verifying
+ * against.
*/
level = be16_to_cpu(block->bb_level);
- switch (block->bb_magic) {
- case cpu_to_be32(XFS_ABTB_CRC_MAGIC):
- fa = xfs_btree_sblock_v5hdr_verify(bp);
- if (fa)
- return fa;
- /* fall through */
- case cpu_to_be32(XFS_ABTB_MAGIC):
- if (pag && pag->pagf_init) {
- if (level >= pag->pagf_levels[XFS_BTNUM_BNOi])
- return __this_address;
- } else if (level >= mp->m_ag_maxlevels)
+ if (bp->b_ops->magic[0] == cpu_to_be32(XFS_ABTC_MAGIC))
+ btnum = XFS_BTNUM_CNTi;
+ if (pag && pag->pagf_init) {
+ if (level >= pag->pagf_levels[btnum])
return __this_address;
- break;
- case cpu_to_be32(XFS_ABTC_CRC_MAGIC):
- fa = xfs_btree_sblock_v5hdr_verify(bp);
- if (fa)
- return fa;
- /* fall through */
- case cpu_to_be32(XFS_ABTC_MAGIC):
- if (pag && pag->pagf_init) {
- if (level >= pag->pagf_levels[XFS_BTNUM_CNTi])
- return __this_address;
- } else if (level >= mp->m_ag_maxlevels)
- return __this_address;
- break;
- default:
+ } else if (level >= mp->m_ag_maxlevels)
return __this_address;
- }
return xfs_btree_sblock_verify(bp, mp->m_alloc_mxr[level != 0]);
}
@@ -379,6 +365,8 @@ xfs_allocbt_write_verify(
const struct xfs_buf_ops xfs_bnobt_buf_ops = {
.name = "xfs_bnobt",
+ .magic = { cpu_to_be32(XFS_ABTB_MAGIC),
+ cpu_to_be32(XFS_ABTB_CRC_MAGIC) },
.verify_read = xfs_allocbt_read_verify,
.verify_write = xfs_allocbt_write_verify,
.verify_struct = xfs_allocbt_verify,
@@ -386,6 +374,8 @@ const struct xfs_buf_ops xfs_bnobt_buf_ops = {
const struct xfs_buf_ops xfs_cntbt_buf_ops = {
.name = "xfs_cntbt",
+ .magic = { cpu_to_be32(XFS_ABTC_MAGIC),
+ cpu_to_be32(XFS_ABTC_CRC_MAGIC) },
.verify_read = xfs_allocbt_read_verify,
.verify_write = xfs_allocbt_write_verify,
.verify_struct = xfs_allocbt_verify,
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (5 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 6/9] xfs: distinguish between bnobt and cntbt magic values Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 17:45 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 8/9] xfs: miscellaneous verifier magic value fixups Brian Foster
` (2 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
The dir2 leaf verifiers share the same underlying structure
verification code, but implement six accessor functions to multiplex
the code across the two verifiers. Further, the magic value isn't
sufficiently abstracted such that the common helper has to manually
fix up the magic from the caller on v5 filesystems.
Use the magic field in the verifier structure to eliminate the
duplicate code and clean this all up. No functional change.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
1 file changed, 20 insertions(+), 68 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 1728a3e6f5cf..a99ae2cd292e 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
*/
static xfs_failaddr_t
xfs_dir3_leaf_verify(
- struct xfs_buf *bp,
- uint16_t magic)
+ struct xfs_buf *bp)
{
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_dir2_leaf *leaf = bp->b_addr;
- ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
+ if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
+ return __this_address;
if (xfs_sb_version_hascrc(&mp->m_sb)) {
struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
- uint16_t magic3;
- magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
- : XFS_DIR3_LEAFN_MAGIC;
-
- if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
- return __this_address;
+ ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
return __this_address;
if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
return __this_address;
- } else {
- if (leaf->hdr.info.magic != cpu_to_be16(magic))
- return __this_address;
}
return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
}
static void
-__read_verify(
- struct xfs_buf *bp,
- uint16_t magic)
+xfs_dir3_leaf_read_verify(
+ struct xfs_buf *bp)
{
struct xfs_mount *mp = bp->b_target->bt_mount;
xfs_failaddr_t fa;
@@ -185,23 +176,22 @@ __read_verify(
!xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
xfs_verifier_error(bp, -EFSBADCRC, __this_address);
else {
- fa = xfs_dir3_leaf_verify(bp, magic);
+ fa = xfs_dir3_leaf_verify(bp);
if (fa)
xfs_verifier_error(bp, -EFSCORRUPTED, fa);
}
}
static void
-__write_verify(
- struct xfs_buf *bp,
- uint16_t magic)
+xfs_dir3_leaf_write_verify(
+ struct xfs_buf *bp)
{
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_buf_log_item *bip = bp->b_log_item;
struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
xfs_failaddr_t fa;
- fa = xfs_dir3_leaf_verify(bp, magic);
+ fa = xfs_dir3_leaf_verify(bp);
if (fa) {
xfs_verifier_error(bp, -EFSCORRUPTED, fa);
return;
@@ -216,60 +206,22 @@ __write_verify(
xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
}
-static xfs_failaddr_t
-xfs_dir3_leaf1_verify(
- struct xfs_buf *bp)
-{
- return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
-}
-
-static void
-xfs_dir3_leaf1_read_verify(
- struct xfs_buf *bp)
-{
- __read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
-}
-
-static void
-xfs_dir3_leaf1_write_verify(
- struct xfs_buf *bp)
-{
- __write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
-}
-
-static xfs_failaddr_t
-xfs_dir3_leafn_verify(
- struct xfs_buf *bp)
-{
- return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
-}
-
-static void
-xfs_dir3_leafn_read_verify(
- struct xfs_buf *bp)
-{
- __read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
-}
-
-static void
-xfs_dir3_leafn_write_verify(
- struct xfs_buf *bp)
-{
- __write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
-}
-
const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
.name = "xfs_dir3_leaf1",
- .verify_read = xfs_dir3_leaf1_read_verify,
- .verify_write = xfs_dir3_leaf1_write_verify,
- .verify_struct = xfs_dir3_leaf1_verify,
+ .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
+ cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
+ .verify_read = xfs_dir3_leaf_read_verify,
+ .verify_write = xfs_dir3_leaf_write_verify,
+ .verify_struct = xfs_dir3_leaf_verify,
};
const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
.name = "xfs_dir3_leafn",
- .verify_read = xfs_dir3_leafn_read_verify,
- .verify_write = xfs_dir3_leafn_write_verify,
- .verify_struct = xfs_dir3_leafn_verify,
+ .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
+ cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
+ .verify_read = xfs_dir3_leaf_read_verify,
+ .verify_write = xfs_dir3_leaf_write_verify,
+ .verify_struct = xfs_dir3_leaf_verify,
};
int
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 8/9] xfs: miscellaneous verifier magic value fixups
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (6 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 17:52 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 9/9] xfs: factor xfs_da3_blkinfo verification into common helper Brian Foster
2019-02-06 18:23 ` [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Darrick J. Wong
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
Most buffer verifiers have hardcoded magic value checks
conditionalized on the version of the filesystem. The magic value
field of the verifier structure facilitates abstraction of some of
this code. Populate the ->magic field of various verifiers to take
advantage of this abstraction. No functional changes.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_alloc.c | 12 ++++++++----
fs/xfs/libxfs/xfs_attr_leaf.c | 12 ++++++------
fs/xfs/libxfs/xfs_attr_remote.c | 8 +++++---
fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++++++-------
fs/xfs/libxfs/xfs_da_btree.c | 12 ++++++------
fs/xfs/libxfs/xfs_dir2_block.c | 10 +++++-----
fs/xfs/libxfs/xfs_dir2_data.c | 12 +++++++-----
fs/xfs/libxfs/xfs_dir2_node.c | 11 ++++++-----
fs/xfs/libxfs/xfs_ialloc.c | 3 ++-
fs/xfs/libxfs/xfs_refcount_btree.c | 3 ++-
fs/xfs/libxfs/xfs_rmap_btree.c | 3 ++-
fs/xfs/libxfs/xfs_sb.c | 4 +++-
fs/xfs/libxfs/xfs_symlink_remote.c | 3 ++-
13 files changed, 60 insertions(+), 46 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 48aab07e7138..bc3367b8b7bb 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -568,9 +568,9 @@ xfs_agfl_verify(
if (!xfs_sb_version_hascrc(&mp->m_sb))
return NULL;
- if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
+ if (!xfs_verify_magic(bp, agfl->agfl_magicnum))
return __this_address;
- if (agfl->agfl_magicnum != cpu_to_be32(XFS_AGFL_MAGIC))
+ if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
/*
* during growfs operations, the perag is not fully initialised,
@@ -643,6 +643,7 @@ xfs_agfl_write_verify(
const struct xfs_buf_ops xfs_agfl_buf_ops = {
.name = "xfs_agfl",
+ .magic = { cpu_to_be32(XFS_AGFL_MAGIC), cpu_to_be32(XFS_AGFL_MAGIC) },
.verify_read = xfs_agfl_read_verify,
.verify_write = xfs_agfl_write_verify,
.verify_struct = xfs_agfl_verify,
@@ -2587,8 +2588,10 @@ xfs_agf_verify(
return __this_address;
}
- if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) &&
- XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
+ if (!xfs_verify_magic(bp, agf->agf_magicnum))
+ return __this_address;
+
+ if (!(XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) &&
be32_to_cpu(agf->agf_flfirst) < xfs_agfl_size(mp) &&
be32_to_cpu(agf->agf_fllast) < xfs_agfl_size(mp) &&
@@ -2670,6 +2673,7 @@ xfs_agf_write_verify(
const struct xfs_buf_ops xfs_agf_buf_ops = {
.name = "xfs_agf",
+ .magic = { cpu_to_be32(XFS_AGF_MAGIC), cpu_to_be32(XFS_AGF_MAGIC) },
.verify_read = xfs_agf_read_verify,
.verify_write = xfs_agf_write_verify,
.verify_struct = xfs_agf_verify,
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index e60eba7f129c..dde3a550d7ce 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -248,21 +248,19 @@ xfs_attr3_leaf_verify(
xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
+ if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
+ return __this_address;
+
if (xfs_sb_version_hascrc(&mp->m_sb)) {
struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
- if (hdr3->info.hdr.magic != cpu_to_be16(XFS_ATTR3_LEAF_MAGIC))
- return __this_address;
-
+ ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
return __this_address;
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
return __this_address;
- } else {
- if (leaf->hdr.info.magic != cpu_to_be16(XFS_ATTR_LEAF_MAGIC))
- return __this_address;
}
/*
* In recovery there is a transient state where count == 0 is valid
@@ -369,6 +367,8 @@ xfs_attr3_leaf_read_verify(
const struct xfs_buf_ops xfs_attr3_leaf_buf_ops = {
.name = "xfs_attr3_leaf",
+ .magic = { cpu_to_be16(XFS_ATTR_LEAF_MAGIC),
+ cpu_to_be16(XFS_ATTR3_LEAF_MAGIC) },
.verify_read = xfs_attr3_leaf_read_verify,
.verify_write = xfs_attr3_leaf_write_verify,
.verify_struct = xfs_attr3_leaf_verify,
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index d89363c6b523..65ff600a8067 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -79,6 +79,7 @@ xfs_attr3_rmt_hdr_ok(
static xfs_failaddr_t
xfs_attr3_rmt_verify(
struct xfs_mount *mp,
+ struct xfs_buf *bp,
void *ptr,
int fsbsize,
xfs_daddr_t bno)
@@ -87,7 +88,7 @@ xfs_attr3_rmt_verify(
if (!xfs_sb_version_hascrc(&mp->m_sb))
return __this_address;
- if (rmt->rm_magic != cpu_to_be32(XFS_ATTR3_RMT_MAGIC))
+ if (!xfs_verify_magic(bp, rmt->rm_magic))
return __this_address;
if (!uuid_equal(&rmt->rm_uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
@@ -131,7 +132,7 @@ __xfs_attr3_rmt_read_verify(
*failaddr = __this_address;
return -EFSBADCRC;
}
- *failaddr = xfs_attr3_rmt_verify(mp, ptr, blksize, bno);
+ *failaddr = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
if (*failaddr)
return -EFSCORRUPTED;
len -= blksize;
@@ -193,7 +194,7 @@ xfs_attr3_rmt_write_verify(
while (len > 0) {
struct xfs_attr3_rmt_hdr *rmt = (struct xfs_attr3_rmt_hdr *)ptr;
- fa = xfs_attr3_rmt_verify(mp, ptr, blksize, bno);
+ fa = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
if (fa) {
xfs_verifier_error(bp, -EFSCORRUPTED, fa);
return;
@@ -220,6 +221,7 @@ xfs_attr3_rmt_write_verify(
const struct xfs_buf_ops xfs_attr3_rmt_buf_ops = {
.name = "xfs_attr3_rmt",
+ .magic = { 0, cpu_to_be32(XFS_ATTR3_RMT_MAGIC) },
.verify_read = xfs_attr3_rmt_read_verify,
.verify_write = xfs_attr3_rmt_write_verify,
.verify_struct = xfs_attr3_rmt_verify_struct,
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index cdb74d2e2a43..aff82ed112c9 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -416,8 +416,10 @@ xfs_bmbt_verify(
xfs_failaddr_t fa;
unsigned int level;
- switch (block->bb_magic) {
- case cpu_to_be32(XFS_BMAP_CRC_MAGIC):
+ if (!xfs_verify_magic(bp, block->bb_magic))
+ return __this_address;
+
+ if (xfs_sb_version_hascrc(&mp->m_sb)) {
/*
* XXX: need a better way of verifying the owner here. Right now
* just make sure there has been one set.
@@ -425,11 +427,6 @@ xfs_bmbt_verify(
fa = xfs_btree_lblock_v5hdr_verify(bp, XFS_RMAP_OWN_UNKNOWN);
if (fa)
return fa;
- /* fall through */
- case cpu_to_be32(XFS_BMAP_MAGIC):
- break;
- default:
- return __this_address;
}
/*
@@ -481,6 +478,8 @@ xfs_bmbt_write_verify(
const struct xfs_buf_ops xfs_bmbt_buf_ops = {
.name = "xfs_bmbt",
+ .magic = { cpu_to_be32(XFS_BMAP_MAGIC),
+ cpu_to_be32(XFS_BMAP_CRC_MAGIC) },
.verify_read = xfs_bmbt_read_verify,
.verify_write = xfs_bmbt_write_verify,
.verify_struct = xfs_bmbt_verify,
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 355322688c9f..6b8ca390eecc 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -129,21 +129,19 @@ xfs_da3_node_verify(
ops->node_hdr_from_disk(&ichdr, hdr);
+ if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
+ return __this_address;
+
if (xfs_sb_version_hascrc(&mp->m_sb)) {
struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
- if (hdr3->info.hdr.magic != cpu_to_be16(XFS_DA3_NODE_MAGIC))
- return __this_address;
-
+ ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
return __this_address;
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
return __this_address;
- } else {
- if (hdr->hdr.info.magic != cpu_to_be16(XFS_DA_NODE_MAGIC))
- return __this_address;
}
if (ichdr.level == 0)
return __this_address;
@@ -257,6 +255,8 @@ xfs_da3_node_verify_struct(
const struct xfs_buf_ops xfs_da3_node_buf_ops = {
.name = "xfs_da3_node",
+ .magic = { cpu_to_be16(XFS_DA_NODE_MAGIC),
+ cpu_to_be16(XFS_DA3_NODE_MAGIC) },
.verify_read = xfs_da3_node_read_verify,
.verify_write = xfs_da3_node_write_verify,
.verify_struct = xfs_da3_node_verify_struct,
diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 30ed5919da72..b7d6d78f4ce2 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -53,18 +53,16 @@ xfs_dir3_block_verify(
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
+ if (!xfs_verify_magic(bp, hdr3->magic))
+ return __this_address;
+
if (xfs_sb_version_hascrc(&mp->m_sb)) {
- if (hdr3->magic != cpu_to_be32(XFS_DIR3_BLOCK_MAGIC))
- return __this_address;
if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
return __this_address;
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
return __this_address;
- } else {
- if (hdr3->magic != cpu_to_be32(XFS_DIR2_BLOCK_MAGIC))
- return __this_address;
}
return __xfs_dir3_data_check(NULL, bp);
}
@@ -112,6 +110,8 @@ xfs_dir3_block_write_verify(
const struct xfs_buf_ops xfs_dir3_block_buf_ops = {
.name = "xfs_dir3_block",
+ .magic = { cpu_to_be32(XFS_DIR2_BLOCK_MAGIC),
+ cpu_to_be32(XFS_DIR3_BLOCK_MAGIC) },
.verify_read = xfs_dir3_block_read_verify,
.verify_write = xfs_dir3_block_write_verify,
.verify_struct = xfs_dir3_block_verify,
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 01162c62ec8f..b7b9ce002cb9 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -252,18 +252,16 @@ xfs_dir3_data_verify(
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
+ if (!xfs_verify_magic(bp, hdr3->magic))
+ return __this_address;
+
if (xfs_sb_version_hascrc(&mp->m_sb)) {
- if (hdr3->magic != cpu_to_be32(XFS_DIR3_DATA_MAGIC))
- return __this_address;
if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
return __this_address;
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
return __this_address;
- } else {
- if (hdr3->magic != cpu_to_be32(XFS_DIR2_DATA_MAGIC))
- return __this_address;
}
return __xfs_dir3_data_check(NULL, bp);
}
@@ -339,6 +337,8 @@ xfs_dir3_data_write_verify(
const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
.name = "xfs_dir3_data",
+ .magic = { cpu_to_be32(XFS_DIR2_DATA_MAGIC),
+ cpu_to_be32(XFS_DIR3_DATA_MAGIC) },
.verify_read = xfs_dir3_data_read_verify,
.verify_write = xfs_dir3_data_write_verify,
.verify_struct = xfs_dir3_data_verify,
@@ -346,6 +346,8 @@ const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
static const struct xfs_buf_ops xfs_dir3_data_reada_buf_ops = {
.name = "xfs_dir3_data_reada",
+ .magic = { cpu_to_be32(XFS_DIR2_DATA_MAGIC),
+ cpu_to_be32(XFS_DIR3_DATA_MAGIC) },
.verify_read = xfs_dir3_data_reada_verify,
.verify_write = xfs_dir3_data_write_verify,
};
diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index f1bb3434f51c..9ccc8819e99f 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -87,20 +87,19 @@ xfs_dir3_free_verify(
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_dir2_free_hdr *hdr = bp->b_addr;
+ if (!xfs_verify_magic(bp, hdr->magic))
+ return __this_address;
+
if (xfs_sb_version_hascrc(&mp->m_sb)) {
struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
- if (hdr3->magic != cpu_to_be32(XFS_DIR3_FREE_MAGIC))
- return __this_address;
+ ASSERT(hdr3->magic == hdr->magic);
if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
return __this_address;
if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
return __this_address;
- } else {
- if (hdr->magic != cpu_to_be32(XFS_DIR2_FREE_MAGIC))
- return __this_address;
}
/* XXX: should bounds check the xfs_dir3_icfree_hdr here */
@@ -151,6 +150,8 @@ xfs_dir3_free_write_verify(
const struct xfs_buf_ops xfs_dir3_free_buf_ops = {
.name = "xfs_dir3_free",
+ .magic = { cpu_to_be32(XFS_DIR2_FREE_MAGIC),
+ cpu_to_be32(XFS_DIR3_FREE_MAGIC) },
.verify_read = xfs_dir3_free_read_verify,
.verify_write = xfs_dir3_free_write_verify,
.verify_struct = xfs_dir3_free_verify,
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index d32152fc8a6c..fe9898875097 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2508,7 +2508,7 @@ xfs_agi_verify(
/*
* Validate the magic number of the agi block.
*/
- if (agi->agi_magicnum != cpu_to_be32(XFS_AGI_MAGIC))
+ if (!xfs_verify_magic(bp, agi->agi_magicnum))
return __this_address;
if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum)))
return __this_address;
@@ -2582,6 +2582,7 @@ xfs_agi_write_verify(
const struct xfs_buf_ops xfs_agi_buf_ops = {
.name = "xfs_agi",
+ .magic = { cpu_to_be32(XFS_AGI_MAGIC), cpu_to_be32(XFS_AGI_MAGIC) },
.verify_read = xfs_agi_read_verify,
.verify_write = xfs_agi_write_verify,
.verify_struct = xfs_agi_verify,
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
index d9eab657b63e..6f47ab876d90 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -209,7 +209,7 @@ xfs_refcountbt_verify(
xfs_failaddr_t fa;
unsigned int level;
- if (block->bb_magic != cpu_to_be32(XFS_REFC_CRC_MAGIC))
+ if (!xfs_verify_magic(bp, block->bb_magic))
return __this_address;
if (!xfs_sb_version_hasreflink(&mp->m_sb))
@@ -264,6 +264,7 @@ xfs_refcountbt_write_verify(
const struct xfs_buf_ops xfs_refcountbt_buf_ops = {
.name = "xfs_refcountbt",
+ .magic = { 0, cpu_to_be32(XFS_REFC_CRC_MAGIC) },
.verify_read = xfs_refcountbt_read_verify,
.verify_write = xfs_refcountbt_write_verify,
.verify_struct = xfs_refcountbt_verify,
diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index f79cf040d745..5738e11055e6 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -310,7 +310,7 @@ xfs_rmapbt_verify(
* from the on disk AGF. Again, we can only check against maximum limits
* in this case.
*/
- if (block->bb_magic != cpu_to_be32(XFS_RMAP_CRC_MAGIC))
+ if (!xfs_verify_magic(bp, block->bb_magic))
return __this_address;
if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
@@ -365,6 +365,7 @@ xfs_rmapbt_write_verify(
const struct xfs_buf_ops xfs_rmapbt_buf_ops = {
.name = "xfs_rmapbt",
+ .magic = { 0, cpu_to_be32(XFS_RMAP_CRC_MAGIC) },
.verify_read = xfs_rmapbt_read_verify,
.verify_write = xfs_rmapbt_write_verify,
.verify_struct = xfs_rmapbt_verify,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index a2f52a958091..4e5029c37966 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -229,7 +229,7 @@ xfs_validate_sb_common(
uint32_t agcount = 0;
uint32_t rem;
- if (dsb->sb_magicnum != cpu_to_be32(XFS_SB_MAGIC)) {
+ if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
xfs_warn(mp, "bad magic number");
return -EWRONGFS;
}
@@ -782,12 +782,14 @@ xfs_sb_write_verify(
const struct xfs_buf_ops xfs_sb_buf_ops = {
.name = "xfs_sb",
+ .magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
.verify_read = xfs_sb_read_verify,
.verify_write = xfs_sb_write_verify,
};
const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
.name = "xfs_sb_quiet",
+ .magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
.verify_read = xfs_sb_quiet_read_verify,
.verify_write = xfs_sb_write_verify,
};
diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
index 77d80106f989..a0ccc253c43d 100644
--- a/fs/xfs/libxfs/xfs_symlink_remote.c
+++ b/fs/xfs/libxfs/xfs_symlink_remote.c
@@ -95,7 +95,7 @@ xfs_symlink_verify(
if (!xfs_sb_version_hascrc(&mp->m_sb))
return __this_address;
- if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
+ if (!xfs_verify_magic(bp, dsl->sl_magic))
return __this_address;
if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
return __this_address;
@@ -159,6 +159,7 @@ xfs_symlink_write_verify(
const struct xfs_buf_ops xfs_symlink_buf_ops = {
.name = "xfs_symlink",
+ .magic = { 0, cpu_to_be32(XFS_SYMLINK_MAGIC) },
.verify_read = xfs_symlink_read_verify,
.verify_write = xfs_symlink_write_verify,
.verify_struct = xfs_symlink_verify,
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 9/9] xfs: factor xfs_da3_blkinfo verification into common helper
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (7 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 8/9] xfs: miscellaneous verifier magic value fixups Brian Foster
@ 2019-02-04 14:52 ` Brian Foster
2019-02-06 18:16 ` Darrick J. Wong
2019-02-06 18:23 ` [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Darrick J. Wong
9 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-04 14:52 UTC (permalink / raw)
To: linux-xfs
With the verifier magic value helper in place, we've left a bit more
duplicate code across the verifiers that involve struct
xfs_da3_blkinfo. This includes the da node, xattr leaf and dir leaf
verifiers, all of which perform similar checks for v4 and v5
filesystems.
Create a common helper to verify an xfs_da3_blkinfo structure,
taking care to only access v5 fields where appropriate, and refactor
the aforementioned verifiers to use the helper. No functional
changes.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/libxfs/xfs_attr_leaf.c | 17 +++----------
fs/xfs/libxfs/xfs_da_btree.c | 46 +++++++++++++++++++++++++----------
fs/xfs/libxfs/xfs_da_format.h | 3 +++
fs/xfs/libxfs/xfs_dir2_leaf.c | 18 +++-----------
4 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index dde3a550d7ce..0c92987f57fc 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -245,23 +245,14 @@ xfs_attr3_leaf_verify(
struct xfs_attr_leaf_entry *entries;
uint32_t end; /* must be 32bit - see below */
int i;
+ xfs_failaddr_t fa;
xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
- if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
- return __this_address;
-
- if (xfs_sb_version_hascrc(&mp->m_sb)) {
- struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
+ fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
+ if (fa)
+ return fa;
- ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
- if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
- return __this_address;
- if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
- return __this_address;
- if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
- return __this_address;
- }
/*
* In recovery there is a transient state where count == 0 is valid
* because we may have transitioned an empty shortform attr to a leaf
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 6b8ca390eecc..622f88a1b1aa 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -116,6 +116,35 @@ xfs_da_state_free(xfs_da_state_t *state)
kmem_zone_free(xfs_da_state_zone, state);
}
+/*
+ * Verify an xfs_da3_blkinfo structure. Note that the da3 fields are only
+ * accessible on v5 filesystems. This header format is common across da node,
+ * attr leaf and dir leaf blocks.
+ */
+xfs_failaddr_t
+xfs_da3_blkinfo_verify(
+ struct xfs_buf *bp,
+ struct xfs_da3_blkinfo *hdr3)
+{
+ struct xfs_mount *mp = bp->b_target->bt_mount;
+ struct xfs_da_blkinfo *hdr = &hdr3->hdr;
+
+ if (!xfs_verify_magic(bp, hdr->magic))
+ return __this_address;
+
+ if (xfs_sb_version_hascrc(&mp->m_sb)) {
+ ASSERT(hdr3->hdr.magic == hdr->magic);
+ if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
+ return __this_address;
+ if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
+ return __this_address;
+ if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
+ return __this_address;
+ }
+
+ return 0;
+}
+
static xfs_failaddr_t
xfs_da3_node_verify(
struct xfs_buf *bp)
@@ -124,25 +153,16 @@ xfs_da3_node_verify(
struct xfs_da_intnode *hdr = bp->b_addr;
struct xfs_da3_icnode_hdr ichdr;
const struct xfs_dir_ops *ops;
+ xfs_failaddr_t fa;
ops = xfs_dir_get_ops(mp, NULL);
ops->node_hdr_from_disk(&ichdr, hdr);
- if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
- return __this_address;
+ fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
+ if (fa)
+ return fa;
- if (xfs_sb_version_hascrc(&mp->m_sb)) {
- struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
-
- ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
- if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
- return __this_address;
- if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
- return __this_address;
- if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
- return __this_address;
- }
if (ichdr.level == 0)
return __this_address;
if (ichdr.level > XFS_DA_NODE_MAXDEPTH)
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 5d5bf3bffc78..ae654e06b2fb 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -869,4 +869,7 @@ static inline unsigned int xfs_dir2_dirblock_bytes(struct xfs_sb *sbp)
return 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
}
+xfs_failaddr_t xfs_da3_blkinfo_verify(struct xfs_buf *bp,
+ struct xfs_da3_blkinfo *hdr3);
+
#endif /* __XFS_DA_FORMAT_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index a99ae2cd292e..094028b7b162 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -146,21 +146,11 @@ xfs_dir3_leaf_verify(
{
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_dir2_leaf *leaf = bp->b_addr;
+ xfs_failaddr_t fa;
- if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
- return __this_address;
-
- if (xfs_sb_version_hascrc(&mp->m_sb)) {
- struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
-
- ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
- if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
- return __this_address;
- if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
- return __this_address;
- if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
- return __this_address;
- }
+ fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
+ if (fa)
+ return fa;
return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
}
--
2.17.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 2/9] xfs: always check magic values in on-disk byte order
2019-02-04 14:52 ` [PATCH v3 2/9] xfs: always check magic values in on-disk byte order Brian Foster
@ 2019-02-06 17:08 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 17:08 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:24AM -0500, Brian Foster wrote:
> Most verifiers that check on-disk magic values convert the CPU
> endian magic value constant to disk endian to facilitate compile
> time optimization of the byte swap and reduce the need for runtime
> byte swaps in buffer verifiers. Several buffer verifiers do not
> follow this pattern. Update those verifiers for consistency.
>
> Also fix up a random typo in the inode readahead verifier name.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_alloc.c | 2 +-
> fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
> fs/xfs/libxfs/xfs_da_btree.c | 4 ++--
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_sb.c | 3 ++-
> 5 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index b715668886a4..48aab07e7138 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -570,7 +570,7 @@ xfs_agfl_verify(
>
> if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> - if (be32_to_cpu(agfl->agfl_magicnum) != XFS_AGFL_MAGIC)
> + if (agfl->agfl_magicnum != cpu_to_be32(XFS_AGFL_MAGIC))
> return __this_address;
> /*
> * during growfs operations, the perag is not fully initialised,
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 2652d00842d6..e60eba7f129c 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -251,7 +251,7 @@ xfs_attr3_leaf_verify(
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
>
> - if (ichdr.magic != XFS_ATTR3_LEAF_MAGIC)
> + if (hdr3->info.hdr.magic != cpu_to_be16(XFS_ATTR3_LEAF_MAGIC))
> return __this_address;
>
> if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> @@ -261,7 +261,7 @@ xfs_attr3_leaf_verify(
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> return __this_address;
> } else {
> - if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
> + if (leaf->hdr.info.magic != cpu_to_be16(XFS_ATTR_LEAF_MAGIC))
> return __this_address;
> }
> /*
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 376bee94b5dd..355322688c9f 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -132,7 +132,7 @@ xfs_da3_node_verify(
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
>
> - if (ichdr.magic != XFS_DA3_NODE_MAGIC)
> + if (hdr3->info.hdr.magic != cpu_to_be16(XFS_DA3_NODE_MAGIC))
> return __this_address;
>
> if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> @@ -142,7 +142,7 @@ xfs_da3_node_verify(
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> return __this_address;
> } else {
> - if (ichdr.magic != XFS_DA_NODE_MAGIC)
> + if (hdr->hdr.info.magic != cpu_to_be16(XFS_DA_NODE_MAGIC))
> return __this_address;
> }
> if (ichdr.level == 0)
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 09d9c8cfa4a0..fd2df5747a3a 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -152,7 +152,7 @@ const struct xfs_buf_ops xfs_inode_buf_ops = {
> };
>
> const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
> - .name = "xxfs_inode_ra",
> + .name = "xfs_inode_ra",
> .verify_read = xfs_inode_buf_readahead_verify,
> .verify_write = xfs_inode_buf_write_verify,
> };
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b5a82acd7dfe..a2f52a958091 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -225,10 +225,11 @@ xfs_validate_sb_common(
> struct xfs_buf *bp,
> struct xfs_sb *sbp)
> {
> + struct xfs_dsb *dsb = XFS_BUF_TO_SBP(bp);
> uint32_t agcount = 0;
> uint32_t rem;
>
> - if (sbp->sb_magicnum != XFS_SB_MAGIC) {
> + if (dsb->sb_magicnum != cpu_to_be32(XFS_SB_MAGIC)) {
> xfs_warn(mp, "bad magic number");
> return -EWRONGFS;
> }
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers
2019-02-04 14:52 ` [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers Brian Foster
@ 2019-02-06 17:45 ` Darrick J. Wong
2019-02-06 18:41 ` Brian Foster
0 siblings, 1 reply; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 17:45 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:29AM -0500, Brian Foster wrote:
> The dir2 leaf verifiers share the same underlying structure
> verification code, but implement six accessor functions to multiplex
> the code across the two verifiers. Further, the magic value isn't
> sufficiently abstracted such that the common helper has to manually
> fix up the magic from the caller on v5 filesystems.
>
> Use the magic field in the verifier structure to eliminate the
> duplicate code and clean this all up. No functional change.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
> 1 file changed, 20 insertions(+), 68 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 1728a3e6f5cf..a99ae2cd292e 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> */
> static xfs_failaddr_t
> xfs_dir3_leaf_verify(
> - struct xfs_buf *bp,
> - uint16_t magic)
> + struct xfs_buf *bp)
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_dir2_leaf *leaf = bp->b_addr;
>
> - ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> + if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> + return __this_address;
>
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> - uint16_t magic3;
>
> - magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> - : XFS_DIR3_LEAFN_MAGIC;
> -
> - if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> - return __this_address;
> + ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
leaf2 and leaf3 directory block headers are supposed to have the magic
at the same offset in the buffer, right? When would this assert fail?
Could we replace this with a BUILD_BUG_ON check of the leaf2/leaf3 magic
number buffer offset in xfs_ondisk.h?
--D
> if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> return __this_address;
> if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> return __this_address;
> - } else {
> - if (leaf->hdr.info.magic != cpu_to_be16(magic))
> - return __this_address;
> }
>
> return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> }
>
> static void
> -__read_verify(
> - struct xfs_buf *bp,
> - uint16_t magic)
> +xfs_dir3_leaf_read_verify(
> + struct xfs_buf *bp)
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
> xfs_failaddr_t fa;
> @@ -185,23 +176,22 @@ __read_verify(
> !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
> xfs_verifier_error(bp, -EFSBADCRC, __this_address);
> else {
> - fa = xfs_dir3_leaf_verify(bp, magic);
> + fa = xfs_dir3_leaf_verify(bp);
> if (fa)
> xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> }
> }
>
> static void
> -__write_verify(
> - struct xfs_buf *bp,
> - uint16_t magic)
> +xfs_dir3_leaf_write_verify(
> + struct xfs_buf *bp)
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_buf_log_item *bip = bp->b_log_item;
> struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
> xfs_failaddr_t fa;
>
> - fa = xfs_dir3_leaf_verify(bp, magic);
> + fa = xfs_dir3_leaf_verify(bp);
> if (fa) {
> xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> return;
> @@ -216,60 +206,22 @@ __write_verify(
> xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
> }
>
> -static xfs_failaddr_t
> -xfs_dir3_leaf1_verify(
> - struct xfs_buf *bp)
> -{
> - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> -}
> -
> -static void
> -xfs_dir3_leaf1_read_verify(
> - struct xfs_buf *bp)
> -{
> - __read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> -}
> -
> -static void
> -xfs_dir3_leaf1_write_verify(
> - struct xfs_buf *bp)
> -{
> - __write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> -}
> -
> -static xfs_failaddr_t
> -xfs_dir3_leafn_verify(
> - struct xfs_buf *bp)
> -{
> - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> -}
> -
> -static void
> -xfs_dir3_leafn_read_verify(
> - struct xfs_buf *bp)
> -{
> - __read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> -}
> -
> -static void
> -xfs_dir3_leafn_write_verify(
> - struct xfs_buf *bp)
> -{
> - __write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> -}
> -
> const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> .name = "xfs_dir3_leaf1",
> - .verify_read = xfs_dir3_leaf1_read_verify,
> - .verify_write = xfs_dir3_leaf1_write_verify,
> - .verify_struct = xfs_dir3_leaf1_verify,
> + .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> + cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> + .verify_read = xfs_dir3_leaf_read_verify,
> + .verify_write = xfs_dir3_leaf_write_verify,
> + .verify_struct = xfs_dir3_leaf_verify,
> };
>
> const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> .name = "xfs_dir3_leafn",
> - .verify_read = xfs_dir3_leafn_read_verify,
> - .verify_write = xfs_dir3_leafn_write_verify,
> - .verify_struct = xfs_dir3_leafn_verify,
> + .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> + cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> + .verify_read = xfs_dir3_leaf_read_verify,
> + .verify_write = xfs_dir3_leaf_write_verify,
> + .verify_struct = xfs_dir3_leaf_verify,
> };
>
> int
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 8/9] xfs: miscellaneous verifier magic value fixups
2019-02-04 14:52 ` [PATCH v3 8/9] xfs: miscellaneous verifier magic value fixups Brian Foster
@ 2019-02-06 17:52 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 17:52 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:30AM -0500, Brian Foster wrote:
> Most buffer verifiers have hardcoded magic value checks
> conditionalized on the version of the filesystem. The magic value
> field of the verifier structure facilitates abstraction of some of
> this code. Populate the ->magic field of various verifiers to take
> advantage of this abstraction. No functional changes.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_alloc.c | 12 ++++++++----
> fs/xfs/libxfs/xfs_attr_leaf.c | 12 ++++++------
> fs/xfs/libxfs/xfs_attr_remote.c | 8 +++++---
> fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++++++-------
> fs/xfs/libxfs/xfs_da_btree.c | 12 ++++++------
> fs/xfs/libxfs/xfs_dir2_block.c | 10 +++++-----
> fs/xfs/libxfs/xfs_dir2_data.c | 12 +++++++-----
> fs/xfs/libxfs/xfs_dir2_node.c | 11 ++++++-----
> fs/xfs/libxfs/xfs_ialloc.c | 3 ++-
> fs/xfs/libxfs/xfs_refcount_btree.c | 3 ++-
> fs/xfs/libxfs/xfs_rmap_btree.c | 3 ++-
> fs/xfs/libxfs/xfs_sb.c | 4 +++-
> fs/xfs/libxfs/xfs_symlink_remote.c | 3 ++-
> 13 files changed, 60 insertions(+), 46 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 48aab07e7138..bc3367b8b7bb 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -568,9 +568,9 @@ xfs_agfl_verify(
> if (!xfs_sb_version_hascrc(&mp->m_sb))
> return NULL;
>
> - if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
> + if (!xfs_verify_magic(bp, agfl->agfl_magicnum))
> return __this_address;
> - if (agfl->agfl_magicnum != cpu_to_be32(XFS_AGFL_MAGIC))
> + if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> /*
> * during growfs operations, the perag is not fully initialised,
> @@ -643,6 +643,7 @@ xfs_agfl_write_verify(
>
> const struct xfs_buf_ops xfs_agfl_buf_ops = {
> .name = "xfs_agfl",
> + .magic = { cpu_to_be32(XFS_AGFL_MAGIC), cpu_to_be32(XFS_AGFL_MAGIC) },
> .verify_read = xfs_agfl_read_verify,
> .verify_write = xfs_agfl_write_verify,
> .verify_struct = xfs_agfl_verify,
> @@ -2587,8 +2588,10 @@ xfs_agf_verify(
> return __this_address;
> }
>
> - if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) &&
> - XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
> + if (!xfs_verify_magic(bp, agf->agf_magicnum))
> + return __this_address;
> +
> + if (!(XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
> be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) &&
> be32_to_cpu(agf->agf_flfirst) < xfs_agfl_size(mp) &&
> be32_to_cpu(agf->agf_fllast) < xfs_agfl_size(mp) &&
> @@ -2670,6 +2673,7 @@ xfs_agf_write_verify(
>
> const struct xfs_buf_ops xfs_agf_buf_ops = {
> .name = "xfs_agf",
> + .magic = { cpu_to_be32(XFS_AGF_MAGIC), cpu_to_be32(XFS_AGF_MAGIC) },
> .verify_read = xfs_agf_read_verify,
> .verify_write = xfs_agf_write_verify,
> .verify_struct = xfs_agf_verify,
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index e60eba7f129c..dde3a550d7ce 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -248,21 +248,19 @@ xfs_attr3_leaf_verify(
>
> xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>
> + if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> + return __this_address;
> +
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
>
> - if (hdr3->info.hdr.magic != cpu_to_be16(XFS_ATTR3_LEAF_MAGIC))
> - return __this_address;
> -
> + ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
Same question as the previous patch -- aren't these magic numbers at the
exact same buffer offset for both da and da3 node formats?
(And the same applies to the ASSERTs added to the other directory
verifiers...)
--D
> if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> return __this_address;
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> return __this_address;
> - } else {
> - if (leaf->hdr.info.magic != cpu_to_be16(XFS_ATTR_LEAF_MAGIC))
> - return __this_address;
> }
> /*
> * In recovery there is a transient state where count == 0 is valid
> @@ -369,6 +367,8 @@ xfs_attr3_leaf_read_verify(
>
> const struct xfs_buf_ops xfs_attr3_leaf_buf_ops = {
> .name = "xfs_attr3_leaf",
> + .magic = { cpu_to_be16(XFS_ATTR_LEAF_MAGIC),
> + cpu_to_be16(XFS_ATTR3_LEAF_MAGIC) },
> .verify_read = xfs_attr3_leaf_read_verify,
> .verify_write = xfs_attr3_leaf_write_verify,
> .verify_struct = xfs_attr3_leaf_verify,
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index d89363c6b523..65ff600a8067 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -79,6 +79,7 @@ xfs_attr3_rmt_hdr_ok(
> static xfs_failaddr_t
> xfs_attr3_rmt_verify(
> struct xfs_mount *mp,
> + struct xfs_buf *bp,
> void *ptr,
> int fsbsize,
> xfs_daddr_t bno)
> @@ -87,7 +88,7 @@ xfs_attr3_rmt_verify(
>
> if (!xfs_sb_version_hascrc(&mp->m_sb))
> return __this_address;
> - if (rmt->rm_magic != cpu_to_be32(XFS_ATTR3_RMT_MAGIC))
> + if (!xfs_verify_magic(bp, rmt->rm_magic))
> return __this_address;
> if (!uuid_equal(&rmt->rm_uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> @@ -131,7 +132,7 @@ __xfs_attr3_rmt_read_verify(
> *failaddr = __this_address;
> return -EFSBADCRC;
> }
> - *failaddr = xfs_attr3_rmt_verify(mp, ptr, blksize, bno);
> + *failaddr = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
> if (*failaddr)
> return -EFSCORRUPTED;
> len -= blksize;
> @@ -193,7 +194,7 @@ xfs_attr3_rmt_write_verify(
> while (len > 0) {
> struct xfs_attr3_rmt_hdr *rmt = (struct xfs_attr3_rmt_hdr *)ptr;
>
> - fa = xfs_attr3_rmt_verify(mp, ptr, blksize, bno);
> + fa = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
> if (fa) {
> xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> return;
> @@ -220,6 +221,7 @@ xfs_attr3_rmt_write_verify(
>
> const struct xfs_buf_ops xfs_attr3_rmt_buf_ops = {
> .name = "xfs_attr3_rmt",
> + .magic = { 0, cpu_to_be32(XFS_ATTR3_RMT_MAGIC) },
> .verify_read = xfs_attr3_rmt_read_verify,
> .verify_write = xfs_attr3_rmt_write_verify,
> .verify_struct = xfs_attr3_rmt_verify_struct,
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index cdb74d2e2a43..aff82ed112c9 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -416,8 +416,10 @@ xfs_bmbt_verify(
> xfs_failaddr_t fa;
> unsigned int level;
>
> - switch (block->bb_magic) {
> - case cpu_to_be32(XFS_BMAP_CRC_MAGIC):
> + if (!xfs_verify_magic(bp, block->bb_magic))
> + return __this_address;
> +
> + if (xfs_sb_version_hascrc(&mp->m_sb)) {
> /*
> * XXX: need a better way of verifying the owner here. Right now
> * just make sure there has been one set.
> @@ -425,11 +427,6 @@ xfs_bmbt_verify(
> fa = xfs_btree_lblock_v5hdr_verify(bp, XFS_RMAP_OWN_UNKNOWN);
> if (fa)
> return fa;
> - /* fall through */
> - case cpu_to_be32(XFS_BMAP_MAGIC):
> - break;
> - default:
> - return __this_address;
> }
>
> /*
> @@ -481,6 +478,8 @@ xfs_bmbt_write_verify(
>
> const struct xfs_buf_ops xfs_bmbt_buf_ops = {
> .name = "xfs_bmbt",
> + .magic = { cpu_to_be32(XFS_BMAP_MAGIC),
> + cpu_to_be32(XFS_BMAP_CRC_MAGIC) },
> .verify_read = xfs_bmbt_read_verify,
> .verify_write = xfs_bmbt_write_verify,
> .verify_struct = xfs_bmbt_verify,
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 355322688c9f..6b8ca390eecc 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -129,21 +129,19 @@ xfs_da3_node_verify(
>
> ops->node_hdr_from_disk(&ichdr, hdr);
>
> + if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
> + return __this_address;
> +
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
>
> - if (hdr3->info.hdr.magic != cpu_to_be16(XFS_DA3_NODE_MAGIC))
> - return __this_address;
> -
> + ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
> if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> return __this_address;
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> return __this_address;
> - } else {
> - if (hdr->hdr.info.magic != cpu_to_be16(XFS_DA_NODE_MAGIC))
> - return __this_address;
> }
> if (ichdr.level == 0)
> return __this_address;
> @@ -257,6 +255,8 @@ xfs_da3_node_verify_struct(
>
> const struct xfs_buf_ops xfs_da3_node_buf_ops = {
> .name = "xfs_da3_node",
> + .magic = { cpu_to_be16(XFS_DA_NODE_MAGIC),
> + cpu_to_be16(XFS_DA3_NODE_MAGIC) },
> .verify_read = xfs_da3_node_read_verify,
> .verify_write = xfs_da3_node_write_verify,
> .verify_struct = xfs_da3_node_verify_struct,
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 30ed5919da72..b7d6d78f4ce2 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -53,18 +53,16 @@ xfs_dir3_block_verify(
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
>
> + if (!xfs_verify_magic(bp, hdr3->magic))
> + return __this_address;
> +
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> - if (hdr3->magic != cpu_to_be32(XFS_DIR3_BLOCK_MAGIC))
> - return __this_address;
> if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> return __this_address;
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> return __this_address;
> - } else {
> - if (hdr3->magic != cpu_to_be32(XFS_DIR2_BLOCK_MAGIC))
> - return __this_address;
> }
> return __xfs_dir3_data_check(NULL, bp);
> }
> @@ -112,6 +110,8 @@ xfs_dir3_block_write_verify(
>
> const struct xfs_buf_ops xfs_dir3_block_buf_ops = {
> .name = "xfs_dir3_block",
> + .magic = { cpu_to_be32(XFS_DIR2_BLOCK_MAGIC),
> + cpu_to_be32(XFS_DIR3_BLOCK_MAGIC) },
> .verify_read = xfs_dir3_block_read_verify,
> .verify_write = xfs_dir3_block_write_verify,
> .verify_struct = xfs_dir3_block_verify,
> diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
> index 01162c62ec8f..b7b9ce002cb9 100644
> --- a/fs/xfs/libxfs/xfs_dir2_data.c
> +++ b/fs/xfs/libxfs/xfs_dir2_data.c
> @@ -252,18 +252,16 @@ xfs_dir3_data_verify(
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
>
> + if (!xfs_verify_magic(bp, hdr3->magic))
> + return __this_address;
> +
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> - if (hdr3->magic != cpu_to_be32(XFS_DIR3_DATA_MAGIC))
> - return __this_address;
> if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> return __this_address;
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> return __this_address;
> - } else {
> - if (hdr3->magic != cpu_to_be32(XFS_DIR2_DATA_MAGIC))
> - return __this_address;
> }
> return __xfs_dir3_data_check(NULL, bp);
> }
> @@ -339,6 +337,8 @@ xfs_dir3_data_write_verify(
>
> const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
> .name = "xfs_dir3_data",
> + .magic = { cpu_to_be32(XFS_DIR2_DATA_MAGIC),
> + cpu_to_be32(XFS_DIR3_DATA_MAGIC) },
> .verify_read = xfs_dir3_data_read_verify,
> .verify_write = xfs_dir3_data_write_verify,
> .verify_struct = xfs_dir3_data_verify,
> @@ -346,6 +346,8 @@ const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
>
> static const struct xfs_buf_ops xfs_dir3_data_reada_buf_ops = {
> .name = "xfs_dir3_data_reada",
> + .magic = { cpu_to_be32(XFS_DIR2_DATA_MAGIC),
> + cpu_to_be32(XFS_DIR3_DATA_MAGIC) },
> .verify_read = xfs_dir3_data_reada_verify,
> .verify_write = xfs_dir3_data_write_verify,
> };
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index f1bb3434f51c..9ccc8819e99f 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -87,20 +87,19 @@ xfs_dir3_free_verify(
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_dir2_free_hdr *hdr = bp->b_addr;
>
> + if (!xfs_verify_magic(bp, hdr->magic))
> + return __this_address;
> +
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
>
> - if (hdr3->magic != cpu_to_be32(XFS_DIR3_FREE_MAGIC))
> - return __this_address;
> + ASSERT(hdr3->magic == hdr->magic);
> if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> return __this_address;
> if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> return __this_address;
> - } else {
> - if (hdr->magic != cpu_to_be32(XFS_DIR2_FREE_MAGIC))
> - return __this_address;
> }
>
> /* XXX: should bounds check the xfs_dir3_icfree_hdr here */
> @@ -151,6 +150,8 @@ xfs_dir3_free_write_verify(
>
> const struct xfs_buf_ops xfs_dir3_free_buf_ops = {
> .name = "xfs_dir3_free",
> + .magic = { cpu_to_be32(XFS_DIR2_FREE_MAGIC),
> + cpu_to_be32(XFS_DIR3_FREE_MAGIC) },
> .verify_read = xfs_dir3_free_read_verify,
> .verify_write = xfs_dir3_free_write_verify,
> .verify_struct = xfs_dir3_free_verify,
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index d32152fc8a6c..fe9898875097 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -2508,7 +2508,7 @@ xfs_agi_verify(
> /*
> * Validate the magic number of the agi block.
> */
> - if (agi->agi_magicnum != cpu_to_be32(XFS_AGI_MAGIC))
> + if (!xfs_verify_magic(bp, agi->agi_magicnum))
> return __this_address;
> if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum)))
> return __this_address;
> @@ -2582,6 +2582,7 @@ xfs_agi_write_verify(
>
> const struct xfs_buf_ops xfs_agi_buf_ops = {
> .name = "xfs_agi",
> + .magic = { cpu_to_be32(XFS_AGI_MAGIC), cpu_to_be32(XFS_AGI_MAGIC) },
> .verify_read = xfs_agi_read_verify,
> .verify_write = xfs_agi_write_verify,
> .verify_struct = xfs_agi_verify,
> diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
> index d9eab657b63e..6f47ab876d90 100644
> --- a/fs/xfs/libxfs/xfs_refcount_btree.c
> +++ b/fs/xfs/libxfs/xfs_refcount_btree.c
> @@ -209,7 +209,7 @@ xfs_refcountbt_verify(
> xfs_failaddr_t fa;
> unsigned int level;
>
> - if (block->bb_magic != cpu_to_be32(XFS_REFC_CRC_MAGIC))
> + if (!xfs_verify_magic(bp, block->bb_magic))
> return __this_address;
>
> if (!xfs_sb_version_hasreflink(&mp->m_sb))
> @@ -264,6 +264,7 @@ xfs_refcountbt_write_verify(
>
> const struct xfs_buf_ops xfs_refcountbt_buf_ops = {
> .name = "xfs_refcountbt",
> + .magic = { 0, cpu_to_be32(XFS_REFC_CRC_MAGIC) },
> .verify_read = xfs_refcountbt_read_verify,
> .verify_write = xfs_refcountbt_write_verify,
> .verify_struct = xfs_refcountbt_verify,
> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
> index f79cf040d745..5738e11055e6 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -310,7 +310,7 @@ xfs_rmapbt_verify(
> * from the on disk AGF. Again, we can only check against maximum limits
> * in this case.
> */
> - if (block->bb_magic != cpu_to_be32(XFS_RMAP_CRC_MAGIC))
> + if (!xfs_verify_magic(bp, block->bb_magic))
> return __this_address;
>
> if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
> @@ -365,6 +365,7 @@ xfs_rmapbt_write_verify(
>
> const struct xfs_buf_ops xfs_rmapbt_buf_ops = {
> .name = "xfs_rmapbt",
> + .magic = { 0, cpu_to_be32(XFS_RMAP_CRC_MAGIC) },
> .verify_read = xfs_rmapbt_read_verify,
> .verify_write = xfs_rmapbt_write_verify,
> .verify_struct = xfs_rmapbt_verify,
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index a2f52a958091..4e5029c37966 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -229,7 +229,7 @@ xfs_validate_sb_common(
> uint32_t agcount = 0;
> uint32_t rem;
>
> - if (dsb->sb_magicnum != cpu_to_be32(XFS_SB_MAGIC)) {
> + if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
> xfs_warn(mp, "bad magic number");
> return -EWRONGFS;
> }
> @@ -782,12 +782,14 @@ xfs_sb_write_verify(
>
> const struct xfs_buf_ops xfs_sb_buf_ops = {
> .name = "xfs_sb",
> + .magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
> .verify_read = xfs_sb_read_verify,
> .verify_write = xfs_sb_write_verify,
> };
>
> const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
> .name = "xfs_sb_quiet",
> + .magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
> .verify_read = xfs_sb_quiet_read_verify,
> .verify_write = xfs_sb_write_verify,
> };
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index 77d80106f989..a0ccc253c43d 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -95,7 +95,7 @@ xfs_symlink_verify(
>
> if (!xfs_sb_version_hascrc(&mp->m_sb))
> return __this_address;
> - if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
> + if (!xfs_verify_magic(bp, dsl->sl_magic))
> return __this_address;
> if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
> return __this_address;
> @@ -159,6 +159,7 @@ xfs_symlink_write_verify(
>
> const struct xfs_buf_ops xfs_symlink_buf_ops = {
> .name = "xfs_symlink",
> + .magic = { 0, cpu_to_be32(XFS_SYMLINK_MAGIC) },
> .verify_read = xfs_symlink_read_verify,
> .verify_write = xfs_symlink_write_verify,
> .verify_struct = xfs_symlink_verify,
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type
2019-02-04 14:52 ` [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type Brian Foster
@ 2019-02-06 18:08 ` Darrick J. Wong
2019-02-06 18:40 ` Brian Foster
0 siblings, 1 reply; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:08 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:23AM -0500, Brian Foster wrote:
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
>
> In xrep_findroot_block, we work out the btree type and correctness of a
> given block by calling different btree verifiers on root block
> candidates. However, we leave the NULL b_ops while ->verify_read
> validates the block, which means that if the verifier calls
> xfs_buf_verifier_error it'll crash on the null b_ops. Fix it to set
> b_ops before calling the verifier and unsetting it if the verifier
> fails.
>
> Furthermore, improve the documentation around xfs_buf_ensure_ops, which
> is the function that is responsible for cleaning up the b_ops state of
> buffers that go through xrep_findroot_block but don't match anything.
>
> [bfoster: Renamed xfs_buf_ensure_ops() and modified comment.]
Heh, I already queued up the original patch for 5.0 so that we don't
oops the kernel. :/
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/scrub/repair.c | 11 ++++++++---
> fs/xfs/xfs_buf.c | 22 ++++++++++++++++------
> fs/xfs/xfs_buf.h | 2 +-
> fs/xfs/xfs_trans_buf.c | 2 +-
> 4 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 1c8eecfe52b8..6acf1bfa0bfe 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -768,18 +768,23 @@ xrep_findroot_block(
> if (!uuid_equal(&btblock->bb_u.s.bb_uuid,
> &mp->m_sb.sb_meta_uuid))
> goto out;
> + /*
> + * Read verifiers can reference b_ops, so we set the pointer
> + * here. If the verifier fails we'll reset the buffer state
> + * to what it was before we touched the buffer.
> + */
> + bp->b_ops = fab->buf_ops;
> fab->buf_ops->verify_read(bp);
> if (bp->b_error) {
> + bp->b_ops = NULL;
> bp->b_error = 0;
> goto out;
> }
>
> /*
> * Some read verifiers will (re)set b_ops, so we must be
> - * careful not to blow away any such assignment.
> + * careful not to change b_ops after running the verifier.
> */
> - if (!bp->b_ops)
> - bp->b_ops = fab->buf_ops;
> }
>
> /*
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index eedc5e0156ff..222b5260ed35 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -776,13 +776,23 @@ _xfs_buf_read(
> }
>
> /*
> - * If the caller passed in an ops structure and the buffer doesn't have ops
> - * assigned, set the ops and use them to verify the contents. If the contents
> - * cannot be verified, we'll clear XBF_DONE. We assume the buffer has no
> - * recorded errors and is already in XBF_DONE state.
> + * Reverify a buffer found in cache without an attached ->b_ops.
> + *
> + * If the caller passed an ops structure and the buffer doesn't have ops
> + * assigned, set the ops and use it to verify the contents. If verification
> + * fails, clear XBF_DONE. We assume the buffer has no recorded errors and is
> + * already in XBF_DONE state on entry.
> + *
> + * Under normal operations, every in-core buffer is verified on read I/O
> + * completion. There are two scenarios that can lead to in-core buffers without
> + * an assigned ->b_ops. The first is during log recovery of buffers on a V4
> + * filesystem, though these buffers are purged at the end of recovery. The
> + * other is online repair, which intentionally reads with a NULL buffer ops to
> + * run several verifiers across an in-core buffer in order to establish buffer
> + * type.
I would like to add a sentence here explicitly saying that if repair
can't establish the buffer type from the verifiers then the buffer is
left in memory with null b_ops.
> */
> int
> -xfs_buf_ensure_ops(
> +xfs_buf_reverify(
I like the name and the comment better than my original version, so I'll
rework this patch as a separate "rename and unfrobulate documentation"
thing and send it out.
--D
> struct xfs_buf *bp,
> const struct xfs_buf_ops *ops)
> {
> @@ -824,7 +834,7 @@ xfs_buf_read_map(
> return bp;
> }
>
> - xfs_buf_ensure_ops(bp, ops);
> + xfs_buf_reverify(bp, ops);
>
> if (flags & XBF_ASYNC) {
> /*
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index b9f5511ea998..1c436a85b71d 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -385,6 +385,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> #define xfs_getsize_buftarg(buftarg) block_size((buftarg)->bt_bdev)
> #define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
>
> -int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> +int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
>
> #endif /* __XFS_BUF_H__ */
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 629f1479c9d2..7d65ebf1e847 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -277,7 +277,7 @@ xfs_trans_read_buf_map(
> * release this buffer when it kills the tranaction.
> */
> ASSERT(bp->b_ops != NULL);
> - error = xfs_buf_ensure_ops(bp, ops);
> + error = xfs_buf_reverify(bp, ops);
> if (error) {
> xfs_buf_ioerror_alert(bp, __func__);
>
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 3/9] xfs: create a separate finobt verifier
2019-02-04 14:52 ` [PATCH v3 3/9] xfs: create a separate finobt verifier Brian Foster
@ 2019-02-06 18:09 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:09 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:25AM -0500, Brian Foster wrote:
> The inobt verifier is reused for the inobt and finobt, which
> prevents the ability to distinguish between magic values on a
> per-tree basis. Create a separate finobt structure in preparation
> for changes to enforce the appropriate magic value for the
> associated tree. This patch has no functional change.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_ag.c | 2 +-
> fs/xfs/libxfs/xfs_ialloc_btree.c | 9 ++++++++-
> fs/xfs/libxfs/xfs_shared.h | 1 +
> fs/xfs/scrub/agheader_repair.c | 2 +-
> fs/xfs/xfs_log_recover.c | 6 ++++--
> 5 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 999ad8d00d43..bde67ef3ff43 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -361,7 +361,7 @@ xfs_ag_init_headers(
> { /* FINO root block */
> .daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_FIBT_BLOCK(mp)),
> .numblks = BTOBB(mp->m_sb.sb_blocksize),
> - .ops = &xfs_inobt_buf_ops,
> + .ops = &xfs_finobt_buf_ops,
> .work = &xfs_btroot_init,
> .type = XFS_BTNUM_FINO,
> .need_init = xfs_sb_version_hasfinobt(&mp->m_sb)
> diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
> index 9b25e7a0df47..798269eb4767 100644
> --- a/fs/xfs/libxfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
> @@ -333,6 +333,13 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
> .verify_struct = xfs_inobt_verify,
> };
>
> +const struct xfs_buf_ops xfs_finobt_buf_ops = {
> + .name = "xfs_finobt",
> + .verify_read = xfs_inobt_read_verify,
> + .verify_write = xfs_inobt_write_verify,
> + .verify_struct = xfs_inobt_verify,
> +};
> +
> STATIC int
> xfs_inobt_keys_inorder(
> struct xfs_btree_cur *cur,
> @@ -389,7 +396,7 @@ static const struct xfs_btree_ops xfs_finobt_ops = {
> .init_rec_from_cur = xfs_inobt_init_rec_from_cur,
> .init_ptr_from_cur = xfs_finobt_init_ptr_from_cur,
> .key_diff = xfs_inobt_key_diff,
> - .buf_ops = &xfs_inobt_buf_ops,
> + .buf_ops = &xfs_finobt_buf_ops,
> .diff_two_keys = xfs_inobt_diff_two_keys,
> .keys_inorder = xfs_inobt_keys_inorder,
> .recs_inorder = xfs_inobt_recs_inorder,
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 1c5debe748f0..9855f4d2f98f 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -36,6 +36,7 @@ extern const struct xfs_buf_ops xfs_dquot_buf_ops;
> extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> extern const struct xfs_buf_ops xfs_agi_buf_ops;
> extern const struct xfs_buf_ops xfs_inobt_buf_ops;
> +extern const struct xfs_buf_ops xfs_finobt_buf_ops;
> extern const struct xfs_buf_ops xfs_inode_buf_ops;
> extern const struct xfs_buf_ops xfs_inode_buf_ra_ops;
> extern const struct xfs_buf_ops xfs_dquot_buf_ops;
> diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> index 03d1e15cceba..673be3cf7b8d 100644
> --- a/fs/xfs/scrub/agheader_repair.c
> +++ b/fs/xfs/scrub/agheader_repair.c
> @@ -879,7 +879,7 @@ xrep_agi(
> },
> [XREP_AGI_FINOBT] = {
> .rmap_owner = XFS_RMAP_OWN_INOBT,
> - .buf_ops = &xfs_inobt_buf_ops,
> + .buf_ops = &xfs_finobt_buf_ops,
> .magic = XFS_FIBT_CRC_MAGIC,
> },
> [XREP_AGI_END] = {
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 9fe88d125f0a..228c754bb137 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2445,11 +2445,13 @@ xlog_recover_validate_buf_type(
> bp->b_ops = &xfs_allocbt_buf_ops;
> break;
> case XFS_IBT_CRC_MAGIC:
> - case XFS_FIBT_CRC_MAGIC:
> case XFS_IBT_MAGIC:
> - case XFS_FIBT_MAGIC:
> bp->b_ops = &xfs_inobt_buf_ops;
> break;
> + case XFS_FIBT_CRC_MAGIC:
> + case XFS_FIBT_MAGIC:
> + bp->b_ops = &xfs_finobt_buf_ops;
> + break;
> case XFS_BMAP_CRC_MAGIC:
> case XFS_BMAP_MAGIC:
> bp->b_ops = &xfs_bmbt_buf_ops;
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 4/9] xfs: distinguish between inobt and finobt magic values
2019-02-04 14:52 ` [PATCH v3 4/9] xfs: distinguish between inobt and finobt magic values Brian Foster
@ 2019-02-06 18:12 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:12 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:26AM -0500, Brian Foster wrote:
> The inode btree verifier code is shared between the inode btree and
> free inode btree because the underlying metadata formats are
> essentially equivalent. A side effect of this is that the verifier
> cannot determine whether a particular btree block should have an
> inobt or finobt magic value.
>
> This logic allows an unfortunate xfs_repair bug to escape detection
> where certain level > 0 nodes of the finobt are stamped with inobt
> magic by xfs_repair finobt reconstruction. This is fortunately not a
> severe problem since the inode btree magic values do not contribute
> to any changes in kernel behavior, but we do need a means to detect
> and prevent this problem in the future.
>
> Add a field to xfs_buf_ops to store the v4 and v5 superblock magic
> values expected by a particular verifier. Add a helper to check an
> on-disk magic value against the value expected by the verifier. Call
> the helper from the shared [f]inobt verifier code for magic value
> verification. This ensures that the inode btree blocks each have the
> appropriate magic value based on specific tree type and superblock
> version.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ialloc_btree.c | 16 +++++++---------
> fs/xfs/xfs_buf.c | 19 +++++++++++++++++++
> fs/xfs/xfs_buf.h | 2 ++
> 3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
> index 798269eb4767..c2df1f89eec8 100644
> --- a/fs/xfs/libxfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
> @@ -260,6 +260,9 @@ xfs_inobt_verify(
> xfs_failaddr_t fa;
> unsigned int level;
>
> + if (!xfs_verify_magic(bp, block->bb_magic))
> + return __this_address;
> +
> /*
> * During growfs operations, we can't verify the exact owner as the
> * perag is not fully initialised and hence not attached to the buffer.
> @@ -270,18 +273,10 @@ xfs_inobt_verify(
> * but beware of the landmine (i.e. need to check pag->pagi_init) if we
> * ever do.
> */
> - switch (block->bb_magic) {
> - case cpu_to_be32(XFS_IBT_CRC_MAGIC):
> - case cpu_to_be32(XFS_FIBT_CRC_MAGIC):
> + if (xfs_sb_version_hascrc(&mp->m_sb)) {
> fa = xfs_btree_sblock_v5hdr_verify(bp);
> if (fa)
> return fa;
> - /* fall through */
> - case cpu_to_be32(XFS_IBT_MAGIC):
> - case cpu_to_be32(XFS_FIBT_MAGIC):
> - break;
> - default:
> - return __this_address;
> }
>
> /* level verification */
> @@ -328,6 +323,7 @@ xfs_inobt_write_verify(
>
> const struct xfs_buf_ops xfs_inobt_buf_ops = {
> .name = "xfs_inobt",
> + .magic = { cpu_to_be32(XFS_IBT_MAGIC), cpu_to_be32(XFS_IBT_CRC_MAGIC) },
> .verify_read = xfs_inobt_read_verify,
> .verify_write = xfs_inobt_write_verify,
> .verify_struct = xfs_inobt_verify,
> @@ -335,6 +331,8 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
>
> const struct xfs_buf_ops xfs_finobt_buf_ops = {
> .name = "xfs_finobt",
> + .magic = { cpu_to_be32(XFS_FIBT_MAGIC),
> + cpu_to_be32(XFS_FIBT_CRC_MAGIC) },
> .verify_read = xfs_inobt_read_verify,
> .verify_write = xfs_inobt_write_verify,
> .verify_struct = xfs_inobt_verify,
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 222b5260ed35..0481c19fe5ae 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2204,3 +2204,22 @@ void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
>
> atomic_set(&bp->b_lru_ref, lru_ref);
> }
> +
> +/*
> + * Verify an on-disk magic value against the magic value specified in the
> + * verifier structure. The verifier magic is in disk byte order so the caller is
> + * expected to pass the value directly from disk.
> + */
> +bool
> +xfs_verify_magic(
> + struct xfs_buf *bp,
> + uint32_t dmagic)
> +{
> + struct xfs_mount *mp = bp->b_target->bt_mount;
> + int idx;
> +
> + idx = xfs_sb_version_hascrc(&mp->m_sb);
> + if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
/me kinda wonders if we ought to have a #define for "no magic number" to
put in the .magic = {}, but OTOH we're never going to have a magic
number of zero.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> + return false;
> + return dmagic == bp->b_ops->magic[idx];
> +}
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 1c436a85b71d..44f9423a1861 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -125,6 +125,7 @@ struct xfs_buf_map {
>
> struct xfs_buf_ops {
> char *name;
> + uint32_t magic[2]; /* v4 and v5 on disk magic values */
> void (*verify_read)(struct xfs_buf *);
> void (*verify_write)(struct xfs_buf *);
> xfs_failaddr_t (*verify_struct)(struct xfs_buf *bp);
> @@ -386,5 +387,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> #define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
>
> int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> +bool xfs_verify_magic(struct xfs_buf *bp, uint32_t dmagic);
>
> #endif /* __XFS_BUF_H__ */
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 5/9] xfs: split up allocation btree verifier
2019-02-04 14:52 ` [PATCH v3 5/9] xfs: split up allocation btree verifier Brian Foster
@ 2019-02-06 18:15 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:15 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:27AM -0500, Brian Foster wrote:
> Similar to the inode btree verifier, the same allocation btree
> verifier structure is shared between the by-bno (bnobt) and by-size
> (cntbt) btrees. This prevents the ability to distinguish magic
> values between them. Separate the verifier into two, one for each
> tree, and assign them appropriately. No functional changes.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ag.c | 4 ++--
> fs/xfs/libxfs/xfs_alloc_btree.c | 14 ++++++++++----
> fs/xfs/libxfs/xfs_shared.h | 3 ++-
> fs/xfs/scrub/agheader_repair.c | 4 ++--
> fs/xfs/xfs_log_recover.c | 6 ++++--
> 5 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index bde67ef3ff43..1ef8acf35e7d 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -339,14 +339,14 @@ xfs_ag_init_headers(
> { /* BNO root block */
> .daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_BNO_BLOCK(mp)),
> .numblks = BTOBB(mp->m_sb.sb_blocksize),
> - .ops = &xfs_allocbt_buf_ops,
> + .ops = &xfs_bnobt_buf_ops,
> .work = &xfs_bnoroot_init,
> .need_init = true
> },
> { /* CNT root block */
> .daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_CNT_BLOCK(mp)),
> .numblks = BTOBB(mp->m_sb.sb_blocksize),
> - .ops = &xfs_allocbt_buf_ops,
> + .ops = &xfs_cntbt_buf_ops,
> .work = &xfs_cntroot_init,
> .need_init = true
> },
> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 4e59cc8a2802..480d0f52da64 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -377,13 +377,19 @@ xfs_allocbt_write_verify(
>
> }
>
> -const struct xfs_buf_ops xfs_allocbt_buf_ops = {
> - .name = "xfs_allocbt",
> +const struct xfs_buf_ops xfs_bnobt_buf_ops = {
> + .name = "xfs_bnobt",
> .verify_read = xfs_allocbt_read_verify,
> .verify_write = xfs_allocbt_write_verify,
> .verify_struct = xfs_allocbt_verify,
> };
>
> +const struct xfs_buf_ops xfs_cntbt_buf_ops = {
> + .name = "xfs_cntbt",
> + .verify_read = xfs_allocbt_read_verify,
> + .verify_write = xfs_allocbt_write_verify,
> + .verify_struct = xfs_allocbt_verify,
> +};
>
> STATIC int
> xfs_bnobt_keys_inorder(
> @@ -448,7 +454,7 @@ static const struct xfs_btree_ops xfs_bnobt_ops = {
> .init_rec_from_cur = xfs_allocbt_init_rec_from_cur,
> .init_ptr_from_cur = xfs_allocbt_init_ptr_from_cur,
> .key_diff = xfs_bnobt_key_diff,
> - .buf_ops = &xfs_allocbt_buf_ops,
> + .buf_ops = &xfs_bnobt_buf_ops,
> .diff_two_keys = xfs_bnobt_diff_two_keys,
> .keys_inorder = xfs_bnobt_keys_inorder,
> .recs_inorder = xfs_bnobt_recs_inorder,
> @@ -470,7 +476,7 @@ static const struct xfs_btree_ops xfs_cntbt_ops = {
> .init_rec_from_cur = xfs_allocbt_init_rec_from_cur,
> .init_ptr_from_cur = xfs_allocbt_init_ptr_from_cur,
> .key_diff = xfs_cntbt_key_diff,
> - .buf_ops = &xfs_allocbt_buf_ops,
> + .buf_ops = &xfs_cntbt_buf_ops,
> .diff_two_keys = xfs_cntbt_diff_two_keys,
> .keys_inorder = xfs_cntbt_keys_inorder,
> .recs_inorder = xfs_cntbt_recs_inorder,
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 9855f4d2f98f..4e909791aeac 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -25,7 +25,8 @@ extern const struct xfs_buf_ops xfs_agf_buf_ops;
> extern const struct xfs_buf_ops xfs_agi_buf_ops;
> extern const struct xfs_buf_ops xfs_agf_buf_ops;
> extern const struct xfs_buf_ops xfs_agfl_buf_ops;
> -extern const struct xfs_buf_ops xfs_allocbt_buf_ops;
> +extern const struct xfs_buf_ops xfs_bnobt_buf_ops;
> +extern const struct xfs_buf_ops xfs_cntbt_buf_ops;
> extern const struct xfs_buf_ops xfs_rmapbt_buf_ops;
> extern const struct xfs_buf_ops xfs_refcountbt_buf_ops;
> extern const struct xfs_buf_ops xfs_attr3_leaf_buf_ops;
> diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> index 673be3cf7b8d..2799d1531639 100644
> --- a/fs/xfs/scrub/agheader_repair.c
> +++ b/fs/xfs/scrub/agheader_repair.c
> @@ -341,12 +341,12 @@ xrep_agf(
> struct xrep_find_ag_btree fab[XREP_AGF_MAX] = {
> [XREP_AGF_BNOBT] = {
> .rmap_owner = XFS_RMAP_OWN_AG,
> - .buf_ops = &xfs_allocbt_buf_ops,
> + .buf_ops = &xfs_bnobt_buf_ops,
> .magic = XFS_ABTB_CRC_MAGIC,
> },
> [XREP_AGF_CNTBT] = {
> .rmap_owner = XFS_RMAP_OWN_AG,
> - .buf_ops = &xfs_allocbt_buf_ops,
> + .buf_ops = &xfs_cntbt_buf_ops,
> .magic = XFS_ABTC_CRC_MAGIC,
/me wonders if this part of repair ought to be refactored to pull the
magic number from the buf_ops, though that's a separate patch for after
all the buf ops have been modified to have magic numbers.
This patch looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> },
> [XREP_AGF_RMAPBT] = {
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 228c754bb137..5ad42d598333 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2439,10 +2439,12 @@ xlog_recover_validate_buf_type(
> case XFS_BLFT_BTREE_BUF:
> switch (magic32) {
> case XFS_ABTB_CRC_MAGIC:
> - case XFS_ABTC_CRC_MAGIC:
> case XFS_ABTB_MAGIC:
> + bp->b_ops = &xfs_bnobt_buf_ops;
> + break;
> + case XFS_ABTC_CRC_MAGIC:
> case XFS_ABTC_MAGIC:
> - bp->b_ops = &xfs_allocbt_buf_ops;
> + bp->b_ops = &xfs_cntbt_buf_ops;
> break;
> case XFS_IBT_CRC_MAGIC:
> case XFS_IBT_MAGIC:
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 6/9] xfs: distinguish between bnobt and cntbt magic values
2019-02-04 14:52 ` [PATCH v3 6/9] xfs: distinguish between bnobt and cntbt magic values Brian Foster
@ 2019-02-06 18:15 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:15 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:28AM -0500, Brian Foster wrote:
> The allocation btree verifiers share code that is unable to detect
> cross-tree magic value corruptions such as a bnobt block with a
> cntbt magic value. Populate the b_ops->magic field of the associated
> verifier structures such that the structure verifier can check the
> magic value against the expected value based on tree type.
>
> The btree level check requires knowledge of the tree type to
> determine the appropriate maximum value. This was previously part of
> the hardcoded magic value checks. With that code removed, peek at
> the first magic value in the verifier to determine the expected tree
> type of the current block.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_alloc_btree.c | 60 ++++++++++++++-------------------
> 1 file changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 480d0f52da64..9fe949f6055e 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -297,48 +297,34 @@ xfs_allocbt_verify(
> struct xfs_perag *pag = bp->b_pag;
> xfs_failaddr_t fa;
> unsigned int level;
> + xfs_btnum_t btnum = XFS_BTNUM_BNOi;
> +
> + if (!xfs_verify_magic(bp, block->bb_magic))
> + return __this_address;
> +
> + if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + fa = xfs_btree_sblock_v5hdr_verify(bp);
> + if (fa)
> + return fa;
> + }
>
> /*
> - * magic number and level verification
> - *
> - * During growfs operations, we can't verify the exact level or owner as
> - * the perag is not fully initialised and hence not attached to the
> - * buffer. In this case, check against the maximum tree depth.
> + * The perag may not be attached during grow operations or fully
> + * initialized from the AGF during log recovery. Therefore we can only
> + * check against maximum tree depth from those contexts.
> *
> - * Similarly, during log recovery we will have a perag structure
> - * attached, but the agf information will not yet have been initialised
> - * from the on disk AGF. Again, we can only check against maximum limits
> - * in this case.
> + * Otherwise check against the per-tree limit. Peek at one of the
> + * verifier magic values to determine the type of tree we're verifying
> + * against.
> */
> level = be16_to_cpu(block->bb_level);
> - switch (block->bb_magic) {
> - case cpu_to_be32(XFS_ABTB_CRC_MAGIC):
> - fa = xfs_btree_sblock_v5hdr_verify(bp);
> - if (fa)
> - return fa;
> - /* fall through */
> - case cpu_to_be32(XFS_ABTB_MAGIC):
> - if (pag && pag->pagf_init) {
> - if (level >= pag->pagf_levels[XFS_BTNUM_BNOi])
> - return __this_address;
> - } else if (level >= mp->m_ag_maxlevels)
> + if (bp->b_ops->magic[0] == cpu_to_be32(XFS_ABTC_MAGIC))
> + btnum = XFS_BTNUM_CNTi;
> + if (pag && pag->pagf_init) {
> + if (level >= pag->pagf_levels[btnum])
> return __this_address;
> - break;
> - case cpu_to_be32(XFS_ABTC_CRC_MAGIC):
> - fa = xfs_btree_sblock_v5hdr_verify(bp);
> - if (fa)
> - return fa;
> - /* fall through */
> - case cpu_to_be32(XFS_ABTC_MAGIC):
> - if (pag && pag->pagf_init) {
> - if (level >= pag->pagf_levels[XFS_BTNUM_CNTi])
> - return __this_address;
> - } else if (level >= mp->m_ag_maxlevels)
> - return __this_address;
> - break;
> - default:
> + } else if (level >= mp->m_ag_maxlevels)
> return __this_address;
> - }
>
> return xfs_btree_sblock_verify(bp, mp->m_alloc_mxr[level != 0]);
> }
> @@ -379,6 +365,8 @@ xfs_allocbt_write_verify(
>
> const struct xfs_buf_ops xfs_bnobt_buf_ops = {
> .name = "xfs_bnobt",
> + .magic = { cpu_to_be32(XFS_ABTB_MAGIC),
> + cpu_to_be32(XFS_ABTB_CRC_MAGIC) },
> .verify_read = xfs_allocbt_read_verify,
> .verify_write = xfs_allocbt_write_verify,
> .verify_struct = xfs_allocbt_verify,
> @@ -386,6 +374,8 @@ const struct xfs_buf_ops xfs_bnobt_buf_ops = {
>
> const struct xfs_buf_ops xfs_cntbt_buf_ops = {
> .name = "xfs_cntbt",
> + .magic = { cpu_to_be32(XFS_ABTC_MAGIC),
> + cpu_to_be32(XFS_ABTC_CRC_MAGIC) },
> .verify_read = xfs_allocbt_read_verify,
> .verify_write = xfs_allocbt_write_verify,
> .verify_struct = xfs_allocbt_verify,
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 9/9] xfs: factor xfs_da3_blkinfo verification into common helper
2019-02-04 14:52 ` [PATCH v3 9/9] xfs: factor xfs_da3_blkinfo verification into common helper Brian Foster
@ 2019-02-06 18:16 ` Darrick J. Wong
0 siblings, 0 replies; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:16 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:31AM -0500, Brian Foster wrote:
> With the verifier magic value helper in place, we've left a bit more
> duplicate code across the verifiers that involve struct
> xfs_da3_blkinfo. This includes the da node, xattr leaf and dir leaf
> verifiers, all of which perform similar checks for v4 and v5
> filesystems.
>
> Create a common helper to verify an xfs_da3_blkinfo structure,
> taking care to only access v5 fields where appropriate, and refactor
> the aforementioned verifiers to use the helper. No functional
> changes.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
Seems ok modulo the questions I had about the ASSERT,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_attr_leaf.c | 17 +++----------
> fs/xfs/libxfs/xfs_da_btree.c | 46 +++++++++++++++++++++++++----------
> fs/xfs/libxfs/xfs_da_format.h | 3 +++
> fs/xfs/libxfs/xfs_dir2_leaf.c | 18 +++-----------
> 4 files changed, 44 insertions(+), 40 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index dde3a550d7ce..0c92987f57fc 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -245,23 +245,14 @@ xfs_attr3_leaf_verify(
> struct xfs_attr_leaf_entry *entries;
> uint32_t end; /* must be 32bit - see below */
> int i;
> + xfs_failaddr_t fa;
>
> xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>
> - if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> - return __this_address;
> -
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> - struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
> + fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> + if (fa)
> + return fa;
>
> - ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
> - if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> - return __this_address;
> - if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> - return __this_address;
> - if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> - return __this_address;
> - }
> /*
> * In recovery there is a transient state where count == 0 is valid
> * because we may have transitioned an empty shortform attr to a leaf
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 6b8ca390eecc..622f88a1b1aa 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -116,6 +116,35 @@ xfs_da_state_free(xfs_da_state_t *state)
> kmem_zone_free(xfs_da_state_zone, state);
> }
>
> +/*
> + * Verify an xfs_da3_blkinfo structure. Note that the da3 fields are only
> + * accessible on v5 filesystems. This header format is common across da node,
> + * attr leaf and dir leaf blocks.
> + */
> +xfs_failaddr_t
> +xfs_da3_blkinfo_verify(
> + struct xfs_buf *bp,
> + struct xfs_da3_blkinfo *hdr3)
> +{
> + struct xfs_mount *mp = bp->b_target->bt_mount;
> + struct xfs_da_blkinfo *hdr = &hdr3->hdr;
> +
> + if (!xfs_verify_magic(bp, hdr->magic))
> + return __this_address;
> +
> + if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + ASSERT(hdr3->hdr.magic == hdr->magic);
> + if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> + return __this_address;
> + if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> + return __this_address;
> + if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> + return __this_address;
> + }
> +
> + return 0;
> +}
> +
> static xfs_failaddr_t
> xfs_da3_node_verify(
> struct xfs_buf *bp)
> @@ -124,25 +153,16 @@ xfs_da3_node_verify(
> struct xfs_da_intnode *hdr = bp->b_addr;
> struct xfs_da3_icnode_hdr ichdr;
> const struct xfs_dir_ops *ops;
> + xfs_failaddr_t fa;
>
> ops = xfs_dir_get_ops(mp, NULL);
>
> ops->node_hdr_from_disk(&ichdr, hdr);
>
> - if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
> - return __this_address;
> + fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> + if (fa)
> + return fa;
>
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> - struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
> -
> - ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
> - if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> - return __this_address;
> - if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> - return __this_address;
> - if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> - return __this_address;
> - }
> if (ichdr.level == 0)
> return __this_address;
> if (ichdr.level > XFS_DA_NODE_MAXDEPTH)
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 5d5bf3bffc78..ae654e06b2fb 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -869,4 +869,7 @@ static inline unsigned int xfs_dir2_dirblock_bytes(struct xfs_sb *sbp)
> return 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
> }
>
> +xfs_failaddr_t xfs_da3_blkinfo_verify(struct xfs_buf *bp,
> + struct xfs_da3_blkinfo *hdr3);
> +
> #endif /* __XFS_DA_FORMAT_H__ */
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index a99ae2cd292e..094028b7b162 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -146,21 +146,11 @@ xfs_dir3_leaf_verify(
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_dir2_leaf *leaf = bp->b_addr;
> + xfs_failaddr_t fa;
>
> - if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> - return __this_address;
> -
> - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> - struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> -
> - ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> - if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> - return __this_address;
> - if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> - return __this_address;
> - if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> - return __this_address;
> - }
> + fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> + if (fa)
> + return fa;
>
> return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> }
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 0/9] xfs: fix [f]inobt magic value verification
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
` (8 preceding siblings ...)
2019-02-04 14:52 ` [PATCH v3 9/9] xfs: factor xfs_da3_blkinfo verification into common helper Brian Foster
@ 2019-02-06 18:23 ` Darrick J. Wong
2019-02-06 18:50 ` Brian Foster
9 siblings, 1 reply; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 18:23 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 04, 2019 at 09:52:22AM -0500, Brian Foster wrote:
> Hi all,
>
> Here's v3 of the verifier magic value fixup series. This version adds a
> couple more patches to separate and convert the allocation btree buffer
> verifier to the new magic value verification scheme. The allocbt is
> similar to the inobt verifier in that it currently uses the same
> verifier code for multiple magic values. This change allows the verifier
> to ensure that bnobt/cntbt blocks have the appropriate magic value for
> v4 and v5 filesytems. The only other change from v2 is a fixup to
> include parameter names in function declarations.
>
> Thoughts, reviews, flames appreciated.
I noticed that inode and quota buf_ops still don't have magic numbers in
them. I think it's fairly trivial to add them to the buffer ops
definitions, though actually using them looks a little tricky for
dquots. I'll send along a couple of patches to add that and clean up
some of the repair code magic handling.
--D
> Brian
>
> v3:
> - Fix function declarations to include parameter names.
> - Convert allocbt buffer verifier.
> v2: https://marc.info/?l=linux-xfs&m=154894958207167&w=2
> - Include djwong's ->b_ops patch w/ modifications.
> - Added patch to fix up existing cpu endian magic checks, fold in typo
> fix.
> - Replace static inline magic verifier helper with out of line variant,
> kill macro.
> - Store on-disk byte order magics in ->b_ops.
> - Added patch to refactor common xfs_da3_blkinfo checks.
> v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
> - Remove endian conversion from helper.
> - Drop finobt bad magic mitigation patch.
> - Additional verifier magic fixups.
> - Add verifier name typo fixup.
> rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
> - Split off finobt verifier into separate patch, assign it
> appropriately.
> - Created helpers for xfs_buf_ops magic value verification.
> - Added error mitigation patch for problematic finobt blocks.
> rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
>
> Brian Foster (8):
> xfs: always check magic values in on-disk byte order
> xfs: create a separate finobt verifier
> xfs: distinguish between inobt and finobt magic values
> xfs: split up allocation btree verifier
> xfs: distinguish between bnobt and cntbt magic values
> xfs: use verifier magic field in dir2 leaf verifiers
> xfs: miscellaneous verifier magic value fixups
> xfs: factor xfs_da3_blkinfo verification into common helper
>
> Darrick J. Wong (1):
> xfs: set buffer ops when repair probes for btree type
>
> fs/xfs/libxfs/xfs_ag.c | 6 +-
> fs/xfs/libxfs/xfs_alloc.c | 12 ++--
> fs/xfs/libxfs/xfs_alloc_btree.c | 74 ++++++++++-----------
> fs/xfs/libxfs/xfs_attr_leaf.c | 21 ++----
> fs/xfs/libxfs/xfs_attr_remote.c | 8 ++-
> fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++--
> fs/xfs/libxfs/xfs_da_btree.c | 50 ++++++++++-----
> fs/xfs/libxfs/xfs_da_format.h | 3 +
> fs/xfs/libxfs/xfs_dir2_block.c | 10 +--
> fs/xfs/libxfs/xfs_dir2_data.c | 12 ++--
> fs/xfs/libxfs/xfs_dir2_leaf.c | 100 ++++++-----------------------
> fs/xfs/libxfs/xfs_dir2_node.c | 11 ++--
> fs/xfs/libxfs/xfs_ialloc.c | 3 +-
> fs/xfs/libxfs/xfs_ialloc_btree.c | 25 +++++---
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_refcount_btree.c | 3 +-
> fs/xfs/libxfs/xfs_rmap_btree.c | 3 +-
> fs/xfs/libxfs/xfs_sb.c | 5 +-
> fs/xfs/libxfs/xfs_shared.h | 4 +-
> fs/xfs/libxfs/xfs_symlink_remote.c | 3 +-
> fs/xfs/scrub/agheader_repair.c | 6 +-
> fs/xfs/scrub/repair.c | 11 +++-
> fs/xfs/xfs_buf.c | 41 ++++++++++--
> fs/xfs/xfs_buf.h | 4 +-
> fs/xfs/xfs_log_recover.c | 12 ++--
> fs/xfs/xfs_trans_buf.c | 2 +-
> 26 files changed, 229 insertions(+), 215 deletions(-)
>
> --
> 2.17.2
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type
2019-02-06 18:08 ` Darrick J. Wong
@ 2019-02-06 18:40 ` Brian Foster
0 siblings, 0 replies; 27+ messages in thread
From: Brian Foster @ 2019-02-06 18:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 10:08:35AM -0800, Darrick J. Wong wrote:
> On Mon, Feb 04, 2019 at 09:52:23AM -0500, Brian Foster wrote:
> > From: "Darrick J. Wong" <darrick.wong@oracle.com>
> >
> > In xrep_findroot_block, we work out the btree type and correctness of a
> > given block by calling different btree verifiers on root block
> > candidates. However, we leave the NULL b_ops while ->verify_read
> > validates the block, which means that if the verifier calls
> > xfs_buf_verifier_error it'll crash on the null b_ops. Fix it to set
> > b_ops before calling the verifier and unsetting it if the verifier
> > fails.
> >
> > Furthermore, improve the documentation around xfs_buf_ensure_ops, which
> > is the function that is responsible for cleaning up the b_ops state of
> > buffers that go through xrep_findroot_block but don't match anything.
> >
> > [bfoster: Renamed xfs_buf_ensure_ops() and modified comment.]
>
> Heh, I already queued up the original patch for 5.0 so that we don't
> oops the kernel. :/
>
Yeah, I noticed.. not really a big deal.
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > fs/xfs/scrub/repair.c | 11 ++++++++---
> > fs/xfs/xfs_buf.c | 22 ++++++++++++++++------
> > fs/xfs/xfs_buf.h | 2 +-
> > fs/xfs/xfs_trans_buf.c | 2 +-
> > 4 files changed, 26 insertions(+), 11 deletions(-)
> >
...
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index eedc5e0156ff..222b5260ed35 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -776,13 +776,23 @@ _xfs_buf_read(
> > }
> >
> > /*
> > - * If the caller passed in an ops structure and the buffer doesn't have ops
> > - * assigned, set the ops and use them to verify the contents. If the contents
> > - * cannot be verified, we'll clear XBF_DONE. We assume the buffer has no
> > - * recorded errors and is already in XBF_DONE state.
> > + * Reverify a buffer found in cache without an attached ->b_ops.
> > + *
> > + * If the caller passed an ops structure and the buffer doesn't have ops
> > + * assigned, set the ops and use it to verify the contents. If verification
> > + * fails, clear XBF_DONE. We assume the buffer has no recorded errors and is
> > + * already in XBF_DONE state on entry.
> > + *
> > + * Under normal operations, every in-core buffer is verified on read I/O
> > + * completion. There are two scenarios that can lead to in-core buffers without
> > + * an assigned ->b_ops. The first is during log recovery of buffers on a V4
> > + * filesystem, though these buffers are purged at the end of recovery. The
> > + * other is online repair, which intentionally reads with a NULL buffer ops to
> > + * run several verifiers across an in-core buffer in order to establish buffer
> > + * type.
>
> I would like to add a sentence here explicitly saying that if repair
> can't establish the buffer type from the verifiers then the buffer is
> left in memory with null b_ops.
>
Ok.
> > */
> > int
> > -xfs_buf_ensure_ops(
> > +xfs_buf_reverify(
>
> I like the name and the comment better than my original version, so I'll
> rework this patch as a separate "rename and unfrobulate documentation"
> thing and send it out.
>
Sounds good to me. Thanks.
Brian
> --D
>
> > struct xfs_buf *bp,
> > const struct xfs_buf_ops *ops)
> > {
> > @@ -824,7 +834,7 @@ xfs_buf_read_map(
> > return bp;
> > }
> >
> > - xfs_buf_ensure_ops(bp, ops);
> > + xfs_buf_reverify(bp, ops);
> >
> > if (flags & XBF_ASYNC) {
> > /*
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index b9f5511ea998..1c436a85b71d 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -385,6 +385,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> > #define xfs_getsize_buftarg(buftarg) block_size((buftarg)->bt_bdev)
> > #define xfs_readonly_buftarg(buftarg) bdev_read_only((buftarg)->bt_bdev)
> >
> > -int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> > +int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> >
> > #endif /* __XFS_BUF_H__ */
> > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> > index 629f1479c9d2..7d65ebf1e847 100644
> > --- a/fs/xfs/xfs_trans_buf.c
> > +++ b/fs/xfs/xfs_trans_buf.c
> > @@ -277,7 +277,7 @@ xfs_trans_read_buf_map(
> > * release this buffer when it kills the tranaction.
> > */
> > ASSERT(bp->b_ops != NULL);
> > - error = xfs_buf_ensure_ops(bp, ops);
> > + error = xfs_buf_reverify(bp, ops);
> > if (error) {
> > xfs_buf_ioerror_alert(bp, __func__);
> >
> > --
> > 2.17.2
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers
2019-02-06 17:45 ` Darrick J. Wong
@ 2019-02-06 18:41 ` Brian Foster
2019-02-06 19:03 ` Darrick J. Wong
0 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-06 18:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 09:45:26AM -0800, Darrick J. Wong wrote:
> On Mon, Feb 04, 2019 at 09:52:29AM -0500, Brian Foster wrote:
> > The dir2 leaf verifiers share the same underlying structure
> > verification code, but implement six accessor functions to multiplex
> > the code across the two verifiers. Further, the magic value isn't
> > sufficiently abstracted such that the common helper has to manually
> > fix up the magic from the caller on v5 filesystems.
> >
> > Use the magic field in the verifier structure to eliminate the
> > duplicate code and clean this all up. No functional change.
> >
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
> > 1 file changed, 20 insertions(+), 68 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > index 1728a3e6f5cf..a99ae2cd292e 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> > */
> > static xfs_failaddr_t
> > xfs_dir3_leaf_verify(
> > - struct xfs_buf *bp,
> > - uint16_t magic)
> > + struct xfs_buf *bp)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > struct xfs_dir2_leaf *leaf = bp->b_addr;
> >
> > - ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > + if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> > + return __this_address;
> >
> > if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > - uint16_t magic3;
> >
> > - magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > - : XFS_DIR3_LEAFN_MAGIC;
> > -
> > - if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > - return __this_address;
> > + ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
>
> leaf2 and leaf3 directory block headers are supposed to have the magic
> at the same offset in the buffer, right? When would this assert fail?
>
Hopefully never.. ;P I added the assert as a mechanical defense measure
simply because these are technically different structures and this
refactoring dictates that we access the magic value through one of the
two rather than both independently. I just wanted to make sure that this
dependency was encoded somewhere because it's not obvious in the code.
> Could we replace this with a BUILD_BUG_ON check of the leaf2/leaf3 magic
> number buffer offset in xfs_ondisk.h?
>
BUILD_BUG_ON() probably makes more sense for this than an assert in
principle. Is there a clean enough way to encode the offset checks
through multiple structures though? We could do something with NULL
pointers:
BUILD_BUG_ON(&((struct xfs_da3_blkinfo *)NULL)->hdr.magic !=
&((struct xfs_da_blkinfo *)NULL)->magic);
... to check the offsets, but that's ugly. I'm not sure manually adding
up the offsetof() results is any better. That said, after this code is
refactored by the last patch this particular instance could probably be
reduced to a simple:
BUILD_BUG_ON(offsetof(struct xfs_da3_blkinfo, hdr) != 0);
... in xfs_da3_blkinfo_verify(). So perhaps the right approach is just
to add a separate BUILD_BUG_ON() for each such layer in the
encapsulating structures. I'll take a closer look at that and see how
far I get.
Also, any particular reason to put it in xfs_ondisk.h vs. where the
asserts currently are? To me this is more context for the verifier code
than some broader requirement (since we're explicitly checking the magic
field), but maybe there are broader header alignment/offset expectations
elsewhere too.
Brian
> --D
>
> > if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > return __this_address;
> > if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > return __this_address;
> > if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > return __this_address;
> > - } else {
> > - if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > - return __this_address;
> > }
> >
> > return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> > }
> >
> > static void
> > -__read_verify(
> > - struct xfs_buf *bp,
> > - uint16_t magic)
> > +xfs_dir3_leaf_read_verify(
> > + struct xfs_buf *bp)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > xfs_failaddr_t fa;
> > @@ -185,23 +176,22 @@ __read_verify(
> > !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
> > xfs_verifier_error(bp, -EFSBADCRC, __this_address);
> > else {
> > - fa = xfs_dir3_leaf_verify(bp, magic);
> > + fa = xfs_dir3_leaf_verify(bp);
> > if (fa)
> > xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > }
> > }
> >
> > static void
> > -__write_verify(
> > - struct xfs_buf *bp,
> > - uint16_t magic)
> > +xfs_dir3_leaf_write_verify(
> > + struct xfs_buf *bp)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > struct xfs_buf_log_item *bip = bp->b_log_item;
> > struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
> > xfs_failaddr_t fa;
> >
> > - fa = xfs_dir3_leaf_verify(bp, magic);
> > + fa = xfs_dir3_leaf_verify(bp);
> > if (fa) {
> > xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > return;
> > @@ -216,60 +206,22 @@ __write_verify(
> > xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
> > }
> >
> > -static xfs_failaddr_t
> > -xfs_dir3_leaf1_verify(
> > - struct xfs_buf *bp)
> > -{
> > - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > -}
> > -
> > -static void
> > -xfs_dir3_leaf1_read_verify(
> > - struct xfs_buf *bp)
> > -{
> > - __read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > -}
> > -
> > -static void
> > -xfs_dir3_leaf1_write_verify(
> > - struct xfs_buf *bp)
> > -{
> > - __write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > -}
> > -
> > -static xfs_failaddr_t
> > -xfs_dir3_leafn_verify(
> > - struct xfs_buf *bp)
> > -{
> > - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > -}
> > -
> > -static void
> > -xfs_dir3_leafn_read_verify(
> > - struct xfs_buf *bp)
> > -{
> > - __read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > -}
> > -
> > -static void
> > -xfs_dir3_leafn_write_verify(
> > - struct xfs_buf *bp)
> > -{
> > - __write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > -}
> > -
> > const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> > .name = "xfs_dir3_leaf1",
> > - .verify_read = xfs_dir3_leaf1_read_verify,
> > - .verify_write = xfs_dir3_leaf1_write_verify,
> > - .verify_struct = xfs_dir3_leaf1_verify,
> > + .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> > + cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> > + .verify_read = xfs_dir3_leaf_read_verify,
> > + .verify_write = xfs_dir3_leaf_write_verify,
> > + .verify_struct = xfs_dir3_leaf_verify,
> > };
> >
> > const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> > .name = "xfs_dir3_leafn",
> > - .verify_read = xfs_dir3_leafn_read_verify,
> > - .verify_write = xfs_dir3_leafn_write_verify,
> > - .verify_struct = xfs_dir3_leafn_verify,
> > + .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> > + cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> > + .verify_read = xfs_dir3_leaf_read_verify,
> > + .verify_write = xfs_dir3_leaf_write_verify,
> > + .verify_struct = xfs_dir3_leaf_verify,
> > };
> >
> > int
> > --
> > 2.17.2
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 0/9] xfs: fix [f]inobt magic value verification
2019-02-06 18:23 ` [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Darrick J. Wong
@ 2019-02-06 18:50 ` Brian Foster
2019-02-06 19:05 ` Darrick J. Wong
0 siblings, 1 reply; 27+ messages in thread
From: Brian Foster @ 2019-02-06 18:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 10:23:31AM -0800, Darrick J. Wong wrote:
> On Mon, Feb 04, 2019 at 09:52:22AM -0500, Brian Foster wrote:
> > Hi all,
> >
> > Here's v3 of the verifier magic value fixup series. This version adds a
> > couple more patches to separate and convert the allocation btree buffer
> > verifier to the new magic value verification scheme. The allocbt is
> > similar to the inobt verifier in that it currently uses the same
> > verifier code for multiple magic values. This change allows the verifier
> > to ensure that bnobt/cntbt blocks have the appropriate magic value for
> > v4 and v5 filesytems. The only other change from v2 is a fixup to
> > include parameter names in function declarations.
> >
> > Thoughts, reviews, flames appreciated.
>
> I noticed that inode and quota buf_ops still don't have magic numbers in
> them. I think it's fairly trivial to add them to the buffer ops
> definitions, though actually using them looks a little tricky for
> dquots. I'll send along a couple of patches to add that and clean up
> some of the repair code magic handling.
>
The inode one looks pretty straightforward. I didn't think it worth the
refactoring for the dquot ops, but it could be done if consistency is
important. I guess I never really viewed this as a wholesale change as
opposed to an optional mechanism to be used where needed (and/or
convenient).
Brian
> --D
>
> > Brian
> >
> > v3:
> > - Fix function declarations to include parameter names.
> > - Convert allocbt buffer verifier.
> > v2: https://marc.info/?l=linux-xfs&m=154894958207167&w=2
> > - Include djwong's ->b_ops patch w/ modifications.
> > - Added patch to fix up existing cpu endian magic checks, fold in typo
> > fix.
> > - Replace static inline magic verifier helper with out of line variant,
> > kill macro.
> > - Store on-disk byte order magics in ->b_ops.
> > - Added patch to refactor common xfs_da3_blkinfo checks.
> > v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
> > - Remove endian conversion from helper.
> > - Drop finobt bad magic mitigation patch.
> > - Additional verifier magic fixups.
> > - Add verifier name typo fixup.
> > rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
> > - Split off finobt verifier into separate patch, assign it
> > appropriately.
> > - Created helpers for xfs_buf_ops magic value verification.
> > - Added error mitigation patch for problematic finobt blocks.
> > rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
> >
> > Brian Foster (8):
> > xfs: always check magic values in on-disk byte order
> > xfs: create a separate finobt verifier
> > xfs: distinguish between inobt and finobt magic values
> > xfs: split up allocation btree verifier
> > xfs: distinguish between bnobt and cntbt magic values
> > xfs: use verifier magic field in dir2 leaf verifiers
> > xfs: miscellaneous verifier magic value fixups
> > xfs: factor xfs_da3_blkinfo verification into common helper
> >
> > Darrick J. Wong (1):
> > xfs: set buffer ops when repair probes for btree type
> >
> > fs/xfs/libxfs/xfs_ag.c | 6 +-
> > fs/xfs/libxfs/xfs_alloc.c | 12 ++--
> > fs/xfs/libxfs/xfs_alloc_btree.c | 74 ++++++++++-----------
> > fs/xfs/libxfs/xfs_attr_leaf.c | 21 ++----
> > fs/xfs/libxfs/xfs_attr_remote.c | 8 ++-
> > fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++--
> > fs/xfs/libxfs/xfs_da_btree.c | 50 ++++++++++-----
> > fs/xfs/libxfs/xfs_da_format.h | 3 +
> > fs/xfs/libxfs/xfs_dir2_block.c | 10 +--
> > fs/xfs/libxfs/xfs_dir2_data.c | 12 ++--
> > fs/xfs/libxfs/xfs_dir2_leaf.c | 100 ++++++-----------------------
> > fs/xfs/libxfs/xfs_dir2_node.c | 11 ++--
> > fs/xfs/libxfs/xfs_ialloc.c | 3 +-
> > fs/xfs/libxfs/xfs_ialloc_btree.c | 25 +++++---
> > fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> > fs/xfs/libxfs/xfs_refcount_btree.c | 3 +-
> > fs/xfs/libxfs/xfs_rmap_btree.c | 3 +-
> > fs/xfs/libxfs/xfs_sb.c | 5 +-
> > fs/xfs/libxfs/xfs_shared.h | 4 +-
> > fs/xfs/libxfs/xfs_symlink_remote.c | 3 +-
> > fs/xfs/scrub/agheader_repair.c | 6 +-
> > fs/xfs/scrub/repair.c | 11 +++-
> > fs/xfs/xfs_buf.c | 41 ++++++++++--
> > fs/xfs/xfs_buf.h | 4 +-
> > fs/xfs/xfs_log_recover.c | 12 ++--
> > fs/xfs/xfs_trans_buf.c | 2 +-
> > 26 files changed, 229 insertions(+), 215 deletions(-)
> >
> > --
> > 2.17.2
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers
2019-02-06 18:41 ` Brian Foster
@ 2019-02-06 19:03 ` Darrick J. Wong
2019-02-06 19:18 ` Brian Foster
0 siblings, 1 reply; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 19:03 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 01:41:22PM -0500, Brian Foster wrote:
> On Wed, Feb 06, 2019 at 09:45:26AM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 04, 2019 at 09:52:29AM -0500, Brian Foster wrote:
> > > The dir2 leaf verifiers share the same underlying structure
> > > verification code, but implement six accessor functions to multiplex
> > > the code across the two verifiers. Further, the magic value isn't
> > > sufficiently abstracted such that the common helper has to manually
> > > fix up the magic from the caller on v5 filesystems.
> > >
> > > Use the magic field in the verifier structure to eliminate the
> > > duplicate code and clean this all up. No functional change.
> > >
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > > fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
> > > 1 file changed, 20 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > index 1728a3e6f5cf..a99ae2cd292e 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> > > */
> > > static xfs_failaddr_t
> > > xfs_dir3_leaf_verify(
> > > - struct xfs_buf *bp,
> > > - uint16_t magic)
> > > + struct xfs_buf *bp)
> > > {
> > > struct xfs_mount *mp = bp->b_target->bt_mount;
> > > struct xfs_dir2_leaf *leaf = bp->b_addr;
> > >
> > > - ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > > + if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> > > + return __this_address;
> > >
> > > if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > > struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > > - uint16_t magic3;
> > >
> > > - magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > > - : XFS_DIR3_LEAFN_MAGIC;
> > > -
> > > - if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > > - return __this_address;
> > > + ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> >
> > leaf2 and leaf3 directory block headers are supposed to have the magic
> > at the same offset in the buffer, right? When would this assert fail?
> >
>
> Hopefully never.. ;P I added the assert as a mechanical defense measure
> simply because these are technically different structures and this
> refactoring dictates that we access the magic value through one of the
> two rather than both independently. I just wanted to make sure that this
> dependency was encoded somewhere because it's not obvious in the code.
<nod>
> > Could we replace this with a BUILD_BUG_ON check of the leaf2/leaf3 magic
> > number buffer offset in xfs_ondisk.h?
> >
>
> BUILD_BUG_ON() probably makes more sense for this than an assert in
> principle. Is there a clean enough way to encode the offset checks
> through multiple structures though? We could do something with NULL
> pointers:
>
> BUILD_BUG_ON(&((struct xfs_da3_blkinfo *)NULL)->hdr.magic !=
> &((struct xfs_da_blkinfo *)NULL)->magic);
>
> ... to check the offsets, but that's ugly. I'm not sure manually adding
> up the offsetof() results is any better. That said, after this code is
> refactored by the last patch this particular instance could probably be
> reduced to a simple:
>
> BUILD_BUG_ON(offsetof(struct xfs_da3_blkinfo, hdr) != 0);
>
> ... in xfs_da3_blkinfo_verify(). So perhaps the right approach is just
> to add a separate BUILD_BUG_ON() for each such layer in the
> encapsulating structures. I'll take a closer look at that and see how
> far I get.
<nod> afaict offsetof can look through substructures, since it all just
turns into a bunch of pointer arithmetic goop.
> Also, any particular reason to put it in xfs_ondisk.h vs. where the
> asserts currently are?
Hmm... we established xfs_ondisk.h to contain all the offset and
structure size checks so that they'd all be in one place instead of
scattered around in the verifiers and whatnot. The macro soup emits
prettier diagnostic data in case someone on an unfamiliar arch hits a
build error and reports it. So if we add this to xfs_ondisk.h:
XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic, 8);
XFS_CHECK_OFFSET(struct xfs_dir2_leaf_hdr, info.magic, 8);
Then the compiler will emit:
include/linux/compiler.h:344:38: error: call to
‘__compiletime_assert_58’ declared with attribute error: XFS:
offsetof(struct xfs_dir3_leaf_hdr, info.hdr.magic) is wrong, expected 8
I guess the strongest argument I have for xfs_ondisk.h is because
"that's where all the other offset and size build checks go".
> To me this is more context for the verifier code
> than some broader requirement (since we're explicitly checking the magic
> field), but maybe there are broader header alignment/offset expectations
> elsewhere too.
Heh, I think this is a hair-splitting thing: are we checking that the
offsets are the same (which indeed is a relational thing that ought to
be in the verifier), or checking that ttwo offsets equal some value,
where the value just happens to be the same number?
--D
> Brian
>
> > --D
> >
> > > if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > > return __this_address;
> > > if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > > return __this_address;
> > > if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > > return __this_address;
> > > - } else {
> > > - if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > > - return __this_address;
> > > }
> > >
> > > return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> > > }
> > >
> > > static void
> > > -__read_verify(
> > > - struct xfs_buf *bp,
> > > - uint16_t magic)
> > > +xfs_dir3_leaf_read_verify(
> > > + struct xfs_buf *bp)
> > > {
> > > struct xfs_mount *mp = bp->b_target->bt_mount;
> > > xfs_failaddr_t fa;
> > > @@ -185,23 +176,22 @@ __read_verify(
> > > !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
> > > xfs_verifier_error(bp, -EFSBADCRC, __this_address);
> > > else {
> > > - fa = xfs_dir3_leaf_verify(bp, magic);
> > > + fa = xfs_dir3_leaf_verify(bp);
> > > if (fa)
> > > xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > > }
> > > }
> > >
> > > static void
> > > -__write_verify(
> > > - struct xfs_buf *bp,
> > > - uint16_t magic)
> > > +xfs_dir3_leaf_write_verify(
> > > + struct xfs_buf *bp)
> > > {
> > > struct xfs_mount *mp = bp->b_target->bt_mount;
> > > struct xfs_buf_log_item *bip = bp->b_log_item;
> > > struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
> > > xfs_failaddr_t fa;
> > >
> > > - fa = xfs_dir3_leaf_verify(bp, magic);
> > > + fa = xfs_dir3_leaf_verify(bp);
> > > if (fa) {
> > > xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > > return;
> > > @@ -216,60 +206,22 @@ __write_verify(
> > > xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
> > > }
> > >
> > > -static xfs_failaddr_t
> > > -xfs_dir3_leaf1_verify(
> > > - struct xfs_buf *bp)
> > > -{
> > > - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > -}
> > > -
> > > -static void
> > > -xfs_dir3_leaf1_read_verify(
> > > - struct xfs_buf *bp)
> > > -{
> > > - __read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > -}
> > > -
> > > -static void
> > > -xfs_dir3_leaf1_write_verify(
> > > - struct xfs_buf *bp)
> > > -{
> > > - __write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > -}
> > > -
> > > -static xfs_failaddr_t
> > > -xfs_dir3_leafn_verify(
> > > - struct xfs_buf *bp)
> > > -{
> > > - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > -}
> > > -
> > > -static void
> > > -xfs_dir3_leafn_read_verify(
> > > - struct xfs_buf *bp)
> > > -{
> > > - __read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > -}
> > > -
> > > -static void
> > > -xfs_dir3_leafn_write_verify(
> > > - struct xfs_buf *bp)
> > > -{
> > > - __write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > -}
> > > -
> > > const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> > > .name = "xfs_dir3_leaf1",
> > > - .verify_read = xfs_dir3_leaf1_read_verify,
> > > - .verify_write = xfs_dir3_leaf1_write_verify,
> > > - .verify_struct = xfs_dir3_leaf1_verify,
> > > + .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> > > + cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> > > + .verify_read = xfs_dir3_leaf_read_verify,
> > > + .verify_write = xfs_dir3_leaf_write_verify,
> > > + .verify_struct = xfs_dir3_leaf_verify,
> > > };
> > >
> > > const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> > > .name = "xfs_dir3_leafn",
> > > - .verify_read = xfs_dir3_leafn_read_verify,
> > > - .verify_write = xfs_dir3_leafn_write_verify,
> > > - .verify_struct = xfs_dir3_leafn_verify,
> > > + .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> > > + cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> > > + .verify_read = xfs_dir3_leaf_read_verify,
> > > + .verify_write = xfs_dir3_leaf_write_verify,
> > > + .verify_struct = xfs_dir3_leaf_verify,
> > > };
> > >
> > > int
> > > --
> > > 2.17.2
> > >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 0/9] xfs: fix [f]inobt magic value verification
2019-02-06 18:50 ` Brian Foster
@ 2019-02-06 19:05 ` Darrick J. Wong
2019-02-06 19:18 ` Brian Foster
0 siblings, 1 reply; 27+ messages in thread
From: Darrick J. Wong @ 2019-02-06 19:05 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 01:50:55PM -0500, Brian Foster wrote:
> On Wed, Feb 06, 2019 at 10:23:31AM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 04, 2019 at 09:52:22AM -0500, Brian Foster wrote:
> > > Hi all,
> > >
> > > Here's v3 of the verifier magic value fixup series. This version adds a
> > > couple more patches to separate and convert the allocation btree buffer
> > > verifier to the new magic value verification scheme. The allocbt is
> > > similar to the inobt verifier in that it currently uses the same
> > > verifier code for multiple magic values. This change allows the verifier
> > > to ensure that bnobt/cntbt blocks have the appropriate magic value for
> > > v4 and v5 filesytems. The only other change from v2 is a fixup to
> > > include parameter names in function declarations.
> > >
> > > Thoughts, reviews, flames appreciated.
> >
> > I noticed that inode and quota buf_ops still don't have magic numbers in
> > them. I think it's fairly trivial to add them to the buffer ops
> > definitions, though actually using them looks a little tricky for
> > dquots. I'll send along a couple of patches to add that and clean up
> > some of the repair code magic handling.
> >
>
> The inode one looks pretty straightforward. I didn't think it worth the
> refactoring for the dquot ops, but it could be done if consistency is
> important. I guess I never really viewed this as a wholesale change as
> opposed to an optional mechanism to be used where needed (and/or
> convenient).
My dquot patch just adds the magic numbers for consistency, so we don't
leave a logic bomb for someone who sees that most of the buf_ops have
magic numbers and assumes that they all do.
--D
> Brian
>
> > --D
> >
> > > Brian
> > >
> > > v3:
> > > - Fix function declarations to include parameter names.
> > > - Convert allocbt buffer verifier.
> > > v2: https://marc.info/?l=linux-xfs&m=154894958207167&w=2
> > > - Include djwong's ->b_ops patch w/ modifications.
> > > - Added patch to fix up existing cpu endian magic checks, fold in typo
> > > fix.
> > > - Replace static inline magic verifier helper with out of line variant,
> > > kill macro.
> > > - Store on-disk byte order magics in ->b_ops.
> > > - Added patch to refactor common xfs_da3_blkinfo checks.
> > > v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
> > > - Remove endian conversion from helper.
> > > - Drop finobt bad magic mitigation patch.
> > > - Additional verifier magic fixups.
> > > - Add verifier name typo fixup.
> > > rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
> > > - Split off finobt verifier into separate patch, assign it
> > > appropriately.
> > > - Created helpers for xfs_buf_ops magic value verification.
> > > - Added error mitigation patch for problematic finobt blocks.
> > > rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
> > >
> > > Brian Foster (8):
> > > xfs: always check magic values in on-disk byte order
> > > xfs: create a separate finobt verifier
> > > xfs: distinguish between inobt and finobt magic values
> > > xfs: split up allocation btree verifier
> > > xfs: distinguish between bnobt and cntbt magic values
> > > xfs: use verifier magic field in dir2 leaf verifiers
> > > xfs: miscellaneous verifier magic value fixups
> > > xfs: factor xfs_da3_blkinfo verification into common helper
> > >
> > > Darrick J. Wong (1):
> > > xfs: set buffer ops when repair probes for btree type
> > >
> > > fs/xfs/libxfs/xfs_ag.c | 6 +-
> > > fs/xfs/libxfs/xfs_alloc.c | 12 ++--
> > > fs/xfs/libxfs/xfs_alloc_btree.c | 74 ++++++++++-----------
> > > fs/xfs/libxfs/xfs_attr_leaf.c | 21 ++----
> > > fs/xfs/libxfs/xfs_attr_remote.c | 8 ++-
> > > fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++--
> > > fs/xfs/libxfs/xfs_da_btree.c | 50 ++++++++++-----
> > > fs/xfs/libxfs/xfs_da_format.h | 3 +
> > > fs/xfs/libxfs/xfs_dir2_block.c | 10 +--
> > > fs/xfs/libxfs/xfs_dir2_data.c | 12 ++--
> > > fs/xfs/libxfs/xfs_dir2_leaf.c | 100 ++++++-----------------------
> > > fs/xfs/libxfs/xfs_dir2_node.c | 11 ++--
> > > fs/xfs/libxfs/xfs_ialloc.c | 3 +-
> > > fs/xfs/libxfs/xfs_ialloc_btree.c | 25 +++++---
> > > fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> > > fs/xfs/libxfs/xfs_refcount_btree.c | 3 +-
> > > fs/xfs/libxfs/xfs_rmap_btree.c | 3 +-
> > > fs/xfs/libxfs/xfs_sb.c | 5 +-
> > > fs/xfs/libxfs/xfs_shared.h | 4 +-
> > > fs/xfs/libxfs/xfs_symlink_remote.c | 3 +-
> > > fs/xfs/scrub/agheader_repair.c | 6 +-
> > > fs/xfs/scrub/repair.c | 11 +++-
> > > fs/xfs/xfs_buf.c | 41 ++++++++++--
> > > fs/xfs/xfs_buf.h | 4 +-
> > > fs/xfs/xfs_log_recover.c | 12 ++--
> > > fs/xfs/xfs_trans_buf.c | 2 +-
> > > 26 files changed, 229 insertions(+), 215 deletions(-)
> > >
> > > --
> > > 2.17.2
> > >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers
2019-02-06 19:03 ` Darrick J. Wong
@ 2019-02-06 19:18 ` Brian Foster
0 siblings, 0 replies; 27+ messages in thread
From: Brian Foster @ 2019-02-06 19:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 11:03:43AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 06, 2019 at 01:41:22PM -0500, Brian Foster wrote:
> > On Wed, Feb 06, 2019 at 09:45:26AM -0800, Darrick J. Wong wrote:
> > > On Mon, Feb 04, 2019 at 09:52:29AM -0500, Brian Foster wrote:
> > > > The dir2 leaf verifiers share the same underlying structure
> > > > verification code, but implement six accessor functions to multiplex
> > > > the code across the two verifiers. Further, the magic value isn't
> > > > sufficiently abstracted such that the common helper has to manually
> > > > fix up the magic from the caller on v5 filesystems.
> > > >
> > > > Use the magic field in the verifier structure to eliminate the
> > > > duplicate code and clean this all up. No functional change.
> > > >
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > > fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
> > > > 1 file changed, 20 insertions(+), 68 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > index 1728a3e6f5cf..a99ae2cd292e 100644
> > > > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> > > > */
> > > > static xfs_failaddr_t
> > > > xfs_dir3_leaf_verify(
> > > > - struct xfs_buf *bp,
> > > > - uint16_t magic)
> > > > + struct xfs_buf *bp)
> > > > {
> > > > struct xfs_mount *mp = bp->b_target->bt_mount;
> > > > struct xfs_dir2_leaf *leaf = bp->b_addr;
> > > >
> > > > - ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > > > + if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> > > > + return __this_address;
> > > >
> > > > if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > > > struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > > > - uint16_t magic3;
> > > >
> > > > - magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > > > - : XFS_DIR3_LEAFN_MAGIC;
> > > > -
> > > > - if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > > > - return __this_address;
> > > > + ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> > >
> > > leaf2 and leaf3 directory block headers are supposed to have the magic
> > > at the same offset in the buffer, right? When would this assert fail?
> > >
> >
> > Hopefully never.. ;P I added the assert as a mechanical defense measure
> > simply because these are technically different structures and this
> > refactoring dictates that we access the magic value through one of the
> > two rather than both independently. I just wanted to make sure that this
> > dependency was encoded somewhere because it's not obvious in the code.
>
> <nod>
>
> > > Could we replace this with a BUILD_BUG_ON check of the leaf2/leaf3 magic
> > > number buffer offset in xfs_ondisk.h?
> > >
> >
> > BUILD_BUG_ON() probably makes more sense for this than an assert in
> > principle. Is there a clean enough way to encode the offset checks
> > through multiple structures though? We could do something with NULL
> > pointers:
> >
> > BUILD_BUG_ON(&((struct xfs_da3_blkinfo *)NULL)->hdr.magic !=
> > &((struct xfs_da_blkinfo *)NULL)->magic);
> >
> > ... to check the offsets, but that's ugly. I'm not sure manually adding
> > up the offsetof() results is any better. That said, after this code is
> > refactored by the last patch this particular instance could probably be
> > reduced to a simple:
> >
> > BUILD_BUG_ON(offsetof(struct xfs_da3_blkinfo, hdr) != 0);
> >
> > ... in xfs_da3_blkinfo_verify(). So perhaps the right approach is just
> > to add a separate BUILD_BUG_ON() for each such layer in the
> > encapsulating structures. I'll take a closer look at that and see how
> > far I get.
>
> <nod> afaict offsetof can look through substructures, since it all just
> turns into a bunch of pointer arithmetic goop.
>
> > Also, any particular reason to put it in xfs_ondisk.h vs. where the
> > asserts currently are?
>
> Hmm... we established xfs_ondisk.h to contain all the offset and
> structure size checks so that they'd all be in one place instead of
> scattered around in the verifiers and whatnot. The macro soup emits
> prettier diagnostic data in case someone on an unfamiliar arch hits a
> build error and reports it. So if we add this to xfs_ondisk.h:
>
> XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic, 8);
> XFS_CHECK_OFFSET(struct xfs_dir2_leaf_hdr, info.magic, 8);
>
> Then the compiler will emit:
>
> include/linux/compiler.h:344:38: error: call to
> ‘__compiletime_assert_58’ declared with attribute error: XFS:
> offsetof(struct xfs_dir3_leaf_hdr, info.hdr.magic) is wrong, expected 8
>
> I guess the strongest argument I have for xfs_ondisk.h is because
> "that's where all the other offset and size build checks go".
>
Ok. Maybe it's just simplest to hardcode the relevant offsets like this.
A comment that explains that the verifier magic checks are the primary
motivation for the magic-specific offset checks mostly addresses my
concern over context. I'll give that a shot.
> > To me this is more context for the verifier code
> > than some broader requirement (since we're explicitly checking the magic
> > field), but maybe there are broader header alignment/offset expectations
> > elsewhere too.
>
> Heh, I think this is a hair-splitting thing: are we checking that the
> offsets are the same (which indeed is a relational thing that ought to
> be in the verifier), or checking that ttwo offsets equal some value,
> where the value just happens to be the same number?
>
The intent was really to check the offsets. The implementation did that
indirectly/hackily via the value comparison.
Brian
> --D
>
> > Brian
> >
> > > --D
> > >
> > > > if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > > > return __this_address;
> > > > if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > > > return __this_address;
> > > > if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > > > return __this_address;
> > > > - } else {
> > > > - if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > > > - return __this_address;
> > > > }
> > > >
> > > > return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> > > > }
> > > >
> > > > static void
> > > > -__read_verify(
> > > > - struct xfs_buf *bp,
> > > > - uint16_t magic)
> > > > +xfs_dir3_leaf_read_verify(
> > > > + struct xfs_buf *bp)
> > > > {
> > > > struct xfs_mount *mp = bp->b_target->bt_mount;
> > > > xfs_failaddr_t fa;
> > > > @@ -185,23 +176,22 @@ __read_verify(
> > > > !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
> > > > xfs_verifier_error(bp, -EFSBADCRC, __this_address);
> > > > else {
> > > > - fa = xfs_dir3_leaf_verify(bp, magic);
> > > > + fa = xfs_dir3_leaf_verify(bp);
> > > > if (fa)
> > > > xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > > > }
> > > > }
> > > >
> > > > static void
> > > > -__write_verify(
> > > > - struct xfs_buf *bp,
> > > > - uint16_t magic)
> > > > +xfs_dir3_leaf_write_verify(
> > > > + struct xfs_buf *bp)
> > > > {
> > > > struct xfs_mount *mp = bp->b_target->bt_mount;
> > > > struct xfs_buf_log_item *bip = bp->b_log_item;
> > > > struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
> > > > xfs_failaddr_t fa;
> > > >
> > > > - fa = xfs_dir3_leaf_verify(bp, magic);
> > > > + fa = xfs_dir3_leaf_verify(bp);
> > > > if (fa) {
> > > > xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > > > return;
> > > > @@ -216,60 +206,22 @@ __write_verify(
> > > > xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
> > > > }
> > > >
> > > > -static xfs_failaddr_t
> > > > -xfs_dir3_leaf1_verify(
> > > > - struct xfs_buf *bp)
> > > > -{
> > > > - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leaf1_read_verify(
> > > > - struct xfs_buf *bp)
> > > > -{
> > > > - __read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leaf1_write_verify(
> > > > - struct xfs_buf *bp)
> > > > -{
> > > > - __write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > > -}
> > > > -
> > > > -static xfs_failaddr_t
> > > > -xfs_dir3_leafn_verify(
> > > > - struct xfs_buf *bp)
> > > > -{
> > > > - return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leafn_read_verify(
> > > > - struct xfs_buf *bp)
> > > > -{
> > > > - __read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leafn_write_verify(
> > > > - struct xfs_buf *bp)
> > > > -{
> > > > - __write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > > -}
> > > > -
> > > > const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> > > > .name = "xfs_dir3_leaf1",
> > > > - .verify_read = xfs_dir3_leaf1_read_verify,
> > > > - .verify_write = xfs_dir3_leaf1_write_verify,
> > > > - .verify_struct = xfs_dir3_leaf1_verify,
> > > > + .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> > > > + cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> > > > + .verify_read = xfs_dir3_leaf_read_verify,
> > > > + .verify_write = xfs_dir3_leaf_write_verify,
> > > > + .verify_struct = xfs_dir3_leaf_verify,
> > > > };
> > > >
> > > > const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> > > > .name = "xfs_dir3_leafn",
> > > > - .verify_read = xfs_dir3_leafn_read_verify,
> > > > - .verify_write = xfs_dir3_leafn_write_verify,
> > > > - .verify_struct = xfs_dir3_leafn_verify,
> > > > + .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> > > > + cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> > > > + .verify_read = xfs_dir3_leaf_read_verify,
> > > > + .verify_write = xfs_dir3_leaf_write_verify,
> > > > + .verify_struct = xfs_dir3_leaf_verify,
> > > > };
> > > >
> > > > int
> > > > --
> > > > 2.17.2
> > > >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 0/9] xfs: fix [f]inobt magic value verification
2019-02-06 19:05 ` Darrick J. Wong
@ 2019-02-06 19:18 ` Brian Foster
0 siblings, 0 replies; 27+ messages in thread
From: Brian Foster @ 2019-02-06 19:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Feb 06, 2019 at 11:05:00AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 06, 2019 at 01:50:55PM -0500, Brian Foster wrote:
> > On Wed, Feb 06, 2019 at 10:23:31AM -0800, Darrick J. Wong wrote:
> > > On Mon, Feb 04, 2019 at 09:52:22AM -0500, Brian Foster wrote:
> > > > Hi all,
> > > >
> > > > Here's v3 of the verifier magic value fixup series. This version adds a
> > > > couple more patches to separate and convert the allocation btree buffer
> > > > verifier to the new magic value verification scheme. The allocbt is
> > > > similar to the inobt verifier in that it currently uses the same
> > > > verifier code for multiple magic values. This change allows the verifier
> > > > to ensure that bnobt/cntbt blocks have the appropriate magic value for
> > > > v4 and v5 filesytems. The only other change from v2 is a fixup to
> > > > include parameter names in function declarations.
> > > >
> > > > Thoughts, reviews, flames appreciated.
> > >
> > > I noticed that inode and quota buf_ops still don't have magic numbers in
> > > them. I think it's fairly trivial to add them to the buffer ops
> > > definitions, though actually using them looks a little tricky for
> > > dquots. I'll send along a couple of patches to add that and clean up
> > > some of the repair code magic handling.
> > >
> >
> > The inode one looks pretty straightforward. I didn't think it worth the
> > refactoring for the dquot ops, but it could be done if consistency is
> > important. I guess I never really viewed this as a wholesale change as
> > opposed to an optional mechanism to be used where needed (and/or
> > convenient).
>
> My dquot patch just adds the magic numbers for consistency, so we don't
> leave a logic bomb for someone who sees that most of the buf_ops have
> magic numbers and assumes that they all do.
>
Ah, I see what you mean. Sounds good..
Brian
> --D
>
> > Brian
> >
> > > --D
> > >
> > > > Brian
> > > >
> > > > v3:
> > > > - Fix function declarations to include parameter names.
> > > > - Convert allocbt buffer verifier.
> > > > v2: https://marc.info/?l=linux-xfs&m=154894958207167&w=2
> > > > - Include djwong's ->b_ops patch w/ modifications.
> > > > - Added patch to fix up existing cpu endian magic checks, fold in typo
> > > > fix.
> > > > - Replace static inline magic verifier helper with out of line variant,
> > > > kill macro.
> > > > - Store on-disk byte order magics in ->b_ops.
> > > > - Added patch to refactor common xfs_da3_blkinfo checks.
> > > > v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
> > > > - Remove endian conversion from helper.
> > > > - Drop finobt bad magic mitigation patch.
> > > > - Additional verifier magic fixups.
> > > > - Add verifier name typo fixup.
> > > > rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
> > > > - Split off finobt verifier into separate patch, assign it
> > > > appropriately.
> > > > - Created helpers for xfs_buf_ops magic value verification.
> > > > - Added error mitigation patch for problematic finobt blocks.
> > > > rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
> > > >
> > > > Brian Foster (8):
> > > > xfs: always check magic values in on-disk byte order
> > > > xfs: create a separate finobt verifier
> > > > xfs: distinguish between inobt and finobt magic values
> > > > xfs: split up allocation btree verifier
> > > > xfs: distinguish between bnobt and cntbt magic values
> > > > xfs: use verifier magic field in dir2 leaf verifiers
> > > > xfs: miscellaneous verifier magic value fixups
> > > > xfs: factor xfs_da3_blkinfo verification into common helper
> > > >
> > > > Darrick J. Wong (1):
> > > > xfs: set buffer ops when repair probes for btree type
> > > >
> > > > fs/xfs/libxfs/xfs_ag.c | 6 +-
> > > > fs/xfs/libxfs/xfs_alloc.c | 12 ++--
> > > > fs/xfs/libxfs/xfs_alloc_btree.c | 74 ++++++++++-----------
> > > > fs/xfs/libxfs/xfs_attr_leaf.c | 21 ++----
> > > > fs/xfs/libxfs/xfs_attr_remote.c | 8 ++-
> > > > fs/xfs/libxfs/xfs_bmap_btree.c | 13 ++--
> > > > fs/xfs/libxfs/xfs_da_btree.c | 50 ++++++++++-----
> > > > fs/xfs/libxfs/xfs_da_format.h | 3 +
> > > > fs/xfs/libxfs/xfs_dir2_block.c | 10 +--
> > > > fs/xfs/libxfs/xfs_dir2_data.c | 12 ++--
> > > > fs/xfs/libxfs/xfs_dir2_leaf.c | 100 ++++++-----------------------
> > > > fs/xfs/libxfs/xfs_dir2_node.c | 11 ++--
> > > > fs/xfs/libxfs/xfs_ialloc.c | 3 +-
> > > > fs/xfs/libxfs/xfs_ialloc_btree.c | 25 +++++---
> > > > fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> > > > fs/xfs/libxfs/xfs_refcount_btree.c | 3 +-
> > > > fs/xfs/libxfs/xfs_rmap_btree.c | 3 +-
> > > > fs/xfs/libxfs/xfs_sb.c | 5 +-
> > > > fs/xfs/libxfs/xfs_shared.h | 4 +-
> > > > fs/xfs/libxfs/xfs_symlink_remote.c | 3 +-
> > > > fs/xfs/scrub/agheader_repair.c | 6 +-
> > > > fs/xfs/scrub/repair.c | 11 +++-
> > > > fs/xfs/xfs_buf.c | 41 ++++++++++--
> > > > fs/xfs/xfs_buf.h | 4 +-
> > > > fs/xfs/xfs_log_recover.c | 12 ++--
> > > > fs/xfs/xfs_trans_buf.c | 2 +-
> > > > 26 files changed, 229 insertions(+), 215 deletions(-)
> > > >
> > > > --
> > > > 2.17.2
> > > >
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-02-06 19:18 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 14:52 [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Brian Foster
2019-02-04 14:52 ` [PATCH v3 1/9] xfs: set buffer ops when repair probes for btree type Brian Foster
2019-02-06 18:08 ` Darrick J. Wong
2019-02-06 18:40 ` Brian Foster
2019-02-04 14:52 ` [PATCH v3 2/9] xfs: always check magic values in on-disk byte order Brian Foster
2019-02-06 17:08 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 3/9] xfs: create a separate finobt verifier Brian Foster
2019-02-06 18:09 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 4/9] xfs: distinguish between inobt and finobt magic values Brian Foster
2019-02-06 18:12 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 5/9] xfs: split up allocation btree verifier Brian Foster
2019-02-06 18:15 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 6/9] xfs: distinguish between bnobt and cntbt magic values Brian Foster
2019-02-06 18:15 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers Brian Foster
2019-02-06 17:45 ` Darrick J. Wong
2019-02-06 18:41 ` Brian Foster
2019-02-06 19:03 ` Darrick J. Wong
2019-02-06 19:18 ` Brian Foster
2019-02-04 14:52 ` [PATCH v3 8/9] xfs: miscellaneous verifier magic value fixups Brian Foster
2019-02-06 17:52 ` Darrick J. Wong
2019-02-04 14:52 ` [PATCH v3 9/9] xfs: factor xfs_da3_blkinfo verification into common helper Brian Foster
2019-02-06 18:16 ` Darrick J. Wong
2019-02-06 18:23 ` [PATCH v3 0/9] xfs: fix [f]inobt magic value verification Darrick J. Wong
2019-02-06 18:50 ` Brian Foster
2019-02-06 19:05 ` Darrick J. Wong
2019-02-06 19:18 ` Brian Foster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.