[PATCH v6 0/3] xfs: fixes for malformed on-disk i_mode

[PATCH v6 0/3] xfs: fixes for malformed on-disk i_mode

[Amir Goldstein]

Darrick,

This is 6th revision of the fixes for malformed on-disk i_mode.

I implemented the xfs specific test case (xfs/348) to test all
possible malformed file type values as you suggested.

Tested with generic/401 with -n ftype=0|1.
Tested with new xfs/348 test with -n ftype=0|1.

Test xfs/348 exposed an ASSERT on attempt to readdir of regular
file that is posing as a directory.

Patch 2 implements your suggestion to address this case.

Patch 3 fixes a very strage upsidedown unlikely() in the
xfs ASSERT macros. I hope I am not tripping...

Amir.

v6:
- Added Reviewed-by Brian for patch 1
- Added patch 2 to address new xfs/348 failures
- Added patch 3 to fix ASSERT() likely

v5:
- remove wrong argument about on-disk malformed mode from commit message
- address Brian's review comments

v4:
- independent fix patch for xfs

Amir Goldstein (3):
  xfs: fix the size of xfs_mode_to_ftype table
  xfs: sanity check directory inode di_size
  xfs: make the ASSERT() condition likely

 fs/xfs/libxfs/xfs_dir2.c      | 21 +++++++++++----------
 fs/xfs/libxfs/xfs_dir2.h      |  4 +++-
 fs/xfs/libxfs/xfs_inode_buf.c |  7 +++++--
 fs/xfs/xfs_iops.c             |  2 +-
 fs/xfs/xfs_linux.h            |  6 +++---
 5 files changed, 23 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v6 1/3] xfs: fix the size of xfs_mode_to_ftype table

[Amir Goldstein]

Fix the size of the xfs_mode_to_ftype conversion table,
which was too small to handle an invalid value of mode=S_IFMT.

Use a convenience macro S_DT(mode) to convert from
mode to dirent file type and change the name of the table
to xfs_dtype_to_ftype to correctly describe its index values.

Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c | 18 +++++++++---------
 fs/xfs/libxfs/xfs_dir2.h |  4 +++-
 fs/xfs/xfs_iops.c        |  2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index c58d72c..48d7c45 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -41,15 +41,15 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
  * for file type specification. This will be propagated into the directory
  * structure if appropriate for the given operation and filesystem config.
  */
-const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
-	[0]			= XFS_DIR3_FT_UNKNOWN,
-	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
-	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
+const unsigned char xfs_dtype_to_ftype[S_DT_MAX+1] = {
+	[0]                = XFS_DIR3_FT_UNKNOWN,
+	[S_DT(S_IFREG)]    = XFS_DIR3_FT_REG_FILE,
+	[S_DT(S_IFDIR)]    = XFS_DIR3_FT_DIR,
+	[S_DT(S_IFCHR)]    = XFS_DIR3_FT_CHRDEV,
+	[S_DT(S_IFBLK)]    = XFS_DIR3_FT_BLKDEV,
+	[S_DT(S_IFIFO)]    = XFS_DIR3_FT_FIFO,
+	[S_DT(S_IFSOCK)]   = XFS_DIR3_FT_SOCK,
+	[S_DT(S_IFLNK)]    = XFS_DIR3_FT_SYMLINK,
 };
 
 /*
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..4934d38 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -35,7 +35,9 @@ extern struct xfs_name	xfs_name_dotdot;
  * directory filetype conversion tables.
  */
 #define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
