* [PATCH v2] xfs: move the inline directory verifiers
@ 2017-03-27 23:03 Darrick J. Wong
2017-03-28 12:51 ` Brian Foster
2017-03-31 17:24 ` Christoph Hellwig
0 siblings, 2 replies; 15+ messages in thread
From: Darrick J. Wong @ 2017-03-27 23:03 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs, Brian Foster
The inline directory verifiers should be called on the inode fork data,
which means after iformat_local on the read side, and prior to
ifork_flush on the write side. This makes the fork verifier more
consistent with the way buffer verifiers work -- i.e. they will operate
on the memory buffer that the code will be reading and writing directly.
Also revise the verifier function to return -EFSCORRUPTED so that we
don't flood the logs with corruption messages and assert notices.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: get the inode d_ops the proper way
---
fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
fs/xfs/libxfs/xfs_inode_fork.h | 2 +
fs/xfs/xfs_inode.c | 19 ++++++------
5 files changed, 66 insertions(+), 56 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
index eb00bc1..39f8604 100644
--- a/fs/xfs/libxfs/xfs_dir2_priv.h
+++ b/fs/xfs/libxfs/xfs_dir2_priv.h
@@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
-extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
- int size);
+extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
/* xfs_dir2_readdir.c */
extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
index 96b45cd..e84af09 100644
--- a/fs/xfs/libxfs/xfs_dir2_sf.c
+++ b/fs/xfs/libxfs/xfs_dir2_sf.c
@@ -632,36 +632,49 @@ xfs_dir2_sf_check(
/* Verify the consistency of an inline directory. */
int
xfs_dir2_sf_verify(
- struct xfs_mount *mp,
- struct xfs_dir2_sf_hdr *sfp,
- int size)
+ struct xfs_inode *ip)
{
+ struct xfs_mount *mp = ip->i_mount;
+ struct xfs_dir2_sf_hdr *sfp;
struct xfs_dir2_sf_entry *sfep;
struct xfs_dir2_sf_entry *next_sfep;
char *endp;
const struct xfs_dir_ops *dops;
+ struct xfs_ifork *ifp;
xfs_ino_t ino;
int i;
int i8count;
int offset;
+ int size;
+ int error;
__uint8_t filetype;
+ ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
+ /*
+ * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
+ * so we can only trust the mountpoint to have the right pointer.
+ */
dops = xfs_dir_get_ops(mp, NULL);
+ ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+ sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
+ size = ifp->if_bytes;
+
/*
* Give up if the directory is way too short.
*/
- XFS_WANT_CORRUPTED_RETURN(mp, size >
- offsetof(struct xfs_dir2_sf_hdr, parent));
- XFS_WANT_CORRUPTED_RETURN(mp, size >=
- xfs_dir2_sf_hdr_size(sfp->i8count));
+ if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
+ size < xfs_dir2_sf_hdr_size(sfp->i8count))
+ return -EFSCORRUPTED;
endp = (char *)sfp + size;
/* Check .. entry */
ino = dops->sf_get_parent_ino(sfp);
i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
- XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
+ error = xfs_dir_ino_validate(mp, ino);
+ if (error)
+ return error;
offset = dops->data_first_offset;
/* Check all reported entries */
@@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
* Check the fixed-offset parts of the structure are
* within the data buffer.
*/
- XFS_WANT_CORRUPTED_RETURN(mp,
- ((char *)sfep + sizeof(*sfep)) < endp);
+ if (((char *)sfep + sizeof(*sfep)) >= endp)
+ return -EFSCORRUPTED;
/* Don't allow names with known bad length. */
- XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
- XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
+ if (sfep->namelen == 0)
+ return -EFSCORRUPTED;
/*
* Check that the variable-length part of the structure is
@@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
* name component, so nextentry is an acceptable test.
*/
next_sfep = dops->sf_nextentry(sfp, sfep);
- XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
+ if (endp < (char *)next_sfep)
+ return -EFSCORRUPTED;
/* Check that the offsets always increase. */
- XFS_WANT_CORRUPTED_RETURN(mp,
- xfs_dir2_sf_get_offset(sfep) >= offset);
+ if (xfs_dir2_sf_get_offset(sfep) < offset)
+ return -EFSCORRUPTED;
/* Check the inode number. */
ino = dops->sf_get_ino(sfp, sfep);
i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
- XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
+ error = xfs_dir_ino_validate(mp, ino);
+ if (error)
+ return error;
/* Check the file type. */
filetype = dops->sf_get_ftype(sfep);
- XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
+ if (filetype >= XFS_DIR3_FT_MAX)
+ return -EFSCORRUPTED;
offset = xfs_dir2_sf_get_offset(sfep) +
dops->data_entsize(sfep->namelen);
sfep = next_sfep;
}
- XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
- XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
+ if (i8count != sfp->i8count)
+ return -EFSCORRUPTED;
+ if ((void *)sfep != (void *)endp)
+ return -EFSCORRUPTED;
/* Make sure this whole thing ought to be in local format. */
- XFS_WANT_CORRUPTED_RETURN(mp, offset +
- (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
- (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
+ if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
+ (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
+ return -EFSCORRUPTED;
return 0;
}
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 9653e96..8a37efe 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -212,6 +212,16 @@ xfs_iformat_fork(
if (error)
return error;
+ /* Check inline dir contents. */
+ if (S_ISDIR(VFS_I(ip)->i_mode) &&
+ dip->di_format == XFS_DINODE_FMT_LOCAL) {
+ error = xfs_dir2_sf_verify(ip);
+ if (error) {
+ xfs_idestroy_fork(ip, XFS_DATA_FORK);
+ return error;
+ }
+ }
+
if (xfs_is_reflink_inode(ip)) {
ASSERT(ip->i_cowfp == NULL);
xfs_ifork_init_cow(ip);
@@ -322,8 +332,6 @@ xfs_iformat_local(
int whichfork,
int size)
{
- int error;
-
/*
* If the size is unreasonable, then something
* is wrong and we just bail out rather than crash in
@@ -339,14 +347,6 @@ xfs_iformat_local(
return -EFSCORRUPTED;
}
- if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
- error = xfs_dir2_sf_verify(ip->i_mount,
- (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
- size);
- if (error)
- return error;
- }
-
xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
return 0;
}
@@ -867,7 +867,7 @@ xfs_iextents_copy(
* In these cases, the format always takes precedence, because the
* format indicates the current state of the fork.
*/
-int
+void
xfs_iflush_fork(
xfs_inode_t *ip,
xfs_dinode_t *dip,
@@ -877,7 +877,6 @@ xfs_iflush_fork(
char *cp;
xfs_ifork_t *ifp;
xfs_mount_t *mp;
- int error;
static const short brootflag[2] =
{ XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
static const short dataflag[2] =
@@ -886,7 +885,7 @@ xfs_iflush_fork(
{ XFS_ILOG_DEXT, XFS_ILOG_AEXT };
if (!iip)
- return 0;
+ return;
ifp = XFS_IFORK_PTR(ip, whichfork);
/*
* This can happen if we gave up in iformat in an error path,
@@ -894,19 +893,12 @@ xfs_iflush_fork(
*/
if (!ifp) {
ASSERT(whichfork == XFS_ATTR_FORK);
- return 0;
+ return;
}
cp = XFS_DFORK_PTR(dip, whichfork);
mp = ip->i_mount;
switch (XFS_IFORK_FORMAT(ip, whichfork)) {
case XFS_DINODE_FMT_LOCAL:
- if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
- error = xfs_dir2_sf_verify(mp,
- (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
- ifp->if_bytes);
- if (error)
- return error;
- }
if ((iip->ili_fields & dataflag[whichfork]) &&
(ifp->if_bytes > 0)) {
ASSERT(ifp->if_u1.if_data != NULL);
@@ -959,7 +951,6 @@ xfs_iflush_fork(
ASSERT(0);
break;
}
- return 0;
}
/*
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 132dc59..7fb8365 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -140,7 +140,7 @@ typedef struct xfs_ifork {
struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
-int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
+void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
struct xfs_inode_log_item *, int);
void xfs_idestroy_fork(struct xfs_inode *, int);
void xfs_idata_realloc(struct xfs_inode *, int, int);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c7fe2c2..7605d83 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -50,6 +50,7 @@
#include "xfs_log.h"
#include "xfs_bmap_btree.h"
#include "xfs_reflink.h"
+#include "xfs_dir2_priv.h"
kmem_zone_t *xfs_inode_zone;
@@ -3475,7 +3476,6 @@ xfs_iflush_int(
struct xfs_inode_log_item *iip = ip->i_itemp;
struct xfs_dinode *dip;
struct xfs_mount *mp = ip->i_mount;
- int error;
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
ASSERT(xfs_isiflocked(ip));
@@ -3547,6 +3547,12 @@ xfs_iflush_int(
if (ip->i_d.di_version < 3)
ip->i_d.di_flushiter++;
+ /* Check the inline directory data. */
+ if (S_ISDIR(VFS_I(ip)->i_mode) &&
+ ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
+ xfs_dir2_sf_verify(ip))
+ goto corrupt_out;
+
/*
* Copy the dirty parts of the inode into the on-disk inode. We always
* copy out the core of the inode, because if the inode is dirty at all
@@ -3558,14 +3564,9 @@ xfs_iflush_int(
if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
ip->i_d.di_flushiter = 0;
- error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
- if (error)
- return error;
- if (XFS_IFORK_Q(ip)) {
- error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
- if (error)
- return error;
- }
+ xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
+ if (XFS_IFORK_Q(ip))
+ xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
xfs_inobp_check(mp, bp);
/*
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-27 23:03 [PATCH v2] xfs: move the inline directory verifiers Darrick J. Wong
@ 2017-03-28 12:51 ` Brian Foster
2017-03-28 15:00 ` Darrick J. Wong
2017-03-31 17:24 ` Christoph Hellwig
1 sibling, 1 reply; 15+ messages in thread
From: Brian Foster @ 2017-03-28 12:51 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Dave Chinner, xfs
On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> The inline directory verifiers should be called on the inode fork data,
> which means after iformat_local on the read side, and prior to
> ifork_flush on the write side. This makes the fork verifier more
> consistent with the way buffer verifiers work -- i.e. they will operate
> on the memory buffer that the code will be reading and writing directly.
>
> Also revise the verifier function to return -EFSCORRUPTED so that we
> don't flood the logs with corruption messages and assert notices.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: get the inode d_ops the proper way
> ---
What does this apply against?
Brian
> fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
> fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
> fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
> fs/xfs/libxfs/xfs_inode_fork.h | 2 +
> fs/xfs/xfs_inode.c | 19 ++++++------
> 5 files changed, 66 insertions(+), 56 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> index eb00bc1..39f8604 100644
> --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> @@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> - int size);
> +extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
>
> /* xfs_dir2_readdir.c */
> extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 96b45cd..e84af09 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -632,36 +632,49 @@ xfs_dir2_sf_check(
> /* Verify the consistency of an inline directory. */
> int
> xfs_dir2_sf_verify(
> - struct xfs_mount *mp,
> - struct xfs_dir2_sf_hdr *sfp,
> - int size)
> + struct xfs_inode *ip)
> {
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_dir2_sf_hdr *sfp;
> struct xfs_dir2_sf_entry *sfep;
> struct xfs_dir2_sf_entry *next_sfep;
> char *endp;
> const struct xfs_dir_ops *dops;
> + struct xfs_ifork *ifp;
> xfs_ino_t ino;
> int i;
> int i8count;
> int offset;
> + int size;
> + int error;
> __uint8_t filetype;
>
> + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> + /*
> + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
> + * so we can only trust the mountpoint to have the right pointer.
> + */
> dops = xfs_dir_get_ops(mp, NULL);
>
> + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> + size = ifp->if_bytes;
> +
> /*
> * Give up if the directory is way too short.
> */
> - XFS_WANT_CORRUPTED_RETURN(mp, size >
> - offsetof(struct xfs_dir2_sf_hdr, parent));
> - XFS_WANT_CORRUPTED_RETURN(mp, size >=
> - xfs_dir2_sf_hdr_size(sfp->i8count));
> + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
> + size < xfs_dir2_sf_hdr_size(sfp->i8count))
> + return -EFSCORRUPTED;
>
> endp = (char *)sfp + size;
>
> /* Check .. entry */
> ino = dops->sf_get_parent_ino(sfp);
> i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> + error = xfs_dir_ino_validate(mp, ino);
> + if (error)
> + return error;
> offset = dops->data_first_offset;
>
> /* Check all reported entries */
> @@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
> * Check the fixed-offset parts of the structure are
> * within the data buffer.
> */
> - XFS_WANT_CORRUPTED_RETURN(mp,
> - ((char *)sfep + sizeof(*sfep)) < endp);
> + if (((char *)sfep + sizeof(*sfep)) >= endp)
> + return -EFSCORRUPTED;
>
> /* Don't allow names with known bad length. */
> - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> + if (sfep->namelen == 0)
> + return -EFSCORRUPTED;
>
> /*
> * Check that the variable-length part of the structure is
> @@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
> * name component, so nextentry is an acceptable test.
> */
> next_sfep = dops->sf_nextentry(sfp, sfep);
> - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> + if (endp < (char *)next_sfep)
> + return -EFSCORRUPTED;
>
> /* Check that the offsets always increase. */
> - XFS_WANT_CORRUPTED_RETURN(mp,
> - xfs_dir2_sf_get_offset(sfep) >= offset);
> + if (xfs_dir2_sf_get_offset(sfep) < offset)
> + return -EFSCORRUPTED;
>
> /* Check the inode number. */
> ino = dops->sf_get_ino(sfp, sfep);
> i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> + error = xfs_dir_ino_validate(mp, ino);
> + if (error)
> + return error;
>
> /* Check the file type. */
> filetype = dops->sf_get_ftype(sfep);
> - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> + if (filetype >= XFS_DIR3_FT_MAX)
> + return -EFSCORRUPTED;
>
> offset = xfs_dir2_sf_get_offset(sfep) +
> dops->data_entsize(sfep->namelen);
>
> sfep = next_sfep;
> }
> - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> + if (i8count != sfp->i8count)
> + return -EFSCORRUPTED;
> + if ((void *)sfep != (void *)endp)
> + return -EFSCORRUPTED;
>
> /* Make sure this whole thing ought to be in local format. */
> - XFS_WANT_CORRUPTED_RETURN(mp, offset +
> - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
> + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> + return -EFSCORRUPTED;
>
> return 0;
> }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 9653e96..8a37efe 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -212,6 +212,16 @@ xfs_iformat_fork(
> if (error)
> return error;
>
> + /* Check inline dir contents. */
> + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> + dip->di_format == XFS_DINODE_FMT_LOCAL) {
> + error = xfs_dir2_sf_verify(ip);
> + if (error) {
> + xfs_idestroy_fork(ip, XFS_DATA_FORK);
> + return error;
> + }
> + }
> +
> if (xfs_is_reflink_inode(ip)) {
> ASSERT(ip->i_cowfp == NULL);
> xfs_ifork_init_cow(ip);
> @@ -322,8 +332,6 @@ xfs_iformat_local(
> int whichfork,
> int size)
> {
> - int error;
> -
> /*
> * If the size is unreasonable, then something
> * is wrong and we just bail out rather than crash in
> @@ -339,14 +347,6 @@ xfs_iformat_local(
> return -EFSCORRUPTED;
> }
>
> - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> - error = xfs_dir2_sf_verify(ip->i_mount,
> - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> - size);
> - if (error)
> - return error;
> - }
> -
> xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> return 0;
> }
> @@ -867,7 +867,7 @@ xfs_iextents_copy(
> * In these cases, the format always takes precedence, because the
> * format indicates the current state of the fork.
> */
> -int
> +void
> xfs_iflush_fork(
> xfs_inode_t *ip,
> xfs_dinode_t *dip,
> @@ -877,7 +877,6 @@ xfs_iflush_fork(
> char *cp;
> xfs_ifork_t *ifp;
> xfs_mount_t *mp;
> - int error;
> static const short brootflag[2] =
> { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> static const short dataflag[2] =
> @@ -886,7 +885,7 @@ xfs_iflush_fork(
> { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
>
> if (!iip)
> - return 0;
> + return;
> ifp = XFS_IFORK_PTR(ip, whichfork);
> /*
> * This can happen if we gave up in iformat in an error path,
> @@ -894,19 +893,12 @@ xfs_iflush_fork(
> */
> if (!ifp) {
> ASSERT(whichfork == XFS_ATTR_FORK);
> - return 0;
> + return;
> }
> cp = XFS_DFORK_PTR(dip, whichfork);
> mp = ip->i_mount;
> switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> case XFS_DINODE_FMT_LOCAL:
> - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> - error = xfs_dir2_sf_verify(mp,
> - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> - ifp->if_bytes);
> - if (error)
> - return error;
> - }
> if ((iip->ili_fields & dataflag[whichfork]) &&
> (ifp->if_bytes > 0)) {
> ASSERT(ifp->if_u1.if_data != NULL);
> @@ -959,7 +951,6 @@ xfs_iflush_fork(
> ASSERT(0);
> break;
> }
> - return 0;
> }
>
> /*
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 132dc59..7fb8365 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
>
> int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> struct xfs_inode_log_item *, int);
> void xfs_idestroy_fork(struct xfs_inode *, int);
> void xfs_idata_realloc(struct xfs_inode *, int, int);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c7fe2c2..7605d83 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -50,6 +50,7 @@
> #include "xfs_log.h"
> #include "xfs_bmap_btree.h"
> #include "xfs_reflink.h"
> +#include "xfs_dir2_priv.h"
>
> kmem_zone_t *xfs_inode_zone;
>
> @@ -3475,7 +3476,6 @@ xfs_iflush_int(
> struct xfs_inode_log_item *iip = ip->i_itemp;
> struct xfs_dinode *dip;
> struct xfs_mount *mp = ip->i_mount;
> - int error;
>
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> ASSERT(xfs_isiflocked(ip));
> @@ -3547,6 +3547,12 @@ xfs_iflush_int(
> if (ip->i_d.di_version < 3)
> ip->i_d.di_flushiter++;
>
> + /* Check the inline directory data. */
> + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> + ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> + xfs_dir2_sf_verify(ip))
> + goto corrupt_out;
> +
> /*
> * Copy the dirty parts of the inode into the on-disk inode. We always
> * copy out the core of the inode, because if the inode is dirty at all
> @@ -3558,14 +3564,9 @@ xfs_iflush_int(
> if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> ip->i_d.di_flushiter = 0;
>
> - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> - if (error)
> - return error;
> - if (XFS_IFORK_Q(ip)) {
> - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> - if (error)
> - return error;
> - }
> + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> + if (XFS_IFORK_Q(ip))
> + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> xfs_inobp_check(mp, bp);
>
> /*
> --
> 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] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-28 12:51 ` Brian Foster
@ 2017-03-28 15:00 ` Darrick J. Wong
2017-03-28 15:11 ` Brian Foster
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2017-03-28 15:00 UTC (permalink / raw)
To: Brian Foster; +Cc: Dave Chinner, xfs
On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> > The inline directory verifiers should be called on the inode fork data,
> > which means after iformat_local on the read side, and prior to
> > ifork_flush on the write side. This makes the fork verifier more
> > consistent with the way buffer verifiers work -- i.e. they will operate
> > on the memory buffer that the code will be reading and writing directly.
> >
> > Also revise the verifier function to return -EFSCORRUPTED so that we
> > don't flood the logs with corruption messages and assert notices.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: get the inode d_ops the proper way
> > ---
>
> What does this apply against?
It ought to apply against the previous inline dir verifier patch.
--D
>
> Brian
>
> > fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
> > fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
> > fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
> > fs/xfs/libxfs/xfs_inode_fork.h | 2 +
> > fs/xfs/xfs_inode.c | 19 ++++++------
> > 5 files changed, 66 insertions(+), 56 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > index eb00bc1..39f8604 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > @@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> > extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> > extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> > extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> > -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> > - int size);
> > +extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
> >
> > /* xfs_dir2_readdir.c */
> > extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> > index 96b45cd..e84af09 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> > @@ -632,36 +632,49 @@ xfs_dir2_sf_check(
> > /* Verify the consistency of an inline directory. */
> > int
> > xfs_dir2_sf_verify(
> > - struct xfs_mount *mp,
> > - struct xfs_dir2_sf_hdr *sfp,
> > - int size)
> > + struct xfs_inode *ip)
> > {
> > + struct xfs_mount *mp = ip->i_mount;
> > + struct xfs_dir2_sf_hdr *sfp;
> > struct xfs_dir2_sf_entry *sfep;
> > struct xfs_dir2_sf_entry *next_sfep;
> > char *endp;
> > const struct xfs_dir_ops *dops;
> > + struct xfs_ifork *ifp;
> > xfs_ino_t ino;
> > int i;
> > int i8count;
> > int offset;
> > + int size;
> > + int error;
> > __uint8_t filetype;
> >
> > + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> > + /*
> > + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
> > + * so we can only trust the mountpoint to have the right pointer.
> > + */
> > dops = xfs_dir_get_ops(mp, NULL);
> >
> > + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > + size = ifp->if_bytes;
> > +
> > /*
> > * Give up if the directory is way too short.
> > */
> > - XFS_WANT_CORRUPTED_RETURN(mp, size >
> > - offsetof(struct xfs_dir2_sf_hdr, parent));
> > - XFS_WANT_CORRUPTED_RETURN(mp, size >=
> > - xfs_dir2_sf_hdr_size(sfp->i8count));
> > + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
> > + size < xfs_dir2_sf_hdr_size(sfp->i8count))
> > + return -EFSCORRUPTED;
> >
> > endp = (char *)sfp + size;
> >
> > /* Check .. entry */
> > ino = dops->sf_get_parent_ino(sfp);
> > i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > + error = xfs_dir_ino_validate(mp, ino);
> > + if (error)
> > + return error;
> > offset = dops->data_first_offset;
> >
> > /* Check all reported entries */
> > @@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
> > * Check the fixed-offset parts of the structure are
> > * within the data buffer.
> > */
> > - XFS_WANT_CORRUPTED_RETURN(mp,
> > - ((char *)sfep + sizeof(*sfep)) < endp);
> > + if (((char *)sfep + sizeof(*sfep)) >= endp)
> > + return -EFSCORRUPTED;
> >
> > /* Don't allow names with known bad length. */
> > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> > + if (sfep->namelen == 0)
> > + return -EFSCORRUPTED;
> >
> > /*
> > * Check that the variable-length part of the structure is
> > @@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
> > * name component, so nextentry is an acceptable test.
> > */
> > next_sfep = dops->sf_nextentry(sfp, sfep);
> > - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> > + if (endp < (char *)next_sfep)
> > + return -EFSCORRUPTED;
> >
> > /* Check that the offsets always increase. */
> > - XFS_WANT_CORRUPTED_RETURN(mp,
> > - xfs_dir2_sf_get_offset(sfep) >= offset);
> > + if (xfs_dir2_sf_get_offset(sfep) < offset)
> > + return -EFSCORRUPTED;
> >
> > /* Check the inode number. */
> > ino = dops->sf_get_ino(sfp, sfep);
> > i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > + error = xfs_dir_ino_validate(mp, ino);
> > + if (error)
> > + return error;
> >
> > /* Check the file type. */
> > filetype = dops->sf_get_ftype(sfep);
> > - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> > + if (filetype >= XFS_DIR3_FT_MAX)
> > + return -EFSCORRUPTED;
> >
> > offset = xfs_dir2_sf_get_offset(sfep) +
> > dops->data_entsize(sfep->namelen);
> >
> > sfep = next_sfep;
> > }
> > - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> > - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> > + if (i8count != sfp->i8count)
> > + return -EFSCORRUPTED;
> > + if ((void *)sfep != (void *)endp)
> > + return -EFSCORRUPTED;
> >
> > /* Make sure this whole thing ought to be in local format. */
> > - XFS_WANT_CORRUPTED_RETURN(mp, offset +
> > - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
> > + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> > + return -EFSCORRUPTED;
> >
> > return 0;
> > }
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 9653e96..8a37efe 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -212,6 +212,16 @@ xfs_iformat_fork(
> > if (error)
> > return error;
> >
> > + /* Check inline dir contents. */
> > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > + dip->di_format == XFS_DINODE_FMT_LOCAL) {
> > + error = xfs_dir2_sf_verify(ip);
> > + if (error) {
> > + xfs_idestroy_fork(ip, XFS_DATA_FORK);
> > + return error;
> > + }
> > + }
> > +
> > if (xfs_is_reflink_inode(ip)) {
> > ASSERT(ip->i_cowfp == NULL);
> > xfs_ifork_init_cow(ip);
> > @@ -322,8 +332,6 @@ xfs_iformat_local(
> > int whichfork,
> > int size)
> > {
> > - int error;
> > -
> > /*
> > * If the size is unreasonable, then something
> > * is wrong and we just bail out rather than crash in
> > @@ -339,14 +347,6 @@ xfs_iformat_local(
> > return -EFSCORRUPTED;
> > }
> >
> > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > - error = xfs_dir2_sf_verify(ip->i_mount,
> > - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> > - size);
> > - if (error)
> > - return error;
> > - }
> > -
> > xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> > return 0;
> > }
> > @@ -867,7 +867,7 @@ xfs_iextents_copy(
> > * In these cases, the format always takes precedence, because the
> > * format indicates the current state of the fork.
> > */
> > -int
> > +void
> > xfs_iflush_fork(
> > xfs_inode_t *ip,
> > xfs_dinode_t *dip,
> > @@ -877,7 +877,6 @@ xfs_iflush_fork(
> > char *cp;
> > xfs_ifork_t *ifp;
> > xfs_mount_t *mp;
> > - int error;
> > static const short brootflag[2] =
> > { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> > static const short dataflag[2] =
> > @@ -886,7 +885,7 @@ xfs_iflush_fork(
> > { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
> >
> > if (!iip)
> > - return 0;
> > + return;
> > ifp = XFS_IFORK_PTR(ip, whichfork);
> > /*
> > * This can happen if we gave up in iformat in an error path,
> > @@ -894,19 +893,12 @@ xfs_iflush_fork(
> > */
> > if (!ifp) {
> > ASSERT(whichfork == XFS_ATTR_FORK);
> > - return 0;
> > + return;
> > }
> > cp = XFS_DFORK_PTR(dip, whichfork);
> > mp = ip->i_mount;
> > switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> > case XFS_DINODE_FMT_LOCAL:
> > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > - error = xfs_dir2_sf_verify(mp,
> > - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> > - ifp->if_bytes);
> > - if (error)
> > - return error;
> > - }
> > if ((iip->ili_fields & dataflag[whichfork]) &&
> > (ifp->if_bytes > 0)) {
> > ASSERT(ifp->if_u1.if_data != NULL);
> > @@ -959,7 +951,6 @@ xfs_iflush_fork(
> > ASSERT(0);
> > break;
> > }
> > - return 0;
> > }
> >
> > /*
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 132dc59..7fb8365 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> > struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
> >
> > int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> > -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > struct xfs_inode_log_item *, int);
> > void xfs_idestroy_fork(struct xfs_inode *, int);
> > void xfs_idata_realloc(struct xfs_inode *, int, int);
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c7fe2c2..7605d83 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -50,6 +50,7 @@
> > #include "xfs_log.h"
> > #include "xfs_bmap_btree.h"
> > #include "xfs_reflink.h"
> > +#include "xfs_dir2_priv.h"
> >
> > kmem_zone_t *xfs_inode_zone;
> >
> > @@ -3475,7 +3476,6 @@ xfs_iflush_int(
> > struct xfs_inode_log_item *iip = ip->i_itemp;
> > struct xfs_dinode *dip;
> > struct xfs_mount *mp = ip->i_mount;
> > - int error;
> >
> > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > ASSERT(xfs_isiflocked(ip));
> > @@ -3547,6 +3547,12 @@ xfs_iflush_int(
> > if (ip->i_d.di_version < 3)
> > ip->i_d.di_flushiter++;
> >
> > + /* Check the inline directory data. */
> > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > + ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> > + xfs_dir2_sf_verify(ip))
> > + goto corrupt_out;
> > +
> > /*
> > * Copy the dirty parts of the inode into the on-disk inode. We always
> > * copy out the core of the inode, because if the inode is dirty at all
> > @@ -3558,14 +3564,9 @@ xfs_iflush_int(
> > if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> > ip->i_d.di_flushiter = 0;
> >
> > - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > - if (error)
> > - return error;
> > - if (XFS_IFORK_Q(ip)) {
> > - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > - if (error)
> > - return error;
> > - }
> > + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > + if (XFS_IFORK_Q(ip))
> > + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > xfs_inobp_check(mp, bp);
> >
> > /*
> > --
> > 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] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-28 15:00 ` Darrick J. Wong
@ 2017-03-28 15:11 ` Brian Foster
2017-03-28 17:24 ` Brian Foster
0 siblings, 1 reply; 15+ messages in thread
From: Brian Foster @ 2017-03-28 15:11 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Dave Chinner, xfs
On Tue, Mar 28, 2017 at 08:00:47AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> > On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> > > The inline directory verifiers should be called on the inode fork data,
> > > which means after iformat_local on the read side, and prior to
> > > ifork_flush on the write side. This makes the fork verifier more
> > > consistent with the way buffer verifiers work -- i.e. they will operate
> > > on the memory buffer that the code will be reading and writing directly.
> > >
> > > Also revise the verifier function to return -EFSCORRUPTED so that we
> > > don't flood the logs with corruption messages and assert notices.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > v2: get the inode d_ops the proper way
> > > ---
> >
> > What does this apply against?
>
> It ought to apply against the previous inline dir verifier patch.
>
Hm, doesn't apply against [1] for me. Care to just repost these as a
series if the dependent code hasn't been merged yet?
Brian
[1] https://patchwork.kernel.org/patch/9626859/
> --D
>
> >
> > Brian
> >
> > > fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
> > > fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
> > > fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
> > > fs/xfs/libxfs/xfs_inode_fork.h | 2 +
> > > fs/xfs/xfs_inode.c | 19 ++++++------
> > > 5 files changed, 66 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > index eb00bc1..39f8604 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > @@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> > > extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> > > extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> > > extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> > > -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> > > - int size);
> > > +extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
> > >
> > > /* xfs_dir2_readdir.c */
> > > extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > index 96b45cd..e84af09 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > @@ -632,36 +632,49 @@ xfs_dir2_sf_check(
> > > /* Verify the consistency of an inline directory. */
> > > int
> > > xfs_dir2_sf_verify(
> > > - struct xfs_mount *mp,
> > > - struct xfs_dir2_sf_hdr *sfp,
> > > - int size)
> > > + struct xfs_inode *ip)
> > > {
> > > + struct xfs_mount *mp = ip->i_mount;
> > > + struct xfs_dir2_sf_hdr *sfp;
> > > struct xfs_dir2_sf_entry *sfep;
> > > struct xfs_dir2_sf_entry *next_sfep;
> > > char *endp;
> > > const struct xfs_dir_ops *dops;
> > > + struct xfs_ifork *ifp;
> > > xfs_ino_t ino;
> > > int i;
> > > int i8count;
> > > int offset;
> > > + int size;
> > > + int error;
> > > __uint8_t filetype;
> > >
> > > + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> > > + /*
> > > + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
> > > + * so we can only trust the mountpoint to have the right pointer.
> > > + */
> > > dops = xfs_dir_get_ops(mp, NULL);
> > >
> > > + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > > + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > > + size = ifp->if_bytes;
> > > +
> > > /*
> > > * Give up if the directory is way too short.
> > > */
> > > - XFS_WANT_CORRUPTED_RETURN(mp, size >
> > > - offsetof(struct xfs_dir2_sf_hdr, parent));
> > > - XFS_WANT_CORRUPTED_RETURN(mp, size >=
> > > - xfs_dir2_sf_hdr_size(sfp->i8count));
> > > + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
> > > + size < xfs_dir2_sf_hdr_size(sfp->i8count))
> > > + return -EFSCORRUPTED;
> > >
> > > endp = (char *)sfp + size;
> > >
> > > /* Check .. entry */
> > > ino = dops->sf_get_parent_ino(sfp);
> > > i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > + error = xfs_dir_ino_validate(mp, ino);
> > > + if (error)
> > > + return error;
> > > offset = dops->data_first_offset;
> > >
> > > /* Check all reported entries */
> > > @@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
> > > * Check the fixed-offset parts of the structure are
> > > * within the data buffer.
> > > */
> > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > - ((char *)sfep + sizeof(*sfep)) < endp);
> > > + if (((char *)sfep + sizeof(*sfep)) >= endp)
> > > + return -EFSCORRUPTED;
> > >
> > > /* Don't allow names with known bad length. */
> > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> > > + if (sfep->namelen == 0)
> > > + return -EFSCORRUPTED;
> > >
> > > /*
> > > * Check that the variable-length part of the structure is
> > > @@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
> > > * name component, so nextentry is an acceptable test.
> > > */
> > > next_sfep = dops->sf_nextentry(sfp, sfep);
> > > - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> > > + if (endp < (char *)next_sfep)
> > > + return -EFSCORRUPTED;
> > >
> > > /* Check that the offsets always increase. */
> > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > - xfs_dir2_sf_get_offset(sfep) >= offset);
> > > + if (xfs_dir2_sf_get_offset(sfep) < offset)
> > > + return -EFSCORRUPTED;
> > >
> > > /* Check the inode number. */
> > > ino = dops->sf_get_ino(sfp, sfep);
> > > i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > + error = xfs_dir_ino_validate(mp, ino);
> > > + if (error)
> > > + return error;
> > >
> > > /* Check the file type. */
> > > filetype = dops->sf_get_ftype(sfep);
> > > - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> > > + if (filetype >= XFS_DIR3_FT_MAX)
> > > + return -EFSCORRUPTED;
> > >
> > > offset = xfs_dir2_sf_get_offset(sfep) +
> > > dops->data_entsize(sfep->namelen);
> > >
> > > sfep = next_sfep;
> > > }
> > > - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> > > - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> > > + if (i8count != sfp->i8count)
> > > + return -EFSCORRUPTED;
> > > + if ((void *)sfep != (void *)endp)
> > > + return -EFSCORRUPTED;
> > >
> > > /* Make sure this whole thing ought to be in local format. */
> > > - XFS_WANT_CORRUPTED_RETURN(mp, offset +
> > > - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
> > > + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> > > + return -EFSCORRUPTED;
> > >
> > > return 0;
> > > }
> > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > > index 9653e96..8a37efe 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > > @@ -212,6 +212,16 @@ xfs_iformat_fork(
> > > if (error)
> > > return error;
> > >
> > > + /* Check inline dir contents. */
> > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > + dip->di_format == XFS_DINODE_FMT_LOCAL) {
> > > + error = xfs_dir2_sf_verify(ip);
> > > + if (error) {
> > > + xfs_idestroy_fork(ip, XFS_DATA_FORK);
> > > + return error;
> > > + }
> > > + }
> > > +
> > > if (xfs_is_reflink_inode(ip)) {
> > > ASSERT(ip->i_cowfp == NULL);
> > > xfs_ifork_init_cow(ip);
> > > @@ -322,8 +332,6 @@ xfs_iformat_local(
> > > int whichfork,
> > > int size)
> > > {
> > > - int error;
> > > -
> > > /*
> > > * If the size is unreasonable, then something
> > > * is wrong and we just bail out rather than crash in
> > > @@ -339,14 +347,6 @@ xfs_iformat_local(
> > > return -EFSCORRUPTED;
> > > }
> > >
> > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > - error = xfs_dir2_sf_verify(ip->i_mount,
> > > - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> > > - size);
> > > - if (error)
> > > - return error;
> > > - }
> > > -
> > > xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> > > return 0;
> > > }
> > > @@ -867,7 +867,7 @@ xfs_iextents_copy(
> > > * In these cases, the format always takes precedence, because the
> > > * format indicates the current state of the fork.
> > > */
> > > -int
> > > +void
> > > xfs_iflush_fork(
> > > xfs_inode_t *ip,
> > > xfs_dinode_t *dip,
> > > @@ -877,7 +877,6 @@ xfs_iflush_fork(
> > > char *cp;
> > > xfs_ifork_t *ifp;
> > > xfs_mount_t *mp;
> > > - int error;
> > > static const short brootflag[2] =
> > > { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> > > static const short dataflag[2] =
> > > @@ -886,7 +885,7 @@ xfs_iflush_fork(
> > > { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
> > >
> > > if (!iip)
> > > - return 0;
> > > + return;
> > > ifp = XFS_IFORK_PTR(ip, whichfork);
> > > /*
> > > * This can happen if we gave up in iformat in an error path,
> > > @@ -894,19 +893,12 @@ xfs_iflush_fork(
> > > */
> > > if (!ifp) {
> > > ASSERT(whichfork == XFS_ATTR_FORK);
> > > - return 0;
> > > + return;
> > > }
> > > cp = XFS_DFORK_PTR(dip, whichfork);
> > > mp = ip->i_mount;
> > > switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> > > case XFS_DINODE_FMT_LOCAL:
> > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > - error = xfs_dir2_sf_verify(mp,
> > > - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> > > - ifp->if_bytes);
> > > - if (error)
> > > - return error;
> > > - }
> > > if ((iip->ili_fields & dataflag[whichfork]) &&
> > > (ifp->if_bytes > 0)) {
> > > ASSERT(ifp->if_u1.if_data != NULL);
> > > @@ -959,7 +951,6 @@ xfs_iflush_fork(
> > > ASSERT(0);
> > > break;
> > > }
> > > - return 0;
> > > }
> > >
> > > /*
> > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > > index 132dc59..7fb8365 100644
> > > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > > @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> > > struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
> > >
> > > int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> > > -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > struct xfs_inode_log_item *, int);
> > > void xfs_idestroy_fork(struct xfs_inode *, int);
> > > void xfs_idata_realloc(struct xfs_inode *, int, int);
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index c7fe2c2..7605d83 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -50,6 +50,7 @@
> > > #include "xfs_log.h"
> > > #include "xfs_bmap_btree.h"
> > > #include "xfs_reflink.h"
> > > +#include "xfs_dir2_priv.h"
> > >
> > > kmem_zone_t *xfs_inode_zone;
> > >
> > > @@ -3475,7 +3476,6 @@ xfs_iflush_int(
> > > struct xfs_inode_log_item *iip = ip->i_itemp;
> > > struct xfs_dinode *dip;
> > > struct xfs_mount *mp = ip->i_mount;
> > > - int error;
> > >
> > > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > > ASSERT(xfs_isiflocked(ip));
> > > @@ -3547,6 +3547,12 @@ xfs_iflush_int(
> > > if (ip->i_d.di_version < 3)
> > > ip->i_d.di_flushiter++;
> > >
> > > + /* Check the inline directory data. */
> > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > + ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> > > + xfs_dir2_sf_verify(ip))
> > > + goto corrupt_out;
> > > +
> > > /*
> > > * Copy the dirty parts of the inode into the on-disk inode. We always
> > > * copy out the core of the inode, because if the inode is dirty at all
> > > @@ -3558,14 +3564,9 @@ xfs_iflush_int(
> > > if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> > > ip->i_d.di_flushiter = 0;
> > >
> > > - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > - if (error)
> > > - return error;
> > > - if (XFS_IFORK_Q(ip)) {
> > > - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > - if (error)
> > > - return error;
> > > - }
> > > + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > + if (XFS_IFORK_Q(ip))
> > > + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > xfs_inobp_check(mp, bp);
> > >
> > > /*
> > > --
> > > 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
> --
> 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] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-28 15:11 ` Brian Foster
@ 2017-03-28 17:24 ` Brian Foster
2017-03-29 18:21 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: Brian Foster @ 2017-03-28 17:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Dave Chinner, xfs
On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> On Tue, Mar 28, 2017 at 08:00:47AM -0700, Darrick J. Wong wrote:
> > On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> > > On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> > > > The inline directory verifiers should be called on the inode fork data,
> > > > which means after iformat_local on the read side, and prior to
> > > > ifork_flush on the write side. This makes the fork verifier more
> > > > consistent with the way buffer verifiers work -- i.e. they will operate
> > > > on the memory buffer that the code will be reading and writing directly.
> > > >
> > > > Also revise the verifier function to return -EFSCORRUPTED so that we
> > > > don't flood the logs with corruption messages and assert notices.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > > v2: get the inode d_ops the proper way
> > > > ---
> > >
> > > What does this apply against?
> >
> > It ought to apply against the previous inline dir verifier patch.
> >
>
> Hm, doesn't apply against [1] for me. Care to just repost these as a
> series if the dependent code hasn't been merged yet?
>
> Brian
>
> [1] https://patchwork.kernel.org/patch/9626859/
>
I lost track of the fact that the first patch went into -rc and thus
confused myself over where this should apply. This applies to 4.11.0-rc4
and looks fine to me:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> > --D
> >
> > >
> > > Brian
> > >
> > > > fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
> > > > fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
> > > > fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
> > > > fs/xfs/libxfs/xfs_inode_fork.h | 2 +
> > > > fs/xfs/xfs_inode.c | 19 ++++++------
> > > > 5 files changed, 66 insertions(+), 56 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > index eb00bc1..39f8604 100644
> > > > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > @@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> > > > extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> > > > extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> > > > extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> > > > -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> > > > - int size);
> > > > +extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
> > > >
> > > > /* xfs_dir2_readdir.c */
> > > > extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> > > > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > index 96b45cd..e84af09 100644
> > > > --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > @@ -632,36 +632,49 @@ xfs_dir2_sf_check(
> > > > /* Verify the consistency of an inline directory. */
> > > > int
> > > > xfs_dir2_sf_verify(
> > > > - struct xfs_mount *mp,
> > > > - struct xfs_dir2_sf_hdr *sfp,
> > > > - int size)
> > > > + struct xfs_inode *ip)
> > > > {
> > > > + struct xfs_mount *mp = ip->i_mount;
> > > > + struct xfs_dir2_sf_hdr *sfp;
> > > > struct xfs_dir2_sf_entry *sfep;
> > > > struct xfs_dir2_sf_entry *next_sfep;
> > > > char *endp;
> > > > const struct xfs_dir_ops *dops;
> > > > + struct xfs_ifork *ifp;
> > > > xfs_ino_t ino;
> > > > int i;
> > > > int i8count;
> > > > int offset;
> > > > + int size;
> > > > + int error;
> > > > __uint8_t filetype;
> > > >
> > > > + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> > > > + /*
> > > > + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
> > > > + * so we can only trust the mountpoint to have the right pointer.
> > > > + */
> > > > dops = xfs_dir_get_ops(mp, NULL);
> > > >
> > > > + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > > > + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > > > + size = ifp->if_bytes;
> > > > +
> > > > /*
> > > > * Give up if the directory is way too short.
> > > > */
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, size >
> > > > - offsetof(struct xfs_dir2_sf_hdr, parent));
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, size >=
> > > > - xfs_dir2_sf_hdr_size(sfp->i8count));
> > > > + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
> > > > + size < xfs_dir2_sf_hdr_size(sfp->i8count))
> > > > + return -EFSCORRUPTED;
> > > >
> > > > endp = (char *)sfp + size;
> > > >
> > > > /* Check .. entry */
> > > > ino = dops->sf_get_parent_ino(sfp);
> > > > i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > > + error = xfs_dir_ino_validate(mp, ino);
> > > > + if (error)
> > > > + return error;
> > > > offset = dops->data_first_offset;
> > > >
> > > > /* Check all reported entries */
> > > > @@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
> > > > * Check the fixed-offset parts of the structure are
> > > > * within the data buffer.
> > > > */
> > > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > > - ((char *)sfep + sizeof(*sfep)) < endp);
> > > > + if (((char *)sfep + sizeof(*sfep)) >= endp)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > /* Don't allow names with known bad length. */
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> > > > + if (sfep->namelen == 0)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > /*
> > > > * Check that the variable-length part of the structure is
> > > > @@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
> > > > * name component, so nextentry is an acceptable test.
> > > > */
> > > > next_sfep = dops->sf_nextentry(sfp, sfep);
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> > > > + if (endp < (char *)next_sfep)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > /* Check that the offsets always increase. */
> > > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > > - xfs_dir2_sf_get_offset(sfep) >= offset);
> > > > + if (xfs_dir2_sf_get_offset(sfep) < offset)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > /* Check the inode number. */
> > > > ino = dops->sf_get_ino(sfp, sfep);
> > > > i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > > + error = xfs_dir_ino_validate(mp, ino);
> > > > + if (error)
> > > > + return error;
> > > >
> > > > /* Check the file type. */
> > > > filetype = dops->sf_get_ftype(sfep);
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> > > > + if (filetype >= XFS_DIR3_FT_MAX)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > offset = xfs_dir2_sf_get_offset(sfep) +
> > > > dops->data_entsize(sfep->namelen);
> > > >
> > > > sfep = next_sfep;
> > > > }
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> > > > + if (i8count != sfp->i8count)
> > > > + return -EFSCORRUPTED;
> > > > + if ((void *)sfep != (void *)endp)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > /* Make sure this whole thing ought to be in local format. */
> > > > - XFS_WANT_CORRUPTED_RETURN(mp, offset +
> > > > - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > > - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
> > > > + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > > + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> > > > + return -EFSCORRUPTED;
> > > >
> > > > return 0;
> > > > }
> > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > > > index 9653e96..8a37efe 100644
> > > > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > > > @@ -212,6 +212,16 @@ xfs_iformat_fork(
> > > > if (error)
> > > > return error;
> > > >
> > > > + /* Check inline dir contents. */
> > > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > > + dip->di_format == XFS_DINODE_FMT_LOCAL) {
> > > > + error = xfs_dir2_sf_verify(ip);
> > > > + if (error) {
> > > > + xfs_idestroy_fork(ip, XFS_DATA_FORK);
> > > > + return error;
> > > > + }
> > > > + }
> > > > +
> > > > if (xfs_is_reflink_inode(ip)) {
> > > > ASSERT(ip->i_cowfp == NULL);
> > > > xfs_ifork_init_cow(ip);
> > > > @@ -322,8 +332,6 @@ xfs_iformat_local(
> > > > int whichfork,
> > > > int size)
> > > > {
> > > > - int error;
> > > > -
> > > > /*
> > > > * If the size is unreasonable, then something
> > > > * is wrong and we just bail out rather than crash in
> > > > @@ -339,14 +347,6 @@ xfs_iformat_local(
> > > > return -EFSCORRUPTED;
> > > > }
> > > >
> > > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > > - error = xfs_dir2_sf_verify(ip->i_mount,
> > > > - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> > > > - size);
> > > > - if (error)
> > > > - return error;
> > > > - }
> > > > -
> > > > xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> > > > return 0;
> > > > }
> > > > @@ -867,7 +867,7 @@ xfs_iextents_copy(
> > > > * In these cases, the format always takes precedence, because the
> > > > * format indicates the current state of the fork.
> > > > */
> > > > -int
> > > > +void
> > > > xfs_iflush_fork(
> > > > xfs_inode_t *ip,
> > > > xfs_dinode_t *dip,
> > > > @@ -877,7 +877,6 @@ xfs_iflush_fork(
> > > > char *cp;
> > > > xfs_ifork_t *ifp;
> > > > xfs_mount_t *mp;
> > > > - int error;
> > > > static const short brootflag[2] =
> > > > { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> > > > static const short dataflag[2] =
> > > > @@ -886,7 +885,7 @@ xfs_iflush_fork(
> > > > { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
> > > >
> > > > if (!iip)
> > > > - return 0;
> > > > + return;
> > > > ifp = XFS_IFORK_PTR(ip, whichfork);
> > > > /*
> > > > * This can happen if we gave up in iformat in an error path,
> > > > @@ -894,19 +893,12 @@ xfs_iflush_fork(
> > > > */
> > > > if (!ifp) {
> > > > ASSERT(whichfork == XFS_ATTR_FORK);
> > > > - return 0;
> > > > + return;
> > > > }
> > > > cp = XFS_DFORK_PTR(dip, whichfork);
> > > > mp = ip->i_mount;
> > > > switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> > > > case XFS_DINODE_FMT_LOCAL:
> > > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > > - error = xfs_dir2_sf_verify(mp,
> > > > - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> > > > - ifp->if_bytes);
> > > > - if (error)
> > > > - return error;
> > > > - }
> > > > if ((iip->ili_fields & dataflag[whichfork]) &&
> > > > (ifp->if_bytes > 0)) {
> > > > ASSERT(ifp->if_u1.if_data != NULL);
> > > > @@ -959,7 +951,6 @@ xfs_iflush_fork(
> > > > ASSERT(0);
> > > > break;
> > > > }
> > > > - return 0;
> > > > }
> > > >
> > > > /*
> > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > > > index 132dc59..7fb8365 100644
> > > > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > > > @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> > > > struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
> > > >
> > > > int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> > > > -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > > +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > > struct xfs_inode_log_item *, int);
> > > > void xfs_idestroy_fork(struct xfs_inode *, int);
> > > > void xfs_idata_realloc(struct xfs_inode *, int, int);
> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > index c7fe2c2..7605d83 100644
> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > > @@ -50,6 +50,7 @@
> > > > #include "xfs_log.h"
> > > > #include "xfs_bmap_btree.h"
> > > > #include "xfs_reflink.h"
> > > > +#include "xfs_dir2_priv.h"
> > > >
> > > > kmem_zone_t *xfs_inode_zone;
> > > >
> > > > @@ -3475,7 +3476,6 @@ xfs_iflush_int(
> > > > struct xfs_inode_log_item *iip = ip->i_itemp;
> > > > struct xfs_dinode *dip;
> > > > struct xfs_mount *mp = ip->i_mount;
> > > > - int error;
> > > >
> > > > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > > > ASSERT(xfs_isiflocked(ip));
> > > > @@ -3547,6 +3547,12 @@ xfs_iflush_int(
> > > > if (ip->i_d.di_version < 3)
> > > > ip->i_d.di_flushiter++;
> > > >
> > > > + /* Check the inline directory data. */
> > > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > > + ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> > > > + xfs_dir2_sf_verify(ip))
> > > > + goto corrupt_out;
> > > > +
> > > > /*
> > > > * Copy the dirty parts of the inode into the on-disk inode. We always
> > > > * copy out the core of the inode, because if the inode is dirty at all
> > > > @@ -3558,14 +3564,9 @@ xfs_iflush_int(
> > > > if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> > > > ip->i_d.di_flushiter = 0;
> > > >
> > > > - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > > - if (error)
> > > > - return error;
> > > > - if (XFS_IFORK_Q(ip)) {
> > > > - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > > - if (error)
> > > > - return error;
> > > > - }
> > > > + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > > + if (XFS_IFORK_Q(ip))
> > > > + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > > xfs_inobp_check(mp, bp);
> > > >
> > > > /*
> > > > --
> > > > 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
> > --
> > 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
> --
> 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] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-28 17:24 ` Brian Foster
@ 2017-03-29 18:21 ` Darrick J. Wong
2017-03-29 18:52 ` Brian Foster
2017-03-29 22:41 ` Dave Chinner
0 siblings, 2 replies; 15+ messages in thread
From: Darrick J. Wong @ 2017-03-29 18:21 UTC (permalink / raw)
To: Brian Foster; +Cc: Dave Chinner, xfs
On Tue, Mar 28, 2017 at 01:24:44PM -0400, Brian Foster wrote:
> On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> > On Tue, Mar 28, 2017 at 08:00:47AM -0700, Darrick J. Wong wrote:
> > > On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> > > > On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> > > > > The inline directory verifiers should be called on the inode fork data,
> > > > > which means after iformat_local on the read side, and prior to
> > > > > ifork_flush on the write side. This makes the fork verifier more
> > > > > consistent with the way buffer verifiers work -- i.e. they will operate
> > > > > on the memory buffer that the code will be reading and writing directly.
> > > > >
> > > > > Also revise the verifier function to return -EFSCORRUPTED so that we
> > > > > don't flood the logs with corruption messages and assert notices.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > > v2: get the inode d_ops the proper way
> > > > > ---
> > > >
> > > > What does this apply against?
> > >
> > > It ought to apply against the previous inline dir verifier patch.
> > >
> >
> > Hm, doesn't apply against [1] for me. Care to just repost these as a
> > series if the dependent code hasn't been merged yet?
> >
> > Brian
> >
> > [1] https://patchwork.kernel.org/patch/9626859/
> >
>
> I lost track of the fact that the first patch went into -rc and thus
> confused myself over where this should apply. This applies to 4.11.0-rc4
> and looks fine to me:
Does anyone have a problem if I send this to Linus for 4.11-rc5?
I'd rather atone for my sins sooner than later. :)
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Ok, thanks.
--D
>
> > > --D
> > >
> > > >
> > > > Brian
> > > >
> > > > > fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
> > > > > fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
> > > > > fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
> > > > > fs/xfs/libxfs/xfs_inode_fork.h | 2 +
> > > > > fs/xfs/xfs_inode.c | 19 ++++++------
> > > > > 5 files changed, 66 insertions(+), 56 deletions(-)
> > > > >
> > > > > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > > index eb00bc1..39f8604 100644
> > > > > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > > @@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> > > > > extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> > > > > extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> > > > > extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> > > > > -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> > > > > - int size);
> > > > > +extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
> > > > >
> > > > > /* xfs_dir2_readdir.c */
> > > > > extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> > > > > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > > index 96b45cd..e84af09 100644
> > > > > --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > > @@ -632,36 +632,49 @@ xfs_dir2_sf_check(
> > > > > /* Verify the consistency of an inline directory. */
> > > > > int
> > > > > xfs_dir2_sf_verify(
> > > > > - struct xfs_mount *mp,
> > > > > - struct xfs_dir2_sf_hdr *sfp,
> > > > > - int size)
> > > > > + struct xfs_inode *ip)
> > > > > {
> > > > > + struct xfs_mount *mp = ip->i_mount;
> > > > > + struct xfs_dir2_sf_hdr *sfp;
> > > > > struct xfs_dir2_sf_entry *sfep;
> > > > > struct xfs_dir2_sf_entry *next_sfep;
> > > > > char *endp;
> > > > > const struct xfs_dir_ops *dops;
> > > > > + struct xfs_ifork *ifp;
> > > > > xfs_ino_t ino;
> > > > > int i;
> > > > > int i8count;
> > > > > int offset;
> > > > > + int size;
> > > > > + int error;
> > > > > __uint8_t filetype;
> > > > >
> > > > > + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> > > > > + /*
> > > > > + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
> > > > > + * so we can only trust the mountpoint to have the right pointer.
> > > > > + */
> > > > > dops = xfs_dir_get_ops(mp, NULL);
> > > > >
> > > > > + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > > > > + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > > > > + size = ifp->if_bytes;
> > > > > +
> > > > > /*
> > > > > * Give up if the directory is way too short.
> > > > > */
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, size >
> > > > > - offsetof(struct xfs_dir2_sf_hdr, parent));
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, size >=
> > > > > - xfs_dir2_sf_hdr_size(sfp->i8count));
> > > > > + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
> > > > > + size < xfs_dir2_sf_hdr_size(sfp->i8count))
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > endp = (char *)sfp + size;
> > > > >
> > > > > /* Check .. entry */
> > > > > ino = dops->sf_get_parent_ino(sfp);
> > > > > i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > > > + error = xfs_dir_ino_validate(mp, ino);
> > > > > + if (error)
> > > > > + return error;
> > > > > offset = dops->data_first_offset;
> > > > >
> > > > > /* Check all reported entries */
> > > > > @@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
> > > > > * Check the fixed-offset parts of the structure are
> > > > > * within the data buffer.
> > > > > */
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > > > - ((char *)sfep + sizeof(*sfep)) < endp);
> > > > > + if (((char *)sfep + sizeof(*sfep)) >= endp)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > /* Don't allow names with known bad length. */
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> > > > > + if (sfep->namelen == 0)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > /*
> > > > > * Check that the variable-length part of the structure is
> > > > > @@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
> > > > > * name component, so nextentry is an acceptable test.
> > > > > */
> > > > > next_sfep = dops->sf_nextentry(sfp, sfep);
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> > > > > + if (endp < (char *)next_sfep)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > /* Check that the offsets always increase. */
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > > > - xfs_dir2_sf_get_offset(sfep) >= offset);
> > > > > + if (xfs_dir2_sf_get_offset(sfep) < offset)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > /* Check the inode number. */
> > > > > ino = dops->sf_get_ino(sfp, sfep);
> > > > > i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > > > + error = xfs_dir_ino_validate(mp, ino);
> > > > > + if (error)
> > > > > + return error;
> > > > >
> > > > > /* Check the file type. */
> > > > > filetype = dops->sf_get_ftype(sfep);
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> > > > > + if (filetype >= XFS_DIR3_FT_MAX)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > offset = xfs_dir2_sf_get_offset(sfep) +
> > > > > dops->data_entsize(sfep->namelen);
> > > > >
> > > > > sfep = next_sfep;
> > > > > }
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> > > > > + if (i8count != sfp->i8count)
> > > > > + return -EFSCORRUPTED;
> > > > > + if ((void *)sfep != (void *)endp)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > /* Make sure this whole thing ought to be in local format. */
> > > > > - XFS_WANT_CORRUPTED_RETURN(mp, offset +
> > > > > - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > > > - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
> > > > > + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > > > + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> > > > > + return -EFSCORRUPTED;
> > > > >
> > > > > return 0;
> > > > > }
> > > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > > > > index 9653e96..8a37efe 100644
> > > > > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > > > > @@ -212,6 +212,16 @@ xfs_iformat_fork(
> > > > > if (error)
> > > > > return error;
> > > > >
> > > > > + /* Check inline dir contents. */
> > > > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > > > + dip->di_format == XFS_DINODE_FMT_LOCAL) {
> > > > > + error = xfs_dir2_sf_verify(ip);
> > > > > + if (error) {
> > > > > + xfs_idestroy_fork(ip, XFS_DATA_FORK);
> > > > > + return error;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > if (xfs_is_reflink_inode(ip)) {
> > > > > ASSERT(ip->i_cowfp == NULL);
> > > > > xfs_ifork_init_cow(ip);
> > > > > @@ -322,8 +332,6 @@ xfs_iformat_local(
> > > > > int whichfork,
> > > > > int size)
> > > > > {
> > > > > - int error;
> > > > > -
> > > > > /*
> > > > > * If the size is unreasonable, then something
> > > > > * is wrong and we just bail out rather than crash in
> > > > > @@ -339,14 +347,6 @@ xfs_iformat_local(
> > > > > return -EFSCORRUPTED;
> > > > > }
> > > > >
> > > > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > > > - error = xfs_dir2_sf_verify(ip->i_mount,
> > > > > - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> > > > > - size);
> > > > > - if (error)
> > > > > - return error;
> > > > > - }
> > > > > -
> > > > > xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> > > > > return 0;
> > > > > }
> > > > > @@ -867,7 +867,7 @@ xfs_iextents_copy(
> > > > > * In these cases, the format always takes precedence, because the
> > > > > * format indicates the current state of the fork.
> > > > > */
> > > > > -int
> > > > > +void
> > > > > xfs_iflush_fork(
> > > > > xfs_inode_t *ip,
> > > > > xfs_dinode_t *dip,
> > > > > @@ -877,7 +877,6 @@ xfs_iflush_fork(
> > > > > char *cp;
> > > > > xfs_ifork_t *ifp;
> > > > > xfs_mount_t *mp;
> > > > > - int error;
> > > > > static const short brootflag[2] =
> > > > > { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> > > > > static const short dataflag[2] =
> > > > > @@ -886,7 +885,7 @@ xfs_iflush_fork(
> > > > > { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
> > > > >
> > > > > if (!iip)
> > > > > - return 0;
> > > > > + return;
> > > > > ifp = XFS_IFORK_PTR(ip, whichfork);
> > > > > /*
> > > > > * This can happen if we gave up in iformat in an error path,
> > > > > @@ -894,19 +893,12 @@ xfs_iflush_fork(
> > > > > */
> > > > > if (!ifp) {
> > > > > ASSERT(whichfork == XFS_ATTR_FORK);
> > > > > - return 0;
> > > > > + return;
> > > > > }
> > > > > cp = XFS_DFORK_PTR(dip, whichfork);
> > > > > mp = ip->i_mount;
> > > > > switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> > > > > case XFS_DINODE_FMT_LOCAL:
> > > > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > > > - error = xfs_dir2_sf_verify(mp,
> > > > > - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> > > > > - ifp->if_bytes);
> > > > > - if (error)
> > > > > - return error;
> > > > > - }
> > > > > if ((iip->ili_fields & dataflag[whichfork]) &&
> > > > > (ifp->if_bytes > 0)) {
> > > > > ASSERT(ifp->if_u1.if_data != NULL);
> > > > > @@ -959,7 +951,6 @@ xfs_iflush_fork(
> > > > > ASSERT(0);
> > > > > break;
> > > > > }
> > > > > - return 0;
> > > > > }
> > > > >
> > > > > /*
> > > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > > > > index 132dc59..7fb8365 100644
> > > > > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > > > > @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> > > > > struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
> > > > >
> > > > > int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> > > > > -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > > > +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > > > struct xfs_inode_log_item *, int);
> > > > > void xfs_idestroy_fork(struct xfs_inode *, int);
> > > > > void xfs_idata_realloc(struct xfs_inode *, int, int);
> > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > > index c7fe2c2..7605d83 100644
> > > > > --- a/fs/xfs/xfs_inode.c
> > > > > +++ b/fs/xfs/xfs_inode.c
> > > > > @@ -50,6 +50,7 @@
> > > > > #include "xfs_log.h"
> > > > > #include "xfs_bmap_btree.h"
> > > > > #include "xfs_reflink.h"
> > > > > +#include "xfs_dir2_priv.h"
> > > > >
> > > > > kmem_zone_t *xfs_inode_zone;
> > > > >
> > > > > @@ -3475,7 +3476,6 @@ xfs_iflush_int(
> > > > > struct xfs_inode_log_item *iip = ip->i_itemp;
> > > > > struct xfs_dinode *dip;
> > > > > struct xfs_mount *mp = ip->i_mount;
> > > > > - int error;
> > > > >
> > > > > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > > > > ASSERT(xfs_isiflocked(ip));
> > > > > @@ -3547,6 +3547,12 @@ xfs_iflush_int(
> > > > > if (ip->i_d.di_version < 3)
> > > > > ip->i_d.di_flushiter++;
> > > > >
> > > > > + /* Check the inline directory data. */
> > > > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > > > + ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> > > > > + xfs_dir2_sf_verify(ip))
> > > > > + goto corrupt_out;
> > > > > +
> > > > > /*
> > > > > * Copy the dirty parts of the inode into the on-disk inode. We always
> > > > > * copy out the core of the inode, because if the inode is dirty at all
> > > > > @@ -3558,14 +3564,9 @@ xfs_iflush_int(
> > > > > if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> > > > > ip->i_d.di_flushiter = 0;
> > > > >
> > > > > - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > > > - if (error)
> > > > > - return error;
> > > > > - if (XFS_IFORK_Q(ip)) {
> > > > > - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > > > - if (error)
> > > > > - return error;
> > > > > - }
> > > > > + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > > > + if (XFS_IFORK_Q(ip))
> > > > > + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > > > xfs_inobp_check(mp, bp);
> > > > >
> > > > > /*
> > > > > --
> > > > > 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
> > > --
> > > 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
> > --
> > 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
> --
> 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] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-29 18:21 ` Darrick J. Wong
@ 2017-03-29 18:52 ` Brian Foster
2017-03-29 22:41 ` Dave Chinner
1 sibling, 0 replies; 15+ messages in thread
From: Brian Foster @ 2017-03-29 18:52 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Dave Chinner, xfs
On Wed, Mar 29, 2017 at 11:21:57AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 28, 2017 at 01:24:44PM -0400, Brian Foster wrote:
> > On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> > > On Tue, Mar 28, 2017 at 08:00:47AM -0700, Darrick J. Wong wrote:
> > > > On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> > > > > On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> > > > > > The inline directory verifiers should be called on the inode fork data,
> > > > > > which means after iformat_local on the read side, and prior to
> > > > > > ifork_flush on the write side. This makes the fork verifier more
> > > > > > consistent with the way buffer verifiers work -- i.e. they will operate
> > > > > > on the memory buffer that the code will be reading and writing directly.
> > > > > >
> > > > > > Also revise the verifier function to return -EFSCORRUPTED so that we
> > > > > > don't flood the logs with corruption messages and assert notices.
> > > > > >
> > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > ---
> > > > > > v2: get the inode d_ops the proper way
> > > > > > ---
> > > > >
> > > > > What does this apply against?
> > > >
> > > > It ought to apply against the previous inline dir verifier patch.
> > > >
> > >
> > > Hm, doesn't apply against [1] for me. Care to just repost these as a
> > > series if the dependent code hasn't been merged yet?
> > >
> > > Brian
> > >
> > > [1] https://patchwork.kernel.org/patch/9626859/
> > >
> >
> > I lost track of the fact that the first patch went into -rc and thus
> > confused myself over where this should apply. This applies to 4.11.0-rc4
> > and looks fine to me:
>
> Does anyone have a problem if I send this to Linus for 4.11-rc5?
> I'd rather atone for my sins sooner than later. :)
>
No issues from me. You might want to give Dave 24h or so to ack/nack
though..
Brian
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> Ok, thanks.
>
> --D
>
> >
> > > > --D
> > > >
> > > > >
> > > > > Brian
> > > > >
> > > > > > fs/xfs/libxfs/xfs_dir2_priv.h | 3 +-
> > > > > > fs/xfs/libxfs/xfs_dir2_sf.c | 63 ++++++++++++++++++++++++++--------------
> > > > > > fs/xfs/libxfs/xfs_inode_fork.c | 35 ++++++++--------------
> > > > > > fs/xfs/libxfs/xfs_inode_fork.h | 2 +
> > > > > > fs/xfs/xfs_inode.c | 19 ++++++------
> > > > > > 5 files changed, 66 insertions(+), 56 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > > > index eb00bc1..39f8604 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > > > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > > > > @@ -125,8 +125,7 @@ extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> > > > > > extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> > > > > > extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> > > > > > extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> > > > > > -extern int xfs_dir2_sf_verify(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *sfp,
> > > > > > - int size);
> > > > > > +extern int xfs_dir2_sf_verify(struct xfs_inode *ip);
> > > > > >
> > > > > > /* xfs_dir2_readdir.c */
> > > > > > extern int xfs_readdir(struct xfs_inode *dp, struct dir_context *ctx,
> > > > > > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > > > index 96b45cd..e84af09 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > > > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > > > > @@ -632,36 +632,49 @@ xfs_dir2_sf_check(
> > > > > > /* Verify the consistency of an inline directory. */
> > > > > > int
> > > > > > xfs_dir2_sf_verify(
> > > > > > - struct xfs_mount *mp,
> > > > > > - struct xfs_dir2_sf_hdr *sfp,
> > > > > > - int size)
> > > > > > + struct xfs_inode *ip)
> > > > > > {
> > > > > > + struct xfs_mount *mp = ip->i_mount;
> > > > > > + struct xfs_dir2_sf_hdr *sfp;
> > > > > > struct xfs_dir2_sf_entry *sfep;
> > > > > > struct xfs_dir2_sf_entry *next_sfep;
> > > > > > char *endp;
> > > > > > const struct xfs_dir_ops *dops;
> > > > > > + struct xfs_ifork *ifp;
> > > > > > xfs_ino_t ino;
> > > > > > int i;
> > > > > > int i8count;
> > > > > > int offset;
> > > > > > + int size;
> > > > > > + int error;
> > > > > > __uint8_t filetype;
> > > > > >
> > > > > > + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> > > > > > + /*
> > > > > > + * xfs_iread calls us before xfs_setup_inode sets up ip->d_ops,
> > > > > > + * so we can only trust the mountpoint to have the right pointer.
> > > > > > + */
> > > > > > dops = xfs_dir_get_ops(mp, NULL);
> > > > > >
> > > > > > + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > > > > > + sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> > > > > > + size = ifp->if_bytes;
> > > > > > +
> > > > > > /*
> > > > > > * Give up if the directory is way too short.
> > > > > > */
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, size >
> > > > > > - offsetof(struct xfs_dir2_sf_hdr, parent));
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, size >=
> > > > > > - xfs_dir2_sf_hdr_size(sfp->i8count));
> > > > > > + if (size <= offsetof(struct xfs_dir2_sf_hdr, parent) ||
> > > > > > + size < xfs_dir2_sf_hdr_size(sfp->i8count))
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > endp = (char *)sfp + size;
> > > > > >
> > > > > > /* Check .. entry */
> > > > > > ino = dops->sf_get_parent_ino(sfp);
> > > > > > i8count = ino > XFS_DIR2_MAX_SHORT_INUM;
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > > > > + error = xfs_dir_ino_validate(mp, ino);
> > > > > > + if (error)
> > > > > > + return error;
> > > > > > offset = dops->data_first_offset;
> > > > > >
> > > > > > /* Check all reported entries */
> > > > > > @@ -672,12 +685,12 @@ xfs_dir2_sf_verify(
> > > > > > * Check the fixed-offset parts of the structure are
> > > > > > * within the data buffer.
> > > > > > */
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > > > > - ((char *)sfep + sizeof(*sfep)) < endp);
> > > > > > + if (((char *)sfep + sizeof(*sfep)) >= endp)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > /* Don't allow names with known bad length. */
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen > 0);
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, sfep->namelen < MAXNAMELEN);
> > > > > > + if (sfep->namelen == 0)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > /*
> > > > > > * Check that the variable-length part of the structure is
> > > > > > @@ -685,33 +698,39 @@ xfs_dir2_sf_verify(
> > > > > > * name component, so nextentry is an acceptable test.
> > > > > > */
> > > > > > next_sfep = dops->sf_nextentry(sfp, sfep);
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, endp >= (char *)next_sfep);
> > > > > > + if (endp < (char *)next_sfep)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > /* Check that the offsets always increase. */
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp,
> > > > > > - xfs_dir2_sf_get_offset(sfep) >= offset);
> > > > > > + if (xfs_dir2_sf_get_offset(sfep) < offset)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > /* Check the inode number. */
> > > > > > ino = dops->sf_get_ino(sfp, sfep);
> > > > > > i8count += ino > XFS_DIR2_MAX_SHORT_INUM;
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, !xfs_dir_ino_validate(mp, ino));
> > > > > > + error = xfs_dir_ino_validate(mp, ino);
> > > > > > + if (error)
> > > > > > + return error;
> > > > > >
> > > > > > /* Check the file type. */
> > > > > > filetype = dops->sf_get_ftype(sfep);
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, filetype < XFS_DIR3_FT_MAX);
> > > > > > + if (filetype >= XFS_DIR3_FT_MAX)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > offset = xfs_dir2_sf_get_offset(sfep) +
> > > > > > dops->data_entsize(sfep->namelen);
> > > > > >
> > > > > > sfep = next_sfep;
> > > > > > }
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, i8count == sfp->i8count);
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, (void *)sfep == (void *)endp);
> > > > > > + if (i8count != sfp->i8count)
> > > > > > + return -EFSCORRUPTED;
> > > > > > + if ((void *)sfep != (void *)endp)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > /* Make sure this whole thing ought to be in local format. */
> > > > > > - XFS_WANT_CORRUPTED_RETURN(mp, offset +
> > > > > > - (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > > > > - (uint)sizeof(xfs_dir2_block_tail_t) <= mp->m_dir_geo->blksize);
> > > > > > + if (offset + (sfp->count + 2) * (uint)sizeof(xfs_dir2_leaf_entry_t) +
> > > > > > + (uint)sizeof(xfs_dir2_block_tail_t) > mp->m_dir_geo->blksize)
> > > > > > + return -EFSCORRUPTED;
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > > > > > index 9653e96..8a37efe 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > > > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > > > > > @@ -212,6 +212,16 @@ xfs_iformat_fork(
> > > > > > if (error)
> > > > > > return error;
> > > > > >
> > > > > > + /* Check inline dir contents. */
> > > > > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > > > > + dip->di_format == XFS_DINODE_FMT_LOCAL) {
> > > > > > + error = xfs_dir2_sf_verify(ip);
> > > > > > + if (error) {
> > > > > > + xfs_idestroy_fork(ip, XFS_DATA_FORK);
> > > > > > + return error;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > if (xfs_is_reflink_inode(ip)) {
> > > > > > ASSERT(ip->i_cowfp == NULL);
> > > > > > xfs_ifork_init_cow(ip);
> > > > > > @@ -322,8 +332,6 @@ xfs_iformat_local(
> > > > > > int whichfork,
> > > > > > int size)
> > > > > > {
> > > > > > - int error;
> > > > > > -
> > > > > > /*
> > > > > > * If the size is unreasonable, then something
> > > > > > * is wrong and we just bail out rather than crash in
> > > > > > @@ -339,14 +347,6 @@ xfs_iformat_local(
> > > > > > return -EFSCORRUPTED;
> > > > > > }
> > > > > >
> > > > > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > > > > - error = xfs_dir2_sf_verify(ip->i_mount,
> > > > > > - (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip),
> > > > > > - size);
> > > > > > - if (error)
> > > > > > - return error;
> > > > > > - }
> > > > > > -
> > > > > > xfs_init_local_fork(ip, whichfork, XFS_DFORK_PTR(dip, whichfork), size);
> > > > > > return 0;
> > > > > > }
> > > > > > @@ -867,7 +867,7 @@ xfs_iextents_copy(
> > > > > > * In these cases, the format always takes precedence, because the
> > > > > > * format indicates the current state of the fork.
> > > > > > */
> > > > > > -int
> > > > > > +void
> > > > > > xfs_iflush_fork(
> > > > > > xfs_inode_t *ip,
> > > > > > xfs_dinode_t *dip,
> > > > > > @@ -877,7 +877,6 @@ xfs_iflush_fork(
> > > > > > char *cp;
> > > > > > xfs_ifork_t *ifp;
> > > > > > xfs_mount_t *mp;
> > > > > > - int error;
> > > > > > static const short brootflag[2] =
> > > > > > { XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
> > > > > > static const short dataflag[2] =
> > > > > > @@ -886,7 +885,7 @@ xfs_iflush_fork(
> > > > > > { XFS_ILOG_DEXT, XFS_ILOG_AEXT };
> > > > > >
> > > > > > if (!iip)
> > > > > > - return 0;
> > > > > > + return;
> > > > > > ifp = XFS_IFORK_PTR(ip, whichfork);
> > > > > > /*
> > > > > > * This can happen if we gave up in iformat in an error path,
> > > > > > @@ -894,19 +893,12 @@ xfs_iflush_fork(
> > > > > > */
> > > > > > if (!ifp) {
> > > > > > ASSERT(whichfork == XFS_ATTR_FORK);
> > > > > > - return 0;
> > > > > > + return;
> > > > > > }
> > > > > > cp = XFS_DFORK_PTR(dip, whichfork);
> > > > > > mp = ip->i_mount;
> > > > > > switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> > > > > > case XFS_DINODE_FMT_LOCAL:
> > > > > > - if (S_ISDIR(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK) {
> > > > > > - error = xfs_dir2_sf_verify(mp,
> > > > > > - (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data,
> > > > > > - ifp->if_bytes);
> > > > > > - if (error)
> > > > > > - return error;
> > > > > > - }
> > > > > > if ((iip->ili_fields & dataflag[whichfork]) &&
> > > > > > (ifp->if_bytes > 0)) {
> > > > > > ASSERT(ifp->if_u1.if_data != NULL);
> > > > > > @@ -959,7 +951,6 @@ xfs_iflush_fork(
> > > > > > ASSERT(0);
> > > > > > break;
> > > > > > }
> > > > > > - return 0;
> > > > > > }
> > > > > >
> > > > > > /*
> > > > > > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > > > > > index 132dc59..7fb8365 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > > > > > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > > > > > @@ -140,7 +140,7 @@ typedef struct xfs_ifork {
> > > > > > struct xfs_ifork *xfs_iext_state_to_fork(struct xfs_inode *ip, int state);
> > > > > >
> > > > > > int xfs_iformat_fork(struct xfs_inode *, struct xfs_dinode *);
> > > > > > -int xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > > > > +void xfs_iflush_fork(struct xfs_inode *, struct xfs_dinode *,
> > > > > > struct xfs_inode_log_item *, int);
> > > > > > void xfs_idestroy_fork(struct xfs_inode *, int);
> > > > > > void xfs_idata_realloc(struct xfs_inode *, int, int);
> > > > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > > > > index c7fe2c2..7605d83 100644
> > > > > > --- a/fs/xfs/xfs_inode.c
> > > > > > +++ b/fs/xfs/xfs_inode.c
> > > > > > @@ -50,6 +50,7 @@
> > > > > > #include "xfs_log.h"
> > > > > > #include "xfs_bmap_btree.h"
> > > > > > #include "xfs_reflink.h"
> > > > > > +#include "xfs_dir2_priv.h"
> > > > > >
> > > > > > kmem_zone_t *xfs_inode_zone;
> > > > > >
> > > > > > @@ -3475,7 +3476,6 @@ xfs_iflush_int(
> > > > > > struct xfs_inode_log_item *iip = ip->i_itemp;
> > > > > > struct xfs_dinode *dip;
> > > > > > struct xfs_mount *mp = ip->i_mount;
> > > > > > - int error;
> > > > > >
> > > > > > ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> > > > > > ASSERT(xfs_isiflocked(ip));
> > > > > > @@ -3547,6 +3547,12 @@ xfs_iflush_int(
> > > > > > if (ip->i_d.di_version < 3)
> > > > > > ip->i_d.di_flushiter++;
> > > > > >
> > > > > > + /* Check the inline directory data. */
> > > > > > + if (S_ISDIR(VFS_I(ip)->i_mode) &&
> > > > > > + ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> > > > > > + xfs_dir2_sf_verify(ip))
> > > > > > + goto corrupt_out;
> > > > > > +
> > > > > > /*
> > > > > > * Copy the dirty parts of the inode into the on-disk inode. We always
> > > > > > * copy out the core of the inode, because if the inode is dirty at all
> > > > > > @@ -3558,14 +3564,9 @@ xfs_iflush_int(
> > > > > > if (ip->i_d.di_flushiter == DI_MAX_FLUSH)
> > > > > > ip->i_d.di_flushiter = 0;
> > > > > >
> > > > > > - error = xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > > > > - if (error)
> > > > > > - return error;
> > > > > > - if (XFS_IFORK_Q(ip)) {
> > > > > > - error = xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > > > > - if (error)
> > > > > > - return error;
> > > > > > - }
> > > > > > + xfs_iflush_fork(ip, dip, iip, XFS_DATA_FORK);
> > > > > > + if (XFS_IFORK_Q(ip))
> > > > > > + xfs_iflush_fork(ip, dip, iip, XFS_ATTR_FORK);
> > > > > > xfs_inobp_check(mp, bp);
> > > > > >
> > > > > > /*
> > > > > > --
> > > > > > 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
> > > > --
> > > > 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
> > > --
> > > 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
> > --
> > 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
> --
> 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] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-29 18:21 ` Darrick J. Wong
2017-03-29 18:52 ` Brian Foster
@ 2017-03-29 22:41 ` Dave Chinner
2017-03-31 16:07 ` Christoph Hellwig
1 sibling, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2017-03-29 22:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Brian Foster, xfs
On Wed, Mar 29, 2017 at 11:21:57AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 28, 2017 at 01:24:44PM -0400, Brian Foster wrote:
> > On Tue, Mar 28, 2017 at 11:11:05AM -0400, Brian Foster wrote:
> > > On Tue, Mar 28, 2017 at 08:00:47AM -0700, Darrick J. Wong wrote:
> > > > On Tue, Mar 28, 2017 at 08:51:05AM -0400, Brian Foster wrote:
> > > > > On Mon, Mar 27, 2017 at 04:03:15PM -0700, Darrick J. Wong wrote:
> > > > > > The inline directory verifiers should be called on the inode fork data,
> > > > > > which means after iformat_local on the read side, and prior to
> > > > > > ifork_flush on the write side. This makes the fork verifier more
> > > > > > consistent with the way buffer verifiers work -- i.e. they will operate
> > > > > > on the memory buffer that the code will be reading and writing directly.
> > > > > >
> > > > > > Also revise the verifier function to return -EFSCORRUPTED so that we
> > > > > > don't flood the logs with corruption messages and assert notices.
> > > > > >
> > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > ---
> > > > > > v2: get the inode d_ops the proper way
> > > > > > ---
> > > > >
> > > > > What does this apply against?
> > > >
> > > > It ought to apply against the previous inline dir verifier patch.
> > > >
> > >
> > > Hm, doesn't apply against [1] for me. Care to just repost these as a
> > > series if the dependent code hasn't been merged yet?
> > >
> > > Brian
> > >
> > > [1] https://patchwork.kernel.org/patch/9626859/
> > >
> >
> > I lost track of the fact that the first patch went into -rc and thus
> > confused myself over where this should apply. This applies to 4.11.0-rc4
> > and looks fine to me:
>
> Does anyone have a problem if I send this to Linus for 4.11-rc5?
> I'd rather atone for my sins sooner than later. :)
There's no urgency required here - it's just a cleanup patch. The
code in the tree works fine, so why risk adding regressions
at a late stage? Just add it to the for-next queue and let it soak
until the merge window.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-29 22:41 ` Dave Chinner
@ 2017-03-31 16:07 ` Christoph Hellwig
2017-03-31 16:24 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2017-03-31 16:07 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, Brian Foster, xfs
On Thu, Mar 30, 2017 at 09:41:10AM +1100, Dave Chinner wrote:
> > > I lost track of the fact that the first patch went into -rc and thus
> > > confused myself over where this should apply. This applies to 4.11.0-rc4
> > > and looks fine to me:
> >
> > Does anyone have a problem if I send this to Linus for 4.11-rc5?
> > I'd rather atone for my sins sooner than later. :)
>
> There's no urgency required here - it's just a cleanup patch. The
> code in the tree works fine, so why risk adding regressions
> at a late stage? Just add it to the for-next queue and let it soak
> until the merge window.
Is current Linus tree ok? I'm pretty sure a recent Linus tree fell
over when running the dir fuzzers for me. Which commit would the
latest actual fix be?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-31 16:07 ` Christoph Hellwig
@ 2017-03-31 16:24 ` Darrick J. Wong
2017-03-31 16:28 ` Christoph Hellwig
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2017-03-31 16:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Dave Chinner, Brian Foster, xfs
On Fri, Mar 31, 2017 at 09:07:43AM -0700, Christoph Hellwig wrote:
> On Thu, Mar 30, 2017 at 09:41:10AM +1100, Dave Chinner wrote:
> > > > I lost track of the fact that the first patch went into -rc and thus
> > > > confused myself over where this should apply. This applies to 4.11.0-rc4
> > > > and looks fine to me:
> > >
> > > Does anyone have a problem if I send this to Linus for 4.11-rc5?
> > > I'd rather atone for my sins sooner than later. :)
> >
> > There's no urgency required here - it's just a cleanup patch. The
> > code in the tree works fine, so why risk adding regressions
> > at a late stage? Just add it to the for-next queue and let it soak
> > until the merge window.
>
> Is current Linus tree ok? I'm pretty sure a recent Linus tree fell
> over when running the dir fuzzers for me. Which commit would the
> latest actual fix be?
I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
--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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-31 16:24 ` Darrick J. Wong
@ 2017-03-31 16:28 ` Christoph Hellwig
2017-03-31 16:38 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2017-03-31 16:28 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, Dave Chinner, Brian Foster, xfs
On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
Yes, that's the one. How are we going to fix that for -rc5?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-31 16:28 ` Christoph Hellwig
@ 2017-03-31 16:38 ` Darrick J. Wong
2017-03-31 16:40 ` Christoph Hellwig
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2017-03-31 16:38 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Dave Chinner, Brian Foster, xfs
On Fri, Mar 31, 2017 at 09:28:17AM -0700, Christoph Hellwig wrote:
> On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> > I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> > the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
>
> Yes, that's the one. How are we going to fix that for -rc5?
This patch fixes all the thinkos in the original patch, so I was just
going to send it to Linus for rc5, but decided to poll the ML to see if
anyone had an objection to that.
--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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-31 16:38 ` Darrick J. Wong
@ 2017-03-31 16:40 ` Christoph Hellwig
2017-04-01 23:17 ` Dave Chinner
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2017-03-31 16:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, Dave Chinner, Brian Foster, xfs
On Fri, Mar 31, 2017 at 09:38:36AM -0700, Darrick J. Wong wrote:
> On Fri, Mar 31, 2017 at 09:28:17AM -0700, Christoph Hellwig wrote:
> > On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> > > I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> > > the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
> >
> > Yes, that's the one. How are we going to fix that for -rc5?
>
> This patch fixes all the thinkos in the original patch, so I was just
> going to send it to Linus for rc5, but decided to poll the ML to see if
> anyone had an objection to that.
That's what I thought and was surprised by the reply from Dave..
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-27 23:03 [PATCH v2] xfs: move the inline directory verifiers Darrick J. Wong
2017-03-28 12:51 ` Brian Foster
@ 2017-03-31 17:24 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2017-03-31 17:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Dave Chinner, xfs, Brian Foster
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] xfs: move the inline directory verifiers
2017-03-31 16:40 ` Christoph Hellwig
@ 2017-04-01 23:17 ` Dave Chinner
0 siblings, 0 replies; 15+ messages in thread
From: Dave Chinner @ 2017-04-01 23:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Darrick J. Wong, Brian Foster, xfs
On Fri, Mar 31, 2017 at 09:40:01AM -0700, Christoph Hellwig wrote:
> On Fri, Mar 31, 2017 at 09:38:36AM -0700, Darrick J. Wong wrote:
> > On Fri, Mar 31, 2017 at 09:28:17AM -0700, Christoph Hellwig wrote:
> > > On Fri, Mar 31, 2017 at 09:24:31AM -0700, Darrick J. Wong wrote:
> > > > I think xfs/348 causes -rc4 to fall over if CONFIG_XFS_DEBUG=y due to
> > > > the XFS_WANT_CORRUPTED_RETURNs that shouldn't be there.
> > >
> > > Yes, that's the one. How are we going to fix that for -rc5?
> >
> > This patch fixes all the thinkos in the original patch, so I was just
> > going to send it to Linus for rc5, but decided to poll the ML to see if
> > anyone had an objection to that.
>
> That's what I thought and was surprised by the reply from Dave..
There's no mention that it fixes a bug, regression or anything else
like that in the commit message. AFAICT from the description, it's
just a cleanup to match how the rest of the checking code is
structured with better error reporting.
Perhaps the commit message needs more work, because I can't tell you
what bug this is fixing from it....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-01 23:17 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-27 23:03 [PATCH v2] xfs: move the inline directory verifiers Darrick J. Wong
2017-03-28 12:51 ` Brian Foster
2017-03-28 15:00 ` Darrick J. Wong
2017-03-28 15:11 ` Brian Foster
2017-03-28 17:24 ` Brian Foster
2017-03-29 18:21 ` Darrick J. Wong
2017-03-29 18:52 ` Brian Foster
2017-03-29 22:41 ` Dave Chinner
2017-03-31 16:07 ` Christoph Hellwig
2017-03-31 16:24 ` Darrick J. Wong
2017-03-31 16:28 ` Christoph Hellwig
2017-03-31 16:38 ` Darrick J. Wong
2017-03-31 16:40 ` Christoph Hellwig
2017-04-01 23:17 ` Dave Chinner
2017-03-31 17:24 ` Christoph Hellwig
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.