Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572235 > unrolled thread

Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to allocate more pages per call

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-02 11:00 +0100
    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Christoph Hellwig <hch@infradead.org> - 2017-02-02 12:00 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-02 12:20 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Jeff Layton <jlayton@redhat.com> - 2017-02-02 14:10 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-03 08:40 +0100
            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-03 19:30 +0100
              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-03 20:10 +0100
                Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-03 20:30 +0100
                  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Steve Capper <steve.capper@linaro.org> - 2017-02-13 11:00 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Christoph Hellwig <hch@infradead.org> - 2017-02-03 08:50 +0100
            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-03 10:00 +0100
              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Christoph Hellwig <hch@infradead.org> - 2017-02-03 12:10 +0100
    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Jan Kara <jack@suse.cz> - 2017-02-02 15:50 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-02 19:30 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Jan Kara <jack@suse.cz> - 2017-02-03 15:50 +0100
    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-04 04:10 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-04 20:30 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-04 23:20 +0100
      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-04 23:20 +0100
        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 03:20 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-05 21:20 +0100
            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 22:10 +0100
              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-05 22:20 +0100
                Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 23:10 +0100
                  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-06 04:10 +0100
                    Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-06 10:10 +0100
                      Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-06 11:00 +0100
                        Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-06 15:20 +0100
                          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-07 08:20 +0100
                            Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-07 12:40 +0100
                              Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-08 07:00 +0100
                                Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-08 12:20 +0100
                  Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Miklos Szeredi <miklos@szeredi.hu> - 2017-02-06 09:40 +0100
          Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to  allocate more pages per call Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-05 22:00 +0100

Page 1 of 2  [1] 2  Next page →

#1572235 — Re: [PATCH v3 0/2] iov_iter: allow iov_iter_get_pages_alloc to allocate more pages per call

On Wed, Jan 25, 2017 at 08:32:03AM -0500, Jeff Layton wrote:
> Small respin of the patch that I sent yesterday for the same thing.
> 
> This moves the maxsize handling into iov_iter_pvec_size, so that we don't
> end up iterating past the max size we'll use anyway when trying to
> determine the pagevec length.
> 
> Also, a respun patch to make ceph use iov_iter_get_pages_alloc instead of
> trying to do it via its own routine.
> 
> Al, if these look ok, do you want to pick these up or shall I ask
> Ilya to merge them via the ceph tree?

I'd rather have that kind of work go through the vfs tree; said that,
I really wonder if this is the right approach.  Most of the users of
iov_iter_get_pages()/iov_iter_get_pages_alloc() look like they want
something like
	iov_iter_for_each_page(iter, size, f, data)
with int (*f)(struct page *page, size_t from, size_t size, void *data)
passed as callback.  Not everything fits that model, but there's a whole
lot of things that do.

Examples:
	* fs/direct_io.c:do_direct_IO().  We loop through the pages
returned by dio_get_page().  For each of those we find the subrange
of page (from/to) and handle IO on that range.  Then we drop the reference
to page and move on to the next one.  dio_get_page() uses dio->pages and
sdio->{head,tail,from,to} to avoid calling iov_iter_get_pages() on each
page - iov_iter_get_pages() is called for bigger chunks (up to 64 pages,
AFAICS) and results are kept in dio->pages for subsequent calls of
dio_get_page().  Unconsumed references are dropped by dio_cleanup();
AFAICS, it could've been called unconditionally right after the call
of do_direct_IO() (or from it, for that matter) - all remaining references
to pages are never looked at after do_direct_IO().  As it is, we call
it immediately on failure return from do_direct_IO() and then unconditionally
after blk_finish_plug().
	That oddity aside (and AFAICS it's really pointless - all pages
we'd done something with in do_direct_IO() won't be affected by
dio_cleanup()), there's potentially more interesting issue.  If
iov_iter_get_pages() fails on write at the moment when we have pending
mapped blocks, we treat that as write from zero page.  Once that has
happened, we remember to stop mapping new blocks and arrange for having
the error eventually treated as if it had come from IO failure.  I'm not
sure if this sucker handles all cases correctly, BTW - can we end up with
a few pages worth of pending mapped blocks?
	But aside of that, it's really a "loop through all page subranges"
kind of code.  The inner loop in do_direct_IO() could be converted into
a callback quite easily

	* nfs_direct_read_schedule_iovec(): same kind of batching, only there
we have an outer loop calling iov_iter_get_pages_alloc() and then the
inner loop goes through the page subranges, with the same work done for
each.  In this case we grab a reference inside the would-be callback and
drop all references from iov_iter_get_pages_alloc() after the inner loop
is done.  Could've gotten rid of grabbing extra refs - that would mean dropping
only the unused ones if the 'callback' (== inner loop body) has told us
to bugger off early.  IMO that would be a better model.  Incidentally, we
keep allocating/freeing the array used to store page references for each batch.

	* nfs_direct_write_schedule_iovec(): very similar to the read side.

	* zerocopy_sg_from_iter(): similar loop, batch size is MAX_SKB_FRAGS
