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

[Al Viro]

On Thu, Sep 15, 2022 at 10:16:25AM +0200, Jan Kara wrote:

> > How would that work?  What protects the area where you want to avoid running
> > into pinned pages from previously acceptable page getting pinned?  If "they
> > must have been successfully unmapped" is a part of what you are planning, we
> > really do have a problem...
> 
> But this is a very good question. So far the idea was that we lock the
> page, unmap (or writeprotect) the page, and then check pincount == 0 and
> that is a reliable method for making sure page data is stable (until we
> unlock the page & release other locks blocking page faults and writes). But
> once suddently ordinary page references can be used to create pins this
> does not work anymore. Hrm.
> 
> Just brainstorming ideas now: So we'd either need to obtain the pins early
> when we still have the virtual address (but I guess that is often not
> practical but should work e.g. for normal direct IO path) or we need some
> way to "simulate" the page fault when pinning the page, just don't map it
> into page tables in the end. This simulated page fault could be perhaps
> avoided if rmap walk shows that the page is already mapped somewhere with
> suitable permissions.

OK...  I'd done some digging; results so far

	* READ vs. WRITE turned out to be an awful way to specify iov_iter
data direction.  Local iov_iter branch so far:
	get rid of unlikely() on page_copy_sane() calls
	csum_and_copy_to_iter(): handle ITER_DISCARD
	[s390] copy_oldmem_kernel() - WRITE is "data source", not destination
	[fsi] WRITE is "data source", not destination...
	[infiniband] READ is "data destination", not source...
	[s390] zcore: WRITE is "data source", not destination...
	[target] fix iov_iter_bvec() "direction" argument
	[vhost] fix 'direction' argument of iov_iter_{init,bvec}()
	[xen] fix "direction" argument of iov_iter_kvec()
	[trace] READ means "data destination", not source...
	iov_iter: saner checks for attempt to copy to/from iterator
	use less confusing names for iov_iter direction initializers
those 8 commits in the middle consist of fixes, some of them with more than
one call site affected.  Folks keep going "oh, we are going to copy data
into that iterator, must be WRITE".  Wrong - WRITE means "as for write(2)",
i.e. the data _source_, not data destination.  And the same kind of bugs
goes in the opposite direction, of course.
	I think something like ITER_DEST vs. ITER_SOURCE would be less
confusing.

	* anything that goes with ITER_SOURCE doesn't need pin.
	* ITER_IOVEC/ITER_UBUF need pin for get_pages and for nothing else.
Need to grab reference on get_pages, obviously.
	* even more obviously, ITER_DISCARD is irrelevant here.
	* ITER_PIPE only modifies anonymous pages that had been allocated
by iov_iter primitives and hadn't been observed by anything outside until
we are done with said ITER_PIPE.
	* quite a few instances are similar to e.g. REQ_OP_READ handling in
/dev/loop - we work with ITER_BVEC there and we do modify the page contents,
but the damn thing would better be given to us locked and stay locked until
all involved modifications (be it real IO/decoding/whatever) is complete.
That ought to be safe, unless I'm missing something.

That doesn't cover everything; still going through the list...