Re: set_page_dirty vs truncate

[Matthew Wilcox <willy@infradead.org>]

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 ...