+#define S_DT(mode)     (((mode) & S_IFMT) >> S_SHIFT)
+#define S_DT_MAX       S_DT(S_IFMT)
+extern const unsigned char xfs_dtype_to_ftype[];
 
 /*
  * directory operations vector for encode/decode routines
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..d2da9ca 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -103,7 +103,7 @@ xfs_dentry_to_name(
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
-	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
+	namep->type = xfs_dtype_to_ftype[S_DT(mode)];
 }
 
 STATIC void
-- 
2.7.4

[PATCH v6 2/3] xfs: sanity check directory inode di_size

[Amir Goldstein]

This changes fixes an assertion hit when fuzzing on-disk
i_mode values.

The easy case to fix is when changing an empty file
i_mode to S_IFDIR. In this case, xfs_dinode_verify()
detects an illegal zero size for directory and fails
to load the inode structure from disk.

For the case of non empty file whose i_mode is changed
to S_IFDIR, the ASSERT() statement in xfs_dir2_isblock()
is replaced with return -EFSCORRUPTED as suggested by
Darrick, to avoid interacting with corrupted jusk also
when XFS_DEBUG is disabled.

Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c      | 3 ++-
 fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 48d7c45..3f73f49 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -631,7 +631,8 @@ xfs_dir2_isblock(
 	if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK)))
 		return rval;
 	rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize;
-	ASSERT(rval == 0 || args->dp->i_d.di_size == args->geo->blksize);
+	if (rval != 0 && args->dp->i_d.di_size != args->geo->blksize)
+		return -EFSCORRUPTED;
 	*vp = rval;
 	return 0;
 }
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index dd483e2..0091ac3 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -386,6 +386,7 @@ xfs_dinode_verify(
 	xfs_ino_t		ino,
 	struct xfs_dinode	*dip)
 {
+	uint16_t		mode;
 	uint16_t		flags;
 	uint64_t		flags2;
 
@@ -396,8 +397,10 @@ xfs_dinode_verify(
 	if (be64_to_cpu(dip->di_size) & (1ULL << 63))
 		return false;
 
-	/* No zero-length symlinks. */
-	if (S_ISLNK(be16_to_cpu(dip->di_mode)) && dip->di_size == 0)
+	mode = be16_to_cpu(dip->di_mode);
+
+	/* No zero-length symlinks/dirs. */
+	if ((S_ISLNK(mode) || S_ISDIR(mode)) && dip->di_size == 0)
 		return false;
 
 	/* only version 3 or greater inodes are extensively verified here */
-- 
2.7.4

[PATCH v6 3/3] xfs: make the ASSERT() condition likely

[Amir Goldstein]

The ASSERT() condition is the normal case, not the exception,
so testing the condition should be likely(), not unlikely().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_linux.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index e467218..7a989de 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -331,11 +331,11 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
 }
 
 #define ASSERT_ALWAYS(expr)	\
