* Re: [linux-next:master 7512/8919] fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
[not found] ` <20190215225524.GE6503@magnolia>
@ 2019-02-16 12:05 ` Brian Foster
2019-02-16 19:54 ` Darrick J. Wong
0 siblings, 1 reply; 4+ messages in thread
From: Brian Foster @ 2019-02-16 12:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Fri, Feb 15, 2019 at 02:55:24PM -0800, Darrick J. Wong wrote:
> [rip all the cc off]
>
cc linux-xfs
> On Sat, Feb 16, 2019 at 04:02:13AM +0800, kbuild test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > head: 7a92eb7cc1dc4c63e3a2fa9ab8e3c1049f199249
> > commit: b8f89801664f8413a88cf2c7539d1aeae07dd3c5 [7512/8919] xfs: distinguish between bnobt and cntbt magic values
> > reproduce:
> > # apt-get install sparse
> > git checkout b8f89801664f8413a88cf2c7539d1aeae07dd3c5
> > make ARCH=x86_64 allmodconfig
> > make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: expected unsigned int [usertype] dmagic
> > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: got restricted __be32 [usertype] bb_magic
>
> Hmmmm, I suspected this was going to happen, though when I built with
> those parameters the endian checking didn't trigger so I decided not to
> press any further. Oh well...
>
Argh. Sorry, I wasn't aware this would result in noise.
> Can we get a fix going for this ASAP, please?
>
FYI it probably won't be Monday until I can spin a proper patch. In the
meantime, what's the preferred solution?
I thought we might be able to address the callers fairly cleanly by
creating a couple xfs_verify_magic[16|32]() wrappers and cast to the
underlying format, but then sparse just generates warnings for the
casts. So AFAICT, the options are to create separate wrappers and
xfs_buf_ops fields (magic16/magic32) for each magic type and use them
appropriately in each verifier or go back to how this patch was written
originally and use the in-core values.
The former seems silly to me. My preference is the latter. Thoughts or
other ideas? Is there some other way to safely cast a "restricted" type?
Brian
> --D
>
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:321:36: sparse: warning: restricted __be32 degrades to integer
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:368:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:368:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:368:20: sparse: got restricted __be32 [usertype]
> > fs/xfs/libxfs/xfs_alloc_btree.c:369:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:369:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:369:20: sparse: got restricted __be32 [usertype]
> > fs/xfs/libxfs/xfs_alloc_btree.c:377:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:377:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:377:20: sparse: got restricted __be32 [usertype]
> > fs/xfs/libxfs/xfs_alloc_btree.c:378:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:378:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:378:20: sparse: got restricted __be32 [usertype]
> >
> > sparse warnings: (new ones prefixed by >>)
> >
> > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: expected unsigned int [usertype] dmagic
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: got restricted __be32 [usertype] bb_magic
> > fs/xfs/libxfs/xfs_alloc_btree.c:321:36: sparse: warning: restricted __be32 degrades to integer
> > fs/xfs/libxfs/xfs_alloc_btree.c:368:20: sparse: warning: incorrect type in initializer (different base types)
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:368:20: sparse: expected unsigned int
> > >> fs/xfs/libxfs/xfs_alloc_btree.c:368:20: sparse: got restricted __be32 [usertype]
> > fs/xfs/libxfs/xfs_alloc_btree.c:369:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:369:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:369:20: sparse: got restricted __be32 [usertype]
> > fs/xfs/libxfs/xfs_alloc_btree.c:377:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:377:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:377:20: sparse: got restricted __be32 [usertype]
> > fs/xfs/libxfs/xfs_alloc_btree.c:378:20: sparse: warning: incorrect type in initializer (different base types)
> > fs/xfs/libxfs/xfs_alloc_btree.c:378:20: sparse: expected unsigned int
> > fs/xfs/libxfs/xfs_alloc_btree.c:378:20: sparse: got restricted __be32 [usertype]
> >
> > vim +302 fs/xfs/libxfs/xfs_alloc_btree.c
> >
> > 290
> > 291 static xfs_failaddr_t
> > 292 xfs_allocbt_verify(
> > 293 struct xfs_buf *bp)
> > 294 {
> > 295 struct xfs_mount *mp = bp->b_target->bt_mount;
> > 296 struct xfs_btree_block *block = XFS_BUF_TO_BLOCK(bp);
> > 297 struct xfs_perag *pag = bp->b_pag;
> > 298 xfs_failaddr_t fa;
> > 299 unsigned int level;
> > 300 xfs_btnum_t btnum = XFS_BTNUM_BNOi;
> > 301
> > > 302 if (!xfs_verify_magic(bp, block->bb_magic))
> > 303 return __this_address;
> > 304
> > 305 if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > 306 fa = xfs_btree_sblock_v5hdr_verify(bp);
> > 307 if (fa)
> > 308 return fa;
> > 309 }
> > 310
> > 311 /*
> > 312 * The perag may not be attached during grow operations or fully
> > 313 * initialized from the AGF during log recovery. Therefore we can only
> > 314 * check against maximum tree depth from those contexts.
> > 315 *
> > 316 * Otherwise check against the per-tree limit. Peek at one of the
> > 317 * verifier magic values to determine the type of tree we're verifying
> > 318 * against.
> > 319 */
> > 320 level = be16_to_cpu(block->bb_level);
> > > 321 if (bp->b_ops->magic[0] == cpu_to_be32(XFS_ABTC_MAGIC))
> > 322 btnum = XFS_BTNUM_CNTi;
> > 323 if (pag && pag->pagf_init) {
> > 324 if (level >= pag->pagf_levels[btnum])
> > 325 return __this_address;
> > 326 } else if (level >= mp->m_ag_maxlevels)
> > 327 return __this_address;
> > 328
> > 329 return xfs_btree_sblock_verify(bp, mp->m_alloc_mxr[level != 0]);
> > 330 }
> > 331
> > 332 static void
> > 333 xfs_allocbt_read_verify(
> > 334 struct xfs_buf *bp)
> > 335 {
> > 336 xfs_failaddr_t fa;
> > 337
> > 338 if (!xfs_btree_sblock_verify_crc(bp))
> > 339 xfs_verifier_error(bp, -EFSBADCRC, __this_address);
> > 340 else {
> > 341 fa = xfs_allocbt_verify(bp);
> > 342 if (fa)
> > 343 xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > 344 }
> > 345
> > 346 if (bp->b_error)
> > 347 trace_xfs_btree_corrupt(bp, _RET_IP_);
> > 348 }
> > 349
> > 350 static void
> > 351 xfs_allocbt_write_verify(
> > 352 struct xfs_buf *bp)
> > 353 {
> > 354 xfs_failaddr_t fa;
> > 355
> > 356 fa = xfs_allocbt_verify(bp);
> > 357 if (fa) {
> > 358 trace_xfs_btree_corrupt(bp, _RET_IP_);
> > 359 xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > 360 return;
> > 361 }
> > 362 xfs_btree_sblock_calc_crc(bp);
> > 363
> > 364 }
> > 365
> > 366 const struct xfs_buf_ops xfs_bnobt_buf_ops = {
> > 367 .name = "xfs_bnobt",
> > > 368 .magic = { cpu_to_be32(XFS_ABTB_MAGIC),
> > 369 cpu_to_be32(XFS_ABTB_CRC_MAGIC) },
> > 370 .verify_read = xfs_allocbt_read_verify,
> > 371 .verify_write = xfs_allocbt_write_verify,
> > 372 .verify_struct = xfs_allocbt_verify,
> > 373 };
> > 374
> >
> > ---
> > 0-DAY kernel test infrastructure Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [linux-next:master 7512/8919] fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
2019-02-16 12:05 ` [linux-next:master 7512/8919] fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types) Brian Foster
@ 2019-02-16 19:54 ` Darrick J. Wong
2019-02-18 13:57 ` Brian Foster
0 siblings, 1 reply; 4+ messages in thread
From: Darrick J. Wong @ 2019-02-16 19:54 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Sat, Feb 16, 2019 at 07:05:28AM -0500, Brian Foster wrote:
> On Fri, Feb 15, 2019 at 02:55:24PM -0800, Darrick J. Wong wrote:
> > [rip all the cc off]
> >
>
> cc linux-xfs
>
> > On Sat, Feb 16, 2019 at 04:02:13AM +0800, kbuild test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > > head: 7a92eb7cc1dc4c63e3a2fa9ab8e3c1049f199249
> > > commit: b8f89801664f8413a88cf2c7539d1aeae07dd3c5 [7512/8919] xfs: distinguish between bnobt and cntbt magic values
> > > reproduce:
> > > # apt-get install sparse
> > > git checkout b8f89801664f8413a88cf2c7539d1aeae07dd3c5
> > > make ARCH=x86_64 allmodconfig
> > > make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > > >> fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
> > > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: expected unsigned int [usertype] dmagic
> > > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: got restricted __be32 [usertype] bb_magic
> >
> > Hmmmm, I suspected this was going to happen, though when I built with
> > those parameters the endian checking didn't trigger so I decided not to
> > press any further. Oh well...
> >
>
> Argh. Sorry, I wasn't aware this would result in noise.
Heh. It turned out that I forgot that my computer was configured to use
smatch instead of sparse, and only sparse does the endian checking.
(Gee, I thought smatch was supposed to be a superset of sparse!)
So with the addition of /even more/ wrapper scripts I can now run
/both/.
> > Can we get a fix going for this ASAP, please?
> >
>
> FYI it probably won't be Monday until I can spin a proper patch. In the
> meantime, what's the preferred solution?
>
> I thought we might be able to address the callers fairly cleanly by
> creating a couple xfs_verify_magic[16|32]() wrappers and cast to the
> underlying format, but then sparse just generates warnings for the
> casts. So AFAICT, the options are to create separate wrappers and
> xfs_buf_ops fields (magic16/magic32) for each magic type and use them
> appropriately in each verifier or go back to how this patch was written
> originally and use the in-core values.
Hmm. It would be sort of ugly to have to re-add the endian conversions
back into the verifier code now; I think I lean towards having a magic16
array and a xfs_verify_magic16.
(Though I cheat and use anonymous unions :P)
> The former seems silly to me. My preference is the latter. Thoughts or
> other ideas? Is there some other way to safely cast a "restricted" type?
None that I know of, sparse complains about any cast between restricted
types. I shut up sparse with the following mostly untested patch:
--D
xfs: fix xfs_buf magic number endian checks
Create a separate magic16 check function so that we don't run afoul of
static checkers.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
fs/xfs/libxfs/xfs_da_btree.c | 8 ++++----
fs/xfs/libxfs/xfs_dir2_leaf.c | 8 ++++----
fs/xfs/libxfs/xfs_dquot_buf.c | 8 ++++----
fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++-----
fs/xfs/xfs_buf.c | 20 +++++++++++++++++++-
fs/xfs/xfs_buf.h | 8 ++++++--
fs/xfs/xfs_log_recover.c | 2 +-
8 files changed, 45 insertions(+), 23 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 0c92987f57fc..1f6e3965ff74 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -358,8 +358,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) },
+ .magic16 = { 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_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index eb2cee428f26..e2737e2ac2ae 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -129,7 +129,7 @@ xfs_da3_blkinfo_verify(
struct xfs_mount *mp = bp->b_target->bt_mount;
struct xfs_da_blkinfo *hdr = &hdr3->hdr;
- if (!xfs_verify_magic(bp, hdr->magic))
+ if (!xfs_verify_magic16(bp, hdr->magic))
return __this_address;
if (xfs_sb_version_hascrc(&mp->m_sb)) {
@@ -141,7 +141,7 @@ xfs_da3_blkinfo_verify(
return __this_address;
}
- return 0;
+ return NULL;
}
static xfs_failaddr_t
@@ -274,8 +274,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) },
+ .magic16 = { 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_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 094028b7b162..9a3767818c50 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -198,8 +198,8 @@ xfs_dir3_leaf_write_verify(
const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
.name = "xfs_dir3_leaf1",
- .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
- cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
+ .magic16 = { 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,
@@ -207,8 +207,8 @@ const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
.name = "xfs_dir3_leafn",
- .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
- cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
+ .magic16 = { 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,
diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index b956638a3532..fb5bd9a804f6 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -277,8 +277,8 @@ xfs_dquot_buf_write_verify(
const struct xfs_buf_ops xfs_dquot_buf_ops = {
.name = "xfs_dquot",
- .magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
- cpu_to_be16(XFS_DQUOT_MAGIC) },
+ .magic16 = { cpu_to_be16(XFS_DQUOT_MAGIC),
+ cpu_to_be16(XFS_DQUOT_MAGIC) },
.verify_read = xfs_dquot_buf_read_verify,
.verify_write = xfs_dquot_buf_write_verify,
.verify_struct = xfs_dquot_buf_verify_struct,
@@ -286,8 +286,8 @@ const struct xfs_buf_ops xfs_dquot_buf_ops = {
const struct xfs_buf_ops xfs_dquot_buf_ra_ops = {
.name = "xfs_dquot_ra",
- .magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
- cpu_to_be16(XFS_DQUOT_MAGIC) },
+ .magic16 = { cpu_to_be16(XFS_DQUOT_MAGIC),
+ cpu_to_be16(XFS_DQUOT_MAGIC) },
.verify_read = xfs_dquot_buf_readahead_verify,
.verify_write = xfs_dquot_buf_write_verify,
};
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index f92f14e93ad3..e021d5133ccb 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -97,7 +97,7 @@ xfs_inode_buf_verify(
dip = xfs_buf_offset(bp, (i << mp->m_sb.sb_inodelog));
unlinked_ino = be32_to_cpu(dip->di_next_unlinked);
- di_ok = xfs_verify_magic(bp, dip->di_magic) &&
+ di_ok = xfs_verify_magic16(bp, dip->di_magic) &&
xfs_dinode_good_version(mp, dip->di_version) &&
xfs_verify_agino_or_null(mp, agno, unlinked_ino);
if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
@@ -146,16 +146,16 @@ xfs_inode_buf_write_verify(
const struct xfs_buf_ops xfs_inode_buf_ops = {
.name = "xfs_inode",
- .magic = { cpu_to_be16(XFS_DINODE_MAGIC),
- cpu_to_be16(XFS_DINODE_MAGIC) },
+ .magic16 = { cpu_to_be16(XFS_DINODE_MAGIC),
+ cpu_to_be16(XFS_DINODE_MAGIC) },
.verify_read = xfs_inode_buf_read_verify,
.verify_write = xfs_inode_buf_write_verify,
};
const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
.name = "xfs_inode_ra",
- .magic = { cpu_to_be16(XFS_DINODE_MAGIC),
- cpu_to_be16(XFS_DINODE_MAGIC) },
+ .magic16 = { cpu_to_be16(XFS_DINODE_MAGIC),
+ cpu_to_be16(XFS_DINODE_MAGIC) },
.verify_read = xfs_inode_buf_readahead_verify,
.verify_write = xfs_inode_buf_write_verify,
};
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 52a382b8cbce..548344e25128 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2213,7 +2213,7 @@ void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
bool
xfs_verify_magic(
struct xfs_buf *bp,
- uint32_t dmagic)
+ __be32 dmagic)
{
struct xfs_mount *mp = bp->b_target->bt_mount;
int idx;
@@ -2223,3 +2223,21 @@ xfs_verify_magic(
return false;
return dmagic == bp->b_ops->magic[idx];
}
+/*
+ * 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_magic16(
+ struct xfs_buf *bp,
+ __be16 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->magic16[idx])))
+ return false;
+ return dmagic == bp->b_ops->magic16[idx];
+}
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 44f9423a1861..d0b96e071cec 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -125,7 +125,10 @@ struct xfs_buf_map {
struct xfs_buf_ops {
char *name;
- uint32_t magic[2]; /* v4 and v5 on disk magic values */
+ union {
+ __be32 magic[2]; /* v4 and v5 on disk magic values */
+ __be16 magic16[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);
@@ -387,6 +390,7 @@ 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);
+bool xfs_verify_magic(struct xfs_buf *bp, __be32 dmagic);
+bool xfs_verify_magic16(struct xfs_buf *bp, __be16 dmagic);
#endif /* __XFS_BUF_H__ */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index f5948d16015b..3371d1ff27c4 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3049,7 +3049,7 @@ xlog_recover_inode_pass2(
* Make sure the place we're flushing out to really looks
* like an inode!
*/
- if (unlikely(!xfs_verify_magic(bp, dip->di_magic))) {
+ if (unlikely(!xfs_verify_magic16(bp, dip->di_magic))) {
xfs_alert(mp,
"%s: Bad inode magic number, dip = "PTR_FMT", dino bp = "PTR_FMT", ino = %Ld",
__func__, dip, bp, in_f->ilf_ino);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [linux-next:master 7512/8919] fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
2019-02-16 19:54 ` Darrick J. Wong
@ 2019-02-18 13:57 ` Brian Foster
2019-02-18 17:20 ` Darrick J. Wong
0 siblings, 1 reply; 4+ messages in thread
From: Brian Foster @ 2019-02-18 13:57 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Sat, Feb 16, 2019 at 11:54:01AM -0800, Darrick J. Wong wrote:
> On Sat, Feb 16, 2019 at 07:05:28AM -0500, Brian Foster wrote:
> > On Fri, Feb 15, 2019 at 02:55:24PM -0800, Darrick J. Wong wrote:
> > > [rip all the cc off]
> > >
> >
> > cc linux-xfs
> >
> > > On Sat, Feb 16, 2019 at 04:02:13AM +0800, kbuild test robot wrote:
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > > > head: 7a92eb7cc1dc4c63e3a2fa9ab8e3c1049f199249
> > > > commit: b8f89801664f8413a88cf2c7539d1aeae07dd3c5 [7512/8919] xfs: distinguish between bnobt and cntbt magic values
> > > > reproduce:
> > > > # apt-get install sparse
> > > > git checkout b8f89801664f8413a88cf2c7539d1aeae07dd3c5
> > > > make ARCH=x86_64 allmodconfig
> > > > make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> > > >
> > > > All warnings (new ones prefixed by >>):
> > > >
> > > > >> fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
> > > > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: expected unsigned int [usertype] dmagic
> > > > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: got restricted __be32 [usertype] bb_magic
> > >
> > > Hmmmm, I suspected this was going to happen, though when I built with
> > > those parameters the endian checking didn't trigger so I decided not to
> > > press any further. Oh well...
> > >
> >
> > Argh. Sorry, I wasn't aware this would result in noise.
>
> Heh. It turned out that I forgot that my computer was configured to use
> smatch instead of sparse, and only sparse does the endian checking.
> (Gee, I thought smatch was supposed to be a superset of sparse!)
>
> So with the addition of /even more/ wrapper scripts I can now run
> /both/.
>
> > > Can we get a fix going for this ASAP, please?
> > >
> >
> > FYI it probably won't be Monday until I can spin a proper patch. In the
> > meantime, what's the preferred solution?
> >
> > I thought we might be able to address the callers fairly cleanly by
> > creating a couple xfs_verify_magic[16|32]() wrappers and cast to the
> > underlying format, but then sparse just generates warnings for the
> > casts. So AFAICT, the options are to create separate wrappers and
> > xfs_buf_ops fields (magic16/magic32) for each magic type and use them
> > appropriately in each verifier or go back to how this patch was written
> > originally and use the in-core values.
>
> Hmm. It would be sort of ugly to have to re-add the endian conversions
> back into the verifier code now; I think I lean towards having a magic16
> array and a xfs_verify_magic16.
>
Ok.
> (Though I cheat and use anonymous unions :P)
>
I think that helps a bit.
> > The former seems silly to me. My preference is the latter. Thoughts or
> > other ideas? Is there some other way to safely cast a "restricted" type?
>
> None that I know of, sparse complains about any cast between restricted
> types. I shut up sparse with the following mostly untested patch:
>
> --D
>
> xfs: fix xfs_buf magic number endian checks
>
> Create a separate magic16 check function so that we don't run afoul of
> static checkers.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
I was originally thinking of splitting out everyting into magic32/16,
but this shows that there aren't too many 16 instances. This looks good
and quiets everything for me as well:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
> fs/xfs/libxfs/xfs_da_btree.c | 8 ++++----
> fs/xfs/libxfs/xfs_dir2_leaf.c | 8 ++++----
> fs/xfs/libxfs/xfs_dquot_buf.c | 8 ++++----
> fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++-----
> fs/xfs/xfs_buf.c | 20 +++++++++++++++++++-
> fs/xfs/xfs_buf.h | 8 ++++++--
> fs/xfs/xfs_log_recover.c | 2 +-
> 8 files changed, 45 insertions(+), 23 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 0c92987f57fc..1f6e3965ff74 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -358,8 +358,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) },
> + .magic16 = { 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_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index eb2cee428f26..e2737e2ac2ae 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -129,7 +129,7 @@ xfs_da3_blkinfo_verify(
> struct xfs_mount *mp = bp->b_target->bt_mount;
> struct xfs_da_blkinfo *hdr = &hdr3->hdr;
>
> - if (!xfs_verify_magic(bp, hdr->magic))
> + if (!xfs_verify_magic16(bp, hdr->magic))
> return __this_address;
>
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> @@ -141,7 +141,7 @@ xfs_da3_blkinfo_verify(
> return __this_address;
> }
>
> - return 0;
> + return NULL;
> }
>
> static xfs_failaddr_t
> @@ -274,8 +274,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) },
> + .magic16 = { 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_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 094028b7b162..9a3767818c50 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -198,8 +198,8 @@ xfs_dir3_leaf_write_verify(
>
> const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> .name = "xfs_dir3_leaf1",
> - .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> - cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> + .magic16 = { 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,
> @@ -207,8 +207,8 @@ const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
>
> const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> .name = "xfs_dir3_leafn",
> - .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> - cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> + .magic16 = { 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,
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index b956638a3532..fb5bd9a804f6 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -277,8 +277,8 @@ xfs_dquot_buf_write_verify(
>
> const struct xfs_buf_ops xfs_dquot_buf_ops = {
> .name = "xfs_dquot",
> - .magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
> - cpu_to_be16(XFS_DQUOT_MAGIC) },
> + .magic16 = { cpu_to_be16(XFS_DQUOT_MAGIC),
> + cpu_to_be16(XFS_DQUOT_MAGIC) },
> .verify_read = xfs_dquot_buf_read_verify,
> .verify_write = xfs_dquot_buf_write_verify,
> .verify_struct = xfs_dquot_buf_verify_struct,
> @@ -286,8 +286,8 @@ const struct xfs_buf_ops xfs_dquot_buf_ops = {
>
> const struct xfs_buf_ops xfs_dquot_buf_ra_ops = {
> .name = "xfs_dquot_ra",
> - .magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
> - cpu_to_be16(XFS_DQUOT_MAGIC) },
> + .magic16 = { cpu_to_be16(XFS_DQUOT_MAGIC),
> + cpu_to_be16(XFS_DQUOT_MAGIC) },
> .verify_read = xfs_dquot_buf_readahead_verify,
> .verify_write = xfs_dquot_buf_write_verify,
> };
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index f92f14e93ad3..e021d5133ccb 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -97,7 +97,7 @@ xfs_inode_buf_verify(
>
> dip = xfs_buf_offset(bp, (i << mp->m_sb.sb_inodelog));
> unlinked_ino = be32_to_cpu(dip->di_next_unlinked);
> - di_ok = xfs_verify_magic(bp, dip->di_magic) &&
> + di_ok = xfs_verify_magic16(bp, dip->di_magic) &&
> xfs_dinode_good_version(mp, dip->di_version) &&
> xfs_verify_agino_or_null(mp, agno, unlinked_ino);
> if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
> @@ -146,16 +146,16 @@ xfs_inode_buf_write_verify(
>
> const struct xfs_buf_ops xfs_inode_buf_ops = {
> .name = "xfs_inode",
> - .magic = { cpu_to_be16(XFS_DINODE_MAGIC),
> - cpu_to_be16(XFS_DINODE_MAGIC) },
> + .magic16 = { cpu_to_be16(XFS_DINODE_MAGIC),
> + cpu_to_be16(XFS_DINODE_MAGIC) },
> .verify_read = xfs_inode_buf_read_verify,
> .verify_write = xfs_inode_buf_write_verify,
> };
>
> const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
> .name = "xfs_inode_ra",
> - .magic = { cpu_to_be16(XFS_DINODE_MAGIC),
> - cpu_to_be16(XFS_DINODE_MAGIC) },
> + .magic16 = { cpu_to_be16(XFS_DINODE_MAGIC),
> + cpu_to_be16(XFS_DINODE_MAGIC) },
> .verify_read = xfs_inode_buf_readahead_verify,
> .verify_write = xfs_inode_buf_write_verify,
> };
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 52a382b8cbce..548344e25128 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2213,7 +2213,7 @@ void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
> bool
> xfs_verify_magic(
> struct xfs_buf *bp,
> - uint32_t dmagic)
> + __be32 dmagic)
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
> int idx;
> @@ -2223,3 +2223,21 @@ xfs_verify_magic(
> return false;
> return dmagic == bp->b_ops->magic[idx];
> }
> +/*
> + * 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_magic16(
> + struct xfs_buf *bp,
> + __be16 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->magic16[idx])))
> + return false;
> + return dmagic == bp->b_ops->magic16[idx];
> +}
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index 44f9423a1861..d0b96e071cec 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -125,7 +125,10 @@ struct xfs_buf_map {
>
> struct xfs_buf_ops {
> char *name;
> - uint32_t magic[2]; /* v4 and v5 on disk magic values */
> + union {
> + __be32 magic[2]; /* v4 and v5 on disk magic values */
> + __be16 magic16[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);
> @@ -387,6 +390,7 @@ 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);
> +bool xfs_verify_magic(struct xfs_buf *bp, __be32 dmagic);
> +bool xfs_verify_magic16(struct xfs_buf *bp, __be16 dmagic);
>
> #endif /* __XFS_BUF_H__ */
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index f5948d16015b..3371d1ff27c4 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3049,7 +3049,7 @@ xlog_recover_inode_pass2(
> * Make sure the place we're flushing out to really looks
> * like an inode!
> */
> - if (unlikely(!xfs_verify_magic(bp, dip->di_magic))) {
> + if (unlikely(!xfs_verify_magic16(bp, dip->di_magic))) {
> xfs_alert(mp,
> "%s: Bad inode magic number, dip = "PTR_FMT", dino bp = "PTR_FMT", ino = %Ld",
> __func__, dip, bp, in_f->ilf_ino);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [linux-next:master 7512/8919] fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
2019-02-18 13:57 ` Brian Foster
@ 2019-02-18 17:20 ` Darrick J. Wong
0 siblings, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2019-02-18 17:20 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
On Mon, Feb 18, 2019 at 08:57:14AM -0500, Brian Foster wrote:
> On Sat, Feb 16, 2019 at 11:54:01AM -0800, Darrick J. Wong wrote:
> > On Sat, Feb 16, 2019 at 07:05:28AM -0500, Brian Foster wrote:
> > > On Fri, Feb 15, 2019 at 02:55:24PM -0800, Darrick J. Wong wrote:
> > > > [rip all the cc off]
> > > >
> > >
> > > cc linux-xfs
> > >
> > > > On Sat, Feb 16, 2019 at 04:02:13AM +0800, kbuild test robot wrote:
> > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> > > > > head: 7a92eb7cc1dc4c63e3a2fa9ab8e3c1049f199249
> > > > > commit: b8f89801664f8413a88cf2c7539d1aeae07dd3c5 [7512/8919] xfs: distinguish between bnobt and cntbt magic values
> > > > > reproduce:
> > > > > # apt-get install sparse
> > > > > git checkout b8f89801664f8413a88cf2c7539d1aeae07dd3c5
> > > > > make ARCH=x86_64 allmodconfig
> > > > > make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> > > > >
> > > > > All warnings (new ones prefixed by >>):
> > > > >
> > > > > >> fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types)
> > > > > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: expected unsigned int [usertype] dmagic
> > > > > fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: got restricted __be32 [usertype] bb_magic
> > > >
> > > > Hmmmm, I suspected this was going to happen, though when I built with
> > > > those parameters the endian checking didn't trigger so I decided not to
> > > > press any further. Oh well...
> > > >
> > >
> > > Argh. Sorry, I wasn't aware this would result in noise.
> >
> > Heh. It turned out that I forgot that my computer was configured to use
> > smatch instead of sparse, and only sparse does the endian checking.
> > (Gee, I thought smatch was supposed to be a superset of sparse!)
> >
> > So with the addition of /even more/ wrapper scripts I can now run
> > /both/.
> >
> > > > Can we get a fix going for this ASAP, please?
> > > >
> > >
> > > FYI it probably won't be Monday until I can spin a proper patch. In the
> > > meantime, what's the preferred solution?
> > >
> > > I thought we might be able to address the callers fairly cleanly by
> > > creating a couple xfs_verify_magic[16|32]() wrappers and cast to the
> > > underlying format, but then sparse just generates warnings for the
> > > casts. So AFAICT, the options are to create separate wrappers and
> > > xfs_buf_ops fields (magic16/magic32) for each magic type and use them
> > > appropriately in each verifier or go back to how this patch was written
> > > originally and use the in-core values.
> >
> > Hmm. It would be sort of ugly to have to re-add the endian conversions
> > back into the verifier code now; I think I lean towards having a magic16
> > array and a xfs_verify_magic16.
> >
>
> Ok.
>
> > (Though I cheat and use anonymous unions :P)
> >
>
> I think that helps a bit.
>
> > > The former seems silly to me. My preference is the latter. Thoughts or
> > > other ideas? Is there some other way to safely cast a "restricted" type?
> >
> > None that I know of, sparse complains about any cast between restricted
> > types. I shut up sparse with the following mostly untested patch:
> >
> > --D
> >
> > xfs: fix xfs_buf magic number endian checks
> >
> > Create a separate magic16 check function so that we don't run afoul of
> > static checkers.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
>
> I was originally thinking of splitting out everyting into magic32/16,
> but this shows that there aren't too many 16 instances. This looks good
> and quiets everything for me as well:
<nod> I'll resend this patch just to make it obvious what we're doing.
Thanks for the review. :)
--D
> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> > fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
> > fs/xfs/libxfs/xfs_da_btree.c | 8 ++++----
> > fs/xfs/libxfs/xfs_dir2_leaf.c | 8 ++++----
> > fs/xfs/libxfs/xfs_dquot_buf.c | 8 ++++----
> > fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++-----
> > fs/xfs/xfs_buf.c | 20 +++++++++++++++++++-
> > fs/xfs/xfs_buf.h | 8 ++++++--
> > fs/xfs/xfs_log_recover.c | 2 +-
> > 8 files changed, 45 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index 0c92987f57fc..1f6e3965ff74 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -358,8 +358,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) },
> > + .magic16 = { 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_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> > index eb2cee428f26..e2737e2ac2ae 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.c
> > +++ b/fs/xfs/libxfs/xfs_da_btree.c
> > @@ -129,7 +129,7 @@ xfs_da3_blkinfo_verify(
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > struct xfs_da_blkinfo *hdr = &hdr3->hdr;
> >
> > - if (!xfs_verify_magic(bp, hdr->magic))
> > + if (!xfs_verify_magic16(bp, hdr->magic))
> > return __this_address;
> >
> > if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > @@ -141,7 +141,7 @@ xfs_da3_blkinfo_verify(
> > return __this_address;
> > }
> >
> > - return 0;
> > + return NULL;
> > }
> >
> > static xfs_failaddr_t
> > @@ -274,8 +274,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) },
> > + .magic16 = { 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_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > index 094028b7b162..9a3767818c50 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > @@ -198,8 +198,8 @@ xfs_dir3_leaf_write_verify(
> >
> > const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> > .name = "xfs_dir3_leaf1",
> > - .magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> > - cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> > + .magic16 = { 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,
> > @@ -207,8 +207,8 @@ const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> >
> > const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> > .name = "xfs_dir3_leafn",
> > - .magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> > - cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> > + .magic16 = { 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,
> > diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> > index b956638a3532..fb5bd9a804f6 100644
> > --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> > @@ -277,8 +277,8 @@ xfs_dquot_buf_write_verify(
> >
> > const struct xfs_buf_ops xfs_dquot_buf_ops = {
> > .name = "xfs_dquot",
> > - .magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
> > - cpu_to_be16(XFS_DQUOT_MAGIC) },
> > + .magic16 = { cpu_to_be16(XFS_DQUOT_MAGIC),
> > + cpu_to_be16(XFS_DQUOT_MAGIC) },
> > .verify_read = xfs_dquot_buf_read_verify,
> > .verify_write = xfs_dquot_buf_write_verify,
> > .verify_struct = xfs_dquot_buf_verify_struct,
> > @@ -286,8 +286,8 @@ const struct xfs_buf_ops xfs_dquot_buf_ops = {
> >
> > const struct xfs_buf_ops xfs_dquot_buf_ra_ops = {
> > .name = "xfs_dquot_ra",
> > - .magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
> > - cpu_to_be16(XFS_DQUOT_MAGIC) },
> > + .magic16 = { cpu_to_be16(XFS_DQUOT_MAGIC),
> > + cpu_to_be16(XFS_DQUOT_MAGIC) },
> > .verify_read = xfs_dquot_buf_readahead_verify,
> > .verify_write = xfs_dquot_buf_write_verify,
> > };
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index f92f14e93ad3..e021d5133ccb 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -97,7 +97,7 @@ xfs_inode_buf_verify(
> >
> > dip = xfs_buf_offset(bp, (i << mp->m_sb.sb_inodelog));
> > unlinked_ino = be32_to_cpu(dip->di_next_unlinked);
> > - di_ok = xfs_verify_magic(bp, dip->di_magic) &&
> > + di_ok = xfs_verify_magic16(bp, dip->di_magic) &&
> > xfs_dinode_good_version(mp, dip->di_version) &&
> > xfs_verify_agino_or_null(mp, agno, unlinked_ino);
> > if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
> > @@ -146,16 +146,16 @@ xfs_inode_buf_write_verify(
> >
> > const struct xfs_buf_ops xfs_inode_buf_ops = {
> > .name = "xfs_inode",
> > - .magic = { cpu_to_be16(XFS_DINODE_MAGIC),
> > - cpu_to_be16(XFS_DINODE_MAGIC) },
> > + .magic16 = { cpu_to_be16(XFS_DINODE_MAGIC),
> > + cpu_to_be16(XFS_DINODE_MAGIC) },
> > .verify_read = xfs_inode_buf_read_verify,
> > .verify_write = xfs_inode_buf_write_verify,
> > };
> >
> > const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
> > .name = "xfs_inode_ra",
> > - .magic = { cpu_to_be16(XFS_DINODE_MAGIC),
> > - cpu_to_be16(XFS_DINODE_MAGIC) },
> > + .magic16 = { cpu_to_be16(XFS_DINODE_MAGIC),
> > + cpu_to_be16(XFS_DINODE_MAGIC) },
> > .verify_read = xfs_inode_buf_readahead_verify,
> > .verify_write = xfs_inode_buf_write_verify,
> > };
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 52a382b8cbce..548344e25128 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -2213,7 +2213,7 @@ void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
> > bool
> > xfs_verify_magic(
> > struct xfs_buf *bp,
> > - uint32_t dmagic)
> > + __be32 dmagic)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > int idx;
> > @@ -2223,3 +2223,21 @@ xfs_verify_magic(
> > return false;
> > return dmagic == bp->b_ops->magic[idx];
> > }
> > +/*
> > + * 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_magic16(
> > + struct xfs_buf *bp,
> > + __be16 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->magic16[idx])))
> > + return false;
> > + return dmagic == bp->b_ops->magic16[idx];
> > +}
> > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> > index 44f9423a1861..d0b96e071cec 100644
> > --- a/fs/xfs/xfs_buf.h
> > +++ b/fs/xfs/xfs_buf.h
> > @@ -125,7 +125,10 @@ struct xfs_buf_map {
> >
> > struct xfs_buf_ops {
> > char *name;
> > - uint32_t magic[2]; /* v4 and v5 on disk magic values */
> > + union {
> > + __be32 magic[2]; /* v4 and v5 on disk magic values */
> > + __be16 magic16[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);
> > @@ -387,6 +390,7 @@ 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);
> > +bool xfs_verify_magic(struct xfs_buf *bp, __be32 dmagic);
> > +bool xfs_verify_magic16(struct xfs_buf *bp, __be16 dmagic);
> >
> > #endif /* __XFS_BUF_H__ */
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index f5948d16015b..3371d1ff27c4 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -3049,7 +3049,7 @@ xlog_recover_inode_pass2(
> > * Make sure the place we're flushing out to really looks
> > * like an inode!
> > */
> > - if (unlikely(!xfs_verify_magic(bp, dip->di_magic))) {
> > + if (unlikely(!xfs_verify_magic16(bp, dip->di_magic))) {
> > xfs_alert(mp,
> > "%s: Bad inode magic number, dip = "PTR_FMT", dino bp = "PTR_FMT", ino = %Ld",
> > __func__, dip, bp, in_f->ilf_ino);
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-18 17:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <201902160410.WC4AmuSu%fengguang.wu@intel.com>
[not found] ` <20190215225524.GE6503@magnolia>
2019-02-16 12:05 ` [linux-next:master 7512/8919] fs/xfs/libxfs/xfs_alloc_btree.c:302:40: sparse: warning: incorrect type in argument 2 (different base types) Brian Foster
2019-02-16 19:54 ` Darrick J. Wong
2019-02-18 13:57 ` Brian Foster
2019-02-18 17:20 ` Darrick J. Wong
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.