linux-kernel - Re: [PATCH] iomap: Handle I/O errors gracefully in page

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

Message-ID: <20200605224949.GK19604@bombadil.infradead.org>
Date:   Fri, 5 Jun 2020 15:49:49 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Dave Chinner <david@...morbit.com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iomap: Handle I/O errors gracefully in page_mkwrite

On Sat, Jun 06, 2020 at 07:48:41AM +1000, Dave Chinner wrote:
> On Fri, Jun 05, 2020 at 05:48:26AM -0700, Matthew Wilcox wrote:
> > ... I don't think that's the interesting path.  I mean, that's
> > the submission path, and usually we discover errors in the completion
> > path, not the submission path.
> 
> Where in the iomap write IO completion path do we call
> ClearPageUptodate()?

Oh, I misread.  You're right, I was looking at the read completion path.

So, this is also inconsistent.  We clear PageUptodate on errors we
discover during submission, but not for errors we discover during
completion.  That doesn't make sense.

> This comes back to my original, underlying worry about the fragility
> of the page fault path: the page fault path is not even checking for
> PageError during faults, and I'm betting that almost no
> ->page_mkwrite implementation is checking it, either....

I think it's a reasonable assumption that user page tables should never
contain a PTE for a page which is !Uptodate.  Otherwise the user can
read stale data.

> > I don't see why it can't be done from the submission path.
> > unmap_mapping_range() calls i_mmap_lock_write(), which is
> > down_write(i_mmap_rwsem) in drag.  There might be a lock ordering
> > issue there, although lockdep should find it pretty quickly.
> > 
> > The bigger problem is the completion path.  We're in softirq context,
> > so that will have to punt to a thread that can take mutexes.
> 
> Punt to workqueue if we aren't already in a workqueue context -
> for a lot of writes on XFS we already will be running completion in
> a workqueue context....

Yep.