* [PATCH] xfs: don't ASSERT on corrupt ftype
@ 2014-09-08 1:06 Eric Sandeen
2014-09-08 12:03 ` Brian Foster
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Eric Sandeen @ 2014-09-08 1:06 UTC (permalink / raw)
To: xfs-oss
xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get
the file type off disk, but ASSERT if it's invalid:
ASSERT(type < XFS_DIR3_FT_MAX);
This might be cut & paste from the "put" functions,
which should be checking that they've not been passed
bad values, but we shouldn't ASSERT on bad values
read from disk.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/fs/xfs/libxfs/xfs_da_format.c b/fs/xfs/libxfs/xfs_da_format.c
index c9aee52..7e42fdf 100644
--- a/fs/xfs/libxfs/xfs_da_format.c
+++ b/fs/xfs/libxfs/xfs_da_format.c
@@ -270,7 +270,6 @@ xfs_dir3_data_get_ftype(
{
__uint8_t ftype = dep->name[dep->namelen];
- ASSERT(ftype < XFS_DIR3_FT_MAX);
if (ftype >= XFS_DIR3_FT_MAX)
return XFS_DIR3_FT_UNKNOWN;
return ftype;
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 5079e05..ea89250 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -635,7 +635,6 @@ xfs_dir2_sf_check(
offset =
xfs_dir2_sf_get_offset(sfep) +
dp->d_ops->data_entsize(sfep->namelen);
- ASSERT(dp->d_ops->sf_get_ftype(sfep) < XFS_DIR3_FT_MAX);
}
ASSERT(i8count == sfp->i8count);
ASSERT((char *)sfep - (char *)sfp == dp->i_d.di_size);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] xfs: don't ASSERT on corrupt ftype 2014-09-08 1:06 [PATCH] xfs: don't ASSERT on corrupt ftype Eric Sandeen @ 2014-09-08 12:03 ` Brian Foster 2014-09-08 13:05 ` Dave Chinner 2014-09-08 22:18 ` [PATCH V2] " Eric Sandeen 2 siblings, 0 replies; 7+ messages in thread From: Brian Foster @ 2014-09-08 12:03 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs-oss On Sun, Sep 07, 2014 at 08:06:35PM -0500, Eric Sandeen wrote: > xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get > the file type off disk, but ASSERT if it's invalid: > > ASSERT(type < XFS_DIR3_FT_MAX); > > This might be cut & paste from the "put" functions, > which should be checking that they've not been passed > bad values, but we shouldn't ASSERT on bad values > read from disk. > Does this lead to a problem of some kind in a valid configuration? > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/xfs/libxfs/xfs_da_format.c b/fs/xfs/libxfs/xfs_da_format.c > index c9aee52..7e42fdf 100644 > --- a/fs/xfs/libxfs/xfs_da_format.c > +++ b/fs/xfs/libxfs/xfs_da_format.c > @@ -270,7 +270,6 @@ xfs_dir3_data_get_ftype( > { > __uint8_t ftype = dep->name[dep->namelen]; > - ASSERT(ftype < XFS_DIR3_FT_MAX); > if (ftype >= XFS_DIR3_FT_MAX) > return XFS_DIR3_FT_UNKNOWN; > return ftype; > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c > index 5079e05..ea89250 100644 > --- a/fs/xfs/libxfs/xfs_dir2_sf.c > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c > @@ -635,7 +635,6 @@ xfs_dir2_sf_check( > offset = > xfs_dir2_sf_get_offset(sfep) + > dp->d_ops->data_entsize(sfep->namelen); > - ASSERT(dp->d_ops->sf_get_ftype(sfep) < XFS_DIR3_FT_MAX); This one is a DEBUG mode only function and the comment suggests the intent is to assert on directory inconsistency..? Brian > } > ASSERT(i8count == sfp->i8count); > ASSERT((char *)sfep - (char *)sfp == dp->i_d.di_size); > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: don't ASSERT on corrupt ftype 2014-09-08 1:06 [PATCH] xfs: don't ASSERT on corrupt ftype Eric Sandeen 2014-09-08 12:03 ` Brian Foster @ 2014-09-08 13:05 ` Dave Chinner 2014-09-08 13:49 ` Eric Sandeen 2014-09-08 22:18 ` [PATCH V2] " Eric Sandeen 2 siblings, 1 reply; 7+ messages in thread From: Dave Chinner @ 2014-09-08 13:05 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs-oss On Sun, Sep 07, 2014 at 08:06:35PM -0500, Eric Sandeen wrote: > xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get > the file type off disk, but ASSERT if it's invalid: > > ASSERT(type < XFS_DIR3_FT_MAX); > > This might be cut & paste from the "put" functions, > which should be checking that they've not been passed > bad values, but we shouldn't ASSERT on bad values > read from disk. No, they weren't cut-n-paste from the put functions. They were actually designed for a metadata block where bad values would not be written to disk, and corrupted disk blocks would be detected by CRC validation failures. So on debug kernels it's quite appropriate to assert fail on a "should never, ever happen" condition. Then the v4 ftype feature bit was rammed in but none of the code got changed to reflect that the values in the ftype fields are not CRC protected on v4 filesystems.... > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c > index 5079e05..ea89250 100644 > --- a/fs/xfs/libxfs/xfs_dir2_sf.c > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c > @@ -635,7 +635,6 @@ xfs_dir2_sf_check( > offset = > xfs_dir2_sf_get_offset(sfep) + > dp->d_ops->data_entsize(sfep->namelen); > - ASSERT(dp->d_ops->sf_get_ftype(sfep) < XFS_DIR3_FT_MAX); > } > ASSERT(i8count == sfp->i8count); > ASSERT((char *)sfep - (char *)sfp == dp->i_d.di_size); That's a debug only function validating that the shortform directory is internally consistent. And as the comment says: /* * Check consistency of shortform directory, assert if bad. */ So that assert should remain because it's checking the in-memory state immediately before, during and after modifications are made to the directory, not when it has just been read of disk.... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: don't ASSERT on corrupt ftype 2014-09-08 13:05 ` Dave Chinner @ 2014-09-08 13:49 ` Eric Sandeen 2014-09-08 14:02 ` Eric Sandeen 0 siblings, 1 reply; 7+ messages in thread From: Eric Sandeen @ 2014-09-08 13:49 UTC (permalink / raw) To: Dave Chinner, Eric Sandeen; +Cc: xfs-oss On 9/8/14 8:05 AM, Dave Chinner wrote: > On Sun, Sep 07, 2014 at 08:06:35PM -0500, Eric Sandeen wrote: >> xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get >> the file type off disk, but ASSERT if it's invalid: >> >> ASSERT(type < XFS_DIR3_FT_MAX); >> >> This might be cut & paste from the "put" functions, >> which should be checking that they've not been passed >> bad values, but we shouldn't ASSERT on bad values >> read from disk. > > No, they weren't cut-n-paste from the put functions. They were > actually designed for a metadata block where bad values would not be > written to disk, and corrupted disk blocks would be detected by CRC > validation failures. So on debug kernels it's quite appropriate to > assert fail on a "should never, ever happen" condition. hohum, ok. > Then the v4 ftype feature bit was rammed in but none of the code got > changed to reflect that the values in the ftype fields are not CRC > protected on v4 filesystems.... > >> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c >> index 5079e05..ea89250 100644 >> --- a/fs/xfs/libxfs/xfs_dir2_sf.c >> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c >> @@ -635,7 +635,6 @@ xfs_dir2_sf_check( >> offset = >> xfs_dir2_sf_get_offset(sfep) + >> dp->d_ops->data_entsize(sfep->namelen); >> - ASSERT(dp->d_ops->sf_get_ftype(sfep) < XFS_DIR3_FT_MAX); >> } >> ASSERT(i8count == sfp->i8count); >> ASSERT((char *)sfep - (char *)sfp == dp->i_d.di_size); > > That's a debug only function validating that the shortform directory > is internally consistent. And as the comment says: > > /* > * Check consistency of shortform directory, assert if bad. > */ > > So that assert should remain because it's checking the in-memory > state immediately before, during and after modifications are made to > the directory, not when it has just been read of disk.... > > Cheers, > > Dave. > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: don't ASSERT on corrupt ftype 2014-09-08 13:49 ` Eric Sandeen @ 2014-09-08 14:02 ` Eric Sandeen 2014-09-08 22:00 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Eric Sandeen @ 2014-09-08 14:02 UTC (permalink / raw) To: Dave Chinner, Eric Sandeen; +Cc: xfs-oss On 9/8/14 8:49 AM, Eric Sandeen wrote: > On 9/8/14 8:05 AM, Dave Chinner wrote: >> On Sun, Sep 07, 2014 at 08:06:35PM -0500, Eric Sandeen wrote: >>> xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get >>> the file type off disk, but ASSERT if it's invalid: >>> >>> ASSERT(type < XFS_DIR3_FT_MAX); >>> >>> This might be cut & paste from the "put" functions, >>> which should be checking that they've not been passed >>> bad values, but we shouldn't ASSERT on bad values >>> read from disk. >> >> No, they weren't cut-n-paste from the put functions. They were >> actually designed for a metadata block where bad values would not be >> written to disk, and corrupted disk blocks would be detected by CRC >> validation failures. So on debug kernels it's quite appropriate to >> assert fail on a "should never, ever happen" condition. > > hohum, ok. So then presumably the reason there is no ASSERT in xfs_dir3_sfe_get_ftype (vs in xfs_dir3_data_get_ftype) is also purely intentional and part of the design, but I'm unable to divine that logic... can you help me out? I guess the only way forward is to create a 3rd set of ops, and have one for dir2, one for dir2-with-ftype, and one for dir3? Because in the op, there's no way to discern between the latter 2, and know if we're previously CRC-protected or not... -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: don't ASSERT on corrupt ftype 2014-09-08 14:02 ` Eric Sandeen @ 2014-09-08 22:00 ` Dave Chinner 0 siblings, 0 replies; 7+ messages in thread From: Dave Chinner @ 2014-09-08 22:00 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss On Mon, Sep 08, 2014 at 09:02:27AM -0500, Eric Sandeen wrote: > On 9/8/14 8:49 AM, Eric Sandeen wrote: > >On 9/8/14 8:05 AM, Dave Chinner wrote: > >>On Sun, Sep 07, 2014 at 08:06:35PM -0500, Eric Sandeen wrote: > >>>xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get > >>>the file type off disk, but ASSERT if it's invalid: > >>> > >>> ASSERT(type < XFS_DIR3_FT_MAX); > >>> > >>>This might be cut & paste from the "put" functions, > >>>which should be checking that they've not been passed > >>>bad values, but we shouldn't ASSERT on bad values > >>>read from disk. > >> > >>No, they weren't cut-n-paste from the put functions. They were > >>actually designed for a metadata block where bad values would not be > >>written to disk, and corrupted disk blocks would be detected by CRC > >>validation failures. So on debug kernels it's quite appropriate to > >>assert fail on a "should never, ever happen" condition. > > > >hohum, ok. > > So then presumably the reason there is no ASSERT in xfs_dir3_sfe_get_ftype > (vs in xfs_dir3_data_get_ftype) is also purely intentional and > part of the design, but I'm unable to divine that logic... can you > help me out? Because that ASSERT check was put in xfs_dir2_sf_check(). Yeah, I know, not consistent, but the shortform code is quite different to the block/leaf/node code.... > I guess the only way forward is to create a 3rd set of ops, and have > one for dir2, one for dir2-with-ftype, and one for dir3? Because > in the op, there's no way to discern between the latter 2, and > know if we're previously CRC-protected or not... No, just kill the asset in xfs_dir3_data_get_ftype(). Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH V2] xfs: don't ASSERT on corrupt ftype 2014-09-08 1:06 [PATCH] xfs: don't ASSERT on corrupt ftype Eric Sandeen 2014-09-08 12:03 ` Brian Foster 2014-09-08 13:05 ` Dave Chinner @ 2014-09-08 22:18 ` Eric Sandeen 2 siblings, 0 replies; 7+ messages in thread From: Eric Sandeen @ 2014-09-08 22:18 UTC (permalink / raw) To: Eric Sandeen, xfs-oss xfs_dir3_data_get_ftype() gets the file type off disk, but ASSERTs if it's invalid: ASSERT(type < XFS_DIR3_FT_MAX); We shouldn't ASSERT on bad values read from disk. V3 dirs are CRC-protected, but V2 dirs + ftype are not. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/fs/xfs/libxfs/xfs_da_format.c b/fs/xfs/libxfs/xfs_da_format.c index c9aee52..7e42fdf 100644 --- a/fs/xfs/libxfs/xfs_da_format.c +++ b/fs/xfs/libxfs/xfs_da_format.c @@ -270,7 +270,6 @@ xfs_dir3_data_get_ftype( { __uint8_t ftype = dep->name[dep->namelen]; - ASSERT(ftype < XFS_DIR3_FT_MAX); if (ftype >= XFS_DIR3_FT_MAX) return XFS_DIR3_FT_UNKNOWN; return ftype; _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-09-08 22:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-08 1:06 [PATCH] xfs: don't ASSERT on corrupt ftype Eric Sandeen 2014-09-08 12:03 ` Brian Foster 2014-09-08 13:05 ` Dave Chinner 2014-09-08 13:49 ` Eric Sandeen 2014-09-08 14:02 ` Eric Sandeen 2014-09-08 22:00 ` Dave Chinner 2014-09-08 22:18 ` [PATCH V2] " Eric Sandeen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.