[PATCH 00/30] ocfs2: split dinode validation into shared predicates

[Joseph Qi <joseph.qi@linux.alibaba.com>]

ocfs2_validate_inode_block() is a flat sequence of about twenty open-coded
on-disk dinode checks.  ocfs2_filecheck_validate_inode_block() duplicates
eight of them, but reports differently: plain mlog() plus an
OCFS2_FILECHECK_ERR_* code instead of ocfs2_error(), because filecheck is
a diagnostic interface an administrator points at a suspect inode and it
must not escalate into ocfs2_handle_error(), which can force the
filesystem read-only or panic.

The duplicated conditions had already drifted apart -- the two
OCFS2_VALID_FL tests were spelled differently -- and filecheck silently
skipped the rest.

This series splits the conditions out and then brings filecheck up to
parity.

Patches 1-20: pure refactor.  Each condition becomes a static bool
ocfs2_dinode_has_*() predicate returning true on bad, following the four
helpers already in inode.c.  Return codes and log messages stay at the
call sites, so the two reporting paths remain distinct and no
ocfs2_error() call site moves.  One check per commit.  No functional
change intended.

Patch 21: a separable fix.  ocfs2_filecheck_repair_inode_block() proceeds
when validation fails but then returns 0 unconditionally, after repairing
only i_blkno, fs_generation and l_next_free_rec.  An inode failing any
other check is reported to userspace as SUCCESS while still corrupt.
This is already true today for the mode, i_rdev and size-without-clusters
checks.  Revalidate after the repairs and return the residual code.  Drop
this patch if you would rather keep the filecheck series purely additive,
but it then widens the set of masked failures considerably.

Patches 22-30: wire the remaining predicates into filecheck.  All reuse
OCFS2_FILECHECK_ERR_INVALIDINO, so no new error code is introduced and the
ocfs2_filecheck_errs[] table with its userspace-visible names is
unchanged.

Points worth a close look:

 - The fast symlink NUL-termination predicate bounds its own strnlen()
   against ocfs2_fast_symlink_chars().  The open-coded version was only
   safe because the preceding i_size check had already bailed; i_size
   comes from disk, so an unbounded strnlen() over i_size + 1 bytes can
   run past the inode block.  A helper should not inherit that ordering
   dependency implicitly, especially once filecheck calls it too.

 - The inline id_count maximum stays a signed int.
   ocfs2_max_inline_data_with_xattr() subtracts the unvalidated
   i_xattr_inline_size and goes negative on small block sizes.  Caching it
   unsigned would turn a negative maximum into a huge bound and silently
   accept the oversized id_count.

 - ocfs2_dinode_is_fast_symlink() is deliberately not shared with the
   similar test inside ocfs2_dinode_has_extents(), which additionally
   requires a non-zero i_size.  A zero-length, zero-cluster symlink
   currently falls through to the extent list checks.

 - The suballoc range predicate keeps its != OCFS2_INVALID_SLOT
   exclusion.  OCFS2_INVALID_SLOT is 65535, so without it every system
   inode mkfs.ocfs2 creates from the global allocator would be rejected.

 - Look-alike tests in suballoc.c are left alone:
   ocfs2_validate_group_descriptor() deliberately permits
   OCFS2_INVALID_SLOT for all inodes, and ocfs2_claim_suballoc_bits() also
   rejects cl_next_free_rec == 0.  Neither is equivalent to the new
   predicates.

The inline xattr check is the one thing filecheck still does not share.
ocfs2_validate_inode_xattr() calls ocfs2_error() through its helpers in
xattr.c, so reusing it would mean threading a reporting flag down into
that file.  Left for a follow-up.

Testing, on top of linux-next 20261103:

Patches 1-20 were checked to be behaviour preserving.  All 27 log
messages, all return codes and all ocfs2_error()/mlog() call sites in
ocfs2_validate_inode_block() are unchanged, each new predicate body was
compared against the condition it replaced, and the disassembly of
ocfs2_filecheck_validate_inode_block() is instruction identical apart
from __LINE__ constants.  The normal validator's disassembly differs by
more than that -- basic blocks are rescheduled and registers reallocated
-- so equivalence there rests on the source level comparison rather than
on an instruction by instruction proof.  The deliberate rewrites are the
ocfs2_read_links_count() reuse in patch 1, the OCFS2_VALID_FL spelling
unification in patch 3, flattening the mutually exclusive suballoc
if/else-if into two tests in patch 5, and the redundant self-bounding
guard added in patch 13.

In a QEMU guest running this series, on 4K blocks with inline-data,
refcount, sparse, usrquota, grpquota, xattr and indexed-dirs enabled:
mount and I/O over seventeen inode types plus all the system inodes
produced no false positive, and filecheck reported SUCCESS for every one
of them.  A separate 512-byte block volume with inline-data, xattr,
sparse and refcount was checked the same way over five inode types, also
with no false positive.

The corruption tests used a third volume formatted with nometaecc, so
that a checksum failure would not mask the field being tested.  Seven
deliberately corrupted dinodes -- zero and oversized extent l_count,
oversized inline id_count, oversized fast symlink i_size, refcount flag
without i_refcount_loc, oversized suballoc bit and oversized
l_next_free_rec -- were each detected with the expected message, with no
ocfs2_error() escalation and the filesystem still read-write afterwards.
fix was then run on two of them: the zero l_count case, which repair
cannot fix, returned INVALIDINO, and the l_next_free_rec case, which it
does fix, returned SUCCESS.

Forcing i_xattr_inline_size up on the 512-byte block volume drove the
inline maximum negative and produced "inline data id_count 312 exceeds
max -88", confirming the signed comparison survived the extraction.

The ocfs2 testsuite and xfstests were not run.

Joseph Qi (30):
  ocfs2: abstract the zero nlink or mode dinode check
  ocfs2: abstract the dinode i_blkno mismatch check
  ocfs2: abstract the dinode OCFS2_VALID_FL check
  ocfs2: abstract the dinode fs_generation mismatch check
  ocfs2: abstract the dinode suballoc slot checks
  ocfs2: abstract the dinode suballoc bit check
  ocfs2: abstract the dinode orphaned slot check
  ocfs2: abstract the dinode DIO orphaned slot check
  ocfs2: abstract the inline data dinode guard and cluster check
  ocfs2: abstract the inline data id_count bound check
  ocfs2: abstract the inline data i_size bound check
  ocfs2: abstract the fast symlink dinode guard and size check
  ocfs2: abstract the fast symlink NUL-termination check
  ocfs2: abstract the chain list dinode guard and count check
  ocfs2: abstract the chain list next free record check
  ocfs2: abstract the chain list bits per cluster check
  ocfs2: abstract the dinode refcount location check
  ocfs2: abstract the zero extent list count check
  ocfs2: abstract the oversized extent list count check
  ocfs2: abstract the extent list next free record check
  ocfs2: report unrepairable corruption from online filecheck fix
  ocfs2: check dinode nlink and mode in online filecheck
  ocfs2: check dinode suballoc slot in online filecheck
  ocfs2: check dinode suballoc bit in online filecheck
  ocfs2: check dinode orphaned slots in online filecheck
  ocfs2: check inline data dinodes in online filecheck
  ocfs2: check fast symlink dinodes in online filecheck
  ocfs2: check chain list dinodes in online filecheck
  ocfs2: check dinode refcount location in online filecheck
  ocfs2: check extent list dinodes in online filecheck

 fs/ocfs2/inode.c | 456 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 399 insertions(+), 57 deletions(-)

-- 
2.39.3