-	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
+	(likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
 
 #ifdef DEBUG
 #define ASSERT(expr)	\
-	(unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
+	(likely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
 
 #ifndef STATIC
 # define STATIC noinline
@@ -346,7 +346,7 @@ static inline __uint64_t howmany_64(__uint64_t x, __uint32_t y)
 #ifdef XFS_WARN
 
 #define ASSERT(expr)	\
-	(unlikely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__))
+	(likely(expr) ? (void)0 : asswarn(#expr, __FILE__, __LINE__))
 
 #ifndef STATIC
 # define STATIC static noinline
-- 
2.7.4

Re: [PATCH v6 1/3] xfs: fix the size of xfs_mode_to_ftype table

[Christoph Hellwig]

On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
> Fix the size of the xfs_mode_to_ftype conversion table,
> which was too small to handle an invalid value of mode=S_IFMT.
> 
> Use a convenience macro S_DT(mode) to convert from
> mode to dirent file type and change the name of the table
> to xfs_dtype_to_ftype to correctly describe its index values.

This looks like an awful lot of magic.  Would a switch statement
generate so much worse code?

Re: [PATCH v6 2/3] xfs: sanity check directory inode di_size

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v6 3/3] xfs: make the ASSERT() condition likely

[Christoph Hellwig]

On Mon, Jan 09, 2017 at 03:06:29PM +0200, Amir Goldstein wrote:
> The ASSERT() condition is the normal case, not the exception,
> so testing the condition should be likely(), not unlikely().

Oh, wow.  That might have wrecked some branch prediction in
WARN or DEBUG builds..

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v6 1/3] xfs: fix the size of xfs_mode_to_ftype table

[Amir Goldstein]

On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
>> Fix the size of the xfs_mode_to_ftype conversion table,
>> which was too small to handle an invalid value of mode=S_IFMT.
>>
>> Use a convenience macro S_DT(mode) to convert from
>> mode to dirent file type and change the name of the table
>> to xfs_dtype_to_ftype to correctly describe its index values.
>
> This looks like an awful lot of magic.  Would a switch statement
> generate so much worse code?

I doubt it really matters that much, so it's down to a matter of taste.
I personally like the existing map table better than switch if anyone cares ;-)
but I think that the strongest argument in favor of the existing code
is that it works, so no reason to change it.
IMHO, this minor fix and cleanup do not justify switching the code over to
switch statement.

Re: [PATCH v6 1/3] xfs: fix the size of xfs_mode_to_ftype table

[Darrick J. Wong]

On Mon, Jan 09, 2017 at 07:21:25PM +0200, Amir Goldstein wrote:
> On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
> >> Fix the size of the xfs_mode_to_ftype conversion table,
> >> which was too small to handle an invalid value of mode=S_IFMT.
> >>
> >> Use a convenience macro S_DT(mode) to convert from
> >> mode to dirent file type and change the name of the table
> >> to xfs_dtype_to_ftype to correctly describe its index values.
> >
> > This looks like an awful lot of magic.  Would a switch statement
> > generate so much worse code?
> 
> I doubt it really matters that much, so it's down to a matter of taste.
> I personally like the existing map table better than switch if anyone cares ;-)
> but I think that the strongest argument in favor of the existing code
> is that it works, so no reason to change it.
> IMHO, this minor fix and cleanup do not justify switching the code over to
> switch statement.

I can't feed a garbage index to a switch statement and have it fly
off the end of an array; plus we can throw asserts in a default: case.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/3] xfs: fix the size of xfs_mode_to_ftype table

[Amir Goldstein]

On Mon, Jan 9, 2017 at 11:17 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Jan 09, 2017 at 07:21:25PM +0200, Amir Goldstein wrote:
>> On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
>> >> Fix the size of the xfs_mode_to_ftype conversion table,
>> >> which was too small to handle an invalid value of mode=S_IFMT.
>> >>
>> >> Use a convenience macro S_DT(mode) to convert from
>> >> mode to dirent file type and change the name of the table
>> >> to xfs_dtype_to_ftype to correctly describe its index values.
>> >
>> > This looks like an awful lot of magic.  Would a switch statement
>> > generate so much worse code?
>>
>> I doubt it really matters that much, so it's down to a matter of taste.
>> I personally like the existing map table better than switch if anyone cares ;-)
>> but I think that the strongest argument in favor of the existing code
>> is that it works, so no reason to change it.
>> IMHO, this minor fix and cleanup do not justify switching the code over to
>> switch statement.
>
> I can't feed a garbage index to a switch statement and have it fly
> off the end of an array; plus we can throw asserts in a default: case.
>

I think we are in agreement that ASSERT for malformed on-disk data
is a bad idea, as patch 2 demonstrates, and that we are better off
returning -EFSCORRUPTED is such cases.

OK, so just that we are all clear on what I think you are suggesting:

1. retire xfs_mode_to_ftype[] table
2. create helper xfs_mode_to_ftype() that uses a switch statement
    and returns XFS_DIR3_FT_UNKNOWN for invalid modes
3. change xfs_dentry_to_name() to return -EFSCORRUPTED for unknown ftype
    and check return value on call sites where mode come from disk
    (e.g. xfs_generic_create(), xfs_vn_link(), xfs_vn_rename())
4. convert call sites xfs_dentry_to_name(n,d,0) (e.g.
xfs_cleanup_inode()) to use
    a helper xfs_dentry_to_name_unknown(n,d) which sets ftype to unknown
    instead of calling xfs_mode_to_ftype()
5. in xfs_dinode_verify(), sanity check that di_mode is a valid ftype

Should I cook this patch?

Amir.