xfs: don't ASSERT on corrupt ftype

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 <[email protected]>

---

fs/xfs/libxfs/xfs_da_format.c +0 -1

index c9aee52..7e42fdf 100644
@@ -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;

fs/xfs/libxfs/xfs_dir2_sf.c +0 -0

index 5079e05..ea89250 100644

fs/xfs/libxfs/xfs_dir2_sf.c +0 -0

 	}
 	ASSERT(i8count == sfp->i8count);
 	ASSERT((char *)sfep - (char *)sfp == dp->i_d.di_size);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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.

Quoted 9 lines · Eric Sandeen

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

fs/xfs/libxfs/xfs_dir2_sf.c xfs_dir2_sf_check( · line 640

Quoted 12 lines

fs/xfs/libxfs/xfs_dir2_sf.c +0 -1

index 5079e05..ea89250 100644
@@ -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.

Quoted 11 lines · Dave Chinner

Quoted 9 lines · Eric Sandeen

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.

Quoted 5 lines

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.

Quoted 18 lines · Eric Sandeen

Quoted 16 lines · Dave Chinner

Quoted 9 lines · Eric Sandeen

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.

Quoted 1 line

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