[PATCH 00/11] xfsprogs-4.17: xfs_repair fixes

[PATCH 00/11] xfsprogs-4.17: xfs_repair fixes

[Darrick J. Wong]

Hi all,

This series is a result of me triaging the fuzz xfstests looking for
places where xfs_repair crashed, failed to detect corruption, or didn't
fix problems correctly.  For the most part this means that we check
metadata that we previously missed, and we're more careful not to update
in-core state until we actually validate entire records.  A few ASSERTs
are also removed in favor of handling in-core state properly.

This probably won't eat your data, and the branch[2] should apply
against for-next.

--D

[1] https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=djwong-devel

[PATCH 01/11] xfs_repair: examine all remote attribute blocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Examine all remote xattr values of a file, not just the XFS_ATTR_ROOT
values.  This enables us to detect and zap corrupt user xattrs, as
tested by xfs/404.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/attr_repair.c |    3 ---
 1 file changed, 3 deletions(-)


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 8b1b8a7..bb5ab3d 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -537,9 +537,6 @@ process_leaf_attr_remote(
 		return -1;
 	}
 
-	if (!(entry->flags & XFS_ATTR_ROOT))
-		goto out;
-
 	value = malloc(be32_to_cpu(remotep->valuelen));
 	if (value == NULL) {
 		do_warn(

Re: [PATCH 01/11] xfs_repair: examine all remote attribute blocks

[Eric Sandeen]

On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Examine all remote xattr values of a file, not just the XFS_ATTR_ROOT
> values.  This enables us to detect and zap corrupt user xattrs, as
> tested by xfs/404.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Whoa.  ;)  Where'd this come from?  At first glance it seems crazy to only
check XFS_ATTR_ROOT but then I stated digging a little...

This is essentially akin to other code we still have in the local
case,

        /* Only check values for root security attributes */
        if (entry->flags & XFS_ATTR_ROOT) {
                if (valuecheck(mp, (char *)&local->nameval[0], NULL,
                                local->namelen, be16_to_cpu(local->valuelen))) {
                        do_warn(
        _("bad security value for attribute entry %d in attr block %u, inode %" PRIu64 "\n"),
                                i, da_bno, ino);
                        return -1;
                }
        }

soooo this patch essentially allows valuecheck on !XFS_ATTR_ROOT attributes,
but valuecheck says:

 * Calls will be made to xfs_mac_valid or xfs_acl_valid routines if the
 * security attributes exist. They will be cleared if invalid.
 * No other values will be checked. 

So, um, what's actually getting fixed?  Ah, ok:

this also allows us to simply try to /get/ the remote attribute:

rmtval_get()

and if that fails:

                do_warn(
        _("remote attribute get failed for entry %d, inode %" PRIu64 "\n"),
                        i, ino);
                goto bad_free_out;

we zap it.

So ... uh...

I think you want to /move/ the XFS_ATTR_ROOT check before the call to valuecheck(),
rather than removing it entirely.

Ok, at this rate I'll have all 11 patches done by June.

-Eric

> ---
>  repair/attr_repair.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> 
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index 8b1b8a7..bb5ab3d 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -537,9 +537,6 @@ process_leaf_attr_remote(
>  		return -1;
>  	}
>  
> -	if (!(entry->flags & XFS_ATTR_ROOT))
> -		goto out;
> -
>  	value = malloc(be32_to_cpu(remotep->valuelen));
>  	if (value == NULL) {
>  		do_warn(
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 01/11] xfs_repair: examine all remote attribute blocks

[Darrick J. Wong]

On Fri, May 04, 2018 at 01:20:42PM -0500, Eric Sandeen wrote:
> On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Examine all remote xattr values of a file, not just the XFS_ATTR_ROOT
> > values.  This enables us to detect and zap corrupt user xattrs, as
> > tested by xfs/404.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Whoa.  ;)  Where'd this come from?  At first glance it seems crazy to only
> check XFS_ATTR_ROOT but then I stated digging a little...
> 
> This is essentially akin to other code we still have in the local
> case,
> 
>         /* Only check values for root security attributes */
>         if (entry->flags & XFS_ATTR_ROOT) {
>                 if (valuecheck(mp, (char *)&local->nameval[0], NULL,
>                                 local->namelen, be16_to_cpu(local->valuelen))) {
>                         do_warn(
>         _("bad security value for attribute entry %d in attr block %u, inode %" PRIu64 "\n"),
>                                 i, da_bno, ino);
>                         return -1;
>                 }
>         }
> 
> soooo this patch essentially allows valuecheck on !XFS_ATTR_ROOT attributes,
> but valuecheck says:
> 
>  * Calls will be made to xfs_mac_valid or xfs_acl_valid routines if the
>  * security attributes exist. They will be cleared if invalid.
>  * No other values will be checked. 
> 
> So, um, what's actually getting fixed?  Ah, ok:
> 
> this also allows us to simply try to /get/ the remote attribute:
> 
> rmtval_get()
> 
> and if that fails:
> 
>                 do_warn(
>         _("remote attribute get failed for entry %d, inode %" PRIu64 "\n"),
>                         i, ino);
>                 goto bad_free_out;
> 
> we zap it.
> 
> So ... uh...
> 
> I think you want to /move/ the XFS_ATTR_ROOT check before the call to valuecheck(),
> rather than removing it entirely.

Yes.  I could just make it part of the valuecheck test...

if (rmtval_get(...)) {
	do_warn(...);
	goto bad_free_out;
}

if ((entry->flags & XFS_ATTR_ROOT) &&
    valuecheck(...)) {
	do_warn(...);
	goto bad_free_out;
}

free(value);

--D

> 
> Ok, at this rate I'll have all 11 patches done by June.
> 
> -Eric
> 
> > ---
> >  repair/attr_repair.c |    3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > 
> > diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> > index 8b1b8a7..bb5ab3d 100644
> > --- a/repair/attr_repair.c
> > +++ b/repair/attr_repair.c
> > @@ -537,9 +537,6 @@ process_leaf_attr_remote(
> >  		return -1;
> >  	}
> >  
> > -	if (!(entry->flags & XFS_ATTR_ROOT))
> > -		goto out;
> > -
> >  	value = malloc(be32_to_cpu(remotep->valuelen));
> >  	if (value == NULL) {
> >  		do_warn(
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 01/11] xfs_repair: examine all remote attribute blocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Examine all remote xattr values of a file, not just the XFS_ATTR_ROOT
values.  This enables us to detect and zap corrupt user xattrs, as
tested by xfs/404.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/attr_repair.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 8b1b8a75..67bb41ec 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -537,9 +537,6 @@ process_leaf_attr_remote(
 		return -1;
 	}
 
-	if (!(entry->flags & XFS_ATTR_ROOT))
-		goto out;
-
 	value = malloc(be32_to_cpu(remotep->valuelen));
 	if (value == NULL) {
 		do_warn(
@@ -555,7 +552,8 @@ process_leaf_attr_remote(
 			i, ino);
 		goto bad_free_out;
 	}
-	if (valuecheck(mp, (char *)&remotep->name[0], value, remotep->namelen,
+	if ((entry->flags & XFS_ATTR_ROOT) &&
+	    valuecheck(mp, (char *)&remotep->name[0], value, remotep->namelen,
 				be32_to_cpu(remotep->valuelen))) {
 		do_warn(
 	_("remote attribute value check failed for entry %d, inode %" PRIu64 "\n"),

Re: [PATCH v2 01/11] xfs_repair: examine all remote attribute blocks

[Allison Henderson]

Alrighty, looks ok.  Thx!

Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/22/2018 08:15 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Examine all remote xattr values of a file, not just the XFS_ATTR_ROOT
> values.  This enables us to detect and zap corrupt user xattrs, as
> tested by xfs/404.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   repair/attr_repair.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index 8b1b8a75..67bb41ec 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -537,9 +537,6 @@ process_leaf_attr_remote(
>   		return -1;
>   	}
>   
> -	if (!(entry->flags & XFS_ATTR_ROOT))
> -		goto out;
> -
>   	value = malloc(be32_to_cpu(remotep->valuelen));
>   	if (value == NULL) {
>   		do_warn(
> @@ -555,7 +552,8 @@ process_leaf_attr_remote(
>   			i, ino);
>   		goto bad_free_out;
>   	}
> -	if (valuecheck(mp, (char *)&remotep->name[0], value, remotep->namelen,
> +	if ((entry->flags & XFS_ATTR_ROOT) &&
> +	    valuecheck(mp, (char *)&remotep->name[0], value, remotep->namelen,
>   				be32_to_cpu(remotep->valuelen))) {
>   		do_warn(
>   	_("remote attribute value check failed for entry %d, inode %" PRIu64 "\n"),
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=Vc_K6AveietpcEeQdJ30FwiEHLOuaBg_hGM_aW3IuTM&s=0ZEyUF20gQ7jZ-6j_k9Nsrddrvue9p8Wr6Xo9hwqMZE&e=
> 

[PATCH 02/11] xfs_repair: don't leak buffer on xattr remote buf verifier error

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we try to read an xattr remote buffer and hit a verifier error,
release the buffer instead of leaking it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/attr_repair.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index bb5ab3d..3e6efd0 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -433,6 +433,7 @@ rmtval_get(xfs_mount_t *mp, xfs_ino_t ino, blkmap_t *blkmap,
 		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
 			do_warn(
 	_("Corrupt remote block for attributes of inode %" PRIu64 "\n"), ino);
+			libxfs_putbuf(bp);
 			clearit = 1;
 			break;
 		}

Re: [PATCH 02/11] xfs_repair: don't leak buffer on xattr remote buf verifier error

[Eric Sandeen]

On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we try to read an xattr remote buffer and hit a verifier error,
> release the buffer instead of leaking it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Good plan.

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

> ---
>  repair/attr_repair.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index bb5ab3d..3e6efd0 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -433,6 +433,7 @@ rmtval_get(xfs_mount_t *mp, xfs_ino_t ino, blkmap_t *blkmap,
>  		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
>  			do_warn(
>  	_("Corrupt remote block for attributes of inode %" PRIu64 "\n"), ino);
> +			libxfs_putbuf(bp);
>  			clearit = 1;
>  			break;
>  		}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 03/11] xfs_repair: validate some of the log space information

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Validate the log space information in a similar manner to the kernel so
that repair will stumble over (and fix) broken log info that prevents
mounting.  Fixes logsunit fuzz-and-fix failures in xfs/350.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/sb.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)


diff --git a/repair/sb.c b/repair/sb.c
index 3dc6538..f2968cd 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -299,6 +299,27 @@ sb_validate_ino_align(struct xfs_sb *sb)
 }
 
 /*
+ * Validate the given log space.  Derived from xfs_log_mount, though we
+ * can't validate the minimum log size until later.
+ * Returns false if the log is garbage.
+ */
+static bool
+verify_sb_loginfo(
+	struct xfs_sb	*sb)
+{
+	if (xfs_sb_version_hascrc(sb) &&
+	    (sb->sb_logblocks == 0 ||
+	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
+	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
+		return false;
+
+	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
+		return false;
+
+	return true;
+}
+
+/*
  * verify a superblock -- does not verify root inode #
  *	can only check that geometry info is internally
  *	consistent.  because of growfs, that's no guarantee
@@ -409,7 +430,8 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
 	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
 	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
-	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
+	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) ||
+	    !verify_sb_loginfo(sb))
 		return XR_BAD_INO_SIZE_DATA;
 
 	if (xfs_sb_version_hassector(sb))  {

Re: [PATCH 03/11] xfs_repair: validate some of the log space information

[Eric Sandeen]

On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Validate the log space information in a similar manner to the kernel so
> that repair will stumble over (and fix) broken log info that prevents
> mounting.  Fixes logsunit fuzz-and-fix failures in xfs/350.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/sb.c |   24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index 3dc6538..f2968cd 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -299,6 +299,27 @@ sb_validate_ino_align(struct xfs_sb *sb)
>  }
>  
>  /*
> + * Validate the given log space.  Derived from xfs_log_mount, though we
> + * can't validate the minimum log size until later.
> + * Returns false if the log is garbage.
> + */
> +static bool
> +verify_sb_loginfo(
> +	struct xfs_sb	*sb)
> +{
> +	if (xfs_sb_version_hascrc(sb) &&

Hm, this could use a comment about why these things only matter on v5 supers.


> +	    (sb->sb_logblocks == 0 ||
> +	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
> +	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
> +		return false;
> +
> +	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
>   * verify a superblock -- does not verify root inode #
>   *	can only check that geometry info is internally
>   *	consistent.  because of growfs, that's no guarantee
> @@ -409,7 +430,8 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
>  	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
>  	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
>  	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
> -	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
> +	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) ||
> +	    !verify_sb_loginfo(sb))
>  		return XR_BAD_INO_SIZE_DATA;

do you really want to return XR_BAD_INO_SIZE_DATA for a bad logblocks count,
which will yield

"bad inode size or inconsistent with number of inodes/block")

This might even want a new XR_BAD_LOG_FOO_BAR error...?

-Eric

>  
>  	if (xfs_sb_version_hassector(sb))  {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 03/11] xfs_repair: validate some of the log space information

[Darrick J. Wong]

On Fri, May 04, 2018 at 02:29:55PM -0500, Eric Sandeen wrote:
> On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Validate the log space information in a similar manner to the kernel so
> > that repair will stumble over (and fix) broken log info that prevents
> > mounting.  Fixes logsunit fuzz-and-fix failures in xfs/350.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/sb.c |   24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/repair/sb.c b/repair/sb.c
> > index 3dc6538..f2968cd 100644
> > --- a/repair/sb.c
> > +++ b/repair/sb.c
> > @@ -299,6 +299,27 @@ sb_validate_ino_align(struct xfs_sb *sb)
> >  }
> >  
> >  /*
> > + * Validate the given log space.  Derived from xfs_log_mount, though we
> > + * can't validate the minimum log size until later.
> > + * Returns false if the log is garbage.
> > + */
> > +static bool
> > +verify_sb_loginfo(
> > +	struct xfs_sb	*sb)
> > +{
> > +	if (xfs_sb_version_hascrc(sb) &&
> 
> Hm, this could use a comment about why these things only matter on v5 supers.

/*
 * We only do this validation on v5 filesystems because the kernel
 * didn't reject too-small logs in the past.
 */

?

> 
> > +	    (sb->sb_logblocks == 0 ||
> > +	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
> > +	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
> > +		return false;
> > +
> > +	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/*
> >   * verify a superblock -- does not verify root inode #
> >   *	can only check that geometry info is internally
> >   *	consistent.  because of growfs, that's no guarantee
> > @@ -409,7 +430,8 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
> >  	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
> >  	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
> >  	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
> > -	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
> > +	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) ||
> > +	    !verify_sb_loginfo(sb))
> >  		return XR_BAD_INO_SIZE_DATA;
> 
> do you really want to return XR_BAD_INO_SIZE_DATA for a bad logblocks count,
> which will yield
> 
> "bad inode size or inconsistent with number of inodes/block")
> 
> This might even want a new XR_BAD_LOG_FOO_BAR error...?

Sure.

"bad log geometry information in superblock" ?

--D

> 
> -Eric
> 
> >  
> >  	if (xfs_sb_version_hassector(sb))  {
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/11] xfs_repair: validate some of the log space information

[Eric Sandeen]

On 5/4/18 3:25 PM, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 02:29:55PM -0500, Eric Sandeen wrote:
>> On 4/17/18 9:46 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Validate the log space information in a similar manner to the kernel so
>>> that repair will stumble over (and fix) broken log info that prevents
>>> mounting.  Fixes logsunit fuzz-and-fix failures in xfs/350.

...

>> Hm, this could use a comment about why these things only matter on v5 supers.
> 
> /*
>  * We only do this validation on v5 filesystems because the kernel
>  * didn't reject too-small logs in the past.
>  */
> 
> ?


"... doesn't reject malformed logs on older revision filesystems"

also, it'll probably happen anyway, but the new check-stuff function should
probably be called on its own and not tacked on via || to the inode checks.

-Eric

[PATCH v2 03/11] xfs_repair: validate some of the log space information

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Validate the log space information in a similar manner to the kernel so
that repair will stumble over (and fix) broken log info that prevents
mounting.  Fixes logsunit fuzz-and-fix failures in xfs/350.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/globals.h    |    3 ++-
 repair/sb.c         |   27 +++++++++++++++++++++++++++
 repair/xfs_repair.c |    2 ++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/repair/globals.h b/repair/globals.h
index e777ba27..164619b0 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -51,7 +51,8 @@
 #define XR_BAD_SVN		19	/* bad shared version number */
 #define XR_BAD_CRC		20	/* Bad CRC */
 #define XR_BAD_DIR_SIZE_DATA	21	/* Bad directory geometry */
-#define XR_BAD_ERR_CODE		22	/* Bad error code */
+#define XR_BAD_LOG_GEOMETRY	22	/* Bad log geometry */
+#define XR_BAD_ERR_CODE		23	/* Bad error code */
 
 /* XFS filesystem (il)legal values */
 
diff --git a/repair/sb.c b/repair/sb.c
index 3dc6538b..ef44e39c 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -298,6 +298,30 @@ sb_validate_ino_align(struct xfs_sb *sb)
 	return false;
 }
 
+/*
+ * Validate the given log space.  Derived from xfs_log_mount, though we
+ * can't validate the minimum log size until later.  We only do this
+ * validation on V5 filesystems because the kernel doesn't reject malformed
+ * log geometry on older revision filesystems.
+ *
+ * Returns false if the log is garbage.
+ */
+static bool
+verify_sb_loginfo(
+	struct xfs_sb	*sb)
+{
+	if (xfs_sb_version_hascrc(sb) &&
+	    (sb->sb_logblocks == 0 ||
+	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
+	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
+		return false;
+
+	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
+		return false;
+
+	return true;
+}
+
 /*
  * verify a superblock -- does not verify root inode #
  *	can only check that geometry info is internally
@@ -412,6 +436,9 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
 		return XR_BAD_INO_SIZE_DATA;
 
+	if (!verify_sb_loginfo(sb))
+		return XR_BAD_LOG_GEOMETRY;
+
 	if (xfs_sb_version_hassector(sb))  {
 
 		/* check to make sure log sector is legal 2^N, 9 <= N <= 15 */
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index ff6a7384..b7c2d267 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -155,6 +155,8 @@ err_string(int err_code)
 			_("bad CRC in superblock");
 		err_message[XR_BAD_DIR_SIZE_DATA] =
 			_("inconsistent directory geometry information");
+		err_message[XR_BAD_LOG_GEOMETRY] =
+			_("inconsistent log geometry information");
 		done = 1;
 	}
 

Re: [PATCH v2 03/11] xfs_repair: validate some of the log space information

[Allison Henderson]

Ok, looks good.

Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/22/2018 08:15 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Validate the log space information in a similar manner to the kernel so
> that repair will stumble over (and fix) broken log info that prevents
> mounting.  Fixes logsunit fuzz-and-fix failures in xfs/350.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   repair/globals.h    |    3 ++-
>   repair/sb.c         |   27 +++++++++++++++++++++++++++
>   repair/xfs_repair.c |    2 ++
>   3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/repair/globals.h b/repair/globals.h
> index e777ba27..164619b0 100644
> --- a/repair/globals.h
> +++ b/repair/globals.h
> @@ -51,7 +51,8 @@
>   #define XR_BAD_SVN		19	/* bad shared version number */
>   #define XR_BAD_CRC		20	/* Bad CRC */
>   #define XR_BAD_DIR_SIZE_DATA	21	/* Bad directory geometry */
> -#define XR_BAD_ERR_CODE		22	/* Bad error code */
> +#define XR_BAD_LOG_GEOMETRY	22	/* Bad log geometry */
> +#define XR_BAD_ERR_CODE		23	/* Bad error code */
>   
>   /* XFS filesystem (il)legal values */
>   
> diff --git a/repair/sb.c b/repair/sb.c
> index 3dc6538b..ef44e39c 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -298,6 +298,30 @@ sb_validate_ino_align(struct xfs_sb *sb)
>   	return false;
>   }
>   
> +/*
> + * Validate the given log space.  Derived from xfs_log_mount, though we
> + * can't validate the minimum log size until later.  We only do this
> + * validation on V5 filesystems because the kernel doesn't reject malformed
> + * log geometry on older revision filesystems.
> + *
> + * Returns false if the log is garbage.
> + */
> +static bool
> +verify_sb_loginfo(
> +	struct xfs_sb	*sb)
> +{
> +	if (xfs_sb_version_hascrc(sb) &&
> +	    (sb->sb_logblocks == 0 ||
> +	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
> +	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
> +		return false;
> +
> +	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
> +		return false;
> +
> +	return true;
> +}
> +
>   /*
>    * verify a superblock -- does not verify root inode #
>    *	can only check that geometry info is internally
> @@ -412,6 +436,9 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
>   	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
>   		return XR_BAD_INO_SIZE_DATA;
>   
> +	if (!verify_sb_loginfo(sb))
> +		return XR_BAD_LOG_GEOMETRY;
> +
>   	if (xfs_sb_version_hassector(sb))  {
>   
>   		/* check to make sure log sector is legal 2^N, 9 <= N <= 15 */
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index ff6a7384..b7c2d267 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -155,6 +155,8 @@ err_string(int err_code)
>   			_("bad CRC in superblock");
>   		err_message[XR_BAD_DIR_SIZE_DATA] =
>   			_("inconsistent directory geometry information");
> +		err_message[XR_BAD_LOG_GEOMETRY] =
> +			_("inconsistent log geometry information");
>   		done = 1;
>   	}
>   
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=nZ0K_Km7ERFhI8CrunSV1T8eZYlZ-DRLdVcNPos7ubQ&s=ZkAtuu-cih4_uXaSekCCbLGpd5WW-a-JeDmxWBBTVwI&e=
> 

[PATCH 04/11] xfs_repair: zap corrupt remote symlink

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If a remote symlink has a corrupted remote block, just zap the symlink.
Fixes total lack of repair activity in xfs/382.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dinode.c |    7 +++++++
 1 file changed, 7 insertions(+)


diff --git a/repair/dinode.c b/repair/dinode.c
index 9af4f05..ceffc52 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -1330,6 +1330,13 @@ _("cannot read inode %" PRIu64 ", file block %d, disk block %" PRIu64 "\n"),
 				lino, i, fsbno);
 			return 1;
 		}
+		if (bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("Corrupt symlink remote, block %" PRIu64 ", inode %" PRIu64 ".\n"),
+				fsbno, lino);
+			libxfs_putbuf(bp);
+			return 1;
+		}
 		if (bp->b_error == -EFSBADCRC) {
 			do_warn(
 _("Bad symlink buffer CRC, block %" PRIu64 ", inode %" PRIu64 ".\n"

Re: [PATCH 04/11] xfs_repair: zap corrupt remote symlink

[Eric Sandeen]

On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a remote symlink has a corrupted remote block, just zap the symlink.
> Fixes total lack of repair activity in xfs/382.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Huh.  Yup! Weird omission...

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

Though I don't think "symlink remote" means much to an admin, *shrug*
I suppose it helps us triage.

> ---
>  repair/dinode.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 9af4f05..ceffc52 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -1330,6 +1330,13 @@ _("cannot read inode %" PRIu64 ", file block %d, disk block %" PRIu64 "\n"),
>  				lino, i, fsbno);
>  			return 1;
>  		}
> +		if (bp->b_error == -EFSCORRUPTED) {
> +			do_warn(
> +_("Corrupt symlink remote, block %" PRIu64 ", inode %" PRIu64 ".\n"),
> +				fsbno, lino);
> +			libxfs_putbuf(bp);
> +			return 1;
> +		}
>  		if (bp->b_error == -EFSBADCRC) {
>  			do_warn(
>  _("Bad symlink buffer CRC, block %" PRIu64 ", inode %" PRIu64 ".\n"
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 04/11] xfs_repair: zap corrupt remote symlink

[Darrick J. Wong]

On Fri, May 04, 2018 at 02:46:35PM -0500, Eric Sandeen wrote:
> On 4/17/18 9:46 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If a remote symlink has a corrupted remote block, just zap the symlink.
> > Fixes total lack of repair activity in xfs/382.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Huh.  Yup! Weird omission...
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> Though I don't think "symlink remote" means much to an admin, *shrug*
> I suppose it helps us triage.
> 
> > ---
> >  repair/dinode.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > 
> > diff --git a/repair/dinode.c b/repair/dinode.c
> > index 9af4f05..ceffc52 100644
> > --- a/repair/dinode.c
> > +++ b/repair/dinode.c
> > @@ -1330,6 +1330,13 @@ _("cannot read inode %" PRIu64 ", file block %d, disk block %" PRIu64 "\n"),
> >  				lino, i, fsbno);
> >  			return 1;
> >  		}
> > +		if (bp->b_error == -EFSCORRUPTED) {
> > +			do_warn(
> > +_("Corrupt symlink remote, block %" PRIu64 ", inode %" PRIu64 ".\n"),

Remove comma, pretty please? ^^^

--D

> > +				fsbno, lino);
> > +			libxfs_putbuf(bp);
> > +			return 1;
> > +		}
> >  		if (bp->b_error == -EFSBADCRC) {
> >  			do_warn(
> >  _("Bad symlink buffer CRC, block %" PRIu64 ", inode %" PRIu64 ".\n"
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/11] xfs_repair: treat zero da btree pointers as corruption

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If a da btree pointer is zero (i.e. the beginning of the fork) report
this as a corrupt tree to the caller instead of telling it that
everything is good.  Fixes assertion errors when fuzzing
nbtree[0].before to zero in xfs/394.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dir2.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/repair/dir2.c b/repair/dir2.c
index 73dff90..47ece00 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -1238,8 +1238,8 @@ process_node_dir2(
 	 * Skip directories with a root marked XFS_DIR2_LEAFN_MAGIC
 	 */
 	if (bno == 0) {
-		release_da_cursor(mp, &da_cursor, 0);
-		return 0;
+		err_release_da_cursor(mp, &da_cursor, 0);
+		return 1;
 	} else {
 		/*
 		 * Now pass cursor and bno into leaf-block processing routine.

Re: [PATCH 05/11] xfs_repair: treat zero da btree pointers as corruption

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a da btree pointer is zero (i.e. the beginning of the fork) report
> this as a corrupt tree to the caller instead of telling it that
> everything is good.  Fixes assertion errors when fuzzing
> nbtree[0].before to zero in xfs/394.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/dir2.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 73dff90..47ece00 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -1238,8 +1238,8 @@ process_node_dir2(
>  	 * Skip directories with a root marked XFS_DIR2_LEAFN_MAGIC
>  	 */
>  	if (bno == 0) {
> -		release_da_cursor(mp, &da_cursor, 0);
> -		return 0;
> +		err_release_da_cursor(mp, &da_cursor, 0);
> +		return 1;

Trying to make sense of this w.r.t. the comment right above it ... oh,
ok the traverse function does:

                if (whichfork == XFS_DATA_FORK &&
                    (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
                    nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)) {
                        if (i != -1) {
                                do_warn(
_("found non-root LEAFN node in inode %" PRIu64 " bno = %u\n"),
                                        da_cursor->ino, bno);
 
I guess the comment should say:

/* Directories with root marked XFS_DIR2_LEAFN_MAGIC are corrupt */

now, but I might fix that on the way in, so:

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

>  	} else {
>  		/*
>  		 * Now pass cursor and bno into leaf-block processing routine.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 06/11] xfs_repair: invalidate dirty dir buffers when we zap a directory

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we decide to rebuild a directory in phase 6, we need to find and
invalidate all of the old directory buffers so that they don't get
written out, which can trigger write verifier errors when we finish.
This fixes the write verifier errors in phase 7 that can occur via
xfs/382.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/phase6.c          |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 09f1428..3b36e0e 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -100,6 +100,7 @@
 #define xfs_dir2_data_make_free		libxfs_dir2_data_make_free
 #define xfs_dir2_data_use_free		libxfs_dir2_data_use_free
 #define xfs_dir2_shrink_inode		libxfs_dir2_shrink_inode
+#define xfs_da_get_buf			libxfs_da_get_buf
 
 #define xfs_inode_from_disk		libxfs_inode_from_disk
 #define xfs_inode_to_disk		libxfs_inode_to_disk
diff --git a/repair/phase6.c b/repair/phase6.c
index 498a3b5..2005e40 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1311,6 +1311,48 @@ entry_junked(
 	return !no_modify;
 }
 
+/* Find and invalidate all the directory's buffers. */
+static int
+dir_binval(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*ip,
+	int			whichfork)
+{
+	struct xfs_iext_cursor	icur;
+	struct xfs_bmbt_irec	rec;
+	struct xfs_ifork	*ifp;
+	struct xfs_da_geometry	*geo;
+	struct xfs_buf		*bp;
+	xfs_dablk_t		dabno, end_dabno;
+	int			error = 0;
+
+	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
+		return 0;
+
+	geo = tp->t_mountp->m_dir_geo;
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	for_each_xfs_iext(ifp, &icur, &rec) {
+		dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
+				geo->fsbcount - 1);
+		end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
+				rec.br_blockcount);
+		for (; dabno <= end_dabno; dabno += geo->fsbcount) {
+			bp = NULL;
+			error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
+					whichfork);
+			if (error)
+				return error;
+			if (!bp)
+				continue;
+			libxfs_trans_binval(tp, bp);
+			libxfs_trans_brelse(tp, bp);
+		}
+	}
+
+	return error;
+}
+
 /*
  * Unexpected failure during the rebuild will leave the entries in
  * lost+found on the next run
@@ -1361,6 +1403,10 @@ longform_dir2_rebuild(
 		res_failed(error);
 	libxfs_trans_ijoin(tp, ip, 0);
 
+	error = dir_binval(tp, ip, XFS_DATA_FORK);
+	if (error)
+		res_failed(error);
+
 	if ((error = -libxfs_bmap_last_offset(ip, &lastblock, XFS_DATA_FORK)))
 		do_error(_("xfs_bmap_last_offset failed -- error - %d\n"),
 			error);

Re: [PATCH 06/11] xfs_repair: invalidate dirty dir buffers when we zap a directory

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we decide to rebuild a directory in phase 6, we need to find and
> invalidate all of the old directory buffers so that they don't get
> written out, which can trigger write verifier errors when we finish.
> This fixes the write verifier errors in phase 7 that can occur via
> xfs/382.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

seems reasonable

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

> ---
>  libxfs/libxfs_api_defs.h |    1 +
>  repair/phase6.c          |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 09f1428..3b36e0e 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -100,6 +100,7 @@
>  #define xfs_dir2_data_make_free		libxfs_dir2_data_make_free
>  #define xfs_dir2_data_use_free		libxfs_dir2_data_use_free
>  #define xfs_dir2_shrink_inode		libxfs_dir2_shrink_inode
> +#define xfs_da_get_buf			libxfs_da_get_buf
>  
>  #define xfs_inode_from_disk		libxfs_inode_from_disk
>  #define xfs_inode_to_disk		libxfs_inode_to_disk
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 498a3b5..2005e40 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1311,6 +1311,48 @@ entry_junked(
>  	return !no_modify;
>  }
>  
> +/* Find and invalidate all the directory's buffers. */
> +static int
> +dir_binval(
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*ip,
> +	int			whichfork)
> +{
> +	struct xfs_iext_cursor	icur;
> +	struct xfs_bmbt_irec	rec;
> +	struct xfs_ifork	*ifp;
> +	struct xfs_da_geometry	*geo;
> +	struct xfs_buf		*bp;
> +	xfs_dablk_t		dabno, end_dabno;
> +	int			error = 0;
> +
> +	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
> +	    ip->i_d.di_format != XFS_DINODE_FMT_BTREE)
> +		return 0;
> +
> +	geo = tp->t_mountp->m_dir_geo;
> +	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	for_each_xfs_iext(ifp, &icur, &rec) {
> +		dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> +				geo->fsbcount - 1);
> +		end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
> +				rec.br_blockcount);
> +		for (; dabno <= end_dabno; dabno += geo->fsbcount) {
> +			bp = NULL;
> +			error = -libxfs_da_get_buf(tp, ip, dabno, -2, &bp,
> +					whichfork);
> +			if (error)
> +				return error;
> +			if (!bp)
> +				continue;
> +			libxfs_trans_binval(tp, bp);
> +			libxfs_trans_brelse(tp, bp);
> +		}
> +	}
> +
> +	return error;
> +}
> +
>  /*
>   * Unexpected failure during the rebuild will leave the entries in
>   * lost+found on the next run
> @@ -1361,6 +1403,10 @@ longform_dir2_rebuild(
>  		res_failed(error);
>  	libxfs_trans_ijoin(tp, ip, 0);
>  
> +	error = dir_binval(tp, ip, XFS_DATA_FORK);
> +	if (error)
> +		res_failed(error);
> +
>  	if ((error = -libxfs_bmap_last_offset(ip, &lastblock, XFS_DATA_FORK)))
>  		do_error(_("xfs_bmap_last_offset failed -- error - %d\n"),
>  			error);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 07/11] xfs_repair: only update in-core extent state after scanning full extent

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In process_bmbt_reclist_int, only update the in-core extent state after
clearing the entire extent for conflicts.  If we encounter conflicts
we'll try rebuilding the fork from rmap data and rescanning the fork.
It is essential to avoid polluting the in-memory state with garbage
data so that we don't end up nuking other files needlessly.  Found by
fuzzing recs[1].blockcount = middlebit in xfs/380.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dinode.c |   30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)


diff --git a/repair/dinode.c b/repair/dinode.c
index ceffc52..c8c1850 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -751,7 +751,6 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
 				/* fall through ... */
 			case XR_E_INUSE1:	/* seen by rmap */
 			case XR_E_UNKNOWN:
-				set_bmap_ext(agno, agbno, blen, XR_E_INUSE);
 				break;
 
 			case XR_E_BAD_STATE:
@@ -773,7 +772,6 @@ _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
 
 			case XR_E_INUSE:
 			case XR_E_MULT:
-				set_bmap_ext(agno, agbno, blen, XR_E_MULT);
 				if (type == XR_INO_DATA &&
 				    xfs_sb_version_hasreflink(&mp->m_sb))
 					break;
@@ -792,6 +790,34 @@ _("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
 				do_error(
 _("illegal state %d in block map %" PRIu64 "\n"),
 					state, b);
+				goto done;
+			}
+		}
+
+		/*
+		 * Update the internal extent map only after we've checked
+		 * every block in this extent.  The first time we reject this
+		 * data fork we'll try to rebuild the bmbt from rmap data.
+		 * After a successful rebuild we'll try this scan again.
+		 * (If the rebuild fails we won't come back here.)
+		 */
+		agbno = XFS_FSB_TO_AGBNO(mp, irec.br_startblock);
+		ebno = agbno + irec.br_blockcount;
+		for (; agbno < ebno; agbno += blen) {
+			state = get_bmap_ext(agno, agbno, ebno, &blen);
+			switch (state)  {
+			case XR_E_FREE:
+			case XR_E_FREE1:
+			case XR_E_INUSE1:
+			case XR_E_UNKNOWN:
+				set_bmap_ext(agno, agbno, blen, XR_E_INUSE);
+				break;
+			case XR_E_INUSE:
+			case XR_E_MULT:
+				set_bmap_ext(agno, agbno, blen, XR_E_MULT);
+				break;
+			default:
+				break;
 			}
 		}
 		if (collect_rmaps) { /* && !check_dups */

Re: [PATCH 07/11] xfs_repair: only update in-core extent state after scanning full extent

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In process_bmbt_reclist_int, only update the in-core extent state after
> clearing the entire extent for conflicts.  If we encounter conflicts
> we'll try rebuilding the fork from rmap data and rescanning the fork.
> It is essential to avoid polluting the in-memory state with garbage
> data so that we don't end up nuking other files needlessly.  Found by
> fuzzing recs[1].blockcount = middlebit in xfs/380.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

AFAICT this makes sense. :)

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

> ---
>  repair/dinode.c |   30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index ceffc52..c8c1850 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -751,7 +751,6 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
>  				/* fall through ... */
>  			case XR_E_INUSE1:	/* seen by rmap */
>  			case XR_E_UNKNOWN:
> -				set_bmap_ext(agno, agbno, blen, XR_E_INUSE);
>  				break;
>  
>  			case XR_E_BAD_STATE:
> @@ -773,7 +772,6 @@ _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
>  
>  			case XR_E_INUSE:
>  			case XR_E_MULT:
> -				set_bmap_ext(agno, agbno, blen, XR_E_MULT);
>  				if (type == XR_INO_DATA &&
>  				    xfs_sb_version_hasreflink(&mp->m_sb))
>  					break;
> @@ -792,6 +790,34 @@ _("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
>  				do_error(
>  _("illegal state %d in block map %" PRIu64 "\n"),
>  					state, b);
> +				goto done;
> +			}
> +		}
> +
> +		/*
> +		 * Update the internal extent map only after we've checked
> +		 * every block in this extent.  The first time we reject this
> +		 * data fork we'll try to rebuild the bmbt from rmap data.
> +		 * After a successful rebuild we'll try this scan again.
> +		 * (If the rebuild fails we won't come back here.)
> +		 */
> +		agbno = XFS_FSB_TO_AGBNO(mp, irec.br_startblock);
> +		ebno = agbno + irec.br_blockcount;
> +		for (; agbno < ebno; agbno += blen) {
> +			state = get_bmap_ext(agno, agbno, ebno, &blen);
> +			switch (state)  {
> +			case XR_E_FREE:
> +			case XR_E_FREE1:
> +			case XR_E_INUSE1:
> +			case XR_E_UNKNOWN:
> +				set_bmap_ext(agno, agbno, blen, XR_E_INUSE);
> +				break;
> +			case XR_E_INUSE:
> +			case XR_E_MULT:
> +				set_bmap_ext(agno, agbno, blen, XR_E_MULT);
> +				break;
> +			default:
> +				break;
>  			}
>  		}
>  		if (collect_rmaps) { /* && !check_dups */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 08/11] xfs_repair: don't crash if da btree is corrupt

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In the recursive verify_da_path call chain, we decide to examine the
next upper level if the current entry points past the end of the
entries.  However, we don't check for a node with zero entries (which
should be impossible) so we run right off the end of the da cursor's
level array and crash.  Found by fuzzing hdr.count in xfs/402.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/da_util.c |    8 ++++++++
 1 file changed, 8 insertions(+)


diff --git a/repair/da_util.c b/repair/da_util.c
index a65652f..bca4060 100644
--- a/repair/da_util.c
+++ b/repair/da_util.c
@@ -526,6 +526,10 @@ verify_da_path(
 	else
 		geo = mp->m_attr_geo;
 
+	/* No buffer at this level, tree is corrupt. */
+	if (cursor->level[this_level].bp == NULL)
+		return 1;
+
 	/*
 	 * index is currently set to point to the entry that
 	 * should be processed now in this level.
@@ -535,6 +539,10 @@ verify_da_path(
 	btree = M_DIROPS(mp)->node_tree_p(node);
 	M_DIROPS(mp)->node_hdr_from_disk(&nodehdr, node);
 
+	/* No entries in this node?  Tree is corrupt. */
+	if (nodehdr.count == 0)
+		return 1;
+
 	/*
 	 * if this block is out of entries, validate this
 	 * block and move on to the next block.

Re: [PATCH 08/11] xfs_repair: don't crash if da btree is corrupt

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In the recursive verify_da_path call chain, we decide to examine the
> next upper level if the current entry points past the end of the
> entries.  However, we don't check for a node with zero entries (which
> should be impossible) so we run right off the end of the da cursor's
> level array and crash.  Found by fuzzing hdr.count in xfs/402.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

> ---
>  repair/da_util.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> 
> diff --git a/repair/da_util.c b/repair/da_util.c
> index a65652f..bca4060 100644
> --- a/repair/da_util.c
> +++ b/repair/da_util.c
> @@ -526,6 +526,10 @@ verify_da_path(
>  	else
>  		geo = mp->m_attr_geo;
>  
> +	/* No buffer at this level, tree is corrupt. */
> +	if (cursor->level[this_level].bp == NULL)
> +		return 1;
> +
>  	/*
>  	 * index is currently set to point to the entry that
>  	 * should be processed now in this level.
> @@ -535,6 +539,10 @@ verify_da_path(
>  	btree = M_DIROPS(mp)->node_tree_p(node);
>  	M_DIROPS(mp)->node_hdr_from_disk(&nodehdr, node);
>  
> +	/* No entries in this node?  Tree is corrupt. */
> +	if (nodehdr.count == 0)
> +		return 1;
> +
>  	/*
>  	 * if this block is out of entries, validate this
>  	 * block and move on to the next block.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 09/11] xfs_repair: don't assert if we run across a dir entry with null ino ptr

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we encounter a directory with an entry that points to inode zero,
we'll crash due to an ASSERT during process_inode_chunk.  Instead, just
set the in-core parent to NULLFSINO so that phase 6 will reset it for
us.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dino_chunks.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 17de95f..2d34079 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -874,7 +874,8 @@ process_inode_chunk(
 			 * be solid then.
 			 */
 			if (!ino_discovery)  {
-				ASSERT(parent != 0);
+				if (parent == 0)
+					parent = NULLFSINO;
 				set_inode_parent(ino_rec, irec_offset, parent);
 				ASSERT(parent ==
 					get_inode_parent(ino_rec, irec_offset));

Re: [PATCH 09/11] xfs_repair: don't assert if we run across a dir entry with null ino ptr

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we encounter a directory with an entry that points to inode zero,
> we'll crash due to an ASSERT during process_inode_chunk.  Instead, just
> set the in-core parent to NULLFSINO so that phase 6 will reset it for
> us.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

So .... this is .... probably ok?  but the ASSERT makes me think that
process_dinode is never supposed to return a 0 parent if isa_dir is set,
and so I'm a little worried about breaking that assumption up and just
catching it later, here.  Usually the asserts mean "the code should never
let this happen and if it does, some prior code is wrong"

Looking at the calls below it, it seems like we generally call verify_inum
which would set NULLFSINO if the inum is bad.

But in process_dir2_data

[17:25]  <sandeen>                 } else if (verify_inum(mp, ent_ino)) {
[17:25]  <sandeen>                         clearreason = _("invalid");
[17:26]  <sandeen> so we probably saw that it's bad, but:
[17:26]  <sandeen>                  * We have a special dot & dotdot fixer-upper below which can
[17:26]  <sandeen>                  * sort out the proper inode number, so don't clear it.
[17:26]  <sandeen>                         clearreason = NULL;
[17:26]  <sandeen> and then:
[17:26]  <sandeen>                  * Special .. entry processing.
[17:26]  <sandeen>                                 *parent = ent_ino;
[17:26]  <sandeen> \o/

so ... I wonder if it wouldn't be more consistent to find the place under
process_inode() where we willingly/wrongly set *parent to an invalid value,
and handle the problem there?  Do you know what path you were on to hit this?

-Eric
> ---
>  repair/dino_chunks.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index 17de95f..2d34079 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -874,7 +874,8 @@ process_inode_chunk(
>  			 * be solid then.
>  			 */
>  			if (!ino_discovery)  {
> -				ASSERT(parent != 0);
> +				if (parent == 0)
> +					parent = NULLFSINO;
>  				set_inode_parent(ino_rec, irec_offset, parent);
>  				ASSERT(parent ==
>  					get_inode_parent(ino_rec, irec_offset));
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 09/11] xfs_repair: don't assert if we run across a dir entry with null ino ptr

[Darrick J. Wong]

On Fri, May 04, 2018 at 05:33:35PM -0500, Eric Sandeen wrote:
> 
> 
> On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If we encounter a directory with an entry that points to inode zero,
> > we'll crash due to an ASSERT during process_inode_chunk.  Instead, just
> > set the in-core parent to NULLFSINO so that phase 6 will reset it for
> > us.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> So .... this is .... probably ok?  but the ASSERT makes me think that
> process_dinode is never supposed to return a 0 parent if isa_dir is set,
> and so I'm a little worried about breaking that assumption up and just
> catching it later, here.  Usually the asserts mean "the code should never
> let this happen and if it does, some prior code is wrong"
> 
> Looking at the calls below it, it seems like we generally call verify_inum
> which would set NULLFSINO if the inum is bad.
> 
> But in process_dir2_data
> 
> [17:25]  <sandeen>                 } else if (verify_inum(mp, ent_ino)) {
> [17:25]  <sandeen>                         clearreason = _("invalid");
> [17:26]  <sandeen> so we probably saw that it's bad, but:
> [17:26]  <sandeen>                  * We have a special dot & dotdot fixer-upper below which can
> [17:26]  <sandeen>                  * sort out the proper inode number, so don't clear it.
> [17:26]  <sandeen>                         clearreason = NULL;
> [17:26]  <sandeen> and then:
> [17:26]  <sandeen>                  * Special .. entry processing.
> [17:26]  <sandeen>                                 *parent = ent_ino;
> [17:26]  <sandeen> \o/
> 
> so ... I wonder if it wouldn't be more consistent to find the place under
> process_inode() where we willingly/wrongly set *parent to an invalid value,
> and handle the problem there?  Do you know what path you were on to hit this?

/me doesn't remember, it was either the sf dir scrub or the dir block
offline repair fuzztest setting the parent pointer to zero.

--D

> -Eric
> > ---
> >  repair/dino_chunks.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> > index 17de95f..2d34079 100644
> > --- a/repair/dino_chunks.c
> > +++ b/repair/dino_chunks.c
> > @@ -874,7 +874,8 @@ process_inode_chunk(
> >  			 * be solid then.
> >  			 */
> >  			if (!ino_discovery)  {
> > -				ASSERT(parent != 0);
> > +				if (parent == 0)
> > +					parent = NULLFSINO;
> >  				set_inode_parent(ino_rec, irec_offset, parent);
> >  				ASSERT(parent ==
> >  					get_inode_parent(ino_rec, irec_offset));
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] xfs_repair: don't assert if we run across a dir entry with null ino ptr

[Darrick J. Wong]

On Fri, May 04, 2018 at 03:45:18PM -0700, Darrick J. Wong wrote:
> On Fri, May 04, 2018 at 05:33:35PM -0500, Eric Sandeen wrote:
> > 
> > 
> > On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > If we encounter a directory with an entry that points to inode zero,
> > > we'll crash due to an ASSERT during process_inode_chunk.  Instead, just
> > > set the in-core parent to NULLFSINO so that phase 6 will reset it for
> > > us.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > So .... this is .... probably ok?  but the ASSERT makes me think that
> > process_dinode is never supposed to return a 0 parent if isa_dir is set,
> > and so I'm a little worried about breaking that assumption up and just
> > catching it later, here.  Usually the asserts mean "the code should never
> > let this happen and if it does, some prior code is wrong"
> > 
> > Looking at the calls below it, it seems like we generally call verify_inum
> > which would set NULLFSINO if the inum is bad.
> > 
> > But in process_dir2_data
> > 
> > [17:25]  <sandeen>                 } else if (verify_inum(mp, ent_ino)) {
> > [17:25]  <sandeen>                         clearreason = _("invalid");
> > [17:26]  <sandeen> so we probably saw that it's bad, but:
> > [17:26]  <sandeen>                  * We have a special dot & dotdot fixer-upper below which can
> > [17:26]  <sandeen>                  * sort out the proper inode number, so don't clear it.
> > [17:26]  <sandeen>                         clearreason = NULL;
> > [17:26]  <sandeen> and then:
> > [17:26]  <sandeen>                  * Special .. entry processing.
> > [17:26]  <sandeen>                                 *parent = ent_ino;
> > [17:26]  <sandeen> \o/
> > 
> > so ... I wonder if it wouldn't be more consistent to find the place under
> > process_inode() where we willingly/wrongly set *parent to an invalid value,
> > and handle the problem there?  Do you know what path you were on to hit this?
> 
> /me doesn't remember, it was either the sf dir scrub or the dir block
> offline repair fuzztest setting the parent pointer to zero.

Aha, it's xfs/386 bu[1].inumber = 0.  I've found the part of
process_dir2_data that inexplicably doesn't check for null parent
pointers & blows up; will rework this patch.

--D

>o
> --D
> 
> > -Eric
> > > ---
> > >  repair/dino_chunks.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> > > index 17de95f..2d34079 100644
> > > --- a/repair/dino_chunks.c
> > > +++ b/repair/dino_chunks.c
> > > @@ -874,7 +874,8 @@ process_inode_chunk(
> > >  			 * be solid then.
> > >  			 */
> > >  			if (!ino_discovery)  {
> > > -				ASSERT(parent != 0);
> > > +				if (parent == 0)
> > > +					parent = NULLFSINO;
> > >  				set_inode_parent(ino_rec, irec_offset, parent);
> > >  				ASSERT(parent ==
> > >  					get_inode_parent(ino_rec, irec_offset));
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 09/11] xfs_repair: actually fix .. entries that point to inode zero

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we encounter a directory with an entry that points to inode zero,
we'll crash due to an ASSERT during process_inode_chunk.  This is due to
process_dir2_data not arranging for phase 6 to fix the parent pointer
when '..' -> 0, so do that.  Found via xfs/386 fuzzing bu[1].inumber to
zero.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dir2.c            |   17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index d588f342..56f9f8ca 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -78,6 +78,7 @@
 #define xfs_bmbt_get_all		libxfs_bmbt_get_all
 #define xfs_rtfree_extent		libxfs_rtfree_extent
 #define xfs_verify_rtbno		libxfs_verify_rtbno
+#define xfs_verify_ino			libxfs_verify_ino
 #define xfs_zero_extent			libxfs_zero_extent
 
 #define xfs_defer_init			libxfs_defer_init
diff --git a/repair/dir2.c b/repair/dir2.c
index fbe88b50..f0371371 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -847,6 +847,23 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "),
 					}
 					*parent = ino;
 				}
+				/*
+				 * Make sure our parent pointer doesn't point
+				 * off into space.
+				 */
+				if (!junkit &&
+				    *parent != NULLFSINO &&
+				    !libxfs_verify_ino(mp, *parent)) {
+					do_warn(
+_("bad .. entry in directory inode %" PRIu64 ", was %" PRIu64 ": "),
+						ino, *parent);
+					if (!no_modify) {
+						do_warn(_("correcting\n"));
+					} else {
+						do_warn(_("would correct\n"));
+					}
+					*parent = NULLFSINO;
+				}
 			}
 			/*
 			 * Can't fix the directory unless we know which ..

[PATCH 10/11] xfs_repair: check inode nsec for obviously garbage values

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

It makes no sense to have an nsec value that is larger than 1 second,
so zero the field if this is the case.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dinode.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)


diff --git a/repair/dinode.c b/repair/dinode.c
index c8c1850..9792293 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2233,6 +2233,28 @@ _("would clear obsolete nlink field in version 2 inode %" PRIu64 ", currently %d
 	return dirty;
 }
 
+/* Check nanoseconds of a timestamp don't exceed 1 second. */
+static void
+check_nsec(
+	const char		*name,
+	xfs_ino_t		lino,
+	struct xfs_timestamp	*t,
+	int			*dirty)
+{
+	if (be32_to_cpu(t->t_nsec) < 1000000000)
+		return;
+
+	do_warn(
+_("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
+	if (no_modify) {
+		do_warn(_("would reset to zero\n"));
+	} else {
+		do_warn(_("resetting to zero\n"));
+		t->t_nsec = 0;
+		*dirty = 1;
+	}
+}
+
 /*
  * returns 0 if the inode is ok, 1 if the inode is corrupt
  * check_dups can be set to 1 *only* when called by the
@@ -2749,6 +2771,13 @@ _("Cannot have CoW extent size of zero on cowextsize inode %" PRIu64 ", "),
 		}
 	}
 
+	/* nsec fields cannot be larger than 1 billion */
+	check_nsec("atime", lino, &dino->di_atime, dirty);
+	check_nsec("mtime", lino, &dino->di_mtime, dirty);
+	check_nsec("ctime", lino, &dino->di_ctime, dirty);
+	if (dino->di_version >= 3)
+		check_nsec("crtime", lino, &dino->di_crtime, dirty);
+
 	/*
 	 * general size/consistency checks:
 	 */

Re: [PATCH 10/11] xfs_repair: check inode nsec for obviously garbage values

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> It makes no sense to have an nsec value that is larger than 1 second,
> so zero the field if this is the case.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

> ---
>  repair/dinode.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index c8c1850..9792293 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2233,6 +2233,28 @@ _("would clear obsolete nlink field in version 2 inode %" PRIu64 ", currently %d
>  	return dirty;
>  }
>  
> +/* Check nanoseconds of a timestamp don't exceed 1 second. */
> +static void
> +check_nsec(
> +	const char		*name,
> +	xfs_ino_t		lino,
> +	struct xfs_timestamp	*t,
> +	int			*dirty)
> +{
> +	if (be32_to_cpu(t->t_nsec) < 1000000000)
> +		return;
> +
> +	do_warn(
> +_("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
> +	if (no_modify) {
> +		do_warn(_("would reset to zero\n"));
> +	} else {
> +		do_warn(_("resetting to zero\n"));
> +		t->t_nsec = 0;
> +		*dirty = 1;
> +	}
> +}
> +
>  /*
>   * returns 0 if the inode is ok, 1 if the inode is corrupt
>   * check_dups can be set to 1 *only* when called by the
> @@ -2749,6 +2771,13 @@ _("Cannot have CoW extent size of zero on cowextsize inode %" PRIu64 ", "),
>  		}
>  	}
>  
> +	/* nsec fields cannot be larger than 1 billion */
> +	check_nsec("atime", lino, &dino->di_atime, dirty);
> +	check_nsec("mtime", lino, &dino->di_mtime, dirty);
> +	check_nsec("ctime", lino, &dino->di_ctime, dirty);
> +	if (dino->di_version >= 3)
> +		check_nsec("crtime", lino, &dino->di_crtime, dirty);
> +
>  	/*
>  	 * general size/consistency checks:
>  	 */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 11/11] xfs_repair: don't assert on bad '.' entry in no-modify mode

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During phase 3, we check the '.' entry of a directory inode and (in
modify mode) zap it if the name isn't valid.  During phase 6, we assert
that the '.' entry now reflects the correct name.  In no-modify mode
this is incorrect because we didn't actually fix anything, so repair
asserts and crashes.

Found by fuzzing bu[0].namelen = 4 in xfs/387 and running xfs_repair -n.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase6.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index 2005e40..e65fc4a 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1884,7 +1884,8 @@ longform_dir2_entry_check_data(
 		 * of when directory is moved to orphanage.
 		 */
 		if (ip->i_ino == inum)  {
-			ASSERT(dep->name[0] == '.' && dep->namelen == 1);
+			ASSERT(no_modify ||
+			       (dep->name[0] == '.' && dep->namelen == 1));
 			add_inode_ref(current_irec, current_ino_offset);
 			if (da_bno != 0 ||
 			    dep != M_DIROPS(mp)->data_entry_p(d)) {

Re: [PATCH 11/11] xfs_repair: don't assert on bad '.' entry in no-modify mode

[Eric Sandeen]

On 4/17/18 9:47 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> During phase 3, we check the '.' entry of a directory inode and (in
> modify mode) zap it if the name isn't valid.  During phase 6, we assert
> that the '.' entry now reflects the correct name.  In no-modify mode
> this is incorrect because we didn't actually fix anything, so repair
> asserts and crashes.
> 
> Found by fuzzing bu[0].namelen = 4 in xfs/387 and running xfs_repair -n.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

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

> ---
>  repair/phase6.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 2005e40..e65fc4a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1884,7 +1884,8 @@ longform_dir2_entry_check_data(
>  		 * of when directory is moved to orphanage.
>  		 */
>  		if (ip->i_ino == inum)  {
> -			ASSERT(dep->name[0] == '.' && dep->namelen == 1);
> +			ASSERT(no_modify ||
> +			       (dep->name[0] == '.' && dep->namelen == 1));
>  			add_inode_ref(current_irec, current_ino_offset);
>  			if (da_bno != 0 ||
>  			    dep != M_DIROPS(mp)->data_entry_p(d)) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>