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

[Jan Kara <jack@suse.cz>]

On Wed 14-09-22 04:51:17, Al Viro wrote:
> On Wed, Sep 07, 2022 at 01:45:26AM -0700, Christoph Hellwig wrote:
> > On Tue, Sep 06, 2022 at 12:21:06PM +0200, Jan Kara wrote:
> > > > 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 <jack@suse.com>
SUSE Labs, CR