(i.e. 16 or 17, depending upon the PAGE_SIZE; unless somebody has done
a port with 2Kb pages it shouldn't be greater than 17).  Array of references
is on stack, skb_fill_page_desc(skb, frag++, page, from, size) should
become the callback.  References are consumed by it and it can't fail, so
there's nothing left to drop.

	* af_alg_make_sg().  Looks like it might be massaged to the same
model; the tricky part is af_alg_free_sg() users.  We keep references to
pages in sgl->pages[] *and* store them in sgl->sg[...] (via sg_set_page()).
af_alg_free_sg() drops them using ->pages[] instead of sg_page(...->sg + ...).
Might or might not be a problem - I'm not familiar with that code.

	* fuse_get_user_pages().  It pretty much fills an equivalent of
bio_vec array; the difference is, array of struct page * and arrays of
(start, len) pairs are kept separately.  The only benefit is using the
first array as destination of iov_iter_get_pages(); might as well work
into a separate batching array instead - copying struct page * is noise
compared to storing (and calculating) start/len pairs we have to do there.
Again, what we do there is a pure loop over page subranges.

	* fuse_copy_fill().  I'm not at all sure that iov_iter_get_pages()
is a good idea there - fuse_copy_do() could bloody well just use
copy_{to,from}_iter().

	* fs/splice.c:iter_to_pipe().  Loop over page subranges, consuming
page references.  Unused ones are dropped.

	* bio_iov_iter_get_pages().  Wants to populate bio_vec array; should've
been a loop calling iov_iter_get_pages(); gets tripped on each iovec boundary
instead.  IMO would've been better off with a loop and separate 'batching'
array; would've killed the "Deep magic" mess in there, while we are at it.

That's the majority of iov_iter_get_pages{,_alloc} callers.  There's one
I'm not sure about in lustre (looks like their O_DIRECT is complicated by
rudiments of lloop stuff), there's a mess in p9_get_mapped_pages() (with
special-casing the kvec-backed iterators using kmap_to_page() and
vmalloc_to_page(), no less), there's default_file_splice_read() and there's
ceph stuff.  Everything else is covered by the 'loop over page subranges'
stuff.

I'm massaging that code (along with a lot of RTFS); the interesting questions
related to VM side of things are
	* what are the relative costs of doing small vs. large batches?  Some
of get_user_pages_fast() instances have comments along the lines of "we ought
to limit the batch size, but then nobody's doing more than 64 pages at a time
anyway".
	* Not a question: any ->fault() that returns VM_FAULT_RETRY when
*not* passed FAULT_FLAG_ALLOW_RETRY in flags ought to be shot.  cxlflash one
sure as hell is.
	* drivers/gpu/drm/vgem/vgem_drv.c:vgem_gem_fault() is bloody odd -
shmem_read_mapping_page() can't return -EBUSY, AFAICS.  vm_insert_page()
used to (and back then vgem_gem_fault() used to be broken), but these days
it looks like dead code...
	* ->page_mkwrite() instances sometimes return VM_FAULT_RETRY; AFAICS,
it's only (ab)used there as 'not zero, but doesn't contain any error bits';
VM_FAULT_RETRY from that source does *not* reach handle_mm_fault() callers,
right?
	* get_user_pages_fast() only returns 0 on zero size.  AFAICS, that's
true and some callers seems to rely upon that.  Correct?
	* aligning the start address passed to get_user_pages_fast() et.al.
Happens in many callers, but not all of them.  Most of the instances
forcibly align it in get_user_pages_fast() itself, but... not the fallback
one.  I'm not sure if it can be used to screw the things up, but it feels
like aligning the sucker in get_user_pages...() would be safer - callers
outnumber them and they are scattered in bad places (including drivers/staging)

	Comments?

[toc] | [next] | [standalone]

#1572276

On Thu, Feb 02, 2017 at 09:51:25AM +0000, Al Viro wrote:
> On Wed, Jan 25, 2017 at 08:32:03AM -0500, Jeff Layton wrote:
> > Small respin of the patch that I sent yesterday for the same thing.
> > 
> > This moves the maxsize handling into iov_iter_pvec_size, so that we don't
> > end up iterating past the max size we'll use anyway when trying to
> > determine the pagevec length.
> > 
> > Also, a respun patch to make ceph use iov_iter_get_pages_alloc instead of
> > trying to do it via its own routine.
> > 
> > Al, if these look ok, do you want to pick these up or shall I ask
> > Ilya to merge them via the ceph tree?
> 
> I'd rather have that kind of work go through the vfs tree; said that,
> I really wonder if this is the right approach.  Most of the users of
> iov_iter_get_pages()/iov_iter_get_pages_alloc() look like they want
> something like
> 	iov_iter_for_each_page(iter, size, f, data)
> with int (*f)(struct page *page, size_t from, size_t size, void *data)
> passed as callback.  Not everything fits that model, but there's a whole
> lot of things that do.

I was planning to do that, mostly because of the iomap dio code that
would not only get a lot cleaner with this, but also support multi-page
bvecs that we hope to have in the block layer soon.  The issue with it
is that we need to touch all the arch get_user_pages_fast
implementations, so it's going to be a relatively invasive change that I
didn't want to fix with just introducing the new direct I/O code.

