* [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode
@ 2017-01-10 15:39 Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
Darrick,
This is 7th revision of the fixes for malformed on-disk i_mode.
Patches 1-3 are simple cleanups, some already reviewed and
one (patch 2) was suggested by you.
Patch 4 replaces xfs_mode_to_ftype table with switch statement
per Christoph's suggestion.
Patches 5-6 use the new conversion helper to sanity test mode
loaded from disk and export the error to the user.
Tested with generic/401 with -n ftype=0|1.
Tested with new xfs/348 test with -n ftype=0|1.
Amir.
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
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 | 40 ++++++++++++++++++++++-------------
fs/xfs/libxfs/xfs_dir2.h | 8 ++++---
fs/xfs/libxfs/xfs_inode_buf.c | 10 +++++++--
fs/xfs/xfs_iops.c | 49 ++++++++++++++++++++++++++++++++++---------
fs/xfs/xfs_linux.h | 6 +++---
5 files changed, 80 insertions(+), 33 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v7 1/6] xfs: make the ASSERT() condition likely
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
@ 2017-01-10 15:39 ` Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 2/6] xfs: sanity check directory inode di_size Amir Goldstein
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 2/6] xfs: sanity check directory inode di_size
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein
@ 2017-01-10 15:39 ` Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 2/6] xfs: sanity check directory inode di_size Amir Goldstein
@ 2017-01-10 15:39 ` Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
` (2 preceding siblings ...)
2017-01-10 15:39 ` [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein
@ 2017-01-10 15:39 ` Amir Goldstein
2017-01-10 23:17 ` Darrick J. Wong
2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein
5 siblings, 1 reply; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
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>
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
` (3 preceding siblings ...)
2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein
@ 2017-01-10 15:39 ` Amir Goldstein
2017-01-10 23:15 ` Darrick J. Wong
2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein
5 siblings, 1 reply; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
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 | 47 ++++++++++++++++++++++++++++++++++++++---------
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 821f08d..ef38d0f 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,8 @@ 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);
+ ASSERT(error == 0);
error = xfs_symlink(XFS_I(dir), &name, symname, mode, &cip);
if (unlikely(error))
@@ -395,6 +417,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 +428,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 6/6] xfs: sanity check inode di_mode
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
` (4 preceding siblings ...)
2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein
@ 2017-01-10 15:39 ` Amir Goldstein
2017-01-10 23:16 ` Darrick J. Wong
5 siblings, 1 reply; 12+ messages in thread
From: Amir Goldstein @ 2017-01-10 15:39 UTC (permalink / raw)
To: Darrick J . Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
Check for invalid file type in xfs_dinode_verify()
and fail to load the inode structure from disk.
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry
2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein
@ 2017-01-10 23:15 ` Darrick J. Wong
2017-01-11 5:08 ` Amir Goldstein
0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2017-01-10 23:15 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
On Tue, Jan 10, 2017 at 05:39:16PM +0200, Amir Goldstein wrote:
> 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 | 47 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 821f08d..ef38d0f 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,8 @@ 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);
> + ASSERT(error == 0);
Why not the usual 'if (error) goto out;' here?
--D
>
> error = xfs_symlink(XFS_I(dir), &name, symname, mode, &cip);
> if (unlikely(error))
> @@ -395,6 +417,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 +428,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
>
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 6/6] xfs: sanity check inode di_mode
2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein
@ 2017-01-10 23:16 ` Darrick J. Wong
0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2017-01-10 23:16 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
On Tue, Jan 10, 2017 at 05:39:17PM +0200, Amir Goldstein wrote:
> Check for invalid file type in xfs_dinode_verify()
> and fail to load the inode structure from disk.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> 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
>
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement
2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein
@ 2017-01-10 23:17 ` Darrick J. Wong
0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2017-01-10 23:17 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
On Tue, Jan 10, 2017 at 05:39:15PM +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>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> 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
>
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry
2017-01-10 23:15 ` Darrick J. Wong
@ 2017-01-11 5:08 ` Amir Goldstein
2017-01-11 7:50 ` Darrick J. Wong
0 siblings, 1 reply; 12+ messages in thread
From: Amir Goldstein @ 2017-01-11 5:08 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
On Wed, Jan 11, 2017 at 1:15 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Tue, Jan 10, 2017 at 05:39:16PM +0200, Amir Goldstein wrote:
>> 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>
>> ---
...
>>
>> 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);
>> + ASSERT(error == 0);
>
> Why not the usual 'if (error) goto out;' here?
>
Because mode here is not coming from disk, nor from user input,
so an error from converting S_IFLNK would be a programming bug.
We could also ignore the return value in this case, let me know if you
prefer that.
BTW, process question. Do you need me to re-send patched with
your Reviewed-by, or will you add those when applying?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry
2017-01-11 5:08 ` Amir Goldstein
@ 2017-01-11 7:50 ` Darrick J. Wong
0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2017-01-11 7:50 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Brian Foster, Christoph Hellwig, linux-xfs
On Wed, Jan 11, 2017 at 07:08:57AM +0200, Amir Goldstein wrote:
> On Wed, Jan 11, 2017 at 1:15 AM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> > On Tue, Jan 10, 2017 at 05:39:16PM +0200, Amir Goldstein wrote:
> >> 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>
> >> ---
> ...
> >>
> >> 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);
> >> + ASSERT(error == 0);
> >
> > Why not the usual 'if (error) goto out;' here?
> >
>
> Because mode here is not coming from disk, nor from user input,
> so an error from converting S_IFLNK would be a programming bug.
> We could also ignore the return value in this case, let me know if you
> prefer that.
Nah, it's just eyebrow-raising since it's a deviation from the usual
paradigm.
> BTW, process question. Do you need me to re-send patched with
> your Reviewed-by, or will you add those when applying?
In general I plan to pull the most recent patch mail and add whatever
tags come in as replies, so if you resend a patch please add the tags
yourself.
--D
>
> Thanks,
> Amir.
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-01-11 7:50 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-10 15:39 [PATCH v7 0/6] xfs: fixes for malformed on-disk i_mode Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 1/6] xfs: make the ASSERT() condition likely Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 2/6] xfs: sanity check directory inode di_size Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 3/6] xfs: add missing include dependencies to xfs_dir2.h Amir Goldstein
2017-01-10 15:39 ` [PATCH v7 4/6] xfs: replace xfs_mode_to_ftype table with switch statement Amir Goldstein
2017-01-10 23:17 ` Darrick J. Wong
2017-01-10 15:39 ` [PATCH v7 5/6] xfs: sanity check inode mode when creating new dentry Amir Goldstein
2017-01-10 23:15 ` Darrick J. Wong
2017-01-11 5:08 ` Amir Goldstein
2017-01-11 7:50 ` Darrick J. Wong
2017-01-10 15:39 ` [PATCH v7 6/6] xfs: sanity check inode di_mode Amir Goldstein
2017-01-10 23:16 ` Darrick J. Wong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.