On Sat, May 16, 2020 at 10:01:43AM -0700, Darrick J. Wong wrote: > On Sat, May 16, 2020 at 03:58:08PM +0200, Christoph Hellwig wrote: > > On Thu, May 14, 2020 at 02:25:41PM -0700, Darrick J. Wong wrote: > > > > [~1000 lines of fullquote deleted until I hit the first comment, sigh..] > > > > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c > > > > index 157f72efec5e9..dfa1533b4edfc 100644 > > > > --- a/fs/xfs/scrub/bmap.c > > > > +++ b/fs/xfs/scrub/bmap.c > > > > @@ -598,7 +598,7 @@ xchk_bmap_check_rmaps( > > > > size = 0; > > > > break; > > > > } > > > > - if (XFS_IFORK_FORMAT(sc->ip, whichfork) != XFS_DINODE_FMT_BTREE && > > > > + if (ifp->if_format != XFS_DINODE_FMT_BTREE && > > > > > > ifp can be null here if bmapbt scrub is called on a file that has no > > > xattrs; this crashed my test vm immediately... > > > > What tests is that? And xfstests auto run did not hit it, even if a > > NULL check here seems sensible. > > In my case it was the xfs_scrub run after generic/001 that did it. > > I think we're covered against null *ifp in most cases because they're > guarded by an if(XFS_IFORK_Q()); it's jut here where I went around > shortcutting. Maybe I should just fix this function for you... :) Hmm, that sounded meaner than I intended it to be. :/ Also, it turns out that it's pretty easy to fix this as part of fixing the contorted logic in patch 1 (aka xchk_bmap_check_rmaps) so I'll do that there. > --D
