xfs
[Top] [All Lists]

Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modif

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 4 Mar 2013 10:36:15 +1100
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Ole Tange <tange@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51326DC0.8030403@xxxxxxxxxxx>
References: <51313DE8.5080104@xxxxxxxxxxx> <51326DC0.8030403@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Sat, Mar 02, 2013 at 03:23:12PM -0600, Eric Sandeen wrote:
> In xfs_repair's no-modify mode (-n), verify_set_agf doesn't fix up
> bad freelist blocks that it finds.  When we get to scan_freelist,
> this can wreak havoc if, for example, first > last and the loop
> never exits; we index agfl->agfl_bno[i] off into the weeds.
> 
> To fix this, re-check the values in no-modify mode, and if
> they're off, warn about it and skip the scan.
> 
> Reported-by: Ole Tange <tange@xxxxxxxxxx>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> 
> V2: Remove dumb mistakes :/
> 
> diff --git a/repair/scan.c b/repair/scan.c
> index 5345094..1d39bdc 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -1066,6 +1066,18 @@ scan_freelist(
>               return;
>       }
>       agfl = XFS_BUF_TO_AGFL(agflbuf);
> +
> +     if (no_modify) {
> +             /* agf values not fixed in verify_set_agf, so recheck */
> +             if (be32_to_cpu(agf->agf_flfirst) >= XFS_AGFL_SIZE(mp) ||
> +                 be32_to_cpu(agf->agf_fllast) >= XFS_AGFL_SIZE(mp)) {
> +                     do_warn(_("agf %d freelist blocks bad, skipping "
> +                               "freelist scan\n"), i);
> +                     return;
> +             }
> +     } else /* should have been fixed in verify_set_agf() */
> +             ASSERT(0);
> +
>       i = be32_to_cpu(agf->agf_flfirst);
>       count = 0;
>       for (;;) {

Looks ok, but IIRC there are overruns in these functions for the
same reason (i.e. unchecked use of agf->agf_flfirst as an array
index)

db/check.c::scan_freelist()
db/freesp.c::scan_freelist()

I found lots of almost-but-not-quite-exact code duplication like
this recenty when doing CRC updates to the userspace code....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>