[toc] | [prev] | [next] | [standalone]

#1572284

On Thu, Feb 02, 2017 at 02:56:51AM -0800, Christoph Hellwig wrote:
> > I really wonder if this is the right approach.  Most of the users of
> > iov_iter_get_pages()/iov_iter_get_pages_alloc() look like they want
> > something like
> > 	iov_iter_for_each_page(iter, size, f, data)
> > with int (*f)(struct page *page, size_t from, size_t size, void *data)
> > passed as callback.  Not everything fits that model, but there's a whole
> > lot of things that do.
> 
> I was planning to do that, mostly because of the iomap dio code that
> would not only get a lot cleaner with this, but also support multi-page
> bvecs that we hope to have in the block layer soon.  The issue with it
> is that we need to touch all the arch get_user_pages_fast
> implementations, so it's going to be a relatively invasive change that I
> didn't want to fix with just introducing the new direct I/O code.

I'm not sure we need to touch any get_user_pages_fast() at all; let it
fill a medium-sized array and use that as a buffer.  In particular,
I *really* don't like the idea of having the callbacks done in an
inconsistent locking environment - sometimes under ->mmap_sem, sometimes
not.

I played with "let it fill bio_vec array", but it doesn't really fit the
users; variant with callbacks is cleaner, IMO.

[toc] | [prev] | [next] | [standalone]

#1572342

On Thu, 2017-02-02 at 11:16 +0000, Al Viro wrote:
> On Thu, Feb 02, 2017 at 02:56:51AM -0800, Christoph Hellwig wrote:
> > > I really wonder if this is the right approach.  Most of the users of
> > > iov_iter_get_pages()/iov_iter_get_pages_alloc() look like they want
> > > something like
> > > 	iov_iter_for_each_page(iter, size, f, data)
> > > with int (*f)(struct page *page, size_t from, size_t size, void *data)
> > > passed as callback.  Not everything fits that model, but there's a whole
> > > lot of things that do.
> > 

Yeah, that does seem nicer.

My only concern is that it sounds more invasive, and I'd like to get
some sort of interim fix in for Ceph in the meantime (preferably
something we can send to stable). It really is trivial to vmsplice some
data into the kernel, and then splice write that to a DIO file --
instant softlockup in my testing.

This patch also makes vectored DIO with small iovecs on NFS a lot more
efficient, which is a nice bonus.
 
> > I was planning to do that, mostly because of the iomap dio code that
> > would not only get a lot cleaner with this, but also support multi-page
> > bvecs that we hope to have in the block layer soon.  The issue with it
> > is that we need to touch all the arch get_user_pages_fast
> > implementations, so it's going to be a relatively invasive change that I
> > didn't want to fix with just introducing the new direct I/O code.
> 
> I'm not sure we need to touch any get_user_pages_fast() at all; let it
> fill a medium-sized array and use that as a buffer.  In particular,
> I *really* don't like the idea of having the callbacks done in an
> inconsistent locking environment - sometimes under ->mmap_sem, sometimes
> not.
> 

Yeah, that might work. You could kmalloc the buffer array according to
the maxsize value. For small ones we could even consider using an on-
stack buffer.

> I played with "let it fill bio_vec array", but it doesn't really fit the
> users; variant with callbacks is cleaner, IMO.

I looked at converting this over to a bio_vec array as well, but that
really doesn't work well for Ceph. I like the callback idea better too.

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1572916

On Thu, Feb 02, 2017 at 08:00:52AM -0500, Jeff Layton wrote:
> > I'm not sure we need to touch any get_user_pages_fast() at all; let it
> > fill a medium-sized array and use that as a buffer.  In particular,
> > I *really* don't like the idea of having the callbacks done in an
> > inconsistent locking environment - sometimes under ->mmap_sem, sometimes
> > not.
> > 
> 
> Yeah, that might work. You could kmalloc the buffer array according to
> the maxsize value. For small ones we could even consider using an on-
> stack buffer.

Yes.  FWIW, I'm not proposing to kill iov_iter_get_pages{,_alloc}()
immediately - the new primitive would initially use the damn thing.
That can be backported without any problems, and conversions would be
one-by-one.  If/when we get to the point where most of the users have
been switched to the new helper, we could try and see if what remains
could be dealt with; if that works, it might be possible to fold
iov_iter_get_pages() into it.  In particular, for ITER_BVEC and
ITER_PIPE we don't need to bother with any intermediate arrays at all, etc.
But that's only after several cycles when everyone is asked to try and use
the new primitive instead of iov_iter_get_pages().

The calling conventions of iov_iter_get_pages() are ugly and I would love to
get rid of them, but doing that in a flagday conversion is a lot of PITA
for no good reason.

Speaking of get_user_pages() and conversions:

get_user_pages() relies upon find_extend_vma() to reject kernel
addresses; the fast side of get_user_pages_fast() doesn't have anything
of that sort in case e.g. HAVE_GENERIC_RCU_GUP.  Sure, usually we have
that verified and rejected earlier anyway, but it's a fairly subtle difference
that doesn't seem to be documented anywhere.

