[PATCH 1/1] Document get_user_pages_fast()

[PATCH 1/1] Document get_user_pages_fast()

[Andy Grover]

While better than get_user_pages(), the usage of gupf(),
especially the return values and the fact that it can
potentially only partially pin the range, warranted some
documentation.

Signed-off-by: Andy Grover <andy.grover@oracle.com>
---
 mm/util.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index 2599e83..55bef16 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -223,6 +223,22 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
 }
 #endif
 
+/**
+ * get_user_pages_fast() - pin user pages in memory
+ * @start:	starting user address
+ * @nr_pages:	number of pages from start to pin
+ * @write:	whether pages will be written to
+ * @pages:	array that receives pointers to the pages pinned.
+ *		Should be at least nr_pages long.
+ *
+ * Attempt to pin user pages in memory without taking mm->mmap_sem.
+ * If not successful, it will fall back to taking the lock and
+ * calling get_user_pages().
+ *
+ * Returns number of pages pinned. This may be fewer than the number
+ * requested. If nr_pages is 0 or negative, returns 0. If no pages
+ * were pinned, returns -errno.
+ */
 int __attribute__((weak)) get_user_pages_fast(unsigned long start,
 				int nr_pages, int write, struct page **pages)
 {
-- 
1.5.6.3

Re: [PATCH 1/1] Document get_user_pages_fast()

[KOSAKI Motohiro]

> +/**
> + * get_user_pages_fast() - pin user pages in memory
> + * @start:	starting user address
> + * @nr_pages:	number of pages from start to pin
> + * @write:	whether pages will be written to
> + * @pages:	array that receives pointers to the pages pinned.
> + *		Should be at least nr_pages long.
> + *
> + * Attempt to pin user pages in memory without taking mm->mmap_sem.
> + * If not successful, it will fall back to taking the lock and
> + * calling get_user_pages().
> + *
> + * Returns number of pages pinned. This may be fewer than the number
> + * requested. If nr_pages is 0 or negative, returns 0. If no pages
> + * were pinned, returns -errno.
> + */
>  int __attribute__((weak)) get_user_pages_fast(unsigned long start,
>  				int nr_pages, int write, struct page **pages)
>  {

There is one problem.
gup and get_page() don't provide any page pinning.

get_page() only gurantee not freed. but don't gurantee to prevent page
reclaim and Cow change owner process.

Yes, current all gup caller have serious bug. especially direct-io, aio, bio
are broken.

Re: [PATCH 1/1] Document get_user_pages_fast()

[Andrew Morton]

On Fri, 10 Apr 2009 11:17:22 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > +/**
> > + * get_user_pages_fast() - pin user pages in memory
> > + * @start:	starting user address
> > + * @nr_pages:	number of pages from start to pin
> > + * @write:	whether pages will be written to
> > + * @pages:	array that receives pointers to the pages pinned.
> > + *		Should be at least nr_pages long.
> > + *
> > + * Attempt to pin user pages in memory without taking mm->mmap_sem.
> > + * If not successful, it will fall back to taking the lock and
> > + * calling get_user_pages().
> > + *
> > + * Returns number of pages pinned. This may be fewer than the number
> > + * requested. If nr_pages is 0 or negative, returns 0. If no pages
> > + * were pinned, returns -errno.
> > + */
> >  int __attribute__((weak)) get_user_pages_fast(unsigned long start,
> >  				int nr_pages, int write, struct page **pages)
> >  {
> 
> There is one problem.
> gup and get_page() don't provide any page pinning.
> 
> get_page() only gurantee not freed. but don't gurantee to prevent page
> reclaim

is_page_cache_freeable() measures page_count()...

> and Cow change owner process.

hm.  buggy userspace? Why is that a problem from a kernel perspective?

> Yes, current all gup caller have serious bug. especially direct-io, aio, bio
> are broken.
> 
> 
> 
> 

Re: [PATCH 1/1] Document get_user_pages_fast()

[KOSAKI Motohiro]

> > There is one problem.
> > gup and get_page() don't provide any page pinning.
> > 
> > get_page() only gurantee not freed. but don't gurantee to prevent page
> > reclaim
> 
> is_page_cache_freeable() measures page_count()...

sorry, my explanation was too short. 

reclaim can unmap the page. but page migration assume unmap successing mean
page migratable, then user-address associate another page and lost writing data
to old page.

IOW

     CPU0              CPU1                 note
------------------------------------------------------
    gup()
                    page-migration         user-address attach another page
 write data to                             write to old page.
 gup()ed page                              userland can't see it.

   put_page()                              free old page.


> 
> > and Cow change owner process.
> 
> hm.  buggy userspace? Why is that a problem from a kernel perspective?

We already started to discuttion in "[PATCH] fork vs gup(-fast) fix" thread.
but this is very difficult problem. it is still in progress ;)



> > Yes, current all gup caller have serious bug. especially direct-io, aio, bio
> > are broken.

Re: [PATCH 1/1] Document get_user_pages_fast()

[KOSAKI Motohiro]

> > > and Cow change owner process.
> > 
> > hm.  buggy userspace? Why is that a problem from a kernel perspective?
> 
> We already started to discuttion in "[PATCH] fork vs gup(-fast) fix" thread.
> but this is very difficult problem. it is still in progress ;)

note:
access_process_vm() tell us right get_user_pages() usage.
mmap_sem prevent fork in critical section. then it don't have cow risk.



> > > Yes, current all gup caller have serious bug. especially direct-io, aio, bio
> > > are broken.