On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:
> Create a centralized function to verify local format inode forks, then
> migrate the existing short format directory verifier to use this
> framework and add verifiers for the other two things we support (attrs
> and symlinks).
>
> Obviously this will get broken up into smaller pieces (one patch to
> refactor/move the existing verifier calls, another one to add the two
> new verifiers), but what do people think?
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
I haven't combed through the details of the verifiers, but just a few
very quick thoughts..
> fs/xfs/libxfs/xfs_attr_leaf.c | 70 ++++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_attr_leaf.h | 1 +
> fs/xfs/libxfs/xfs_inode_fork.c | 10 -----
> fs/xfs/libxfs/xfs_shared.h | 1 +
> fs/xfs/libxfs/xfs_symlink_remote.c | 29 +++++++++++++++
> fs/xfs/xfs_icache.c | 46 ++++++++++++++++++++++++
> fs/xfs/xfs_icache.h | 1 +
> fs/xfs/xfs_inode.c | 6 +--
> 8 files changed, 150 insertions(+), 14 deletions(-)
>
...
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 0e80f34..5484211 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,16 +183,6 @@ 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;
> - }
> - }
> -
What's the purpose of moving this? Just to accommodate userspace? As it
is, it looks like this leaves a coverage gap in a log recovery case
where xfs_iget() is not used (xfs_recover_inode_owner_change()).
Also, I think this is best split up into two or three smaller patches.
E.g., one to move/rename the existing mechanism (with justification as
to why), one or more to add the additional verifiers for the other
formats.
> if (xfs_is_reflink_inode(ip)) {
> ASSERT(ip->i_cowfp == NULL);
> xfs_ifork_init_cow(ip);
...
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index c484877..96e2957 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
> xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
> ifp->if_bytes - 1);
> }
> +
> +/* Verify the consistency of an inline symlink. */
> +int
> +xfs_symlink_sf_verify(
> + struct xfs_inode *ip)
> +{
> + char *sfp;
> + char *endp;
> + struct xfs_ifork *ifp;
> + int size;
> +
> + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + sfp = (char *)ifp->if_u1.if_data;
> + size = ifp->if_bytes;
> + endp = sfp + size;
> +
> + /* No negative sizes or overly long symlink targets. */
> + if (size < 0 || size > XFS_SYMLINK_MAXLEN)
> + return -EFSCORRUPTED;
> +
> + /* No NULLs in the target either. */
> + for (; sfp < endp; sfp++) {
> + if (*sfp == 0)
> + return -EFSCORRUPTED;
> + }
memchr() ?
Brian
> +
> + return 0;
> +}
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 0a9e698..fd1d430 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -34,6 +34,11 @@
> #include "xfs_dquot_item.h"
> #include "xfs_dquot.h"
> #include "xfs_reflink.h"
> +#include "xfs_da_format.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_dir2_priv.h"
> +#include "xfs_attr_leaf.h"
> +#include "xfs_shared.h"
>
> #include <linux/kthread.h>
> #include <linux/freezer.h>
> @@ -449,6 +454,43 @@ xfs_iget_cache_hit(
> return error;
> }
>
> +/* Check inline fork contents. */
> +int
> +xfs_iget_verify_forks(
> + struct xfs_inode *ip)
> +{
> + int error;
> +
> + /* Check the attribute fork if there is one. */
> + if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
> + ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> + error = xfs_attr_shortform_verify(ip);
> + if (error) {
> + xfs_alert(ip->i_mount,
> + "%s: bad inode %Lu inline attr fork",
> + __func__, ip->i_ino);
> + return error;
> + }
> + }
> +
> + /* Non-local data fork, jump out. */
> + if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> + return 0;
> +
> + /* Check the inline data fork if there is one. */
> + if (S_ISDIR(VFS_I(ip)->i_mode))
> + error = xfs_dir2_sf_verify(ip);
> + else if (S_ISLNK(VFS_I(ip)->i_mode))
> + error = xfs_symlink_sf_verify(ip);
> + else
> + error = -EFSCORRUPTED;
> +
> + if (error)
> + xfs_alert(ip->i_mount, "%s: bad inode %Lu inline data fork",
> + __func__, ip->i_ino);
> + return error;
> +}
> +
>
> static int
> xfs_iget_cache_miss(
> @@ -473,6 +515,10 @@ xfs_iget_cache_miss(
> if (error)
> goto out_destroy;
>
> + error = xfs_iget_verify_forks(ip);
> + if (error)
> + goto out_destroy;
> +
> trace_xfs_iget_miss(ip);
>
> if ((VFS_I(ip)->i_mode == 0) && !(flags & XFS_IGET_CREATE)) {
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index bff4d85..abf1cff 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -129,5 +129,6 @@ xfs_fs_eofblocks_from_user(
>
> int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
> xfs_ino_t ino, bool *inuse);
> +int xfs_iget_verify_forks(struct xfs_inode *ip);
>
> #endif
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ceef77c..7ca8336 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3547,10 +3547,8 @@ 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))
> + /* Check the inline fork data. */
> + if (xfs_iget_verify_forks(ip))
> goto corrupt_out;
>
> /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
