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... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
