On Fri, Dec 18, 2020 at 04:05:31PM +0000, Matthew Wilcox wrote: > A number of implementations of ->set_page_dirty check whether the page > has been truncated (ie page->mapping has become NULL since entering > set_page_dirty()). Several other implementations assume that they can do > page->mapping->host to get to the inode. So either some implementations > are doing unnecessary checks or others are vulnerable to a NULL pointer > dereference if truncate() races with set_page_dirty(). > > I'm touching ->set_page_dirty() anyway as part of the page folio > conversion. I'm thinking about passing in the mapping so there's no > need to look at page->mapping. > > The comments on set_page_dirty() and set_page_dirty_lock() suggests > there's no consistency in whether truncation is blocked or not; we're > only guaranteed that the inode itself won't go away. But maybe the > comments are stale. The comments are, I believe, not stale. Here's some syzbot reports which indicate that ext4 is seeing races between set_page_dirty() and truncate(): https://groups.google.com/g/syzkaller-lts-bugs/c/s9fHu162zhQ/m/Phnf6ucaAwAJ The reproducer includes calls to ftruncate(), so that would suggest that's what's going on. I would suggest just deleting this line: WARN_ON_ONCE(!page_has_buffers(page)); I'm not sure what value the other WARN_ON_ONCE adds. Maybe just replace ext4_set_page_dirty with __set_page_dirty_buffers in the aops? I'd defer to an ext4 expert on this ...
