linux-ext4 - Re: writeback completion soft lockup BUG in folio_wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <YjOlJL7xwktKoLFN@casper.infradead.org>
Date:   Thu, 17 Mar 2022 21:16:20 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Brian Foster <bfoster@...hat.com>, Linux-MM <linux-mm@...ck.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Namjae Jeon <namjae.jeon@...sung.com>,
        Ashish Sangwan <a.sangwan@...sung.com>,
        Theodore Ts'o <tytso@....edu>, Jan Kara <jack@...e.cz>,
        linux-ext4@...r.kernel.org
Subject: Re: writeback completion soft lockup BUG in folio_wake_bit()

On Thu, Mar 17, 2022 at 12:26:35PM -0700, Linus Torvalds wrote:
> On Thu, Mar 17, 2022 at 8:04 AM Matthew Wilcox <willy@...radead.org> wrote:
> >
> > So how about we do something like this:
> >
> >  - Make folio_start_writeback() and set_page_writeback() return void,
> >    fixing up AFS and NFS.
> >  - Add a folio_wait_start_writeback() to use in the VFS
> >  - Remove the calls to set_page_writeback() in the filesystems
> 
> That sounds lovely, but it does worry me a bit. Not just the odd
> 'keepwrite' thing, but also the whole ordering between the folio bit
> and the tagging bits. Does the ordering possibly matter?

I wouldn't change the ordering of setting the xarray bits and the
writeback flag; they'd just be set a little earlier.  It'd all be done
while the page was still locked.  But you're right, there's lots of
subtle interactions here.

> That whole "xyz_writeback_keepwrite()" thing seems odd. It's used in
> only one place (the folio version isn't used at all):
> 
>   ext4_writepage():
> 
>      ext4_walk_page_buffers() fails:
>                 redirty_page_for_writepage(wbc, page);
>                 keep_towrite = true;
>       ext4_bio_write_page().
> 
> which just looks odd. Why does it even try to continue to do the
> writepage when the page buffer thing has failed?
> 
> In the regular write path (ie ext4_write_begin()), a
> ext4_walk_page_buffers() failure is fatal or causes a retry). Why is
> ext4_writepage() any different? Particularly since it wants to keep
> the page dirty, then trying to do the writeback just seems wrong.
> 
> So this code is all a bit odd, I suspect there are decades of "people
> continued to do what they historically did" changes, and it is all
> worrisome.

I found the commit: 1c8349a17137 ("ext4: fix data integrity sync in
ordered mode").  Fortunately, we have a documented test for this,
generic/127, so we'll know if we've broken it.