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@xxxxxxxxxx> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925 >> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx> >> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx> >> --- >> >> 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
