Commit Message

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

On 9/30/18 1:05 AM, Dave Chinner wrote:
> On Sun, Sep 30, 2018 at 12:06:44AM -0500, Eric Sandeen wrote:>> On 9/29/18 10:25 PM, Dave Chinner wrote:>>> On Mon, Sep 24, 2018 at 10:00:39PM -0500, Eric Sandeen wrote:
...
>>> 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?> > Seems to me like another cases that the verifiers have uncovered> another situation that even repair doesn't handle correctly. (Which> is why I like to look at these things from first principles, rather> than just from the "what does reapir do" POV?). It's like to be rare> because who removes all the attributes on a file apart from when> unlinking the inode?
Sure, I get it that repair is not the canonical truth for format, I was
just surprised that you hit it fairly quickly with the kernel verifier,
but apparently didn't see it prior to this patch in a post-test repair run.
In fact I don't think I've ever seen this check fail in repair. So it
just seemed odd. It might be interesting to see if we can provoke it in
xfs_repair?
> xfs_attr_fork_remove() has set the di_forkoff back to zero> when the last attribute is removed from the inode for a long time> (I stopped looking when I got to ~2008...), so this isn't a new> situation. i.e. trying to define what are valid values has> demonstrated that there are more cases we need to take into account> than anyone has realised.
Yeah, I have a vague recollection of other bugs related to the fork
offset that led to the sense of nearly nondeterministic behavior for
when things move in and out of local.
-Eric