On Mon, May 01, 2023 at 02:24:34PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > In commit 8ee81ed581ff, Ye Bin complained about an ASSERT in the bmapx > code that trips if we encounter a delalloc extent after flushing the > pagecache to disk. The ioctl code does not hold MMAPLOCK so it's > entirely possible that a racing write page fault can create a delalloc > extent after the file has been flushed. The proposed solution was to > replace the assertion with an early return that avoids filling out the > bmap recordset with a delalloc entry if the caller didn't ask for it. > > At the time, I recall thinking that the forward logic sounded ok, but > felt hesitant because I suspected that changing this code would cause > something /else/ to burst loose due to some other subtlety. > > syzbot of course found that subtlety. If all the extent mappings found > after the flush are delalloc mappings, we'll reach the end of the data > fork without ever incrementing bmv->bmv_entries. This is new, since > before we'd have emitted the delalloc mappings even though the caller > didn't ask for them. Once we reach the end, we'll try to set > BMV_OF_LAST on the -1st entry (because bmv_entries is zero) and go > corrupt something else in memory. Yay. > > I really dislike all these stupid patches that fiddle around with debug > code and break things that otherwise worked well enough. Nobody was > complaining that calling XFS_IOC_BMAPX without BMV_IF_DELALLOC would > return BMV_OF_DELALLOC records, and now we've gone from "weird behavior > that nobody cared about" to "bad behavior that must be addressed > immediately". > > Maybe I'll just ignore anything from Huawei from now on for my own sake. > > Reported-by: syzbot+c103d3808a0de5faaf80@xxxxxxxxxxxxxxxxxxxxxxxxx > Link: https://lore.kernel.org/linux-xfs/20230412024907.GP360889@frogsfrogsfrogs/ > Fixes: 8ee81ed581ff ("xfs: fix BUG_ON in xfs_getbmap()") > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> > --- > fs/xfs/xfs_bmap_util.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Ugh. Yet again we add weight to the approach of "if it ain't broke, don't fix it" for maintaining code that has not changed for a long time... Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> -- Dave Chinner david@xxxxxxxxxxxxx
