[PATCH 0/4] xfsprogs: a handful of fixes

[PATCH 0/4] xfsprogs: a handful of fixes

[Eric Sandeen]

4 fixes found while running xfs_repair & xfs_db over fuzzed images...

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/4] xfs_repair: validate & fix inode CRCs

[Eric Sandeen]

xfs_repair doesn't ever check an inode's CRC, so it never repairs
them.  If the root inode or realtime inodes have bad crcs, the
fs won't even mount and can't be fixed (without using xfs_db).

It's fairly straightforward to just test the inode CRC before
we do any other checking or modification of the inode, once we
get past the "verify only" phase of process_dinode_int();
just mark it dirty if it's wrong and needs to be re-written.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Resend of patch from a while ago, but with CRC verification
now earlier in the function, prior to any changes are made.

diff --git a/repair/dinode.c b/repair/dinode.c
index 5d9094b..384e2dd 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2291,6 +2291,27 @@ process_dinode_int(xfs_mount_t *mp,
 	 */
 	ASSERT(uncertain == 0 || verify_mode != 0);
 
+	/*
+	 * We'd really like to know if the CRC is bad before we
+	 * go fixing anything; that way we have some hint about
+	 * bit-rot vs bugs.  Also, any changes will invalidate the
+	 * existing CRC, so this is the only valid point to test it.
+	 *
+	 * Of course if we make any modifications after this, the
+	 * inode gets rewritten, and CRC is updated automagically.
+	 */
+	if (!verify_mode && xfs_sb_version_hascrc(&mp->m_sb)) {
+		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
+                		XFS_DINODE_CRC_OFF)) {
+			do_warn(_("bad CRC for inode %" PRIu64), lino);
+			if (!no_modify) {
+				do_warn(_(", will rewrite\n"));
+				*dirty = 1;
+			} else
+				do_warn(_(", would rewrite\n"));
+		}
+	}
+
 	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
 		retval = 1;
 		if (!uncertain)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] xfs_repair: validate & fix inode CRCs

[Carlos Maiolino]

Looks good to me.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

On Wed, Mar 04, 2015 at 02:58:06PM -0600, Eric Sandeen wrote:
> xfs_repair doesn't ever check an inode's CRC, so it never repairs
> them.  If the root inode or realtime inodes have bad crcs, the
> fs won't even mount and can't be fixed (without using xfs_db).
> 
> It's fairly straightforward to just test the inode CRC before
> we do any other checking or modification of the inode, once we
> get past the "verify only" phase of process_dinode_int();
> just mark it dirty if it's wrong and needs to be re-written.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Resend of patch from a while ago, but with CRC verification
> now earlier in the function, prior to any changes are made.
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 5d9094b..384e2dd 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2291,6 +2291,27 @@ process_dinode_int(xfs_mount_t *mp,
>  	 */
>  	ASSERT(uncertain == 0 || verify_mode != 0);
>  
> +	/*
> +	 * We'd really like to know if the CRC is bad before we
> +	 * go fixing anything; that way we have some hint about
> +	 * bit-rot vs bugs.  Also, any changes will invalidate the
> +	 * existing CRC, so this is the only valid point to test it.
> +	 *
> +	 * Of course if we make any modifications after this, the
> +	 * inode gets rewritten, and CRC is updated automagically.
> +	 */
> +	if (!verify_mode && xfs_sb_version_hascrc(&mp->m_sb)) {
> +		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
> +                		XFS_DINODE_CRC_OFF)) {
> +			do_warn(_("bad CRC for inode %" PRIu64), lino);
> +			if (!no_modify) {
> +				do_warn(_(", will rewrite\n"));
> +				*dirty = 1;
> +			} else
> +				do_warn(_(", would rewrite\n"));
> +		}
> +	}
> +
>  	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
>  		retval = 1;
>  		if (!uncertain)
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/4] xfs_repair: validate & fix inode CRCs

[Brian Foster]

