From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C41FCC56201 for ; Wed, 25 Nov 2020 09:20:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7424420B80 for ; Wed, 25 Nov 2020 09:20:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727837AbgKYJUL (ORCPT ); Wed, 25 Nov 2020 04:20:11 -0500 Received: from mx2.suse.de ([195.135.220.15]:43090 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725938AbgKYJUK (ORCPT ); Wed, 25 Nov 2020 04:20:10 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 7E1A2AE42; Wed, 25 Nov 2020 09:20:08 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 96F841E130F; Wed, 25 Nov 2020 10:20:07 +0100 (CET) Date: Wed, 25 Nov 2020 10:20:07 +0100 From: Jan Kara To: Matthew Wilcox Cc: Hugh Dickins , Linus Torvalds , Jan Kara , syzbot , Andreas Dilger , Ext4 Developers List , Linux Kernel Mailing List , syzkaller-bugs , Theodore Ts'o , Linux-MM , Oleg Nesterov , Andrew Morton , "Kirill A. Shutemov" , Nicholas Piggin , Alex Shi , Qian Cai , Christoph Hellwig , "Darrick J. Wong" , William Kucharski , Jens Axboe , linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org Subject: Re: kernel BUG at fs/ext4/inode.c:LINE! Message-ID: <20201125092007.GA16944@quack2.suse.cz> References: <000000000000d3a33205add2f7b2@google.com> <20200828100755.GG7072@quack2.suse.cz> <20200831100340.GA26519@quack2.suse.cz> <20201124121912.GZ4327@casper.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201124121912.GZ4327@casper.infradead.org> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue 24-11-20 12:19:12, Matthew Wilcox wrote: > On Mon, Nov 23, 2020 at 08:07:24PM -0800, Hugh Dickins wrote: > > Twice now, when exercising ext4 looped on shmem huge pages, I have crashed > > on the PF_ONLY_HEAD check inside PageWaiters(): ext4_finish_bio() calling > > end_page_writeback() calling wake_up_page() on tail of a shmem huge page, > > no longer an ext4 page at all. > > > > The problem is that PageWriteback is not accompanied by a page reference > > (as the NOTE at the end of test_clear_page_writeback() acknowledges): as > > soon as TestClearPageWriteback has been done, that page could be removed > > from page cache, freed, and reused for something else by the time that > > wake_up_page() is reached. > > > > https://lore.kernel.org/linux-mm/20200827122019.GC14765@casper.infradead.org/ > > Matthew Wilcox suggested avoiding or weakening the PageWaiters() tail > > check; but I'm paranoid about even looking at an unreferenced struct page, > > lest its memory might itself have already been reused or hotremoved (and > > wake_up_page_bit() may modify that memory with its ClearPageWaiters()). > > > > Then on crashing a second time, realized there's a stronger reason against > > that approach. If my testing just occasionally crashes on that check, > > when the page is reused for part of a compound page, wouldn't it be much > > more common for the page to get reused as an order-0 page before reaching > > wake_up_page()? And on rare occasions, might that reused page already be > > marked PageWriteback by its new user, and already be waited upon? What > > would that look like? > > > > It would look like BUG_ON(PageWriteback) after wait_on_page_writeback() > > in write_cache_pages() (though I have never seen that crash myself). > > I don't think this is it. write_cache_pages() holds a reference to the > page -- indeed, it holds the page lock! So this particular race cannot > cause the page to get recycled. I still have no good ideas what this > is :-( But does it really matter what write_cache_pages() does? I mean we start page writeback. I mean struct bio holds no reference to the page it writes. The only thing that prevents the page from being freed under bio's hands is PageWriteback bit. So when the bio is completing we do (e.g. in ext4_end_bio()), we usually walk all pages in a bio bio_for_each_segment_all() and for each page call end_page_writeback(), now once end_page_writeback() calls test_clear_page_writeback() which clears PageWriteback(), the page can get freed. And that can happen before the wake_up_page() call in end_page_writeback(). So a race will be like: CPU1 CPU2 ext4_end_bio() ... end_page_writeback(page) test_clear_page_writeback(page) free page reallocate page for something else we can even dirty & start to writeback 'page' wake_up_page(page) and we have a "spurious" wake up on 'page'. Honza -- Jan Kara SUSE Labs, CR