For example, /dev/st* reads and writes (st_read()/st_write()) feed the
address of buffer to get_user_pages_unlocked().  If somebody replaced
those with get_user_pages_fast(), we'd be in trouble as soon as
some code got tricked into using kernel_write() on /dev/st*.

access_ok() in HAVE_GENERIC_RCU_GUP {__,}get_user_pages_fast() obviously
doesn't help in that scenario.  What am I missing here?

[toc] | [prev] | [next] | [standalone]

#1573360

On Thu, Feb 2, 2017 at 11:29 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> get_user_pages() relies upon find_extend_vma() to reject kernel
> addresses; the fast side of get_user_pages_fast() doesn't have anything
> of that sort

It is *supposed* to have it.

See pte_allows_gup(), for example. In particular, it requires the
_PAGE_USER bit in the PTE (and the devpte case should require
_PAGE_BIT_DEVMAP).

So no, get_user_pages_fast() can not look up kernel pages. If it does,
that would be a bug.

              Linus

[toc] | [prev] | [next] | [standalone]

#1573382

On Fri, Feb 03, 2017 at 10:29:23AM -0800, Linus Torvalds wrote:
> On Thu, Feb 2, 2017 at 11:29 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > get_user_pages() relies upon find_extend_vma() to reject kernel
> > addresses; the fast side of get_user_pages_fast() doesn't have anything
> > of that sort
> 
> It is *supposed* to have it.
> 
> See pte_allows_gup(), for example. In particular, it requires the
> _PAGE_USER bit in the PTE (and the devpte case should require
> _PAGE_BIT_DEVMAP).

On x86 it does.  I don't see anything equivalent in mm/gup.c one, and the
only kinda-sorta similar thing (access_ok() in __get_user_pages_fast()
there) is vulnerable to e.g. access via kernel_write().  The comment in
there
 * Before activating this code, please be aware that the following assumptions
 * are currently made:
 *
 *  *) HAVE_RCU_TABLE_FREE is enabled, and tlb_remove_table is used to free
 *      pages containing page tables.
 *
 *  *) ptes can be read atomically by the architecture.
 *
 *  *) access_ok is sufficient to validate userspace address ranges.
 *
 * The last two assumptions can be relaxed by the addition of helper functions.

doesn't look promising - access_ok() is never sufficient.  Something like
_PAGE_USER tests in x86 one solves that problem, but if anything similar
works for HAVE_GENERIC_RCU_GUP I don't see it.  Thus the question re
what am I missing here...

[toc] | [prev] | [next] | [standalone]

#1573387

On Fri, Feb 3, 2017 at 11:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On x86 it does.  I don't see anything equivalent in mm/gup.c one, and the
> only kinda-sorta similar thing (access_ok() in __get_user_pages_fast()
> there) is vulnerable to e.g. access via kernel_write().

Yeah, access_ok() is bogus. It needs to just check against TASK_SIZE
or whatever.

> doesn't look promising - access_ok() is never sufficient.  Something like
> _PAGE_USER tests in x86 one solves that problem, but if anything similar
> works for HAVE_GENERIC_RCU_GUP I don't see it.  Thus the question re
> what am I missing here...

Ok, I definitely agree that it looks like __get_user_pages_fast() just
needs to get rid of the access_ok() and replace it with a proper check
for the user address space range.

Looks like arm[64] and powerpc.are the current users. Adding in some
people involved with the original submission a few years ago.

I do note that the x86 __get_user_pages_fast() thing looks dodgy too.

