Re: [PATCH 2/2 V3] xfs: verify size-vs-format for symlinks & dirs

From: Dave Chinner <david@fromorbit.com>
To: Eric Sandeen <sandeen@redhat.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>, "Xu, Wen" <wen.xu@gatech.edu>
Subject: Re: [PATCH 2/2 V3] xfs: verify size-vs-format for symlinks & dirs
Date: Sun, 30 Sep 2018 13:25:53 +1000	[thread overview]
Message-ID: <20180930032553.GJ31060@dastard> (raw)
In-Reply-To: <1e55b111-eae4-b0cf-0221-c96eb3f17b77@redhat.com>

On Mon, Sep 24, 2018 at 10:00:39PM -0500, Eric Sandeen wrote:
> Today, xfs_ifork_verify_data() will simply skip verification if the inode
> claims to be in non-local format.  However, nothing catches the case where
> the size for the format is too small to be non-local.  xfs_repair tests
> for this mismatch in process_check_inode_sizes(), so do the same in this
> verifier.
> 
> Reported-by: Xu, Wen <wen.xu@gatech.edu>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> V2: restructure code & tests per Dave's suggestion on the V1 patch.
> V3: rewrite dave's comments per brian's suggestions
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index f9acf1d436f6..d1a58e7a872f 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -704,12 +704,33 @@ xfs_ifork_verify_data(
>  	struct xfs_inode	*ip,
>  	struct xfs_ifork_ops	*ops)
>  {
> -	/* Non-local data fork, we're done. */
> -	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> +	struct xfs_mount	*mp = ip->i_mount;
> +	int			mode = VFS_I(ip)->i_mode;
> +
> +	/*
> +	 * Verify non-local format forks have a valid size. Symlinks must have
> +	 * outgrown the data fork size. The same goes for non-local dirs, but
> +	 * dirs grow at dirblock granularity. Perform a slightly stronger check
> +	 * and require the dir is at least one dirblock in size.
> +	 */
> +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
> +		switch (mode & S_IFMT) {
> +		case S_IFDIR:
> +			if (ip->i_d.di_size < mp->m_dir_geo->blksize)
> +				return __this_address;
> +			break;
> +		case S_IFLNK:
> +			if (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip))
> +				return __this_address;

Just had this fire in inode writeback from generic/390. I'm going to
drop it for the moment, because I'm not sure what the correct fix is
yet.  Consider this:

	create symlink XFS_LITINO bytes in length
	  fits in inode, so put inline. size <= IFORK_DSIZE
	[....]
	add attr to symlink
	  creates attr fork
	    inline data fork too large, size > new IFORK_DSIZE
	      xfs_symlink_local_to_remote()
		data fork goes to extent format, size remains unchanged
	[....]
	remove last attrs from inode
	  remove attr fork
	    IFORK_DSIZE grows again, now size = IFORK_DSIZE again
	    data fork remains in extent format
	[....]
	inode writeback
	  size = IFORK_DSIZE, extent format
	    xfs_ifork_verify_data verifier fails.

With this process, I think a symlink can be out of line even if it
is less than the size of the data fork. I think this can happen even
for symlinks much smaller than XFS_LITINO, because the attribute
fork can grow into free space in the literal area and push local
data larger than XFS_BMDR_SPACE_CALC(MINDBTPTRS) bytes to extent
format.

#define MINDBTPTRS 3

#define XFS_BMDR_SPACE_CALC(nrecs) \
        (int)(sizeof(xfs_bmdr_block_t) + \
	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t)))) 

= 4 + 3 * (8 + 8)
= 52 bytes
= 56 bytes when rounded up to 8 byte offset

So, yeah, I think that this check needs to be different because I
think we could have symlinks as short at 56 bytes in extent format,
even when the inode has no attribute fork...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com