diff mbox series

xfs: properly handle free inodes in extent hint validators

Message ID 4203c670-0064-8735-0931-573fdbeeaa67@redhat.com (mailing list archive)
State Accepted
Headers show
Series xfs: properly handle free inodes in extent hint validators | expand

Commit Message

Eric Sandeen July 24, 2018, 6 p.m. UTC 
When inodes are freed in xfs_ifree(), di_flags is cleared (so extent size
hints are removed) but the actual extent size fields are left intact.
This causes the extent hint validators to fail on freed inodes which once
had extent size hints. 

This can be observed (for example) by running xfs/229 twice on a
non-crc xfs filesystem, or presumably on V5 with ikeep.

Fixes: 7d71a67 ("xfs: verify extent size hint is valid in inode verifier")
Fixes: 02a0fda ("xfs: verify COW extent size hint is valid in inode verifier")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---



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

Comments

Darrick J. Wong July 24, 2018, 6:10 p.m. UTC | #1 
On Tue, Jul 24, 2018 at 11:00:39AM -0700, Eric Sandeen wrote:
> When inodes are freed in xfs_ifree(), di_flags is cleared (so extent size
> hints are removed) but the actual extent size fields are left intact.
> This causes the extent hint validators to fail on freed inodes which once
> had extent size hints. 
> 
> This can be observed (for example) by running xfs/229 twice on a
> non-crc xfs filesystem, or presumably on V5 with ikeep.

I couldn't get it to reproduce by running x/229 twice, but I did see
x/242 blow up on the same problem overnight.  Which is funny since it
hadn't blown up until now.

> Fixes: 7d71a67 ("xfs: verify extent size hint is valid in inode verifier")
> Fixes: 02a0fda ("xfs: verify COW extent size hint is valid in inode verifier")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Separate patch, but can you also modify xfs_ifree to zero the
extsize/cowextsize fields so that 4.16-4.17 kernels without this patch
are less likely to trip over this?

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 33dc34655ac3..30d1d60f1d46 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -731,7 +731,8 @@ xfs_inode_validate_extsize(
>  	if ((hint_flag || inherit_flag) && extsize == 0)
>  		return __this_address;
>  
> -	if (!(hint_flag || inherit_flag) && extsize != 0)
> +	/* free inodes get flags set to zero but extsize remains */
> +	if (mode && !(hint_flag || inherit_flag) && extsize != 0)
>  		return __this_address;
>  
>  	if (extsize_bytes % blocksize_bytes)
> @@ -777,7 +778,8 @@ xfs_inode_validate_cowextsize(
>  	if (hint_flag && cowextsize == 0)
>  		return __this_address;
>  
> -	if (!hint_flag && cowextsize != 0)
> +	/* free inodes get flags set to zero but cowextsize remains */
> +	if (mode && !hint_flag && cowextsize != 0)
>  		return __this_address;
>  
>  	if (hint_flag && rt_flag)
> 
> 
> --
> 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
Eric Sandeen July 24, 2018, 6:14 p.m. UTC | #2 
On 7/24/18 11:10 AM, Darrick J. Wong wrote:
> On Tue, Jul 24, 2018 at 11:00:39AM -0700, Eric Sandeen wrote:
>> When inodes are freed in xfs_ifree(), di_flags is cleared (so extent size
>> hints are removed) but the actual extent size fields are left intact.
>> This causes the extent hint validators to fail on freed inodes which once
>> had extent size hints. 
>>
>> This can be observed (for example) by running xfs/229 twice on a
>> non-crc xfs filesystem, or presumably on V5 with ikeep.
> 
> I couldn't get it to reproduce by running x/229 twice, but I did see
> x/242 blow up on the same problem overnight.  Which is funny since it
> hadn't blown up until now.

Huh.  Can you try it on a 16G fs?  Not sure what else might be unique
about my setup.

>> Fixes: 7d71a67 ("xfs: verify extent size hint is valid in inode verifier")
>> Fixes: 02a0fda ("xfs: verify COW extent size hint is valid in inode verifier")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Separate patch, but can you also modify xfs_ifree to zero the
> extsize/cowextsize fields so that 4.16-4.17 kernels without this patch
> are less likely to trip over this?

I'm confused - 7d71a67 & 02a0fda (above) went into 4.18, so 4.1[67]
shouldn't have the validator problem, right?

Are you talking about scrub here?

Sorry for waxing philosophical but my fear is that explicitly zeroing the
fields /now/ will muddy the waters w.r.t. what we should expect to see on
disk.  We had wrong verifiers, not wrong freeing routines - we need to
just fix the verifiers IMHO.  Scrub is experimental for a reason, right?

Thanks,
-Eric
--
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
Darrick J. Wong July 24, 2018, 6:31 p.m. UTC | #3 
On Tue, Jul 24, 2018 at 11:14:59AM -0700, Eric Sandeen wrote:
> On 7/24/18 11:10 AM, Darrick J. Wong wrote:
> > On Tue, Jul 24, 2018 at 11:00:39AM -0700, Eric Sandeen wrote:
> >> When inodes are freed in xfs_ifree(), di_flags is cleared (so extent size
> >> hints are removed) but the actual extent size fields are left intact.
> >> This causes the extent hint validators to fail on freed inodes which once
> >> had extent size hints. 
> >>
> >> This can be observed (for example) by running xfs/229 twice on a
> >> non-crc xfs filesystem, or presumably on V5 with ikeep.
> > 
> > I couldn't get it to reproduce by running x/229 twice, but I did see
> > x/242 blow up on the same problem overnight.  Which is funny since it
> > hadn't blown up until now.
> 
> Huh.  Can you try it on a 16G fs?  Not sure what else might be unique
> about my setup.

Ok, I'll try that.

> >> Fixes: 7d71a67 ("xfs: verify extent size hint is valid in inode verifier")
> >> Fixes: 02a0fda ("xfs: verify COW extent size hint is valid in inode verifier")
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > Separate patch, but can you also modify xfs_ifree to zero the
> > extsize/cowextsize fields so that 4.16-4.17 kernels without this patch
> > are less likely to trip over this?
> 
> I'm confused - 7d71a67 & 02a0fda (above) went into 4.18, so 4.1[67]
> shouldn't have the validator problem, right?
> 
> Are you talking about scrub here?

I ... actually meant (and foolishly did not write, my apologies) commit
8bb82bc12a ("xfs: move inode extent size hint validation to libxfs")
since that's where the broken code came from.

Though I guess 4.17 isn't seriously affected since the only user of
xfs_inode_validate_extsize is scrub, and scrub doesn't look at free
inodes at all.  It's 4.18 where we hooked it up to the inode verifiers
and now we can see this in regular usage.

Ok, so I think this needs a third fixes tag and I'll try to test this
quickly while evaluating it into 4.18-rc7.

> Sorry for waxing philosophical but my fear is that explicitly zeroing the
> fields /now/ will muddy the waters w.r.t. what we should expect to see on
> disk.  We had wrong verifiers, not wrong freeing routines - we need to
> just fix the verifiers IMHO.  Scrub is experimental for a reason, right?

<nod>.

--D

> Thanks,
> -Eric
> --
> 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
Brian Foster July 25, 2018, 11:26 a.m. UTC | #4 
On Tue, Jul 24, 2018 at 11:00:39AM -0700, Eric Sandeen wrote:
> When inodes are freed in xfs_ifree(), di_flags is cleared (so extent size
> hints are removed) but the actual extent size fields are left intact.
> This causes the extent hint validators to fail on freed inodes which once
> had extent size hints. 
> 
> This can be observed (for example) by running xfs/229 twice on a
> non-crc xfs filesystem, or presumably on V5 with ikeep.
> 
> Fixes: 7d71a67 ("xfs: verify extent size hint is valid in inode verifier")
> Fixes: 02a0fda ("xfs: verify COW extent size hint is valid in inode verifier")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 33dc34655ac3..30d1d60f1d46 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -731,7 +731,8 @@ xfs_inode_validate_extsize(
>  	if ((hint_flag || inherit_flag) && extsize == 0)
>  		return __this_address;
>  
> -	if (!(hint_flag || inherit_flag) && extsize != 0)
> +	/* free inodes get flags set to zero but extsize remains */
> +	if (mode && !(hint_flag || inherit_flag) && extsize != 0)
>  		return __this_address;
>  
>  	if (extsize_bytes % blocksize_bytes)
> @@ -777,7 +778,8 @@ xfs_inode_validate_cowextsize(
>  	if (hint_flag && cowextsize == 0)
>  		return __this_address;
>  
> -	if (!hint_flag && cowextsize != 0)
> +	/* free inodes get flags set to zero but cowextsize remains */
> +	if (mode && !hint_flag && cowextsize != 0)
>  		return __this_address;
>  
>  	if (hint_flag && rt_flag)
> 
> 
> --
> 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
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 33dc34655ac3..30d1d60f1d46 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -731,7 +731,8 @@  xfs_inode_validate_extsize(
 	if ((hint_flag || inherit_flag) && extsize == 0)
 		return __this_address;
 
-	if (!(hint_flag || inherit_flag) && extsize != 0)
+	/* free inodes get flags set to zero but extsize remains */
+	if (mode && !(hint_flag || inherit_flag) && extsize != 0)
 		return __this_address;
 
 	if (extsize_bytes % blocksize_bytes)
@@ -777,7 +778,8 @@  xfs_inode_validate_cowextsize(
 	if (hint_flag && cowextsize == 0)
 		return __this_address;
 
-	if (!hint_flag && cowextsize != 0)
+	/* free inodes get flags set to zero but cowextsize remains */
+	if (mode && !hint_flag && cowextsize != 0)
 		return __this_address;
 
 	if (hint_flag && rt_flag)