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]

Date:   Fri, 20 Apr 2018 09:28:59 +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 Wed, Apr 18, 2018 at 05:40:37PM -0700, Matthew Wilcox wrote:
> On Thu, Apr 19, 2018 at 10:13:43AM +1000, Dave Chinner wrote:
> > 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?
> 
> I've been thinking about this.  Ideally we want to pass some kind of
> notification all the way up to the desktop and tell the user to plug the
> damn stick back in.  Then have the USB stick become the same blockdev
> that it used to be, and complete the writeback.  We are so far from
> being able to do that right now that it's not even funny.

*nod*

But in the meantime, device unplug (should give ENODEV, not EIO) is
a fatal error and we need to toss away the data.

> > > > > 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?
> 
> I wasn't thinking about kernel ringbuffer based reporting; I was thinking
> about errseq_t based reporting, so the application can tell the fsync
> failed and maybe does something application-level to recover like send
> the transactions across to another node in the cluster (or whatever this
> hypothetical application is).

But if it's still failing, then we should be still trying to report
the error. i.e. if fsync fails and the page remains dirty, then the
next attmept to write it is a new error and fsync should report
that. IOWs, I think we should be returning errors at every occasion
errors need to be reported if we have a persistent writeback
failure...

> > > > > 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()....
> 
> Right.  But then that's on the application.

Which we know don't do the right thing. Seems like a lot of hoops to
jump through given it still won't work if the appliction isn't
changed to support linux specific error handling requirements...

Cheers,

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