Re: [PATCH v2 4/7] iov_iter: new iov_iter_pin_pages*() routines

On Wed 14-09-22 04:51:17, Al Viro wrote:

On Wed, Sep 07, 2022 at 01:45:26AM -0700, Christoph Hellwig wrote:

quoted

On Tue, Sep 06, 2022 at 12:21:06PM +0200, Jan Kara wrote:

quoted

quoted

For FOLL_PIN callers, never pin bvec and kvec pages:  For file systems
not acquiring a reference is obviously safe, and the other callers will
need an audit, but I can't think of why it woul  ever be unsafe.

Are you sure about "For file systems not acquiring a reference is obviously
safe"? I can see places e.g. in orangefs, afs, etc. which create bvec iters
from pagecache pages. And then we have iter_file_splice_write() which
creates bvec from pipe pages (which can also be pagecache pages if
vmsplice() is used). So perhaps there are no lifetime issues even without
acquiring a reference (but looking at the code I would not say it is
obvious) but I definitely don't see how it would be safe to not get a pin
to signal to filesystem backing the pagecache page that there is DMA
happening to/from the page.

I mean in the context of iov_iter_get_pages callers, that is direct
I/O.  Direct callers of iov_iter_bvec which then pass that iov to
->read_iter / ->write_iter will need to hold references (those are
the references that the callers of iov_iter_get_pages rely on!).

Unless I'm misreading Jan, the question is whether they should get or
pin.  AFAICS, anyone who passes the sucker to ->read_iter() (or ->recvmsg(),
or does direct copy_to_iter()/zero_iter(), etc.) is falling under
=================================================================================
CASE 5: Pinning in order to write to the data within the page
-------------------------------------------------------------
Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
write to a page's data, unpin" can cause a problem. Case 5 may be considered a
superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
other words, if the code is neither Case 1 nor Case 2, it may still require
FOLL_PIN, for patterns like this:

Correct (uses FOLL_PIN calls):
    pin_user_pages()
    write to the data within the pages
    unpin_user_pages()

INCORRECT (uses FOLL_GET calls):
    get_user_pages()
    write to the data within the pages
    put_page()
=================================================================================

Yes, that was my point.

Regarding iter_file_splice_write() case, do we need to pin pages
when we are not going to modify the data in those?

Strictly speaking not. So far we are pinning pages even if they serve as
data source because it is simpler not to bother about data access direction
but I'm not really aware of anything that would mandate that.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR