xfs: don't treat unknown di_flags[2] as corruption in scrub

Commit Message

Eric Sandeen Sept. 18, 2018, 2:41 a.m. UTC

xchk_inode_flags[2]() currently treats any di_flags[2] values that the
running kernel doesn't recognize as corruption, and calls
xchk_ino_set_corrupt() if they are set.  However, it's entirely possible
that these flags were set in some newer kernel and are quite valid,
but ignored in this kernel.

(Validators don't care one bit about unknown di_flags[2].)

Call xchk_ino_set_warning instead, because this may or may not actually
indicate a problem.

Signed-off-by: Eric Sandeen <[e-mail]>
---

Comments

Darrick J. Wong Sept. 18, 2018, 3:18 a.m. UTC | #1

On Mon, Sep 17, 2018 at 09:41:35PM -0500, Eric Sandeen wrote:
> xchk_inode_flags[2]() currently treats any di_flags[2] values that the
> running kernel doesn't recognize as corruption, and calls
> xchk_ino_set_corrupt() if they are set.  However, it's entirely possible
> that these flags were set in some newer kernel and are quite valid,
> but ignored in this kernel.
> 
> (Validators don't care one bit about unknown di_flags[2].)
> 
> Call xchk_ino_set_warning instead, because this may or may not actually
> indicate a problem.
> 
> Signed-off-by: Eric Sandeen <[e-mail]>
> ---
> 
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index 5b3b177..e53ed83 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -126,8 +126,9 @@

Would be nice to know which function this is, but it otherwise seems
fine to me:

Reviewed-by: Darrick J. Wong <[e-mail]>

--D

>  {
>  	struct xfs_mount	*mp = sc->mp;
>  
> +	/* Unknown di_flags could simply be from newer kernel */
>  	if (flags & ~XFS_DIFLAG_ANY)
> -		goto bad;
> +		xchk_ino_set_warning(sc, ino);
>  
>  	/* rt flags require rt device */
>  	if ((flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) &&
> @@ -172,8 +173,9 @@
>  {
>  	struct xfs_mount	*mp = sc->mp;
>  
> +	/* Unknown di_flags2 could simply be from newer kernel */
>  	if (flags2 & ~XFS_DIFLAG2_ANY)
> -		goto bad;
> +		xchk_ino_set_warning(sc, ino);
>  
>  	/* reflink flag requires reflink feature */
>  	if ((flags2 & XFS_DIFLAG2_REFLINK) &&
>

Dave Chinner Sept. 18, 2018, 5:20 a.m. UTC | #2

On Mon, Sep 17, 2018 at 09:41:35PM -0500, Eric Sandeen wrote:
> xchk_inode_flags[2]() currently treats any di_flags[2] values that the
> running kernel doesn't recognize as corruption, and calls
> xchk_ino_set_corrupt() if they are set.  However, it's entirely possible
> that these flags were set in some newer kernel and are quite valid,
> but ignored in this kernel.
> 
> (Validators don't care one bit about unknown di_flags[2].)
> 
> Call xchk_ino_set_warning instead, because this may or may not actually
> indicate a problem.
> 
> Signed-off-by: Eric Sandeen <[e-mail]>
> ---
> 
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index 5b3b177..e53ed83 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -126,8 +126,9 @@
>  {
>  	struct xfs_mount	*mp = sc->mp;
>  
> +	/* Unknown di_flags could simply be from newer kernel */
>  	if (flags & ~XFS_DIFLAG_ANY)
> -		goto bad;
> +		xchk_ino_set_warning(sc, ino);

There's only one flag in that set, right? And we only need that flag
for a future v2 inode features we add? i.e. any new feature will be
on a v3 inode format because the v2 format is the legacy inode
format and we're not developing new features for it.

[ There's also the minor issue that the remaining flag bit in
di_flags is reserved for the "more flags" flag bit so that we know
to grab flags from some other padding in the inode we redefined to
hold more flags in the v2 inode format. But that's irrelevant now
because it's a legacy format. ]

IOWs, I think the original code here is just fine because we're not
going to add new v2 format inode features in the future.

>  
>  	/* rt flags require rt device */
>  	if ((flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) &&
> @@ -172,8 +173,9 @@
>  {
>  	struct xfs_mount	*mp = sc->mp;
>  
> +	/* Unknown di_flags2 could simply be from newer kernel */
>  	if (flags2 & ~XFS_DIFLAG2_ANY)
> -		goto bad;
> +		xchk_ino_set_warning(sc, ino);

This bit it fine, though :)

Cheers,

Dave.

Eric Sandeen Sept. 18, 2018, 12:11 p.m. UTC | #3

On 9/18/18 12:20 AM, Dave Chinner wrote:
> On Mon, Sep 17, 2018 at 09:41:35PM -0500, Eric Sandeen wrote:
>> xchk_inode_flags[2]() currently treats any di_flags[2] values that the
>> running kernel doesn't recognize as corruption, and calls
>> xchk_ino_set_corrupt() if they are set.  However, it's entirely possible
>> that these flags were set in some newer kernel and are quite valid,
>> but ignored in this kernel.
>>
>> (Validators don't care one bit about unknown di_flags[2].)
>>
>> Call xchk_ino_set_warning instead, because this may or may not actually
>> indicate a problem.
>>
>> Signed-off-by: Eric Sandeen <[e-mail]>
>> ---
>>
>> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
>> index 5b3b177..e53ed83 100644
>> --- a/fs/xfs/scrub/inode.c
>> +++ b/fs/xfs/scrub/inode.c
>> @@ -126,8 +126,9 @@
>>  {
>>  	struct xfs_mount	*mp = sc->mp;
>>  
>> +	/* Unknown di_flags could simply be from newer kernel */
>>  	if (flags & ~XFS_DIFLAG_ANY)
>> -		goto bad;
>> +		xchk_ino_set_warning(sc, ino);
> 
> There's only one flag in that set, right?

Yes, (1 << 15).

> And we only need that flag
> for a future v2 inode features we add? i.e. any new feature will be
> on a v3 inode format because the v2 format is the legacy inode
> format and we're not developing new features for it.

Ok...

> [ There's also the minor issue that the remaining flag bit in
> di_flags is reserved for the "more flags" flag bit so that we know
> to grab flags from some other padding in the inode we redefined to
> hold more flags in the v2 inode format. But that's irrelevant now
> because it's a legacy format. ]
> 
> IOWs, I think the original code here is just fine because we're not
> going to add new v2 format inode features in the future.

Ok, if we're absolutely 100% sure that no future kernel will ever use
that flag, then yes, it's corruption if it's ever found to be set.
I wasn't quite ready to draw that line in the sand.

Should probably #define a new XFS_DIFLAG_NEVER or something then, to make
it crystal clear?

-Eric

Dave Chinner Sept. 18, 2018, 12:18 p.m. UTC | #4

On Tue, Sep 18, 2018 at 07:11:12AM -0500, Eric Sandeen wrote:
> On 9/18/18 12:20 AM, Dave Chinner wrote:
> > On Mon, Sep 17, 2018 at 09:41:35PM -0500, Eric Sandeen wrote:
> >> xchk_inode_flags[2]() currently treats any di_flags[2] values that the
> >> running kernel doesn't recognize as corruption, and calls
> >> xchk_ino_set_corrupt() if they are set.  However, it's entirely possible
> >> that these flags were set in some newer kernel and are quite valid,
> >> but ignored in this kernel.
> >>
> >> (Validators don't care one bit about unknown di_flags[2].)
> >>
> >> Call xchk_ino_set_warning instead, because this may or may not actually
> >> indicate a problem.
> >>
> >> Signed-off-by: Eric Sandeen <[e-mail]>
> >> ---
> >>
> >> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> >> index 5b3b177..e53ed83 100644
> >> --- a/fs/xfs/scrub/inode.c
> >> +++ b/fs/xfs/scrub/inode.c
> >> @@ -126,8 +126,9 @@
> >>  {
> >>  	struct xfs_mount	*mp = sc->mp;
> >>  
> >> +	/* Unknown di_flags could simply be from newer kernel */
> >>  	if (flags & ~XFS_DIFLAG_ANY)
> >> -		goto bad;
> >> +		xchk_ino_set_warning(sc, ino);
> > 
> > There's only one flag in that set, right?
> 
> Yes, (1 << 15).
> 
> > And we only need that flag
> > for a future v2 inode features we add? i.e. any new feature will be
> > on a v3 inode format because the v2 format is the legacy inode
> > format and we're not developing new features for it.
> 
> Ok...
> 
> > [ There's also the minor issue that the remaining flag bit in
> > di_flags is reserved for the "more flags" flag bit so that we know
> > to grab flags from some other padding in the inode we redefined to
> > hold more flags in the v2 inode format. But that's irrelevant now
> > because it's a legacy format. ]
> > 
> > IOWs, I think the original code here is just fine because we're not
> > going to add new v2 format inode features in the future.
> 
> Ok, if we're absolutely 100% sure that no future kernel will ever use
> that flag, then yes, it's corruption if it's ever found to be set.
> I wasn't quite ready to draw that line in the sand.
> 
> Should probably #define a new XFS_DIFLAG_NEVER or something then, to make
> it crystal clear?

Perhaps so. Don't really care one way or the other.

Cheers,

Dave.

Patch

diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index 5b3b177..e53ed83 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -126,8 +126,9 @@ 
 {
 	struct xfs_mount	*mp = sc->mp;
 
+	/* Unknown di_flags could simply be from newer kernel */
 	if (flags & ~XFS_DIFLAG_ANY)
-		goto bad;
+		xchk_ino_set_warning(sc, ino);
 
 	/* rt flags require rt device */
 	if ((flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) &&
@@ -172,8 +173,9 @@ 
 {
 	struct xfs_mount	*mp = sc->mp;
 
+	/* Unknown di_flags2 could simply be from newer kernel */
 	if (flags2 & ~XFS_DIFLAG2_ANY)
-		goto bad;
+		xchk_ino_set_warning(sc, ino);
 
 	/* reflink flag requires reflink feature */
 	if ((flags2 & XFS_DIFLAG2_REFLINK) &&