linux-ext4 - Re: fsync() errors is unsafe and risks data loss

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180419001343.GF27893@dastard>
Date:   Thu, 19 Apr 2018 10:13:43 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Jeff Layton <jlayton@...hat.com>,
        lsf-pc <lsf-pc@...ts.linuxfoundation.org>,
        Andres Freund <andres@...razel.de>,
        Andreas Dilger <adilger@...ger.ca>,
        "Theodore Y. Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        "Joshua D. Drake" <jd@...mandprompt.com>
Subject: Re: fsync() errors is unsafe and risks data loss

On Fri, Apr 13, 2018 at 07:38:14PM -0700, Matthew Wilcox wrote:
> On Sat, Apr 14, 2018 at 11:47:52AM +1000, Dave Chinner wrote:
> > On Fri, Apr 13, 2018 at 07:02:32AM -0700, Matthew Wilcox wrote:
> > > 1. If we get an error while wbc->for_background is true, we should not clear
> > >    uptodate on the page, rather SetPageError and SetPageDirty.
> > 
> > So you're saying we should treat it as a transient error rather than
> > a permanent error.
> 
> Yes, I'm proposing leaving the data in memory in case the user wants to
> try writing it somewhere else.

And if it's getting IO errors because of USB stick pull? What
then?

> > > 2. Background writebacks should skip pages which are PageError.
> > 
> > That seems decidedly dodgy in the case where there is a transient
> > error - it requires a user to specifically run sync to get the data
> > to disk after the transient error has occurred. Say they don't
> > notice the problem because it's fleeting and doesn't cause any
> > obvious problems?
> 
> That's fair.  What I want to avoid is triggering the same error every
> 30 seconds (or whatever the periodic writeback threshold is set to).

So if kernel ring buffer overflows and so users miss the first error
report, they'll have no idea that the data writeback is still
failing?

> > e.g. XFS gets to enospc, runs out of reserve pool blocks so can't
> > allocate space to write back the page, then space is freed up a few
> > seconds later and so the next write will work just fine.
> > 
> > This is a recipe for "I lost data that I wrote /days/ before the
> > system crashed" bug reports.
> 
> So ... exponential backoff on retries?

Maybe, but I don't think that actually helps anything and adds yet
more "when should we write this" complication to inode writeback....

> > > 3. for_sync writebacks should attempt one last write.  Maybe it'll
> > >    succeed this time.  If it does, just ClearPageError.  If not, we have
> > >    somebody to report this writeback error to, and ClearPageUptodate.
> > 
> > Which may well be unmount. Are we really going to wait until unmount
> > to report fatal errors?
> 
> Goodness, no.  The errors would be immediately reportable using the wb_err
> mechanism, as soon as the first error was encountered.

But if there are no open files when the error occurs, that error
won't get reported to anyone. Which means the next time anyone
accesses that inode from a user context could very well be unmount
or a third party sync/syncfs()....

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com