* [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-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
* 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
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