Received-SPF: pass (google.com: domain of linux-ext4+bounces-915-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 25 Jan 2024 14:06:58 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Roman Smirnov <r.smirnov@omp.ru>
Cc: stable@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Alexey Khoroshilov <khoroshilov@ispras.ru>,
	Sergey Shtylyov <s.shtylyov@omp.ru>,
	Karina Yankevich <k.yankevich@omp.ru>, lvc-project@linuxtesting.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-ext4@vger.kernel.org,
	Theodore Ts'o <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>, Jan Kara <jack@suse.com>
Subject: Re: [PATCH 5.10/5.15 v2 0/1 RFC] mm/truncate: fix WARNING in
 ext4_set_page_dirty()
Message-ID: <ZbJrAvCIufx1K2PU@casper.infradead.org>
References: <20240125130947.600632-1-r.smirnov@omp.ru>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240125130947.600632-1-r.smirnov@omp.ru>

On Thu, Jan 25, 2024 at 01:09:46PM +0000, Roman Smirnov wrote:
> Syzkaller reports warning in ext4_set_page_dirty() in 5.10 and 5.15
> stable releases. It happens because invalidate_inode_page() frees pages
> that are needed for the system. To fix this we need to add additional
> checks to the function. page_mapped() checks if a page exists in the 
> page tables, but this is not enough. The page can be used in other places:
> https://elixir.bootlin.com/linux/v6.8-rc1/source/include/linux/page_ref.h#L71
> 
> Kernel outputs an error line related to direct I/O:
> https://syzkaller.appspot.com/text?tag=CrashLog&x=14ab52dac80000

OK, this is making a lot more sense.

The invalidate_inode_page() path (after the page_mapped check) calls
try_to_release_page() which strips the buffers from the page.
__remove_mapping() tries to freeze the page and presuambly fails.

ext4 is checking there are still buffer heads attached to the page.
I'm not sure why it's doing that; it's legitimate to strip the
bufferheads from a page and then reattach them later (if they're
attached to a dirty page, they are created dirty).

So the only question in my mind is whether ext4 is right to have this
assert in the first place.  It seems wrong to me, but perhaps someone
from ext4 can explain why it's correct.

> The problem can be fixed in 5.10 and 5.15 stable releases by the 
> following patch.
> 
> The patch replaces page_mapped() call with check that finds additional
> references to the page excluding page cache and filesystem private data.
> If additional references exist, the page cannot be freed.
> 
> This version does not include the first patch from the first version.
> The problem can be fixed without it. 
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Link: https://syzkaller.appspot.com/bug?extid=02f21431b65c214aa1d6
> 
> Matthew Wilcox (Oracle) (1):
>   mm/truncate: Replace page_mapped() call in invalidate_inode_page()
> 
>  mm/truncate.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> -- 
> 2.34.1
>