* [PATCH] xfs_repair: skip freelist scan of dodgy agf in no-modify mode @ 2013-03-01 23:46 Eric Sandeen 2013-03-02 1:18 ` Dave Chinner 2013-03-02 21:23 ` [PATCH V2] xfs_repair: skip freelist scan of corrupt " Eric Sandeen 0 siblings, 2 replies; 10+ messages in thread From: Eric Sandeen @ 2013-03-01 23:46 UTC (permalink / raw) To: xfs-oss; +Cc: Ole Tange In 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. In addition, add a check to verify_set_agf() to ensure that first <= last. Reported-by: Ole Tange <tange@binf.ku.dk> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/repair/agheader.c b/repair/agheader.c index 769022d..68789fe 100644 --- a/repair/agheader.c +++ b/repair/agheader.c @@ -86,6 +86,14 @@ verify_set_agf(xfs_mount_t *mp, xfs_agf_t *agf, xfs_agnumber_t i) * check first/last AGF fields. if need be, lose the free * space in the AGFL, we'll reclaim it later. */ + if (be32_to_cpu(agf->agf_flfirst) > be32_to_cpu(agf->agf_fllast)) { + do_warn(_("flfirst %d in agf %d > fllast %d\n"), + be32_to_cpu(agf->agf_flfirst), + i, be32_to_cpu(agf->agf_fllast)); + if (!no_modify) + agf->agf_fllast = agf->agf_flfirst = cpu_to_be32(0); + } + if (be32_to_cpu(agf->agf_flfirst) >= XFS_AGFL_SIZE(mp)) { do_warn(_("flfirst %d in agf %d too large (max = %zu)\n"), be32_to_cpu(agf->agf_flfirst), diff --git a/repair/scan.c b/repair/scan.c index 5345094..0f83fb4 100644 --- a/repair/scan.c +++ b/repair/scan.c @@ -1067,6 +1067,17 @@ scan_freelist( } agfl = XFS_BUF_TO_AGFL(agflbuf); i = be32_to_cpu(agf->agf_flfirst); + if (no_modify) { + /* agf values not sanitized, so double check */ + if (i >= XFS_AGFL_SIZE(mp) || + be32_to_cpu(agf->agf_fllast) >= XFS_AGFL_SIZE(mp) || + i > be32_to_cpu(agf->agf_fllast)) + do_warn(_("agf %d freelist blocks bad, skipping scan\n"), + i); + return; + } else /* should have been fixed in verify_set_agf() */ + ASSERT(0); + count = 0; for (;;) { bno = be32_to_cpu(agfl->agfl_bno[i]); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs_repair: skip freelist scan of dodgy agf in no-modify mode 2013-03-01 23:46 [PATCH] xfs_repair: skip freelist scan of dodgy agf in no-modify mode Eric Sandeen @ 2013-03-02 1:18 ` Dave Chinner 2013-03-02 1:22 ` Eric Sandeen 2013-03-02 21:23 ` [PATCH V2] xfs_repair: skip freelist scan of corrupt " Eric Sandeen 1 sibling, 1 reply; 10+ messages in thread From: Dave Chinner @ 2013-03-02 1:18 UTC (permalink / raw) To: Eric Sandeen; +Cc: Ole Tange, xfs-oss On Fri, Mar 01, 2013 at 05:46:48PM -0600, Eric Sandeen wrote: > In 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. > > In addition, add a check to verify_set_agf() to ensure that > first <= last. > > Reported-by: Ole Tange <tange@binf.ku.dk> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/repair/agheader.c b/repair/agheader.c > index 769022d..68789fe 100644 > --- a/repair/agheader.c > +++ b/repair/agheader.c > @@ -86,6 +86,14 @@ verify_set_agf(xfs_mount_t *mp, xfs_agf_t *agf, xfs_agnumber_t i) > * check first/last AGF fields. if need be, lose the free > * space in the AGFL, we'll reclaim it later. > */ > + if (be32_to_cpu(agf->agf_flfirst) > be32_to_cpu(agf->agf_fllast)) { > + do_warn(_("flfirst %d in agf %d > fllast %d\n"), > + be32_to_cpu(agf->agf_flfirst), > + i, be32_to_cpu(agf->agf_fllast)); > + if (!no_modify) > + agf->agf_fllast = agf->agf_flfirst = cpu_to_be32(0); > + } I don't think that test is correct. The free list is circular and is indexed as a pair of head/tail pointers. Hence flfirst > fllast can be actually valid. e.g. flcount = 4, flfirst=126, fllast = 2 is a valid free list. > + > if (be32_to_cpu(agf->agf_flfirst) >= XFS_AGFL_SIZE(mp)) { > do_warn(_("flfirst %d in agf %d too large (max = %zu)\n"), > be32_to_cpu(agf->agf_flfirst), > diff --git a/repair/scan.c b/repair/scan.c > index 5345094..0f83fb4 100644 > --- a/repair/scan.c > +++ b/repair/scan.c > @@ -1067,6 +1067,17 @@ scan_freelist( > } > agfl = XFS_BUF_TO_AGFL(agflbuf); > i = be32_to_cpu(agf->agf_flfirst); > + if (no_modify) { > + /* agf values not sanitized, so double check */ > + if (i >= XFS_AGFL_SIZE(mp) || > + be32_to_cpu(agf->agf_fllast) >= XFS_AGFL_SIZE(mp) || > + i > be32_to_cpu(agf->agf_fllast)) > + do_warn(_("agf %d freelist blocks bad, skipping scan\n"), "skipping freelist scan" > + i); > + return; Also, you might be missing a set of {} there - that will always return immediately if no_modify is set.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xfs_repair: skip freelist scan of dodgy agf in no-modify mode 2013-03-02 1:18 ` Dave Chinner @ 2013-03-02 1:22 ` Eric Sandeen 0 siblings, 0 replies; 10+ messages in thread From: Eric Sandeen @ 2013-03-02 1:22 UTC (permalink / raw) To: Dave Chinner; +Cc: Ole Tange, xfs-oss On Mar 1, 2013, at 7:18 PM, Dave Chinner <david@fromorbit.com> wrote: > On Fri, Mar 01, 2013 at 05:46:48PM -0600, Eric Sandeen wrote: >> In 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. >> >> In addition, add a check to verify_set_agf() to ensure that >> first <= last. >> >> Reported-by: Ole Tange <tange@binf.ku.dk> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/repair/agheader.c b/repair/agheader.c >> index 769022d..68789fe 100644 >> --- a/repair/agheader.c >> +++ b/repair/agheader.c >> @@ -86,6 +86,14 @@ verify_set_agf(xfs_mount_t *mp, xfs_agf_t *agf, xfs_agnumber_t i) >> * check first/last AGF fields. if need be, lose the free >> * space in the AGFL, we'll reclaim it later. >> */ >> + if (be32_to_cpu(agf->agf_flfirst) > be32_to_cpu(agf->agf_fllast)) { >> + do_warn(_("flfirst %d in agf %d > fllast %d\n"), >> + be32_to_cpu(agf->agf_flfirst), >> + i, be32_to_cpu(agf->agf_fllast)); >> + if (!no_modify) >> + agf->agf_fllast = agf->agf_flfirst = cpu_to_be32(0); >> + } > > I don't think that test is correct. The free list is circular and is > indexed as a pair of head/tail pointers. Hence flfirst > fllast can > be actually valid. e.g. flcount = 4, flfirst=126, fllast = 2 is a > valid free list. > Doh, ok. Will fix. >> + >> if (be32_to_cpu(agf->agf_flfirst) >= XFS_AGFL_SIZE(mp)) { >> do_warn(_("flfirst %d in agf %d too large (max = %zu)\n"), >> be32_to_cpu(agf->agf_flfirst), >> diff --git a/repair/scan.c b/repair/scan.c >> index 5345094..0f83fb4 100644 >> --- a/repair/scan.c >> +++ b/repair/scan.c >> @@ -1067,6 +1067,17 @@ scan_freelist( >> } >> agfl = XFS_BUF_TO_AGFL(agflbuf); >> i = be32_to_cpu(agf->agf_flfirst); >> + if (no_modify) { >> + /* agf values not sanitized, so double check */ >> + if (i >= XFS_AGFL_SIZE(mp) || >> + be32_to_cpu(agf->agf_fllast) >= XFS_AGFL_SIZE(mp) || >> + i > be32_to_cpu(agf->agf_fllast)) >> + do_warn(_("agf %d freelist blocks bad, skipping scan\n"), > > "skipping freelist scan" > >> + i); >> + return; > > Also, you might be missing a set of {} there - that will always > return immediately if no_modify is set.... > Cripes. 1 demerit for me, sorry. Will fix and resend. Thanks for the embarrassing review. :) -Eric > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-01 23:46 [PATCH] xfs_repair: skip freelist scan of dodgy agf in no-modify mode Eric Sandeen 2013-03-02 1:18 ` Dave Chinner @ 2013-03-02 21:23 ` Eric Sandeen 2013-03-03 23:36 ` Dave Chinner 2013-03-08 20:31 ` Rich Johnston 1 sibling, 2 replies; 10+ messages in thread From: Eric Sandeen @ 2013-03-02 21:23 UTC (permalink / raw) To: xfs-oss; +Cc: Ole Tange 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@binf.ku.dk> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- 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 (;;) { _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-02 21:23 ` [PATCH V2] xfs_repair: skip freelist scan of corrupt " Eric Sandeen @ 2013-03-03 23:36 ` Dave Chinner 2013-03-08 20:31 ` Rich Johnston 1 sibling, 0 replies; 10+ messages in thread From: Dave Chinner @ 2013-03-03 23:36 UTC (permalink / raw) To: Eric Sandeen; +Cc: Ole Tange, xfs-oss 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@binf.ku.dk> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > 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@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-02 21:23 ` [PATCH V2] xfs_repair: skip freelist scan of corrupt " Eric Sandeen 2013-03-03 23:36 ` Dave Chinner @ 2013-03-08 20:31 ` Rich Johnston 2013-03-08 20:31 ` Eric Sandeen ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Rich Johnston @ 2013-03-08 20:31 UTC (permalink / raw) To: Eric Sandeen; +Cc: Ole Tange, xfs-oss This version looks good. ;) Reviewed-by: Rich Johnston <rjohnston@sgi.com> This has been committed. commit 7e8e3cce00f38ee1533df0e7bda6bcb584b03e96 Author: Eric Sandeen <sandeen@sandeen.net> Date: Sat Mar 2 21:23:12 2013 +0000 xfsprogs: xfs_repair skip freelist scan of corrupt agf in no-modify mode 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. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-08 20:31 ` Rich Johnston @ 2013-03-08 20:31 ` Eric Sandeen 2013-03-09 9:00 ` Dave Chinner 2013-03-09 15:00 ` Eric Sandeen 2 siblings, 0 replies; 10+ messages in thread From: Eric Sandeen @ 2013-03-08 20:31 UTC (permalink / raw) To: Rich Johnston; +Cc: Ole Tange, xfs-oss On 3/8/13 2:31 PM, Rich Johnston wrote: > This version looks good. ;) > > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > This has been committed. Thanks; from what Dave said there are probably other spots that need love as well. -Eric > commit 7e8e3cce00f38ee1533df0e7bda6bcb584b03e96 > Author: Eric Sandeen <sandeen@sandeen.net> > Date: Sat Mar 2 21:23:12 2013 +0000 > > xfsprogs: xfs_repair skip freelist scan of corrupt agf in no-modify mode > > 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. > > > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-08 20:31 ` Rich Johnston 2013-03-08 20:31 ` Eric Sandeen @ 2013-03-09 9:00 ` Dave Chinner 2013-03-11 12:20 ` Rich Johnston 2013-03-09 15:00 ` Eric Sandeen 2 siblings, 1 reply; 10+ messages in thread From: Dave Chinner @ 2013-03-09 9:00 UTC (permalink / raw) To: Rich Johnston; +Cc: xfs-oss, Eric Sandeen, Ole Tange On Fri, Mar 08, 2013 at 02:31:23PM -0600, Rich Johnston wrote: > This version looks good. ;) > > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > This has been committed. Except that all review comments have not been addressed. i.e there are two places that have the same bug and they haven't been fixed. It's great that you want to commit quickly, but commits should not happen while there are unaddressed issues still outstanding.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-09 9:00 ` Dave Chinner @ 2013-03-11 12:20 ` Rich Johnston 0 siblings, 0 replies; 10+ messages in thread From: Rich Johnston @ 2013-03-11 12:20 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs-oss, Eric Sandeen, Ole Tange On 03/09/2013 03:00 AM, Dave Chinner wrote: > On Fri, Mar 08, 2013 at 02:31:23PM -0600, Rich Johnston wrote: >> This version looks good. ;) >> >> Reviewed-by: Rich Johnston <rjohnston@sgi.com> >> >> This has been committed. > > Except that all review comments have not been addressed. i.e there > are two places that have the same bug and they haven't been fixed. > > It's great that you want to commit quickly, but commits should not > happen while there are unaddressed issues still outstanding.... Sorry Dave I misunderstood your comments. I took them to mean you were going to make those changes with your CRC patch. I will make sure to clarify before committing. > > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] xfs_repair: skip freelist scan of corrupt agf in no-modify mode 2013-03-08 20:31 ` Rich Johnston 2013-03-08 20:31 ` Eric Sandeen 2013-03-09 9:00 ` Dave Chinner @ 2013-03-09 15:00 ` Eric Sandeen 2 siblings, 0 replies; 10+ messages in thread From: Eric Sandeen @ 2013-03-09 15:00 UTC (permalink / raw) To: Rich Johnston; +Cc: xfs-oss, Ole Tange On 3/8/13 2:31 PM, Rich Johnston wrote: > This version looks good. ;) Actually it's not quite: scan.c: In function ‘scan_freelist’: scan.c:1074: warning: ‘i’ may be used uninitialized in this function I don't know what's wrong with my brain these days. :/ I will send a patch to fix that along w/ the other scan_freelists shortly. Sorry :/ -Eric > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > This has been committed. > > commit 7e8e3cce00f38ee1533df0e7bda6bcb584b03e96 > Author: Eric Sandeen <sandeen@sandeen.net> > Date: Sat Mar 2 21:23:12 2013 +0000 > > xfsprogs: xfs_repair skip freelist scan of corrupt agf in no-modify mode > > 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. > > > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-03-11 12:20 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-01 23:46 [PATCH] xfs_repair: skip freelist scan of dodgy agf in no-modify mode Eric Sandeen 2013-03-02 1:18 ` Dave Chinner 2013-03-02 1:22 ` Eric Sandeen 2013-03-02 21:23 ` [PATCH V2] xfs_repair: skip freelist scan of corrupt " Eric Sandeen 2013-03-03 23:36 ` Dave Chinner 2013-03-08 20:31 ` Rich Johnston 2013-03-08 20:31 ` Eric Sandeen 2013-03-09 9:00 ` Dave Chinner 2013-03-11 12:20 ` Rich Johnston 2013-03-09 15:00 ` Eric Sandeen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox