[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: don't ASSERT on corrupt ftype
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 08 Sep 2014 08:49:50 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140908130507.GN30012@dastard>
References: <540D011B.2000807@xxxxxxxxxx> <20140908130507.GN30012@dastard>
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) +
-               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....



<Prev in Thread] Current Thread [Next in Thread>