On Wed, Mar 04, 2015 at 02:58:06PM -0600, Eric Sandeen wrote:
> xfs_repair doesn't ever check an inode's CRC, so it never repairs
> them.  If the root inode or realtime inodes have bad crcs, the
> fs won't even mount and can't be fixed (without using xfs_db).
> 
> It's fairly straightforward to just test the inode CRC before
> we do any other checking or modification of the inode, once we
> get past the "verify only" phase of process_dinode_int();
> just mark it dirty if it's wrong and needs to be re-written.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Resend of patch from a while ago, but with CRC verification
> now earlier in the function, prior to any changes are made.
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 5d9094b..384e2dd 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2291,6 +2291,27 @@ process_dinode_int(xfs_mount_t *mp,
>  	 */
>  	ASSERT(uncertain == 0 || verify_mode != 0);
>  
> +	/*
> +	 * We'd really like to know if the CRC is bad before we
> +	 * go fixing anything; that way we have some hint about
> +	 * bit-rot vs bugs.  Also, any changes will invalidate the
> +	 * existing CRC, so this is the only valid point to test it.
> +	 *
> +	 * Of course if we make any modifications after this, the
> +	 * inode gets rewritten, and CRC is updated automagically.
> +	 */
> +	if (!verify_mode && xfs_sb_version_hascrc(&mp->m_sb)) {

What about verify_mode? It should probably report the inode as bunk if
the crc is bad. Some of the subsequent checks handle this with some
logic to just return 1 if something is bogus and we're in verify mode.

> +		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
> +                		XFS_DINODE_CRC_OFF)) {

I got a whitespace damage warning here:

/.../xfsprogs-dev/.git/rebase-apply/patch:25: space before tab in indent.
                		XFS_DINODE_CRC_OFF)) {
warning: 1 line adds whitespace errors.

Brian

> +			do_warn(_("bad CRC for inode %" PRIu64), lino);
> +			if (!no_modify) {
> +				do_warn(_(", will rewrite\n"));
> +				*dirty = 1;
> +			} else
> +				do_warn(_(", would rewrite\n"));
> +		}
> +	}
> +
>  	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
>  		retval = 1;
>  		if (!uncertain)
> 
> _______________________________________________
> 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

Re: [PATCH 1/4] xfs_repair: validate & fix inode CRCs

[Eric Sandeen]

On 3/13/15 8:20 AM, Brian Foster wrote:

...

> What about verify_mode? It should probably report the inode as bunk if
> the crc is bad. Some of the subsequent checks handle this with some
> logic to just return 1 if something is bogus and we're in verify mode.

So, I read through the verify_mode portion on the plane ride back; I think
it's gotten a bit inconsistent over time.  I feel like I understand it a
bit better now, and will send a v3 which matches what I understand to be
its intent, and another patch to document it & make the rest consistent.

Thanks for keeping me honest.  ;)

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/4] xfs_repair: clear need_root_dotdot if we rebuild the root dir

[Eric Sandeen]