In particular, we do it right in the *real* get_user_pages_fast(), see
commit 7f8189068726 ("x86: don't use 'access_ok()' as a range check in
get_user_pages_fast()"). But then the same bug was re-introduced when
the "irq safe" version was merged. As well as in the GENERIC_RCU_GUP
version.

Gaah. Apparently PeterZ copied the old buggy version before the fix
when he added __get_user_pages_fast() in commit 465a454f254e ("x86,
mm: Add __get_user_pages_fast()").

I guess it could be considered a merge error (both happened during the
2.6.31 merge window).

                   Linus

[toc] | [prev] | [next] | [standalone]

#1579558

On Fri, Feb 03, 2017 at 11:28:48AM -0800, Linus Torvalds wrote:
> On Fri, Feb 3, 2017 at 11:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On x86 it does.  I don't see anything equivalent in mm/gup.c one, and the
> > only kinda-sorta similar thing (access_ok() in __get_user_pages_fast()
> > there) is vulnerable to e.g. access via kernel_write().
> 
> Yeah, access_ok() is bogus. It needs to just check against TASK_SIZE
> or whatever.
> 
> > doesn't look promising - access_ok() is never sufficient.  Something like
> > _PAGE_USER tests in x86 one solves that problem, but if anything similar
> > works for HAVE_GENERIC_RCU_GUP I don't see it.  Thus the question re
> > what am I missing here...
> 
> Ok, I definitely agree that it looks like __get_user_pages_fast() just
> needs to get rid of the access_ok() and replace it with a proper check
> for the user address space range.
> 
> Looks like arm[64] and powerpc.are the current users. Adding in some
> people involved with the original submission a few years ago.

Hi,

[ Apologies for my late reply, I was on vacation then catchup... ]

> 
> I do note that the x86 __get_user_pages_fast() thing looks dodgy too.
> 
> In particular, we do it right in the *real* get_user_pages_fast(), see
> commit 7f8189068726 ("x86: don't use 'access_ok()' as a range check in
> get_user_pages_fast()"). But then the same bug was re-introduced when
> the "irq safe" version was merged. As well as in the GENERIC_RCU_GUP
> version.
> 
> Gaah. Apparently PeterZ copied the old buggy version before the fix
> when he added __get_user_pages_fast() in commit 465a454f254e ("x86,
> mm: Add __get_user_pages_fast()").
> 
> I guess it could be considered a merge error (both happened during the
> 2.6.31 merge window).
> 

Okay so looking at what we have for access_ok(.) on arm64, my
understanding is that we perform a 65-bit add/compare (in assembler) to
see whether or not the range is below the current_thread_info->addr_limit.
So I think this is a roundabout way of checking for no-wrap around and <= TASK_SIZE.

Looking at powerpc, I see it's a little different...

So if it sounds reasonable to folk I was going to send a patch to
replace the call to access_ok(.) with a wraparound + TASK_SIZE check
written explicitly in C? (and remove some of the comments talking about
access_ok(.)).

Cheers,
-- 
Steve

[toc] | [prev] | [next] | [standalone]

#1572917

On Thu, Feb 02, 2017 at 08:00:52AM -0500, Jeff Layton wrote:
> Yeah, that might work. You could kmalloc the buffer array according to
> the maxsize value. For small ones we could even consider using an on-
> stack buffer.

For the block direct I/O code we defintively want to avoid any
allocations for small I/O a that shows up in the performance numbers.
And we'd like to reuse the on-stack bio_vec so that the defintion of
a small I/O can be as big as possible without blowing up the stack.

[toc] | [prev] | [next] | [standalone]

#1572954

On Thu, Feb 02, 2017 at 11:49:01PM -0800, Christoph Hellwig wrote:
> On Thu, Feb 02, 2017 at 08:00:52AM -0500, Jeff Layton wrote:
> > Yeah, that might work. You could kmalloc the buffer array according to
> > the maxsize value. For small ones we could even consider using an on-
> > stack buffer.
> 
> For the block direct I/O code we defintively want to avoid any
> allocations for small I/O a that shows up in the performance numbers.
> And we'd like to reuse the on-stack bio_vec so that the defintion of
> a small I/O can be as big as possible without blowing up the stack.

Hmm...  Reuse part is really nasty ;-/  OTOH, it might make sense to have
a "fill bio_vec array" as separate primitive - having that sucker come
from bio looks like an artificial restriction.

OK, next question, seeing that you've dealt with O_DIRECT guts more than
I have.  When we have iov_iter_get_pages() fail on do_direct_IO() write
with some blocks already allocated, we pick zero page as data source.
So far, so good, but:
	* should we bother zeroing anything unless buffer_new() is true?
	* why, in case of more than a page worth of pending allocated
blocks, do we bother with calling iov_iter_get_pages() again and again?
We *do* take care not to allocate anything else after that point, but
dio_get_page() will be calling iov_iter_get_pages() every time in that
case - there's only one page in queue.

[toc] | [prev] | [next] | [standalone]

#1573029

On Fri, Feb 03, 2017 at 08:54:15AM +0000, Al Viro wrote:
> Hmm...  Reuse part is really nasty ;-/  OTOH, it might make sense to have
> a "fill bio_vec array" as separate primitive - having that sucker come
> from bio looks like an artificial restriction.

Or just the only usecase :)  But yes, it could be generalized to take a
bio_vec without too much effort.

> OK, next question, seeing that you've dealt with O_DIRECT guts more than
> I have.  When we have iov_iter_get_pages() fail on do_direct_IO() write
> with some blocks already allocated, we pick zero page as data source.
> So far, so good, but:
> 	* should we bother zeroing anything unless buffer_new() is true?

I don't think so.

> 	* why, in case of more than a page worth of pending allocated
> blocks, do we bother with calling iov_iter_get_pages() again and again?
> We *do* take care not to allocate anything else after that point, but
> dio_get_page() will be calling iov_iter_get_pages() every time in that
> case - there's only one page in queue.

There shouldn't be a need for it.

But take it with a grain of salt - fs/direct-io.c is a hairy mess,
that's one of the reasons why I replaced it with the new iomap
code instead of trying to gradually move it to iomap API.

[toc] | [prev] | [next] | [standalone]

#1572427

On Thu 02-02-17 09:51:25, Al Viro wrote:
> I'm massaging that code (along with a lot of RTFS); the interesting questions
> related to VM side of things are
> 	* what are the relative costs of doing small vs. large batches?  Some
> of get_user_pages_fast() instances have comments along the lines of "we ought
> to limit the batch size, but then nobody's doing more than 64 pages at a time
> anyway".

Well, I believe it's only about the cost of setting up page table walk and
walking down those several levels of page tables. It is cheaper to copy
several PTE entries from the leaf level than doing the full walk every
time. But I didn't ever profile it to get actual numbers.

Large batches are only a problem because of possible soft-lockups and irq
latencies AFAIK. So the batch just should not be insanely large...

> 	* Not a question: any ->fault() that returns VM_FAULT_RETRY when
> *not* passed FAULT_FLAG_ALLOW_RETRY in flags ought to be shot.  cxlflash one
> sure as hell is.

Yep.

> 	* drivers/gpu/drm/vgem/vgem_drv.c:vgem_gem_fault() is bloody odd -
> shmem_read_mapping_page() can't return -EBUSY, AFAICS.  vm_insert_page()
> used to (and back then vgem_gem_fault() used to be broken), but these days
> it looks like dead code...

Looks like that.

> 	* ->page_mkwrite() instances sometimes return VM_FAULT_RETRY; AFAICS,
> it's only (ab)used there as 'not zero, but doesn't contain any error bits';
> VM_FAULT_RETRY from that source does *not* reach handle_mm_fault() callers,
> right?

I can see only Lustre doing it and IMHO it is abuse. VM_FAULT_RETRY is used
for mmap_sem latency reduction when paging in pages and so not everybody
handles it. If a handler wants to simply retry the fault, returning
VM_FAULT_NOPAGE is a more common way to do that...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1572583

On Thu, Feb 02, 2017 at 03:48:17PM +0100, Jan Kara wrote:

> > 	* ->page_mkwrite() instances sometimes return VM_FAULT_RETRY; AFAICS,
> > it's only (ab)used there as 'not zero, but doesn't contain any error bits';
> > VM_FAULT_RETRY from that source does *not* reach handle_mm_fault() callers,
> > right?
> 
> I can see only Lustre doing it and IMHO it is abuse. VM_FAULT_RETRY is used
> for mmap_sem latency reduction when paging in pages and so not everybody
> handles it. If a handler wants to simply retry the fault, returning
> VM_FAULT_NOPAGE is a more common way to do that...

/* Convert errno to return value from ->page_mkwrite() call */
static inline int block_page_mkwrite_return(int err)
{
        if (err == 0)
                return VM_FAULT_LOCKED;
        if (err == -EFAULT)
                return VM_FAULT_NOPAGE;
        if (err == -ENOMEM)
                return VM_FAULT_OOM;
        if (err == -EAGAIN)
                return VM_FAULT_RETRY;
        /* -ENOSPC, -EDQUOT, -EIO ... */
        return VM_FAULT_SIGBUS;
}

and a bunch of ->page_mkwrite() instances using that.  However, the only
callers of ->page_mkwrite() are wp_page_shared()->do_page_mkwrite() and
do_shared_fault()->do_page_mkwrite().  do_page_mkwrite() treates
VM_FAULT_RETRY as "lock page and return VM_FAULT_RETRY|VM_FAULT_LOCKED".
Both callers do the same check -
                if (unlikely(!tmp || (tmp &
                                      (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
and the return value if that predicate is false.  FWIW, use of VM_FAULT_RETRY
comes from your patch back in 2011 and AFAICS the same analysis used to
apply back then, except for the open-coded method calls where we use
do_page_mkwrite() these days...

[toc] | [prev] | [next] | [standalone]

#1573127

On Thu 02-02-17 18:28:02, Al Viro wrote:
> On Thu, Feb 02, 2017 at 03:48:17PM +0100, Jan Kara wrote:
> 
> > > 	* ->page_mkwrite() instances sometimes return VM_FAULT_RETRY; AFAICS,
> > > it's only (ab)used there as 'not zero, but doesn't contain any error bits';
> > > VM_FAULT_RETRY from that source does *not* reach handle_mm_fault() callers,
> > > right?
> > 
> > I can see only Lustre doing it and IMHO it is abuse. VM_FAULT_RETRY is used
> > for mmap_sem latency reduction when paging in pages and so not everybody
> > handles it. If a handler wants to simply retry the fault, returning
> > VM_FAULT_NOPAGE is a more common way to do that...
> 
> /* Convert errno to return value from ->page_mkwrite() call */
> static inline int block_page_mkwrite_return(int err)
> {
>         if (err == 0)
>                 return VM_FAULT_LOCKED;
>         if (err == -EFAULT)
>                 return VM_FAULT_NOPAGE;
>         if (err == -ENOMEM)
>                 return VM_FAULT_OOM;
>         if (err == -EAGAIN)
>                 return VM_FAULT_RETRY;
>         /* -ENOSPC, -EDQUOT, -EIO ... */
>         return VM_FAULT_SIGBUS;
> }
> 
> and a bunch of ->page_mkwrite() instances using that.  However, the only
> callers of ->page_mkwrite() are wp_page_shared()->do_page_mkwrite() and
> do_shared_fault()->do_page_mkwrite().  do_page_mkwrite() treates
> VM_FAULT_RETRY as "lock page and return VM_FAULT_RETRY|VM_FAULT_LOCKED".
> Both callers do the same check -
>                 if (unlikely(!tmp || (tmp &
>                                       (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
> and the return value if that predicate is false.  FWIW, use of VM_FAULT_RETRY
> comes from your patch back in 2011 and AFAICS the same analysis used to
> apply back then, except for the open-coded method calls where we use
> do_page_mkwrite() these days...

Yeah, back then I was not aware of VM_FAULT_RETRY limitations and your
analysis above just shows that its handling from do_page_mkwrite() is
simply broken (or better non-existent). Actually that VM_FAULT_RETRY
return was added by fs freeze handling patch. The freeze handling was
later changed but that change to block_page_mkwrite_return() remained.
I'll send a patch to remove it. Thanks for spotting this.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1573580

On Thu, Feb 02, 2017 at 09:51:25AM +0000, Al Viro wrote:

> 	* fuse_copy_fill().  I'm not at all sure that iov_iter_get_pages()
> is a good idea there - fuse_copy_do() could bloody well just use
> copy_{to,from}_iter().

Miklos, could you explain why does lock_request() prohibit page faults until
the matching unlock_request()?  All it does is setting FR_LOCKED on
our request and the only thing that even looks at that is fuse_abort_conn(),
which doesn't (AFAICS) wait for anything.

Where does the deadlock come from, and if it's not a deadlock - what is
it?  Or is that comment stale since "fuse: simplify request abort"?

[toc] | [prev] | [next] | [standalone]

#1573736

On Sat, Feb 04, 2017 at 03:08:42AM +0000, Al Viro wrote:
> On Thu, Feb 02, 2017 at 09:51:25AM +0000, Al Viro wrote:
> 
> > 	* fuse_copy_fill().  I'm not at all sure that iov_iter_get_pages()
> > is a good idea there - fuse_copy_do() could bloody well just use
> > copy_{to,from}_iter().
> 
> Miklos, could you explain why does lock_request() prohibit page faults until
> the matching unlock_request()?  All it does is setting FR_LOCKED on
> our request and the only thing that even looks at that is fuse_abort_conn(),
> which doesn't (AFAICS) wait for anything.
> 
> Where does the deadlock come from, and if it's not a deadlock - what is
> it?  Or is that comment stale since "fuse: simplify request abort"?

While we are at it...  How can fuse_copy_page() ever get called with
*pagep == NULL?  AFAICS, for that to happen you need either
	i < req->num_pages && req->pages[i] == NULL  
or in fuse_notify_store() have
                err = fuse_copy_page(cs, &page, offset, this_num, 0);
reached with page == NULL.  The latter is flat-out impossible - we have
                if (!page)
                        goto out_iput;

                this_num = min_t(unsigned, num, PAGE_SIZE - offset);
immediately before the call in question.  As for the former...  I don't see
any place where we would increase ->num_pages without having all additional
->pages[] elements guaranteed to be non-NULL.  Stores to ->num_pages are

in cuse_send_init():
	req->num_pages = 1;
with
        req->pages[0] = page;
shortly before that and
        if (!page)
                goto err_put_req;
earlier.

In fuse_retrieve():
                if (!page)
                        break;

                this_num = min_t(unsigned, num, PAGE_SIZE - offset);
                req->pages[req->num_pages] = page;
                req->page_descs[req->num_pages].length = this_num;
                req->num_pages++;

In fuse_readdir():
        req->num_pages = 1;
        req->pages[0] = page;
with
        if (!page) {
                fuse_put_request(fc, req);
                return -ENOMEM;
        }
several lines above.

In fuse_do_readpage():
        req->num_pages = 1;
        req->pages[0] = page;
with page dereferenced earlier.

In fuse_fill_write_pages():
                req->pages[req->num_pages] = page;
                req->page_descs[req->num_pages].length = tmp;
                req->num_pages++;
with
                if (!page)
                        break;
earlier.

In fuse_get_user_pages():
                ret = iov_iter_get_pages(ii, &req->pages[req->num_pages],
                                        *nbytesp - nbytes, 
                                        req->max_pages - req->num_pages,
                                        &start);
                if (ret < 0)
                        break;

                iov_iter_advance(ii, ret);
                nbytes += ret;

                ret += start;
                npages = (ret + PAGE_SIZE - 1) / PAGE_SIZE;

                req->page_descs[req->num_pages].offset = start;
                fuse_page_descs_length_init(req, req->num_pages, npages);

                req->num_pages += npages;
which also shouldn't leave any NULLs in the area in question.

In fuse_writepage_locked():
        req->num_pages = 1;
        req->pages[0] = tmp_page;
with
        if (!tmp_page)
                goto err_free;
done earlier.

In fuse_writepage_in_flight():
	req->num_pages = 1;
with
        BUG_ON(new_req->num_pages != 0);
earlier and
        req->pages[req->num_pages] = tmp_page;
done in the caller (which passes req as new_req).  Earlier in the caller
we have
        if (!tmp_page)
                goto out_unlock;

In fuse_writepages_fill():
		req->num_pages = 0;
is obviously OK and
        req->num_pages++;
in the end of the same function is preceded by the same
        req->pages[req->num_pages] = tmp_page;
(fuse_writepages_fill() is the caller of fuse_writepage_in_flight();
reassignment in fuse_writepage_in_flight() happens only in case when
it returns 1 and in that case we don't reach the increment in
fuse_writepages_fill() at all).

In fuse_do_ioctl():
        memcpy(req->pages, pages, sizeof(req->pages[0]) * num_pages);
        req->num_pages = num_pages;
and all increments of num_pages in there are
                pages[num_pages] = alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
                if (!pages[num_pages])
                        goto out;
                num_pages++;
so the array we copy into req->pages has the same property wrt num_pages.

req->pages is assigned only in fuse_request_init(); that gets called
in two places - one is at request allocation time, another (from
put_reserved_req()) passes the current req->pages value, so that leaves it
unchanged.  The contents of req->pages[] is changed only in the aforementioned
places + fuse_request_init(), which zeros ->num_pages + fuse_copy_page()
called from fuse_copy_pages() with &req->pages[i] as argument.  The last
one can modify the damn thing, but only if it hits
                                err = fuse_try_move_page(cs, pagep);
and that sucker never stores a NULL in there -
                *pagep = newpage;
with get_page(newpage) upstream from that point.

What am I missing here?  Looks like those checks in fuse_copy_page() are
dead code...  They had been there since the initial merge, but AFAICS
they were just as useless in 2.6.14.  Rudiments of some prehistorical stuff
that never had been cleaned out, perhaps?

[toc] | [prev] | [next] | [standalone]

#1573764

On Sat, Feb 4, 2017 at 8:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Feb 04, 2017 at 03:08:42AM +0000, Al Viro wrote:

> What am I missing here?  Looks like those checks in fuse_copy_page() are
> dead code...  They had been there since the initial merge, but AFAICS
> they were just as useless in 2.6.14.  Rudiments of some prehistorical stuff
> that never had been cleaned out, perhaps?

Yep, that one is probably historical.  I'll double check to make sure, though.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1573765

On Sat, Feb 4, 2017 at 4:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Feb 02, 2017 at 09:51:25AM +0000, Al Viro wrote:
>
>>       * fuse_copy_fill().  I'm not at all sure that iov_iter_get_pages()
>> is a good idea there - fuse_copy_do() could bloody well just use
>> copy_{to,from}_iter().
>
> Miklos, could you explain why does lock_request() prohibit page faults until
> the matching unlock_request()?  All it does is setting FR_LOCKED on
> our request and the only thing that even looks at that is fuse_abort_conn(),
> which doesn't (AFAICS) wait for anything.
>
> Where does the deadlock come from, and if it's not a deadlock - what is
> it?  Or is that comment stale since "fuse: simplify request abort"?

Well, it's not historical; at least not yet.  The deadlock is there
alright: mmap fuse file to addr; read byte from mapped page -> page
locked; this triggeres read request served in same process but
separate thread; write addr-headerlen to fuse dev; trying to lock same
page -> deadlock.

The deadlock can be broken by aborting or force unmounting: return
error for original read request; page unlocked; device write can get
page lock and return.

The reason we need to prohibit pagefault while copying is that when
request is aborted and the caller returns the memory in the request
may become invalid (e.g. data from stack).

Another solution would be to copy all data and keep a ref on the copy
by the request even after being aborted.  This is the plan for the
future.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1573799

On Sat, Feb 04, 2017 at 11:11:27PM +0100, Miklos Szeredi wrote:

> Well, it's not historical; at least not yet.  The deadlock is there
> alright: mmap fuse file to addr; read byte from mapped page -> page
> locked; this triggeres read request served in same process but
> separate thread; write addr-headerlen to fuse dev; trying to lock same
> page -> deadlock.

Let me see if I got it straight - you have the same fuse file mmapped
in two processes, one of them being fuse server (either sharing the
entire address space, or the same area mapped in both).  Another process
faults the sucker in; filemap_fault() locks the page and goes
fuse_readpage() -> fuse_do_readpage() -> fuse_send_read() ->
-> fuse_request_send() -> __fuse_request_send() which puts request into
queue and goes to sleep in request_wait_answer().  Eventually, read()
on /dev/fuse (or splice(), whatever) by server picks that request and reply
is formed and fed back into /dev/fuse.  There we (in fuse_do_dev_write())
call copy_out_args(), which tries to copy into our (still locked) page
a piece of data coming from server-supplied iovec.  As it is, you
are calling get_user_pages_fast(), triggering handle_mm_fault().  Since that
malicous FPOS of a server tried to feed you the _same_ mmapped file, you
hit a deadlock.  In server's context.  Correct?

Convoluted, but possible.  But.  Why the hell do we care whether that deadlock
hits in get_user_pages_fast() or in copy_from_user()?  Put it another way,
what difference does it make whether we take that fault with or without
FR_LOCKED in req->flags?

> The deadlock can be broken by aborting or force unmounting: return
> error for original read request; page unlocked; device write can get
> page lock and return.
> 
> The reason we need to prohibit pagefault while copying is that when
> request is aborted and the caller returns the memory in the request
> may become invalid (e.g. data from stack).

???

IDGI.  Your request is marked aborted and should presumably fail, so
that when request_wait_answer() wakes up and finds it screwed, fuse_readpage()
would just return an error and filemap_fault() will return VM_FAULT_SIGBUS,
with page left not uptodate and _not_ inserted into page tables.  What's
leaking where?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web