linux-kernel - Re: [PATCH 22/23] fs: default to generic_file_splice_read for files having ->read

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:   Thu, 30 Jul 2020 17:20:46 +0200
From:   Christoph Hellwig <hch@....de>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Christoph Hellwig <hch@....de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Kees Cook <keescook@...omium.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 22/23] fs: default to generic_file_splice_read for
 files having ->read_iter

On Thu, Jul 30, 2020 at 04:08:26PM +0100, Al Viro wrote:
> > I think we need to fix that in the instances, as we really expect
> > ->splice_read to just work instead of the caller knowing what could
> > work and what might not.
> 
> Er...  generic_file_splice_read() is a library helper; the decision to use
> is up to the filesystem/driver/protocol in question, and so's making sure
> it's not used with ->read_iter() that isn't fit for it.

Yes, but..  The problem is that while right now generic_file_splice_read
is the only user of ITER_PIPE there is absolutely not guarantee that
it remains the only user.  Having ->read_iter instances lingering that
can't deal with it is at best a mine field waiting for victims.

Fortunately I think the fix is pretty easy - remove the special pipe
zero copy optimization from copy_page_to_iter, and just have the
callers actually want it because they have pagecache or similar
refcountable pages use it explicitly for the ITER_PIPE case.  That gives
us a safe default with an opt-in into the optimized variant.  I'm
currently auditing all the users of for how it is used and that looks
pretty promising.

> Note that we *do* have instances where we have different ->splice_read()
> (sometimes using generic_file_splice_read(), sometimes not) even though
> ->read_iter() is there.
> 
> Your patch ignores those (thankfully), but commit message is rather
> misleading - it strongly implies that generic_file_splice_read() is
> *always* the right thing when ->read_iter() is there, not just that
> in such cases it makes a better fallback than default_file_splice_read().

I don't think it always is right.  Not without a major audit and more
work at least.