[PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode

[PATCH v8 0/6] xfs: fixes for malformed on-disk i_mode

[Amir Goldstein]

Darrick,

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

Patches 1-2 are reviewed by Christoph and IMO, you should consider
patch 1 and perhaprs also patch 2 for 4.10 and stable 4.9.

Patches 4-6 replace xfs_mode_to_ftype table with switch statement
per Christoph's suggestion and add sanity checks.

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

Amir.

v8:
- Address review comments by Darrick
- Added Reviewed-by Darrick to patches 4,6

v7:
- Replaced xfs_mode_to_ftype table with switch statement
- Reordered patches so reviewed trivial patches are first
- Added Reviewed-by Christoph to first 2 patches
- Added cleanup patch for xfs_dir2.h
- Added sanity checks for invalid mode in more places

v6:
- Added Reviewed-by Brian for patch 1
- Added patch to address new xfs/348 failures
- Added patch 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

*** BLURB HERE ***

Amir Goldstein (6):
  xfs: make the ASSERT() condition likely
  xfs: sanity check directory inode di_size
  xfs: add missing include dependencies to xfs_dir2.h
  xfs: replace xfs_mode_to_ftype table with switch statement
  xfs: sanity check inode mode when creating new dentry
  xfs: sanity check inode di_mode

 fs/xfs/libxfs/xfs_dir2.c      | 39 ++++++++++++++++++++-------------
 fs/xfs/libxfs/xfs_dir2.h      |  8 ++++---
 fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++++--
 fs/xfs/xfs_iops.c             | 50 ++++++++++++++++++++++++++++++++++---------
 fs/xfs/xfs_linux.h            |  6 +++---
 5 files changed, 80 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH v8 1/6] 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().

Reviewed-by: Christoph Hellwig <hch@lst.de>
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 v8 1/6] xfs: make the ASSERT() condition likely

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 10:24:05AM +0200, Amir Goldstein wrote:
> The ASSERT() condition is the normal case, not the exception,
> so testing the condition should be likely(), not unlikely().

Btw, I think at least this one is 4.10-rc material, although the others
in this series might be candidates as well.

Re: [PATCH v8 1/6] xfs: make the ASSERT() condition likely

[Darrick J. Wong]

On Fri, Jan 13, 2017 at 08:25:04AM +0100, Christoph Hellwig wrote:
> On Wed, Jan 11, 2017 at 10:24:05AM +0200, Amir Goldstein wrote:
> > The ASSERT() condition is the normal case, not the exception,
> > so testing the condition should be likely(), not unlikely().
> 
> Btw, I think at least this one is 4.10-rc material, although the others
> in this series might be candidates as well.

They are, I can crash the kernel with a zero-length directory, which
one of the patches in this series fixes.

Applied, soaking into for-next now.

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

[PATCH v8 2/6] 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, to avoid interacting
with corrupted jusk also when XFS_DEBUG is disabled.

Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 c58d72c..4f7913f 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

Re: [PATCH v8 2/6] xfs: sanity check directory inode di_size

[Darrick J. Wong]

On Wed, Jan 11, 2017 at 10:24:06AM +0200, Amir Goldstein wrote:
> 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, to avoid interacting
> with corrupted jusk also when XFS_DEBUG is disabled.
> 
> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 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 c58d72c..4f7913f 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)

Drat, I just hit this with the fuzzer tests, so I'll pull this in
for rc5.  Good work! :)

--D

>  		return false;
>  
>  	/* only version 3 or greater inodes are extensively verified here */
> -- 
> 2.7.4
> 
> --
> 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

[PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h

[Amir Goldstein]

xfs_dir2.h dereferences some data types in inline functions
and fails to include those type definitions, e.g.:
xfs_dir2_data_aoff_t, struct xfs_da_geometry.

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

diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..72df0dc 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -18,6 +18,9 @@
 #ifndef __XFS_DIR2_H__
 #define __XFS_DIR2_H__
 
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
+
 struct xfs_defer_ops;
 struct xfs_da_args;
 struct xfs_inode;
-- 
2.7.4

Re: [PATCH v8 3/6] xfs: add missing include dependencies to xfs_dir2.h

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 10:24:07AM +0200, Amir Goldstein wrote:
> xfs_dir2.h dereferences some data types in inline functions
> and fails to include those type definitions, e.g.:
> xfs_dir2_data_aoff_t, struct xfs_da_geometry.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I'd normally fold this into the patch that actually exposes the
dependency.  But except for that it looks fine to me:

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

[PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement

[Amir Goldstein]

The size of the xfs_mode_to_ftype[] conversion table
was too small to handle an invalid value of mode=S_IFMT.

Instead of fixing the table size, replace the conversion table
with a conversion helper that uses a switch statement.

Suggested-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c | 36 ++++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_dir2.h |  5 ++---
 fs/xfs/xfs_iops.c        |  2 +-
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 4f7913f..eb64f38 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -36,21 +36,29 @@
 struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
 
 /*
- * @mode, if set, indicates that the type field needs to be set up.
- * This uses the transformation from file mode to DT_* as defined in linux/fs.h
- * for file type specification. This will be propagated into the directory
- * structure if appropriate for the given operation and filesystem config.
+ * Convert inode mode to directory entry filetype
  */
-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_mode_to_ftype(int mode)
+{
+	switch (mode & S_IFMT) {
+	case S_IFREG:
+		return XFS_DIR3_FT_REG_FILE;
+	case S_IFDIR:
+		return XFS_DIR3_FT_DIR;
+	case S_IFCHR:
+		return XFS_DIR3_FT_CHRDEV;
+	case S_IFBLK:
+		return XFS_DIR3_FT_BLKDEV;
+	case S_IFIFO:
+		return XFS_DIR3_FT_FIFO;
+	case S_IFSOCK:
+		return XFS_DIR3_FT_SOCK;
+	case S_IFLNK:
+		return XFS_DIR3_FT_SYMLINK;
+	default:
+		return XFS_DIR3_FT_UNKNOWN;
+	}
+}
 
 /*
  * ASCII case-insensitive (ie. A-Z) support for directories that was
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 72df0dc..d4b77ab 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -35,10 +35,9 @@ struct xfs_dir2_data_unused;
 extern struct xfs_name	xfs_name_dotdot;
 
 /*
- * directory filetype conversion tables.
+ * Convert inode mode to directory entry filetype
  */
-#define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
+extern const unsigned char xfs_mode_to_ftype(int mode);
 
 /*
  * directory operations vector for encode/decode routines
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..821f08d 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_mode_to_ftype(mode);
 }
 
 STATIC void
-- 
2.7.4

Re: [PATCH v8 4/6] xfs: replace xfs_mode_to_ftype table with switch statement

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 10:24:08AM +0200, Amir Goldstein wrote:
> The size of the xfs_mode_to_ftype[] conversion table
> was too small to handle an invalid value of mode=S_IFMT.
> 
> Instead of fixing the table size, replace the conversion table
> with a conversion helper that uses a switch statement.
> 
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks fine,

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

[PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry

[Amir Goldstein]

The helper xfs_dentry_to_name() is used by 2 different
classes of callers: Callers that pass zero mode and don't care
about the returned name.type field and Callers that pass
non zero mode and do care about the name.type field.

Change xfs_dentry_to_name() to not take the mode argument and
change the call sites of the first class to not pass the mode
argument.

Create a new helper xfs_dentry_mode_to_name() which does pass
the mode argument and returns -EFSCORRUPTED if mode is invalid.
Callers that translate non zero mode to on-disk file type now
check the return value and will export the error to user instead
of staging an invalid file type to be written to directory entry.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_iops.c | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 821f08d..22c1615 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -98,12 +98,27 @@ xfs_init_security(
 static void
 xfs_dentry_to_name(
 	struct xfs_name	*namep,
+	struct dentry	*dentry)
+{
+	namep->name = dentry->d_name.name;
+	namep->len = dentry->d_name.len;
+	namep->type = XFS_DIR3_FT_UNKNOWN;
+}
+
+static int
+xfs_dentry_mode_to_name(
+	struct xfs_name	*namep,
 	struct dentry	*dentry,
 	int		mode)
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
 	namep->type = xfs_mode_to_ftype(mode);
+
+	if (unlikely(namep->type == XFS_DIR3_FT_UNKNOWN))
+		return -EFSCORRUPTED;
+
+	return 0;
 }
 
 STATIC void
@@ -119,7 +134,7 @@ xfs_cleanup_inode(
 	 * xfs_init_security we must back out.
 	 * ENOSPC can hit here, among other things.
 	 */
-	xfs_dentry_to_name(&teardown, dentry, 0);
+	xfs_dentry_to_name(&teardown, dentry);
 
 	xfs_remove(XFS_I(dir), &teardown, XFS_I(inode));
 }
@@ -154,8 +169,12 @@ xfs_generic_create(
 	if (error)
 		return error;
 
+	/* Verify mode is valid also for tmpfile case */
+	error = xfs_dentry_mode_to_name(&name, dentry, mode);
+	if (unlikely(error))
+		goto out_free_acl;
+
 	if (!tmpfile) {
-		xfs_dentry_to_name(&name, dentry, mode);
 		error = xfs_create(XFS_I(dir), &name, mode, rdev, &ip);
 	} else {
 		error = xfs_create_tmpfile(XFS_I(dir), dentry, mode, &ip);
@@ -248,7 +267,7 @@ xfs_vn_lookup(
 	if (dentry->d_name.len >= MAXNAMELEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	xfs_dentry_to_name(&name, dentry, 0);
+	xfs_dentry_to_name(&name, dentry);
 	error = xfs_lookup(XFS_I(dir), &name, &cip, NULL);
 	if (unlikely(error)) {
 		if (unlikely(error != -ENOENT))
@@ -275,7 +294,7 @@ xfs_vn_ci_lookup(
 	if (dentry->d_name.len >= MAXNAMELEN)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	xfs_dentry_to_name(&xname, dentry, 0);
+	xfs_dentry_to_name(&xname, dentry);
 	error = xfs_lookup(XFS_I(dir), &xname, &ip, &ci_name);
 	if (unlikely(error)) {
 		if (unlikely(error != -ENOENT))
@@ -310,7 +329,9 @@ xfs_vn_link(
 	struct xfs_name	name;
 	int		error;
 
-	xfs_dentry_to_name(&name, dentry, inode->i_mode);
+	error = xfs_dentry_mode_to_name(&name, dentry, inode->i_mode);
+	if (unlikely(error))
+		return error;
 
 	error = xfs_link(XFS_I(dir), XFS_I(inode), &name);
 	if (unlikely(error))
@@ -329,7 +350,7 @@ xfs_vn_unlink(
 	struct xfs_name	name;
 	int		error;
 
-	xfs_dentry_to_name(&name, dentry, 0);
+	xfs_dentry_to_name(&name, dentry);
 
 	error = xfs_remove(XFS_I(dir), &name, XFS_I(d_inode(dentry)));
 	if (error)
@@ -359,7 +380,9 @@ xfs_vn_symlink(
 
 	mode = S_IFLNK |
 		(irix_symlink_mode ? 0777 & ~current_umask() : S_IRWXUGO);
-	xfs_dentry_to_name(&name, dentry, mode);
+	error = xfs_dentry_mode_to_name(&name, dentry, mode);
+	if (unlikely(error))
+		goto out;
 
 	error = xfs_symlink(XFS_I(dir), &name, symname, mode, &cip);
 	if (unlikely(error))
@@ -395,6 +418,7 @@ xfs_vn_rename(
 {
 	struct inode	*new_inode = d_inode(ndentry);
 	int		omode = 0;
+	int		error;
 	struct xfs_name	oname;
 	struct xfs_name	nname;
 
@@ -405,8 +429,14 @@ xfs_vn_rename(
 	if (flags & RENAME_EXCHANGE)
 		omode = d_inode(ndentry)->i_mode;
 
-	xfs_dentry_to_name(&oname, odentry, omode);
-	xfs_dentry_to_name(&nname, ndentry, d_inode(odentry)->i_mode);
+	error = xfs_dentry_mode_to_name(&oname, odentry, omode);
+	if (omode && unlikely(error))
+		return error;
+
+	error = xfs_dentry_mode_to_name(&nname, ndentry,
+					d_inode(odentry)->i_mode);
+	if (unlikely(error))
+		return error;
 
 	return xfs_rename(XFS_I(odir), &oname, XFS_I(d_inode(odentry)),
 			  XFS_I(ndir), &nname,
-- 
2.7.4

Re: [PATCH v8 5/6] xfs: sanity check inode mode when creating new dentry

[Christoph Hellwig]

Looks fine,

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

Although I wonder if we should simply pass down qstr ( + mode if needed)
and kill off struct xfs_name..

[PATCH v8 6/6] xfs: sanity check inode di_mode

[Amir Goldstein]

Check for invalid file type in xfs_dinode_verify()
and fail to load the inode structure from disk.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 0091ac3..d93f9d9 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -29,6 +29,7 @@
 #include "xfs_icache.h"
 #include "xfs_trans.h"
 #include "xfs_ialloc.h"
+#include "xfs_dir2.h"
 
 /*
  * Check that none of the inode's in the buffer have a next
@@ -398,6 +399,8 @@ xfs_dinode_verify(
 		return false;
 
 	mode = be16_to_cpu(dip->di_mode);
+	if (mode && xfs_mode_to_ftype(mode) == XFS_DIR3_FT_UNKNOWN)
+		return false;
 
 	/* No zero-length symlinks/dirs. */
 	if ((S_ISLNK(mode) || S_ISDIR(mode)) && dip->di_size == 0)
-- 
2.7.4

Re: [PATCH v8 6/6] xfs: sanity check inode di_mode

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 10:24:10AM +0200, Amir Goldstein wrote:
> Check for invalid file type in xfs_dinode_verify()
> and fail to load the inode structure from disk.
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks fine,

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