* Re: [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub
2018-09-18 2:41 [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub Eric Sandeen
@ 2018-09-18 3:18 ` Darrick J. Wong
2018-09-18 5:20 ` Dave Chinner
2018-09-18 13:50 ` [PATCH V2] xfs: don't treat unknown di_flags2 " Eric Sandeen
2 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2018-09-18 3:18 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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 <sandeen@redhat.com>
> ---
>
> 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 <darrick.wong@oracle.com>
--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) &&
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub
2018-09-18 2:41 [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub Eric Sandeen
2018-09-18 3:18 ` Darrick J. Wong
@ 2018-09-18 5:20 ` Dave Chinner
2018-09-18 12:11 ` Eric Sandeen
2018-09-18 13:50 ` [PATCH V2] xfs: don't treat unknown di_flags2 " Eric Sandeen
2 siblings, 1 reply; 7+ messages in thread
From: Dave Chinner @ 2018-09-18 5:20 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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 <sandeen@redhat.com>
> ---
>
> 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub
2018-09-18 5:20 ` Dave Chinner
@ 2018-09-18 12:11 ` Eric Sandeen
2018-09-18 12:18 ` Dave Chinner
0 siblings, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2018-09-18 12:11 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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 <sandeen@redhat.com>
>> ---
>>
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub
2018-09-18 12:11 ` Eric Sandeen
@ 2018-09-18 12:18 ` Dave Chinner
0 siblings, 0 replies; 7+ messages in thread
From: Dave Chinner @ 2018-09-18 12:18 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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 <sandeen@redhat.com>
> >> ---
> >>
> >> 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2] xfs: don't treat unknown di_flags2 as corruption in scrub
2018-09-18 2:41 [PATCH] xfs: don't treat unknown di_flags[2] as corruption in scrub Eric Sandeen
2018-09-18 3:18 ` Darrick J. Wong
2018-09-18 5:20 ` Dave Chinner
@ 2018-09-18 13:50 ` Eric Sandeen
2018-09-18 14:15 ` Darrick J. Wong
2 siblings, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2018-09-18 13:50 UTC (permalink / raw)
To: Eric Sandeen, linux-xfs
xchk_inode_flags2() currently treats any di_flags2 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_flags2.)
Call xchk_ino_set_warning instead, because this may or may not actually
indicate a problem.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: continue to treat unknown di_flag value as corruption, and add
comments about how the last di_flag bit cannot be used.
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 059bc44c27e8..afbe336600e1 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1016,6 +1016,8 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
#define XFS_DIFLAG_EXTSZINHERIT_BIT 12 /* inherit inode extent size */
#define XFS_DIFLAG_NODEFRAG_BIT 13 /* do not reorganize/defragment */
#define XFS_DIFLAG_FILESTREAM_BIT 14 /* use filestream allocator */
+/* Do not use bit 15, di_flags is legacy and unchanging now */
+
#define XFS_DIFLAG_REALTIME (1 << XFS_DIFLAG_REALTIME_BIT)
#define XFS_DIFLAG_PREALLOC (1 << XFS_DIFLAG_PREALLOC_BIT)
#define XFS_DIFLAG_NEWRTBM (1 << XFS_DIFLAG_NEWRTBM_BIT)
diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index 5b3b177c0fc9..8fb29540af45 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -126,6 +126,7 @@ xchk_inode_flags(
{
struct xfs_mount *mp = sc->mp;
+ /* di_flags are all taken, last bit cannot be used */
if (flags & ~XFS_DIFLAG_ANY)
goto bad;
@@ -172,8 +173,9 @@ xchk_inode_flags2(
{
struct xfs_mount *mp = sc->mp;
+ /* Unknown di_flags2 could be from a future kernel */
if (flags2 & ~XFS_DIFLAG2_ANY)
- goto bad;
+ xchk_ino_set_warning(sc, ino);
/* reflink flag requires reflink feature */
if ((flags2 & XFS_DIFLAG2_REFLINK) &&
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH V2] xfs: don't treat unknown di_flags2 as corruption in scrub
2018-09-18 13:50 ` [PATCH V2] xfs: don't treat unknown di_flags2 " Eric Sandeen
@ 2018-09-18 14:15 ` Darrick J. Wong
0 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2018-09-18 14:15 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs
On Tue, Sep 18, 2018 at 08:50:14AM -0500, Eric Sandeen wrote:
> xchk_inode_flags2() currently treats any di_flags2 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_flags2.)
>
> Call xchk_ino_set_warning instead, because this may or may not actually
> indicate a problem.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
>
> V2: continue to treat unknown di_flag value as corruption, and add
> comments about how the last di_flag bit cannot be used.
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 059bc44c27e8..afbe336600e1 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1016,6 +1016,8 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
> #define XFS_DIFLAG_EXTSZINHERIT_BIT 12 /* inherit inode extent size */
> #define XFS_DIFLAG_NODEFRAG_BIT 13 /* do not reorganize/defragment */
> #define XFS_DIFLAG_FILESTREAM_BIT 14 /* use filestream allocator */
> +/* Do not use bit 15, di_flags is legacy and unchanging now */
> +
> #define XFS_DIFLAG_REALTIME (1 << XFS_DIFLAG_REALTIME_BIT)
> #define XFS_DIFLAG_PREALLOC (1 << XFS_DIFLAG_PREALLOC_BIT)
> #define XFS_DIFLAG_NEWRTBM (1 << XFS_DIFLAG_NEWRTBM_BIT)
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index 5b3b177c0fc9..8fb29540af45 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -126,6 +126,7 @@ xchk_inode_flags(
> {
> struct xfs_mount *mp = sc->mp;
>
> + /* di_flags are all taken, last bit cannot be used */
> if (flags & ~XFS_DIFLAG_ANY)
> goto bad;
>
> @@ -172,8 +173,9 @@ xchk_inode_flags2(
> {
> struct xfs_mount *mp = sc->mp;
>
> + /* Unknown di_flags2 could be from a future kernel */
> if (flags2 & ~XFS_DIFLAG2_ANY)
> - goto bad;
> + xchk_ino_set_warning(sc, ino);
>
> /* reflink flag requires reflink feature */
> if ((flags2 & XFS_DIFLAG2_REFLINK) &&
>
^ permalink raw reply [flat|nested] 7+ messages in thread