[PATCH] xfs: don't ASSERT on corrupt ftype

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

Signed-off-by: Eric 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);

[Brian Foster]

Post by Eric Sandeen
xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get
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?

Post by Eric Sandeen
---
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

Post by Eric Sandeen
}
ASSERT(i8count == sfp->i8count);
ASSERT((char *)sfep - (char *)sfp == dp->i_d.di_size);
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Dave Chinner]

Post by Eric Sandeen
xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get
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....

Post by Eric Sandeen
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.

[Eric Sandeen]

Post by Dave Chinner

Post by Eric Sandeen
xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get
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.

Post by Dave Chinner
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....

Post by Eric Sandeen
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
/*
* 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.

[Eric Sandeen]

Post by Eric Sandeen

Post by Dave Chinner

Post by Eric Sandeen
xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get
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

[Dave Chinner]

Post by Eric Sandeen

Post by Eric Sandeen

Post by Dave Chinner

Post by Eric Sandeen
xfs_dir3_data_get_ftype() and xfs_dir2_sf_check() get
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....

Post by Eric Sandeen
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.

[Eric Sandeen]

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 <***@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;