On Mon, May 29, 2023 at 10:08:23AM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@xxxxxxxxxx> > > When a v4 filesystem has fl_last - fl_first != fl_count, we do not > not detect the corruption and allow the AGF to be used as it if was > fully valid. On V5 filesystems, we reset the AGFL to empty in these > cases and avoid the corruption at a small cost of leaked blocks. > > If we don't catch the corruption on V4 filesystems, bad things > happen later when an allocation attempts to trim the free list > and either double-frees stale entries in the AGFl or tries to free > NULLAGBNO entries. > > Either way, this is bad. Prevent this from happening by using the > AGFL_NEED_RESET logic for v4 filesysetms, too. > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> Seems logical, Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> --D > --- > fs/xfs/libxfs/xfs_alloc.c | 59 ++++++++++++++++++++++++++++----------- > 1 file changed, 42 insertions(+), 17 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index 61eb65be17f3..fd3293a8c659 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -628,6 +628,25 @@ xfs_alloc_fixup_trees( > return 0; > } > > +/* > + * We do not verify the AGFL contents against AGF-based index counters here, > + * even though we may have access to the perag that contains shadow copies. We > + * don't know if the AGF based counters have been checked, and if they have they > + * still may be inconsistent because they haven't yet been reset on the first > + * allocation after the AGF has been read in. > + * > + * This means we can only check that all agfl entries contain valid or null > + * values because we can't reliably determine the active range to exclude > + * NULLAGBNO as a valid value. > + * > + * However, we can't even do that for v4 format filesystems because there are > + * old versions of mkfs out there that does not initialise the AGFL to known, > + * verifiable values. HEnce we can't tell the difference between a AGFL block > + * allocated by mkfs and a corrupted AGFL block here on v4 filesystems. > + * > + * As a result, we can only fully validate AGFL block numbers when we pull them > + * from the freelist in xfs_alloc_get_freelist(). > + */ > static xfs_failaddr_t > xfs_agfl_verify( > struct xfs_buf *bp) > @@ -637,12 +656,6 @@ xfs_agfl_verify( > __be32 *agfl_bno = xfs_buf_to_agfl_bno(bp); > int i; > > - /* > - * There is no verification of non-crc AGFLs because mkfs does not > - * initialise the AGFL to zero or NULL. Hence the only valid part of the > - * AGFL is what the AGF says is active. We can't get to the AGF, so we > - * can't verify just those entries are valid. > - */ > if (!xfs_has_crc(mp)) > return NULL; > > @@ -2321,12 +2334,16 @@ xfs_free_agfl_block( > } > > /* > - * Check the agfl fields of the agf for inconsistency or corruption. The purpose > - * is to detect an agfl header padding mismatch between current and early v5 > - * kernels. This problem manifests as a 1-slot size difference between the > - * on-disk flcount and the active [first, last] range of a wrapped agfl. This > - * may also catch variants of agfl count corruption unrelated to padding. Either > - * way, we'll reset the agfl and warn the user. > + * Check the agfl fields of the agf for inconsistency or corruption. > + * > + * The original purpose was to detect an agfl header padding mismatch between > + * current and early v5 kernels. This problem manifests as a 1-slot size > + * difference between the on-disk flcount and the active [first, last] range of > + * a wrapped agfl. > + * > + * However, we need to use these same checks to catch agfl count corruptions > + * unrelated to padding. This could occur on any v4 or v5 filesystem, so either > + * way, we need to reset the agfl and warn the user. > * > * Return true if a reset is required before the agfl can be used, false > * otherwise. > @@ -2342,10 +2359,6 @@ xfs_agfl_needs_reset( > int agfl_size = xfs_agfl_size(mp); > int active; > > - /* no agfl header on v4 supers */ > - if (!xfs_has_crc(mp)) > - return false; > - > /* > * The agf read verifier catches severe corruption of these fields. > * Repeat some sanity checks to cover a packed -> unpacked mismatch if > @@ -2889,6 +2902,19 @@ xfs_alloc_put_freelist( > return 0; > } > > +/* > + * Verify the AGF is consistent. > + * > + * We do not verify the AGFL indexes in the AGF are fully consistent here > + * because of issues with variable on-disk structure sizes. Instead, we check > + * the agfl indexes for consistency when we initialise the perag from the AGF > + * information after a read completes. > + * > + * If the index is inconsistent, then we mark the perag as needing an AGFL > + * reset. The first AGFL update performed then resets the AGFL indexes and > + * refills the AGFL with known good free blocks, allowing the filesystem to > + * continue operating normally at the cost of a few leaked free space blocks. > + */ > static xfs_failaddr_t > xfs_agf_verify( > struct xfs_buf *bp) > @@ -2962,7 +2988,6 @@ xfs_agf_verify( > return __this_address; > > return NULL; > - > } > > static void > -- > 2.40.1 >