It's possible to enter longform_dir2_rebuild with need_root_dotdot
set; rebuilding it will add "..", and if need_root_dotdot stays set,
we'll add another ".." entry, causing a second repair to find and
fix that newly-introduced problem.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/repair/phase6.c b/repair/phase6.c
index 0ec4f07..1728609 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1354,6 +1354,9 @@ longform_dir2_rebuild(
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
+	if (ino == mp->m_sb.sb_rootino)
+		need_root_dotdot = 0;
+
 	/* go through the hash list and re-add the inodes */
 
 	for (p = hashtab->first; p; p = p->nextbyorder) {


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/4] xfs_repair: clear need_root_dotdot if we rebuild the root dir

[Carlos Maiolino]

Looks good

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

On Wed, Mar 04, 2015 at 02:59:02PM -0600, Eric Sandeen wrote:
> It's possible to enter longform_dir2_rebuild with need_root_dotdot
> set; rebuilding it will add "..", and if need_root_dotdot stays set,
> we'll add another ".." entry, causing a second repair to find and
> fix that newly-introduced problem.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 0ec4f07..1728609 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1354,6 +1354,9 @@ longform_dir2_rebuild(
>  
>  	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
>  
> +	if (ino == mp->m_sb.sb_rootino)
> +		need_root_dotdot = 0;
> +
>  	/* go through the hash list and re-add the inodes */
>  
>  	for (p = hashtab->first; p; p = p->nextbyorder) {
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

[Eric Sandeen]

Printing inodes with di_version == 3 skips the nlink
fields, because they are only printed if di_version == 2.
This was intended to separate them from di_version == 1,
but it mistakenly excluded di_version == 3, which also contains
these fields.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Not sure; is >= 2 ok, or should it be == 2 || == 3?
Choose your poison, I guess.

diff --git a/db/inode.c b/db/inode.c
index 982acb7..c26e1a0 100644
--- a/db/inode.c
+++ b/db/inode.c
@@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int
@@ -382,7 +382,7 @@ inode_core_onlink_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int
@@ -395,7 +395,7 @@ inode_core_projid_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

[Carlos Maiolino]

I'm not sure why we differentiate inode versions here, to print the nlink, I
probably have never seen a filesystem with di_version == 1 (feeling young :)

Why does it needs to be tested? I don't think di_version 1 didn't have nlink
field, but if it did, and if this is the only difference, in this subject, I
don't see a problem in having it tested with >= 2. But, if there could be more
than only this difference, maybe some kind of helper function, or even a macro
to make this test, taking into account the di_version.

On Wed, Mar 04, 2015 at 03:00:47PM -0600, Eric Sandeen wrote:
> Printing inodes with di_version == 3 skips the nlink
> fields, because they are only printed if di_version == 2.
> This was intended to separate them from di_version == 1,
> but it mistakenly excluded di_version == 3, which also contains
> these fields.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Not sure; is >= 2 ok, or should it be == 2 || == 3?
> Choose your poison, I guess.
> 
> diff --git a/db/inode.c b/db/inode.c
> index 982acb7..c26e1a0 100644
> --- a/db/inode.c
> +++ b/db/inode.c
> @@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
>  	ASSERT(startoff == 0);
>  	ASSERT(obj == iocur_top->data);
>  	dic = obj;
> -	return dic->di_version == 2;
> +	return dic->di_version >= 2;
>  }
>  
>  static int
> @@ -382,7 +382,7 @@ inode_core_onlink_count(
>  	ASSERT(startoff == 0);
>  	ASSERT(obj == iocur_top->data);
>  	dic = obj;
> -	return dic->di_version == 2;
> +	return dic->di_version >= 2;
>  }
>  
>  static int
> @@ -395,7 +395,7 @@ inode_core_projid_count(
>  	ASSERT(startoff == 0);
>  	ASSERT(obj == iocur_top->data);
>  	dic = obj;
> -	return dic->di_version == 2;
> +	return dic->di_version >= 2;
>  }
>  
>  static int
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

[Eric Sandeen]

On 3/9/15 2:49 PM, Carlos Maiolino wrote:
> I'm not sure why we differentiate inode versions here, to print the nlink, I
> probably have never seen a filesystem with di_version == 1 (feeling young :)

di_version=1 had only a 16-bit nlink field, now called di_onlink ("old
nlink").  Later a new 32-bit structure member was added for this purpose.

xfs_db decides what to print, from this table:

        { "nlinkv1", FLDT_UINT16D, OI(COFF(onlink)), inode_core_nlinkv1_count,
          FLD_COUNT, TYP_NONE },
        { "nlinkv2", FLDT_UINT32D, OI(COFF(nlink)), inode_core_nlinkv2_count,
          FLD_COUNT, TYP_NONE },
        { "onlink", FLDT_UINT16D, OI(COFF(onlink)), inode_core_onlink_count,
          FLD_COUNT, TYP_NONE },

where the specified functions test whether the field should be printed.

This means "nlinkv1" will be displayed only if di_version==1, and
"onlink" & "nlinkv2" will be displayed only if di_version==2 (today)
or >= 2 (with my patch).

(the di_nlink field got renamed to di_onlink, and a new 16-bit di_nlink
member was added.  xfs_db doesn't always print the actual structure names
verbatim)

> Why does it needs to be tested? I don't think di_version 1 didn't have nlink
> field, but if it did, and if this is the only difference, in this subject, I
> don't see a problem in having it tested with >= 2. But, if there could be more
> than only this difference, maybe some kind of helper function, or even a macro
> to make this test, taking into account the di_version.

It's tested because on a version 1 inode, the 32-bit nlink field doesn't even
exist.  We'd be printing random data from the disk.

-Eric

> On Wed, Mar 04, 2015 at 03:00:47PM -0600, Eric Sandeen wrote:
>> Printing inodes with di_version == 3 skips the nlink
>> fields, because they are only printed if di_version == 2.
>> This was intended to separate them from di_version == 1,
>> but it mistakenly excluded di_version == 3, which also contains
>> these fields.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Not sure; is >= 2 ok, or should it be == 2 || == 3?
>> Choose your poison, I guess.
>>
>> diff --git a/db/inode.c b/db/inode.c
>> index 982acb7..c26e1a0 100644
>> --- a/db/inode.c
>> +++ b/db/inode.c
>> @@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
>>  	ASSERT(startoff == 0);
>>  	ASSERT(obj == iocur_top->data);
>>  	dic = obj;
>> -	return dic->di_version == 2;
>> +	return dic->di_version >= 2;
>>  }
>>  
>>  static int
>> @@ -382,7 +382,7 @@ inode_core_onlink_count(
>>  	ASSERT(startoff == 0);
>>  	ASSERT(obj == iocur_top->data);
>>  	dic = obj;
>> -	return dic->di_version == 2;
>> +	return dic->di_version >= 2;
>>  }
>>  
>>  static int
>> @@ -395,7 +395,7 @@ inode_core_projid_count(
>>  	ASSERT(startoff == 0);
>>  	ASSERT(obj == iocur_top->data);
>>  	dic = obj;
>> -	return dic->di_version == 2;
>> +	return dic->di_version >= 2;
>>  }
>>  
>>  static int
>>
>>
>> _______________________________________________
>> 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

Re: [PATCH 3/4] xfs_db: show nlink fields for di_version == 3, too

[Brian Foster]

On Wed, Mar 04, 2015 at 03:00:47PM -0600, Eric Sandeen wrote:
> Printing inodes with di_version == 3 skips the nlink
> fields, because they are only printed if di_version == 2.
> This was intended to separate them from di_version == 1,
> but it mistakenly excluded di_version == 3, which also contains
> these fields.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Not sure; is >= 2 ok, or should it be == 2 || == 3?
> Choose your poison, I guess.
> 

Seems fine to me. I think it's better to only have to worry about this
code if something related to these fields changes rather than every time
a version update occurs.

Reviewed-by: Brian Foster <bfoster@redhat.com>

> diff --git a/db/inode.c b/db/inode.c
> index 982acb7..c26e1a0 100644
> --- a/db/inode.c
> +++ b/db/inode.c
> @@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
>  	ASSERT(startoff == 0);
>  	ASSERT(obj == iocur_top->data);
>  	dic = obj;
> -	return dic->di_version == 2;
> +	return dic->di_version >= 2;
>  }
>  
>  static int
> @@ -382,7 +382,7 @@ inode_core_onlink_count(
>  	ASSERT(startoff == 0);
>  	ASSERT(obj == iocur_top->data);
>  	dic = obj;
> -	return dic->di_version == 2;
> +	return dic->di_version >= 2;
>  }
>  
>  static int
> @@ -395,7 +395,7 @@ inode_core_projid_count(
>  	ASSERT(startoff == 0);
>  	ASSERT(obj == iocur_top->data);
>  	dic = obj;
> -	return dic->di_version == 2;
> +	return dic->di_version >= 2;
>  }
>  
>  static int
> 
> 
> _______________________________________________
> 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

[PATCH 4/4] xfs_repair: set *parent if process_dir2_data() fixes root inode's parent

[Eric Sandeen]

process_dir2_data() may fix the root dir's parent inode:

"bad .. entry in root directory inode 6912, was 7159: correcting"

But we don't update the *parent passed in in that case; this then leads to
an assert later in process_dir2, because *parent is still the wrong value:

xfs_repair: dir2.c:2039: process_dir2:
  Assertion `(ino != mp->m_sb.sb_rootino && ino != *parent) ||
  (ino == mp->m_sb.sb_rootino && (ino == *parent || need_root_dotdot == 1))'
  failed.

Updating the value of *parent when we fix the parent value resolves this
problem.  Do it whether or not we're in no_modify mode.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/repair/dir2.c b/repair/dir2.c
index 6b8964d..67cd9d1 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -1468,6 +1468,7 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "),
 					} else {
 						do_warn(_("would correct\n"));
 					}
+					*parent = ino;
 				}
 			}
 			/*

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] xfs_repair: set *parent if process_dir2_data() fixes root inode's parent

[Carlos Maiolino]

Makes sense to me.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Thanks Eric :-)

Cheers,

On Wed, Mar 04, 2015 at 03:02:10PM -0600, Eric Sandeen wrote:
> process_dir2_data() may fix the root dir's parent inode:
> 
> "bad .. entry in root directory inode 6912, was 7159: correcting"
> 
> But we don't update the *parent passed in in that case; this then leads to
> an assert later in process_dir2, because *parent is still the wrong value:
> 
> xfs_repair: dir2.c:2039: process_dir2:
>   Assertion `(ino != mp->m_sb.sb_rootino && ino != *parent) ||
>   (ino == mp->m_sb.sb_rootino && (ino == *parent || need_root_dotdot == 1))'
>   failed.
> 
> Updating the value of *parent when we fix the parent value resolves this
> problem.  Do it whether or not we're in no_modify mode.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 6b8964d..67cd9d1 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -1468,6 +1468,7 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "),
>  					} else {
>  						do_warn(_("would correct\n"));
>  					}
> +					*parent = ino;
>  				}
>  			}
>  			/*
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs