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

From: Eric Sandeen <sandeen@sandeen.net>
To: Dave Chinner <david@fromorbit.com>, 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 00:06:44 -0500	[thread overview]
Message-ID: <71034ef7-836b-a462-a4f9-53d9004339bc@sandeen.net> (raw)
In-Reply-To: <20180930032553.GJ31060@dastard>

On 9/29/18 10:25 PM, Dave Chinner wrote:
> 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...

Hrmph.  And yet, xfs_repair:

static int
process_symlink_extlist(xfs_mount_t *mp, xfs_ino_t lino, xfs_dinode_t *dino)
{
        xfs_fileoff_t           expected_offset;
        xfs_bmbt_rec_t          *rp;
        xfs_bmbt_irec_t         irec;
        int                     numrecs;
        int                     i;
        int                     max_blocks;

        if (be64_to_cpu(dino->di_size) <= XFS_DFORK_DSIZE(dino, mp)) {
                if (dino->di_format == XFS_DINODE_FMT_LOCAL)
                        return 0;
                do_warn(
_("mismatch between format (%d) and size (%" PRId64 ") in symlink ino %" PRIu64 "\n"),
                        dino->di_format,
                        (int64_t)be64_to_cpu(dino->di_size), lino);
                return 1;
        }

seems to clearly call "non-local symlink with size < XFS_DFORK_DSIZE" corruption.
What gives?

-Eric