Re: [PATCH 1/2] mm: add locked parameter to get_user_pages()

2016-11-07 Thread Lorenzo Stoakes 
On Mon, Nov 07, 2016 at 11:49:18AM +0100, Jesper Nilsson wrote:
> For the cris-part:
> Acked-by: Jesper Nilsson 

Thanks very much for that, however just to avoid any confusion, I realised this
series was not not the right way forward after discussion with Paolo and rather
it makes more sense to keep the API as it is and to update callers where it
makes sense to.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mm: remove get_user_pages_locked()

2016-10-31 Thread Lorenzo Stoakes 
On Mon, Oct 31, 2016 at 06:55:33PM +0100, Paolo Bonzini wrote:
> > 2. There is currently only one caller of get_user_pages_locked() in
> >mm/frame_vector.c which seems to suggest this function isn't widely
> >used/known.
>
> Or not widely necessary. :)

Well, quite :)
>
> > 3. This change results in all slow-path get_user_pages*() functions having 
> > the
> >ability to use VM_FAULT_RETRY logic rather than 'defaulting' to using
> >get_user_pages() that doesn't let you do this even if you wanted to.
>
> This is only true if someone does the work though.  From a quick look
> at your series, the following files can be trivially changed to use
> get_user_pages_unlocked:
>
> - drivers/gpu/drm/via/via_dmablit.c
> - drivers/platform/goldfish/goldfish_pipe.c
> - drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> - drivers/rapidio/devices/rio_mport_cdev.c
> - drivers/virt/fsl_hypervisor.c
>

Ah indeed, I rather glossed through the callers and noticed that at least some
held locks long enough or were called with a lock held sufficient that I wasn't
sure it could be released.

> Also, videobuf_dma_init_user could be changed to use retry by adding a
> *locked argument to videobuf_dma_init_user_locked, prototype patch
> after my signature.
>

Very nice!

> Everything else is probably best kept using get_user_pages.
>
> > 4. It's somewhat confusing/redundant having both get_user_pages_locked() and
> >get_user_pages() functions which both require mmap_sem to be held (i.e. 
> > both
> >are 'locked' versions.)
> >
> >> If all callers were changed, then sure removing the _locked suffix would
> >> be a good idea.
> >
> > It seems many callers cannot release mmap_sem so VM_FAULT_RETRY logic 
> > couldn't
> > happen anyway in this cases and in these cases we couldn't change the 
> > caller.
>
> But then get_user_pages_locked remains a less-common case, so perhaps
> it's a good thing to give it a longer name!

My (somewhat minor) concern was that there would be confusion due to the
existence of the triumvirate of g_u_p()/g_u_p_unlocked()/g_u_p_locked(), however
the comments do a decent enough job of explaining the situation.

>
> > Overall, an alternative here might be to remove get_user_pages() and update
> > get_user_pages_locked() to add a 'vmas' parameter, however doing this 
> > renders
> > get_user_pages_unlocked() asymmetric as it would lack an vmas parameter 
> > (adding
> > one there would make no sense as VM_FAULT_RETRY logic invalidates VMAs) 
> > though
> > perhaps not such a big issue as it makes sense as to why this is the case.
>
> Adding the 'vmas' parameter to get_user_pages_locked would make little
> sense.  Since VM_FAULT_RETRY invalidates it and g_u_p_locked can and
> does retry, it would generally not be useful.

I meant only in the case where we'd remove get_user_pages() and instead be left
with get_user_pages_[un]locked() only, meaning we'd have to add some means of
retrieving vmas if locked was set NULL, of course in cases where locked is not
NULL it makes no sense to add it.

>
> So I think we have the right API now:
>
> - do not have lock?  Use get_user_pages_unlocked, get retry for free,
> no need to handle  mmap_sem and the locked argument; cannot get back vmas.
>
> - have and cannot drop lock?  User get_user_pages, no need to pass NULL
> for the locked argument; can get back vmas.
>
> - have but can drop lock (rare case)?  Use get_user_pages_locked,
> cannot get back vams.

Yeah I think this is sane as it is actually, this patch set was a lot less
convincing of a cleanup than prior ones and overall it seems we are better off
with the existing API.

I wonder whether a better patch series to come out of this would be to find
cases where the lock could be dropped (i.e. the ones you mention above) and to
switch to using get_user_pages_[un]locked() where it makes sense to.

I am happy to look into these cases (though of course I must leave your
suggested patch here to you :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] mm: remove get_user_pages_locked()

2016-10-31 Thread Lorenzo Stoakes 
On Mon, Oct 31, 2016 at 12:45:36PM +0100, Paolo Bonzini wrote:
>
>
> On 31/10/2016 11:02, Lorenzo Stoakes wrote:
> > - *
> > - * get_user_pages should be phased out in favor of
> > - * get_user_pages_locked|unlocked or get_user_pages_fast. Nothing
> > - * should use get_user_pages because it cannot pass
> > - * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault.
>
> This comment should be preserved in some way.  In addition, removing

Could you clarify what you think should be retained?

The comment seems to me to be largely rendered redundant by this change -
get_user_pages() now offers identical behaviour, and of course the latter part
of the comment ('because it cannot pass FAULT_FLAG_ALLOW_RETRY') is rendered
incorrect by this change.

It could be replaced with a recommendation to make use of VM_FAULT_RETRY logic
if possible. Note that the line above retains the reference to
'get_user_pages_fast()' - 'See also get_user_pages_fast, for performance
critical applications.'

One important thing to note here is this is a comment on get_user_pages_remote()
for which it was already incorrect syntactically (I'm guessing once
get_user_pages_remote() was added it 'stole' get_user_pages()'s comment :)

> get_user_pages_locked() makes it harder (compared to a simple "git grep
> -w") to identify callers that lack allow-retry functionality).  So I'm
> not sure about the benefits of these patches.
>

That's a fair point, and certainly this cleanup series is less obviously helpful
than previous ones.

However, there are a few points in its favour:

1. get_user_pages_remote() was the mirror of get_user_pages() prior to adding an
   int *locked parameter to the former (necessary to allow for the unexport of
   __get_user_pages_unlocked()), differing only in task/mm being specified and
   FOLL_REMOTE being set. This patch series keeps these functions 'synchronised'
   in this respect.

2. There is currently only one caller of get_user_pages_locked() in
   mm/frame_vector.c which seems to suggest this function isn't widely
   used/known.

3. This change results in all slow-path get_user_pages*() functions having the
   ability to use VM_FAULT_RETRY logic rather than 'defaulting' to using
   get_user_pages() that doesn't let you do this even if you wanted to.

4. It's somewhat confusing/redundant having both get_user_pages_locked() and
   get_user_pages() functions which both require mmap_sem to be held (i.e. both
   are 'locked' versions.)

> If all callers were changed, then sure removing the _locked suffix would
> be a good idea.

It seems many callers cannot release mmap_sem so VM_FAULT_RETRY logic couldn't
happen anyway in this cases and in these cases we couldn't change the caller.


Overall, an alternative here might be to remove get_user_pages() and update
get_user_pages_locked() to add a 'vmas' parameter, however doing this renders
get_user_pages_unlocked() asymmetric as it would lack an vmas parameter (adding
one there would make no sense as VM_FAULT_RETRY logic invalidates VMAs) though
perhaps not such a big issue as it makes sense as to why this is the case.

get_user_pages_unlocked() definitely seems to be a useful helper and therefore
makes sense to retain.

Of course another alternative is to leave things be :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] mm: remove get_user_pages_locked()

2016-10-31 Thread Lorenzo Stoakes 
get_user_pages() now has an int *locked parameter which renders
get_user_pages_locked() redundant, so remove it.

This patch should not introduce any functional changes.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 include/linux/mm.h |  2 --
 mm/frame_vector.c  |  4 ++--
 mm/gup.c   | 56 +++---
 mm/nommu.c |  8 
 4 files changed, 22 insertions(+), 48 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 396984e..4db3147 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1278,8 +1278,6 @@ long get_user_pages_remote(struct task_struct *tsk, 
struct mm_struct *mm,
 long get_user_pages(unsigned long start, unsigned long nr_pages,
unsigned int gup_flags, struct page **pages,
struct vm_area_struct **vmas, int *locked);
-long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
-   unsigned int gup_flags, struct page **pages, int *locked);
 long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm,
   unsigned long start, unsigned long nr_pages,
   struct page **pages, unsigned int gup_flags);
diff --git a/mm/frame_vector.c b/mm/frame_vector.c
index db77dcb..c5ce1b1 100644
--- a/mm/frame_vector.c
+++ b/mm/frame_vector.c
@@ -55,8 +55,8 @@ int get_vaddr_frames(unsigned long start, unsigned int 
nr_frames,
if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
vec->got_ref = true;
vec->is_pfns = false;
-   ret = get_user_pages_locked(start, nr_frames,
-   gup_flags, (struct page **)(vec->ptrs), );
+   ret = get_user_pages(start, nr_frames,
+   gup_flags, (struct page **)(vec->ptrs), NULL, );
goto out;
}
 
diff --git a/mm/gup.c b/mm/gup.c
index 6b5539e..6cec693 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -826,37 +826,6 @@ static __always_inline long __get_user_pages_locked(struct 
task_struct *tsk,
 }
 
 /*
- * We can leverage the VM_FAULT_RETRY functionality in the page fault
- * paths better by using either get_user_pages_locked() or
- * get_user_pages_unlocked().
- *
- * get_user_pages_locked() is suitable to replace the form:
- *
- *  down_read(>mmap_sem);
- *  do_something()
- *  get_user_pages(tsk, mm, ..., pages, NULL, NULL);
- *  up_read(>mmap_sem);
- *
- *  to:
- *
- *  int locked = 1;
- *  down_read(>mmap_sem);
- *  do_something()
- *  get_user_pages_locked(tsk, mm, ..., pages, );
- *  if (locked)
- *  up_read(>mmap_sem);
- */
-long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
-  unsigned int gup_flags, struct page **pages,
-  int *locked)
-{
-   return __get_user_pages_locked(current, current->mm, start, nr_pages,
-  pages, NULL, locked, true,
-  gup_flags | FOLL_TOUCH);
-}
-EXPORT_SYMBOL(get_user_pages_locked);
-
-/*
  * Same as get_user_pages_unlocked(, FOLL_TOUCH) but it allows to
  * pass additional gup_flags as last parameter (like FOLL_HWPOISON).
  *
@@ -954,11 +923,6 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
  * use the correct cache flushing APIs.
  *
  * See also get_user_pages_fast, for performance critical applications.
- *
- * get_user_pages should be phased out in favor of
- * get_user_pages_locked|unlocked or get_user_pages_fast. Nothing
- * should use get_user_pages because it cannot pass
- * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault.
  */
 long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
unsigned long start, unsigned long nr_pages,
@@ -976,6 +940,26 @@ EXPORT_SYMBOL(get_user_pages_remote);
  * less-flexible calling convention where we assume that the task
  * and mm being operated on are the current task's.  We also
  * obviously don't pass FOLL_REMOTE in here.
+ *
+ * We can leverage the VM_FAULT_RETRY functionality in the page fault
+ * paths better by using either get_user_pages()'s locked parameter or
+ * get_user_pages_unlocked().
+ *
+ * get_user_pages()'s locked parameter is suitable to replace the form:
+ *
+ *  down_read(>mmap_sem);
+ *  do_something()
+ *  get_user_pages(tsk, mm, ..., pages, NULL, NULL);
+ *  up_read(>mmap_sem);
+ *
+ *  to:
+ *
+ *  int locked = 1;
+ *  down_read(>mmap_sem);
+ *  do_something()
+ *  get_user_pages(tsk, mm, ..., pages, NULL, );
+ *  if (locked)
+ *  up_read(>mmap_sem);
  */
 long get_user_pages(unsigned long start, unsigned long nr_pages,
unsigned int gup_flags, struct page **pages,
diff --git a/mm/nommu.c b/mm/nommu.c
index 82aaa33..3d38d40 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -168,14 +168,6 @@ long get_use

[PATCH 0/2] mm: remove get_user_pages_locked()

2016-10-31 Thread Lorenzo Stoakes 
by adding an int *locked parameter to get_user_pages() callers to this function
can now utilise VM_FAULT_RETRY functionality.

Taken in conjunction with the patch series adding the same parameter to
get_user_pages_remote() this means all slow-path get_user_pages*() functions
will now have the ability to utilise VM_FAULT_RETRY.

Additionally get_user_pages() and get_user_pages_remote() previously mirrored
one another in functionality differing only in the ability to specify task/mm,
this patch series reinstates this relationship.

This patch series should not introduce any functional changes.

Lorenzo Stoakes (2):
  mm: add locked parameter to get_user_pages()
  mm: remove get_user_pages_locked()

 arch/cris/arch-v32/drivers/cryptocop.c |  2 +
 arch/ia64/kernel/err_inject.c  |  2 +-
 arch/x86/mm/mpx.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c|  2 +-
 drivers/gpu/drm/radeon/radeon_ttm.c|  2 +-
 drivers/gpu/drm/via/via_dmablit.c  |  2 +-
 drivers/infiniband/core/umem.c |  2 +-
 drivers/infiniband/hw/mthca/mthca_memfree.c|  3 +-
 drivers/infiniband/hw/qib/qib_user_pages.c |  2 +-
 drivers/infiniband/hw/usnic/usnic_uiom.c   |  2 +-
 drivers/media/v4l2-core/videobuf-dma-sg.c  |  2 +-
 drivers/misc/mic/scif/scif_rma.c   |  1 +
 drivers/misc/sgi-gru/grufault.c|  3 +-
 drivers/platform/goldfish/goldfish_pipe.c  |  2 +-
 drivers/rapidio/devices/rio_mport_cdev.c   |  2 +-
 .../interface/vchiq_arm/vchiq_2835_arm.c   |  3 +-
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  3 +-
 drivers/virt/fsl_hypervisor.c  |  2 +-
 include/linux/mm.h |  4 +-
 mm/frame_vector.c  |  4 +-
 mm/gup.c   | 62 --
 mm/ksm.c   |  3 +-
 mm/mempolicy.c |  2 +-
 mm/nommu.c | 10 +---
 virt/kvm/kvm_main.c|  4 +-
 25 files changed, 55 insertions(+), 73 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] mm: add locked parameter to get_user_pages()

2016-10-31 Thread Lorenzo Stoakes 
This patch adds an int *locked parameter to get_user_pages() to allow
VM_FAULT_RETRY faulting behaviour similar to get_user_pages_[un]locked().

It additionally clears the way for get_user_pages_locked() to be removed as its
sole remaining useful characteristic was to allow for VM_FAULT_RETRY behaviour
when faulting in pages.

It should not introduce any functional changes, however it does allow for
subsequent changes to get_user_pages() callers to take advantage of
VM_FAULT_RETRY.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 arch/cris/arch-v32/drivers/cryptocop.c| 2 ++
 arch/ia64/kernel/err_inject.c | 2 +-
 arch/x86/mm/mpx.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_ttm.c   | 2 +-
 drivers/gpu/drm/via/via_dmablit.c | 2 +-
 drivers/infiniband/core/umem.c| 2 +-
 drivers/infiniband/hw/mthca/mthca_memfree.c   | 3 ++-
 drivers/infiniband/hw/qib/qib_user_pages.c| 2 +-
 drivers/infiniband/hw/usnic/usnic_uiom.c  | 2 +-
 drivers/media/v4l2-core/videobuf-dma-sg.c | 2 +-
 drivers/misc/mic/scif/scif_rma.c  | 1 +
 drivers/misc/sgi-gru/grufault.c   | 3 ++-
 drivers/platform/goldfish/goldfish_pipe.c | 2 +-
 drivers/rapidio/devices/rio_mport_cdev.c  | 2 +-
 .../staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c| 3 ++-
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 ++-
 drivers/virt/fsl_hypervisor.c | 2 +-
 include/linux/mm.h| 2 +-
 mm/gup.c  | 8 
 mm/ksm.c  | 3 ++-
 mm/mempolicy.c| 2 +-
 mm/nommu.c| 4 ++--
 virt/kvm/kvm_main.c   | 4 ++--
 24 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/arch/cris/arch-v32/drivers/cryptocop.c 
b/arch/cris/arch-v32/drivers/cryptocop.c
index 0068fd4..7444b45 100644
--- a/arch/cris/arch-v32/drivers/cryptocop.c
+++ b/arch/cris/arch-v32/drivers/cryptocop.c
@@ -2723,6 +2723,7 @@ static int cryptocop_ioctl_process(struct inode *inode, 
struct file *filp, unsig
 noinpages,
 0,  /* read access only for in data */
 inpages,
+NULL,
 NULL);
 
if (err < 0) {
@@ -2737,6 +2738,7 @@ static int cryptocop_ioctl_process(struct inode *inode, 
struct file *filp, unsig
 nooutpages,
 FOLL_WRITE, /* write access for out data */
 outpages,
+NULL,
 NULL);
up_read(>mm->mmap_sem);
if (err < 0) {
diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c
index 5ed0ea9..99f3fa2 100644
--- a/arch/ia64/kernel/err_inject.c
+++ b/arch/ia64/kernel/err_inject.c
@@ -142,7 +142,7 @@ store_virtual_to_phys(struct device *dev, struct 
device_attribute *attr,
u64 virt_addr=simple_strtoull(buf, NULL, 16);
int ret;
 
-   ret = get_user_pages(virt_addr, 1, FOLL_WRITE, NULL, NULL);
+   ret = get_user_pages(virt_addr, 1, FOLL_WRITE, NULL, NULL, NULL);
if (ret<=0) {
 #ifdef ERR_INJ_DEBUG
printk("Virtual address %lx is not existing.\n",virt_addr);
diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index e4f8009..b74a7b2 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -546,7 +546,7 @@ static int mpx_resolve_fault(long __user *addr, int write)
int nr_pages = 1;
 
gup_ret = get_user_pages((unsigned long)addr, nr_pages,
-   write ? FOLL_WRITE : 0, NULL, NULL);
+   write ? FOLL_WRITE : 0, NULL, NULL, NULL);
/*
 * get_user_pages() returns number of pages gotten.
 * 0 means we failed to fault in and get anything,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index dcaf691..3d9a195 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -584,7 +584,7 @@ int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct 
page **pages)
list_add(, >guptasks);
spin_unlock(>guptasklock);
 
-

Re: [PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags

2016-10-19 Thread Lorenzo Stoakes 
On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> I am wondering whether we can go further. E.g. it is not really clear to
> me whether we need an explicit FOLL_REMOTE when we can in fact check
> mm != current->mm and imply that. Maybe there are some contexts which
> wouldn't work, I haven't checked.

This flag is set even when /proc/self/mem is used. I've not looked deeply into
this flag but perhaps accessing your own memory this way can be considered
'remote' since you're not accessing it directly. On the other hand, perhaps this
is just mistaken in this case?

> I guess there is more work in that area and I do not want to impose all
> that work on you, but I couldn't resist once I saw you playing in that
> area ;) Definitely a good start!

Thanks, I am more than happy to go as far down this rabbit hole as is helpful,
no imposition at all :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags

2016-10-19 Thread Lorenzo Stoakes 
On Wed, Oct 19, 2016 at 10:13:52AM +0200, Michal Hocko wrote:
> On Wed 19-10-16 09:59:03, Jan Kara wrote:
> > On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote:
> > > This patch removes the write parameter from __access_remote_vm() and 
> > > replaces it
> > > with a gup_flags parameter as use of this function previously _implied_
> > > FOLL_FORCE, whereas after this patch callers explicitly pass this flag.
> > >
> > > We make this explicit as use of FOLL_FORCE can result in surprising 
> > > behaviour
> > > (and hence bugs) within the mm subsystem.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
> >
> > So I'm not convinced this (and the following two patches) is actually
> > helping much. By grepping for FOLL_FORCE we will easily see that any caller
> > of access_remote_vm() gets that semantics and can thus continue search
>
> I am really wondering. Is there anything inherent that would require
> FOLL_FORCE for access_remote_vm? I mean FOLL_FORCE is a really
> non-trivial thing. It doesn't obey vma permissions so we should really
> minimize its usage. Do all of those users really need FOLL_FORCE?

I wonder about this also, for example by accessing /proc/self/mem you trigger
access_remote_vm() and consequently get_user_pages_remote() meaning FOLL_FORCE
is implied and you can use /proc/self/mem to override any VMA permissions. I
wonder if this is desirable behaviour or whether this ought to be limited to
ptrace system calls. Regardless, by making the flag more visible it makes it
easier to see that this is happening.

Note that this /proc/self/mem behaviour is what triggered the bug that brought
about this discussion in the first place -
https://marc.info/?l=linux-mm=147363447105059 - as using /proc/self/mem this
way on PROT_NONE memory broke some assumptions.

On the other hand I see your point Jan in that you know any caller of these
functions will have FOLL_FORCE implied, and you have to decide to stop passing
the flag at _some_ point in the stack, however it seems fairly sane to have that
stopping point exist outside of exported gup functions so the gup interface
forces explicitness but callers can wrap things as they need.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags

2016-10-19 Thread Lorenzo Stoakes 
On Wed, Oct 19, 2016 at 10:52:05AM +0200, Michal Hocko wrote:
> yes this is the desirable and expected behavior.
>
> > wonder if this is desirable behaviour or whether this ought to be limited to
> > ptrace system calls. Regardless, by making the flag more visible it makes it
> > easier to see that this is happening.
>
> mem_open already enforces PTRACE_MODE_ATTACH

Ah I missed this, that makes a lot of sense, thanks!

I still wonder whether other invocations of access_remote_vm() in fs/proc/base.c
(the principle caller of this function) need FOLL_FORCE, for example the various
calls that simply read data from other processes, so I think the point stands
about keeping this explicit.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] mm: replace get_user_pages_locked() write/force parameters with gup_flags

2016-10-18 Thread Lorenzo Stoakes 
On Tue, Oct 18, 2016 at 02:54:25PM +0200, Jan Kara wrote:
> > @@ -1282,7 +1282,7 @@ long get_user_pages(unsigned long start, unsigned 
> > long nr_pages,
> > int write, int force, struct page **pages,
> > struct vm_area_struct **vmas);
> >  long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
> > -   int write, int force, struct page **pages, int *locked);
> > +   unsigned int gup_flags, struct page **pages, int *locked);
>
> Hum, the prototype is inconsistent with e.g. __get_user_pages_unlocked()
> where gup_flags come after **pages argument. Actually it makes more sense
> to have it before **pages so that input arguments come first and output
> arguments second but I don't care that much. But it definitely should be
> consistent...

It was difficult to decide quite how to arrange parameters as there was
inconsitency with regards to parameter ordering already - for example
__get_user_pages() places its flags argument before pages whereas, as you note,
__get_user_pages_unlocked() puts them afterwards.

I ended up compromising by trying to match the existing ordering of the function
as much as I could by replacing write, force pairs with gup_flags in the same
location (with the exception of get_user_pages_unlocked() which I felt should
match __get_user_pages_unlocked() in signature) or if there was already a
gup_flags parameter as in the case of __get_user_pages_unlocked() I simply
removed the write, force pair and left the flags as the last parameter.

I am happy to rearrange parameters as needed, however I am not sure if it'd be
worthwhile for me to do so (I am keen to try to avoid adding too much noise here
:)

If we were to rearrange parameters for consistency I'd suggest adjusting
__get_user_pages_unlocked() to put gup_flags before pages and do the same with
get_user_pages_unlocked(), let me know what you think.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/10] mm: replace get_user_pages_locked() write/force parameters with gup_flags

2016-10-13 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from get_user_pages_locked()
and replaces them with a gup_flags parameter to make the use of FOLL_FORCE
explicit in callers as use of this flag can result in surprising behaviour (and
hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 include/linux/mm.h |  2 +-
 mm/frame_vector.c  |  8 +++-
 mm/gup.c   | 12 +++-
 mm/nommu.c |  5 -
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6adc4bc..27ab538 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1282,7 +1282,7 @@ long get_user_pages(unsigned long start, unsigned long 
nr_pages,
int write, int force, struct page **pages,
struct vm_area_struct **vmas);
 long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
-   int write, int force, struct page **pages, int *locked);
+   unsigned int gup_flags, struct page **pages, int *locked);
 long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm,
   unsigned long start, unsigned long nr_pages,
   struct page **pages, unsigned int gup_flags);
diff --git a/mm/frame_vector.c b/mm/frame_vector.c
index 381bb07..81b6749 100644
--- a/mm/frame_vector.c
+++ b/mm/frame_vector.c
@@ -41,10 +41,16 @@ int get_vaddr_frames(unsigned long start, unsigned int 
nr_frames,
int ret = 0;
int err;
int locked;
+   unsigned int gup_flags = 0;
 
if (nr_frames == 0)
return 0;
 
+   if (write)
+   gup_flags |= FOLL_WRITE;
+   if (force)
+   gup_flags |= FOLL_FORCE;
+
if (WARN_ON_ONCE(nr_frames > vec->nr_allocated))
nr_frames = vec->nr_allocated;
 
@@ -59,7 +65,7 @@ int get_vaddr_frames(unsigned long start, unsigned int 
nr_frames,
vec->got_ref = true;
vec->is_pfns = false;
ret = get_user_pages_locked(start, nr_frames,
-   write, force, (struct page **)(vec->ptrs), );
+   gup_flags, (struct page **)(vec->ptrs), );
goto out;
}
 
diff --git a/mm/gup.c b/mm/gup.c
index cfcb014..7a0d033 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -838,18 +838,12 @@ static __always_inline long 
__get_user_pages_locked(struct task_struct *tsk,
  *  up_read(>mmap_sem);
  */
 long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
-  int write, int force, struct page **pages,
+  unsigned int gup_flags, struct page **pages,
   int *locked)
 {
-   unsigned int flags = FOLL_TOUCH;
-
-   if (write)
-   flags |= FOLL_WRITE;
-   if (force)
-   flags |= FOLL_FORCE;
-
return __get_user_pages_locked(current, current->mm, start, nr_pages,
-  pages, NULL, locked, true, flags);
+  pages, NULL, locked, true,
+  gup_flags | FOLL_TOUCH);
 }
 EXPORT_SYMBOL(get_user_pages_locked);
 
diff --git a/mm/nommu.c b/mm/nommu.c
index 7e27add..842cfdd 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -176,9 +176,12 @@ long get_user_pages(unsigned long start, unsigned long 
nr_pages,
 EXPORT_SYMBOL(get_user_pages);
 
 long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
-   int write, int force, struct page **pages,
+   unsigned int gup_flags, struct page **pages,
int *locked)
 {
+   int write = gup_flags & FOLL_WRITE;
+   int force = gup_flags & FOLL_FORCE;
+
return get_user_pages(start, nr_pages, write, force, pages, NULL);
 }
 EXPORT_SYMBOL(get_user_pages_locked);
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/10] mm: replace access_process_vm() write parameter with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write parameter from access_process_vm() and replaces it
with a gup_flags parameter as use of this function previously _implied_
FOLL_FORCE, whereas after this patch callers explicitly pass this flag.

We make this explicit as use of FOLL_FORCE can result in surprising behaviour
(and hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 arch/alpha/kernel/ptrace.c |  9 ++---
 arch/blackfin/kernel/ptrace.c  |  5 +++--
 arch/cris/arch-v32/kernel/ptrace.c |  4 ++--
 arch/ia64/kernel/ptrace.c  | 14 +-
 arch/m32r/kernel/ptrace.c  | 15 ++-
 arch/mips/kernel/ptrace32.c|  5 +++--
 arch/powerpc/kernel/ptrace32.c |  5 +++--
 arch/score/kernel/ptrace.c | 10 ++
 arch/sparc/kernel/ptrace_64.c  | 24 
 arch/x86/kernel/step.c |  3 ++-
 arch/x86/um/ptrace_32.c|  3 ++-
 arch/x86/um/ptrace_64.c|  3 ++-
 include/linux/mm.h |  3 ++-
 kernel/ptrace.c| 16 ++--
 mm/memory.c|  8 ++--
 mm/nommu.c |  6 +++---
 mm/util.c  |  5 +++--
 17 files changed, 84 insertions(+), 54 deletions(-)

diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c
index d9ee817..940dfb4 100644
--- a/arch/alpha/kernel/ptrace.c
+++ b/arch/alpha/kernel/ptrace.c
@@ -157,14 +157,16 @@ put_reg(struct task_struct *task, unsigned long regno, 
unsigned long data)
 static inline int
 read_int(struct task_struct *task, unsigned long addr, int * data)
 {
-   int copied = access_process_vm(task, addr, data, sizeof(int), 0);
+   int copied = access_process_vm(task, addr, data, sizeof(int),
+   FOLL_FORCE);
return (copied == sizeof(int)) ? 0 : -EIO;
 }
 
 static inline int
 write_int(struct task_struct *task, unsigned long addr, int data)
 {
-   int copied = access_process_vm(task, addr, , sizeof(int), 1);
+   int copied = access_process_vm(task, addr, , sizeof(int),
+   FOLL_FORCE | FOLL_WRITE);
return (copied == sizeof(int)) ? 0 : -EIO;
 }
 
@@ -281,7 +283,8 @@ long arch_ptrace(struct task_struct *child, long request,
/* When I and D space are separate, these will need to be fixed.  */
case PTRACE_PEEKTEXT: /* read word at location addr. */
case PTRACE_PEEKDATA:
-   copied = access_process_vm(child, addr, , sizeof(tmp), 0);
+   copied = access_process_vm(child, addr, , sizeof(tmp),
+   FOLL_FORCE);
ret = -EIO;
if (copied != sizeof(tmp))
break;
diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index 8b8fe67..8d79286 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -271,7 +271,7 @@ long arch_ptrace(struct task_struct *child, long request,
case BFIN_MEM_ACCESS_CORE:
case BFIN_MEM_ACCESS_CORE_ONLY:
copied = access_process_vm(child, addr, ,
-  to_copy, 0);
+  to_copy, FOLL_FORCE);
if (copied)
break;
 
@@ -324,7 +324,8 @@ long arch_ptrace(struct task_struct *child, long request,
case BFIN_MEM_ACCESS_CORE:
case BFIN_MEM_ACCESS_CORE_ONLY:
copied = access_process_vm(child, addr, ,
-  to_copy, 1);
+  to_copy,
+  FOLL_FORCE | 
FOLL_WRITE);
break;
case BFIN_MEM_ACCESS_DMA:
if (safe_dma_memcpy(paddr, , to_copy))
diff --git a/arch/cris/arch-v32/kernel/ptrace.c 
b/arch/cris/arch-v32/kernel/ptrace.c
index f085229..f0df654 100644
--- a/arch/cris/arch-v32/kernel/ptrace.c
+++ b/arch/cris/arch-v32/kernel/ptrace.c
@@ -147,7 +147,7 @@ long arch_ptrace(struct task_struct *child, long request,
/* The trampoline page is globally mapped, no 
page table to traverse.*/
tmp = *(unsigned long*)addr;
} else {
-   copied = access_process_vm(child, addr, , 
sizeof(tmp), 0);
+   copied = access_process_vm(child, addr, , 
sizeof(tmp), FOLL_FORCE);
 
if (copied != sizeof(tmp))
break;
@@ -279,7 +279,7 @@ static int insn_size(struct task_struct *child, unsigned 
long pc)
   int opsize = 0;
 
   /

[PATCH 06/10] mm: replace get_user_pages() write/force parameters with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from get_user_pages() and
replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit
in callers as use of this flag can result in surprising behaviour (and hence
bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 arch/cris/arch-v32/drivers/cryptocop.c |  4 +---
 arch/ia64/kernel/err_inject.c  |  2 +-
 arch/x86/mm/mpx.c  |  5 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c|  7 +--
 drivers/gpu/drm/radeon/radeon_ttm.c|  3 ++-
 drivers/gpu/drm/via/via_dmablit.c  |  4 ++--
 drivers/infiniband/core/umem.c |  6 +-
 drivers/infiniband/hw/mthca/mthca_memfree.c|  2 +-
 drivers/infiniband/hw/qib/qib_user_pages.c |  3 ++-
 drivers/infiniband/hw/usnic/usnic_uiom.c   |  5 -
 drivers/media/v4l2-core/videobuf-dma-sg.c  |  7 +--
 drivers/misc/mic/scif/scif_rma.c   |  3 +--
 drivers/misc/sgi-gru/grufault.c|  2 +-
 drivers/platform/goldfish/goldfish_pipe.c  |  3 ++-
 drivers/rapidio/devices/rio_mport_cdev.c   |  3 ++-
 .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c |  3 +--
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  3 +--
 drivers/virt/fsl_hypervisor.c  |  4 ++--
 include/linux/mm.h |  2 +-
 mm/gup.c   | 12 +++-
 mm/mempolicy.c |  2 +-
 mm/nommu.c | 18 --
 22 files changed, 49 insertions(+), 54 deletions(-)

diff --git a/arch/cris/arch-v32/drivers/cryptocop.c 
b/arch/cris/arch-v32/drivers/cryptocop.c
index b5698c8..099e170 100644
--- a/arch/cris/arch-v32/drivers/cryptocop.c
+++ b/arch/cris/arch-v32/drivers/cryptocop.c
@@ -2722,7 +2722,6 @@ static int cryptocop_ioctl_process(struct inode *inode, 
struct file *filp, unsig
err = get_user_pages((unsigned long int)(oper.indata + prev_ix),
 noinpages,
 0,  /* read access only for in data */
-0, /* no force */
 inpages,
 NULL);
 
@@ -2736,8 +2735,7 @@ static int cryptocop_ioctl_process(struct inode *inode, 
struct file *filp, unsig
if (oper.do_cipher){
err = get_user_pages((unsigned long int)oper.cipher_outdata,
 nooutpages,
-1, /* write access for out data */
-0, /* no force */
+FOLL_WRITE, /* write access for out data */
 outpages,
 NULL);
up_read(>mm->mmap_sem);
diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c
index 09f8457..5ed0ea9 100644
--- a/arch/ia64/kernel/err_inject.c
+++ b/arch/ia64/kernel/err_inject.c
@@ -142,7 +142,7 @@ store_virtual_to_phys(struct device *dev, struct 
device_attribute *attr,
u64 virt_addr=simple_strtoull(buf, NULL, 16);
int ret;
 
-   ret = get_user_pages(virt_addr, 1, VM_READ, 0, NULL, NULL);
+   ret = get_user_pages(virt_addr, 1, FOLL_WRITE, NULL, NULL);
if (ret<=0) {
 #ifdef ERR_INJ_DEBUG
printk("Virtual address %lx is not existing.\n",virt_addr);
diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index 8047687..e4f8009 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -544,10 +544,9 @@ static int mpx_resolve_fault(long __user *addr, int write)
 {
long gup_ret;
int nr_pages = 1;
-   int force = 0;
 
-   gup_ret = get_user_pages((unsigned long)addr, nr_pages, write,
-   force, NULL, NULL);
+   gup_ret = get_user_pages((unsigned long)addr, nr_pages,
+   write ? FOLL_WRITE : 0, NULL, NULL);
/*
 * get_user_pages() returns number of pages gotten.
 * 0 means we failed to fault in and get anything,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 887483b..dcaf691 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -555,10 +555,13 @@ struct amdgpu_ttm_tt {
 int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages)
 {
struct amdgpu_ttm_tt *gtt = (void *)ttm;
-   int write = !(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY);
+   unsigned int flags = 0;
unsigned pinned = 0;
int r;
 
+   if (!(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY))
+   flags |= F

[PATCH 05/10] mm: replace get_vaddr_frames() write/force parameters with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from get_vaddr_frames() and
replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit
in callers as use of this flag can result in surprising behaviour (and hence
bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 drivers/gpu/drm/exynos/exynos_drm_g2d.c|  3 ++-
 drivers/media/platform/omap/omap_vout.c|  2 +-
 drivers/media/v4l2-core/videobuf2-memops.c |  6 +-
 include/linux/mm.h |  2 +-
 mm/frame_vector.c  | 13 ++---
 5 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c 
b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index aa92dec..fbd13fa 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -488,7 +488,8 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct 
drm_device *drm_dev,
goto err_free;
}
 
-   ret = get_vaddr_frames(start, npages, true, true, g2d_userptr->vec);
+   ret = get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRITE,
+   g2d_userptr->vec);
if (ret != npages) {
DRM_ERROR("failed to get user pages from userptr.\n");
if (ret < 0)
diff --git a/drivers/media/platform/omap/omap_vout.c 
b/drivers/media/platform/omap/omap_vout.c
index e668dde..a31b95c 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -214,7 +214,7 @@ static int omap_vout_get_userptr(struct videobuf_buffer 
*vb, u32 virtp,
if (!vec)
return -ENOMEM;
 
-   ret = get_vaddr_frames(virtp, 1, true, false, vec);
+   ret = get_vaddr_frames(virtp, 1, FOLL_WRITE, vec);
if (ret != 1) {
frame_vector_destroy(vec);
return -EINVAL;
diff --git a/drivers/media/v4l2-core/videobuf2-memops.c 
b/drivers/media/v4l2-core/videobuf2-memops.c
index 3c3b517..1cd322e 100644
--- a/drivers/media/v4l2-core/videobuf2-memops.c
+++ b/drivers/media/v4l2-core/videobuf2-memops.c
@@ -42,6 +42,10 @@ struct frame_vector *vb2_create_framevec(unsigned long start,
unsigned long first, last;
unsigned long nr;
struct frame_vector *vec;
+   unsigned int flags = FOLL_FORCE;
+
+   if (write)
+   flags |= FOLL_WRITE;
 
first = start >> PAGE_SHIFT;
last = (start + length - 1) >> PAGE_SHIFT;
@@ -49,7 +53,7 @@ struct frame_vector *vb2_create_framevec(unsigned long start,
vec = frame_vector_create(nr);
if (!vec)
return ERR_PTR(-ENOMEM);
-   ret = get_vaddr_frames(start & PAGE_MASK, nr, write, true, vec);
+   ret = get_vaddr_frames(start & PAGE_MASK, nr, flags, vec);
if (ret < 0)
goto out_destroy;
/* We accept only complete set of PFNs */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 27ab538..5ff084f6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1305,7 +1305,7 @@ struct frame_vector {
 struct frame_vector *frame_vector_create(unsigned int nr_frames);
 void frame_vector_destroy(struct frame_vector *vec);
 int get_vaddr_frames(unsigned long start, unsigned int nr_pfns,
-bool write, bool force, struct frame_vector *vec);
+unsigned int gup_flags, struct frame_vector *vec);
 void put_vaddr_frames(struct frame_vector *vec);
 int frame_vector_to_pages(struct frame_vector *vec);
 void frame_vector_to_pfns(struct frame_vector *vec);
diff --git a/mm/frame_vector.c b/mm/frame_vector.c
index 81b6749..db77dcb 100644
--- a/mm/frame_vector.c
+++ b/mm/frame_vector.c
@@ -11,10 +11,7 @@
  * get_vaddr_frames() - map virtual addresses to pfns
  * @start: starting user address
  * @nr_frames: number of pages / pfns from start to map
- * @write: whether pages will be written to by the caller
- * @force: whether to force write access even if user mapping is
- * readonly. See description of the same argument of
-   get_user_pages().
+ * @gup_flags: flags modifying lookup behaviour
  * @vec:   structure which receives pages / pfns of the addresses mapped.
  * It should have space for at least nr_frames entries.
  *
@@ -34,23 +31,17 @@
  * This function takes care of grabbing mmap_sem as necessary.
  */
 int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
-bool write, bool force, struct frame_vector *vec)
+unsigned int gup_flags, struct frame_vector *vec)
 {
struct mm_struct *mm = current->mm;
struct vm_area_struct *vma;
int ret = 0;
int err;
int locked;
-   unsigned int gup_flags = 0;
 
if (nr_frames == 0)
return 0;
 
-   if (write)
-   gup_flags |= FOLL_WRITE;
-   if (force)
-

[PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write parameter from __access_remote_vm() and replaces it
with a gup_flags parameter as use of this function previously _implied_
FOLL_FORCE, whereas after this patch callers explicitly pass this flag.

We make this explicit as use of FOLL_FORCE can result in surprising behaviour
(and hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 mm/memory.c | 23 +++
 mm/nommu.c  |  9 ++---
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 20a9adb..79ebed3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3869,14 +3869,11 @@ EXPORT_SYMBOL_GPL(generic_access_phys);
  * given task for page fault accounting.
  */
 static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
-   unsigned long addr, void *buf, int len, int write)
+   unsigned long addr, void *buf, int len, unsigned int gup_flags)
 {
struct vm_area_struct *vma;
void *old_buf = buf;
-   unsigned int flags = FOLL_FORCE;
-
-   if (write)
-   flags |= FOLL_WRITE;
+   int write = gup_flags & FOLL_WRITE;
 
down_read(>mmap_sem);
/* ignore errors, just check how much was successfully transferred */
@@ -3886,7 +3883,7 @@ static int __access_remote_vm(struct task_struct *tsk, 
struct mm_struct *mm,
struct page *page = NULL;
 
ret = get_user_pages_remote(tsk, mm, addr, 1,
-   flags, , );
+   gup_flags, , );
if (ret <= 0) {
 #ifndef CONFIG_HAVE_IOREMAP_PROT
break;
@@ -3945,7 +3942,12 @@ static int __access_remote_vm(struct task_struct *tsk, 
struct mm_struct *mm,
 int access_remote_vm(struct mm_struct *mm, unsigned long addr,
void *buf, int len, int write)
 {
-   return __access_remote_vm(NULL, mm, addr, buf, len, write);
+   unsigned int flags = FOLL_FORCE;
+
+   if (write)
+   flags |= FOLL_WRITE;
+
+   return __access_remote_vm(NULL, mm, addr, buf, len, flags);
 }
 
 /*
@@ -3958,12 +3960,17 @@ int access_process_vm(struct task_struct *tsk, unsigned 
long addr,
 {
struct mm_struct *mm;
int ret;
+   unsigned int flags = FOLL_FORCE;
 
mm = get_task_mm(tsk);
if (!mm)
return 0;
 
-   ret = __access_remote_vm(tsk, mm, addr, buf, len, write);
+   if (write)
+   flags |= FOLL_WRITE;
+
+   ret = __access_remote_vm(tsk, mm, addr, buf, len, flags);
+
mmput(mm);
 
return ret;
diff --git a/mm/nommu.c b/mm/nommu.c
index 70cb844..bde7df3 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1809,9 +1809,10 @@ void filemap_map_pages(struct fault_env *fe,
 EXPORT_SYMBOL(filemap_map_pages);
 
 static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
-   unsigned long addr, void *buf, int len, int write)
+   unsigned long addr, void *buf, int len, unsigned int gup_flags)
 {
struct vm_area_struct *vma;
+   int write = gup_flags & FOLL_WRITE;
 
down_read(>mmap_sem);
 
@@ -1853,7 +1854,8 @@ static int __access_remote_vm(struct task_struct *tsk, 
struct mm_struct *mm,
 int access_remote_vm(struct mm_struct *mm, unsigned long addr,
void *buf, int len, int write)
 {
-   return __access_remote_vm(NULL, mm, addr, buf, len, write);
+   return __access_remote_vm(NULL, mm, addr, buf, len,
+   write ? FOLL_WRITE : 0);
 }
 
 /*
@@ -1871,7 +1873,8 @@ int access_process_vm(struct task_struct *tsk, unsigned 
long addr, void *buf, in
if (!mm)
return 0;
 
-   len = __access_remote_vm(tsk, mm, addr, buf, len, write);
+   len = __access_remote_vm(tsk, mm, addr, buf, len,
+   write ? FOLL_WRITE : 0);
 
mmput(mm);
return len;
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/10] mm: replace access_remote_vm() write parameter with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write parameter from access_remote_vm() and replaces it
with a gup_flags parameter as use of this function previously _implied_
FOLL_FORCE, whereas after this patch callers explicitly pass this flag.

We make this explicit as use of FOLL_FORCE can result in surprising behaviour
(and hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 fs/proc/base.c | 19 +--
 include/linux/mm.h |  2 +-
 mm/memory.c| 11 +++
 mm/nommu.c |  7 +++
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index c2964d8..8e65446 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -252,7 +252,7 @@ static ssize_t proc_pid_cmdline_read(struct file *file, 
char __user *buf,
 * Inherently racy -- command line shares address space
 * with code and data.
 */
-   rv = access_remote_vm(mm, arg_end - 1, , 1, 0);
+   rv = access_remote_vm(mm, arg_end - 1, , 1, FOLL_FORCE);
if (rv <= 0)
goto out_free_page;
 
@@ -270,7 +270,8 @@ static ssize_t proc_pid_cmdline_read(struct file *file, 
char __user *buf,
int nr_read;
 
_count = min3(count, len, PAGE_SIZE);
-   nr_read = access_remote_vm(mm, p, page, _count, 0);
+   nr_read = access_remote_vm(mm, p, page, _count,
+   FOLL_FORCE);
if (nr_read < 0)
rv = nr_read;
if (nr_read <= 0)
@@ -305,7 +306,8 @@ static ssize_t proc_pid_cmdline_read(struct file *file, 
char __user *buf,
bool final;
 
_count = min3(count, len, PAGE_SIZE);
-   nr_read = access_remote_vm(mm, p, page, _count, 0);
+   nr_read = access_remote_vm(mm, p, page, _count,
+   FOLL_FORCE);
if (nr_read < 0)
rv = nr_read;
if (nr_read <= 0)
@@ -354,7 +356,8 @@ static ssize_t proc_pid_cmdline_read(struct file *file, 
char __user *buf,
bool final;
 
_count = min3(count, len, PAGE_SIZE);
-   nr_read = access_remote_vm(mm, p, page, _count, 0);
+   nr_read = access_remote_vm(mm, p, page, _count,
+   FOLL_FORCE);
if (nr_read < 0)
rv = nr_read;
if (nr_read <= 0)
@@ -832,6 +835,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
unsigned long addr = *ppos;
ssize_t copied;
char *page;
+   unsigned int flags = FOLL_FORCE;
 
if (!mm)
return 0;
@@ -844,6 +848,9 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
if (!atomic_inc_not_zero(>mm_users))
goto free;
 
+   if (write)
+   flags |= FOLL_WRITE;
+
while (count > 0) {
int this_len = min_t(int, count, PAGE_SIZE);
 
@@ -852,7 +859,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
break;
}
 
-   this_len = access_remote_vm(mm, addr, page, this_len, write);
+   this_len = access_remote_vm(mm, addr, page, this_len, flags);
if (!this_len) {
if (!copied)
copied = -EIO;
@@ -965,7 +972,7 @@ static ssize_t environ_read(struct file *file, char __user 
*buf,
this_len = min(max_len, this_len);
 
retval = access_remote_vm(mm, (env_start + src),
-   page, this_len, 0);
+   page, this_len, FOLL_FORCE);
 
if (retval <= 0) {
ret = retval;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 2a481d3..3e5234e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1268,7 +1268,7 @@ static inline int fixup_user_fault(struct task_struct 
*tsk,
 
 extern int access_process_vm(struct task_struct *tsk, unsigned long addr, void 
*buf, int len, int write);
 extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
-   void *buf, int len, int write);
+   void *buf, int len, unsigned int gup_flags);
 
 long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
  unsigned long start, unsigned long nr_pages,
diff --git a/mm/memory.c b/mm/memory.c
index 79ebed3..bac2d99 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3935,19 +3935,14 @@ static int __access_remote_vm(struct task_struct *tsk, 
struct mm_struct *mm,
  * @addr:  start address to access
  * @buf:   source or destination buffer
  * @len:   nu

[PATCH 07/10] mm: replace get_user_pages_remote() write/force parameters with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from get_user_pages_remote()
and replaces them with a gup_flags parameter to make the use of FOLL_FORCE
explicit in callers as use of this flag can result in surprising behaviour (and
hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c   |  7 +--
 drivers/gpu/drm/i915/i915_gem_userptr.c |  6 +-
 drivers/infiniband/core/umem_odp.c  |  7 +--
 fs/exec.c   |  9 +++--
 include/linux/mm.h  |  2 +-
 kernel/events/uprobes.c |  6 --
 mm/gup.c| 22 +++---
 mm/memory.c |  6 +-
 security/tomoyo/domain.c|  2 +-
 9 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c 
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 5ce3603..0370b84 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -748,19 +748,22 @@ static struct page **etnaviv_gem_userptr_do_get_pages(
int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
struct page **pvec;
uintptr_t ptr;
+   unsigned int flags = 0;
 
pvec = drm_malloc_ab(npages, sizeof(struct page *));
if (!pvec)
return ERR_PTR(-ENOMEM);
 
+   if (!etnaviv_obj->userptr.ro)
+   flags |= FOLL_WRITE;
+
pinned = 0;
ptr = etnaviv_obj->userptr.ptr;
 
down_read(>mmap_sem);
while (pinned < npages) {
ret = get_user_pages_remote(task, mm, ptr, npages - pinned,
-   !etnaviv_obj->userptr.ro, 0,
-   pvec + pinned, NULL);
+   flags, pvec + pinned, NULL);
if (ret < 0)
break;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c 
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index e537930..c6f780f 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -508,6 +508,10 @@ __i915_gem_userptr_get_pages_worker(struct work_struct 
*_work)
pvec = drm_malloc_gfp(npages, sizeof(struct page *), GFP_TEMPORARY);
if (pvec != NULL) {
struct mm_struct *mm = obj->userptr.mm->mm;
+   unsigned int flags = 0;
+
+   if (!obj->userptr.read_only)
+   flags |= FOLL_WRITE;
 
ret = -EFAULT;
if (atomic_inc_not_zero(>mm_users)) {
@@ -517,7 +521,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct 
*_work)
(work->task, mm,
 obj->userptr.ptr + pinned * PAGE_SIZE,
 npages - pinned,
-!obj->userptr.read_only, 0,
+flags,
 pvec + pinned, NULL);
if (ret < 0)
break;
diff --git a/drivers/infiniband/core/umem_odp.c 
b/drivers/infiniband/core/umem_odp.c
index 75077a0..1f0fe32 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -527,6 +527,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 
user_virt, u64 bcnt,
u64 off;
int j, k, ret = 0, start_idx, npages = 0;
u64 base_virt_addr;
+   unsigned int flags = 0;
 
if (access_mask == 0)
return -EINVAL;
@@ -556,6 +557,9 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 
user_virt, u64 bcnt,
goto out_put_task;
}
 
+   if (access_mask & ODP_WRITE_ALLOWED_BIT)
+   flags |= FOLL_WRITE;
+
start_idx = (user_virt - ib_umem_start(umem)) >> PAGE_SHIFT;
k = start_idx;
 
@@ -574,8 +578,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 
user_virt, u64 bcnt,
 */
npages = get_user_pages_remote(owning_process, owning_mm,
user_virt, gup_num_pages,
-   access_mask & ODP_WRITE_ALLOWED_BIT,
-   0, local_page_list, NULL);
+   flags, local_page_list, NULL);
up_read(_mm->mmap_sem);
 
if (npages < 0)
diff --git a/fs/exec.c b/fs/exec.c
index 6fcfb3f..4e497b9 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -191,6 +191,7 @@ static struct page *get_arg_page(struct linux_binprm *bprm, 
unsigned long pos,
 {
struct page *page;
int ret;
+   unsigned int gup_flags = FOLL_FORCE;
 
 #ifdef CONFIG_STACK_GROWSUP
if (write) {
@@ -199,12 +20

[PATCH 01/10] mm: remove write/force parameters from __get_user_pages_locked()

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from __get_user_pages_locked()
to make the use of FOLL_FORCE explicit in callers as use of this flag can result
in surprising behaviour (and hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 mm/gup.c | 47 +--
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 96b2b2f..ba83942 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -729,7 +729,6 @@ static __always_inline long __get_user_pages_locked(struct 
task_struct *tsk,
struct mm_struct *mm,
unsigned long start,
unsigned long nr_pages,
-   int write, int force,
struct page **pages,
struct vm_area_struct **vmas,
int *locked, bool notify_drop,
@@ -747,10 +746,6 @@ static __always_inline long __get_user_pages_locked(struct 
task_struct *tsk,
 
if (pages)
flags |= FOLL_GET;
-   if (write)
-   flags |= FOLL_WRITE;
-   if (force)
-   flags |= FOLL_FORCE;
 
pages_done = 0;
lock_dropped = false;
@@ -846,9 +841,15 @@ long get_user_pages_locked(unsigned long start, unsigned 
long nr_pages,
   int write, int force, struct page **pages,
   int *locked)
 {
+   unsigned int flags = FOLL_TOUCH;
+
+   if (write)
+   flags |= FOLL_WRITE;
+   if (force)
+   flags |= FOLL_FORCE;
+
return __get_user_pages_locked(current, current->mm, start, nr_pages,
-  write, force, pages, NULL, locked, true,
-  FOLL_TOUCH);
+  pages, NULL, locked, true, flags);
 }
 EXPORT_SYMBOL(get_user_pages_locked);
 
@@ -869,9 +870,15 @@ __always_inline long __get_user_pages_unlocked(struct 
task_struct *tsk, struct m
 {
long ret;
int locked = 1;
+
+   if (write)
+   gup_flags |= FOLL_WRITE;
+   if (force)
+   gup_flags |= FOLL_FORCE;
+
down_read(>mmap_sem);
-   ret = __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
- pages, NULL, , false, gup_flags);
+   ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL,
+ , false, gup_flags);
if (locked)
up_read(>mmap_sem);
return ret;
@@ -963,9 +970,15 @@ long get_user_pages_remote(struct task_struct *tsk, struct 
mm_struct *mm,
int write, int force, struct page **pages,
struct vm_area_struct **vmas)
 {
-   return __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
-  pages, vmas, NULL, false,
-  FOLL_TOUCH | FOLL_REMOTE);
+   unsigned int flags = FOLL_TOUCH | FOLL_REMOTE;
+
+   if (write)
+   flags |= FOLL_WRITE;
+   if (force)
+   flags |= FOLL_FORCE;
+
+   return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas,
+  NULL, false, flags);
 }
 EXPORT_SYMBOL(get_user_pages_remote);
 
@@ -979,9 +992,15 @@ long get_user_pages(unsigned long start, unsigned long 
nr_pages,
int write, int force, struct page **pages,
struct vm_area_struct **vmas)
 {
+   unsigned int flags = FOLL_TOUCH;
+
+   if (write)
+   flags |= FOLL_WRITE;
+   if (force)
+   flags |= FOLL_FORCE;
+
return __get_user_pages_locked(current, current->mm, start, nr_pages,
-  write, force, pages, vmas, NULL, false,
-  FOLL_TOUCH);
+  pages, vmas, NULL, false, flags);
 }
 EXPORT_SYMBOL(get_user_pages);
 
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/10] mm: replace get_user_pages_unlocked() write/force parameters with gup_flags

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from get_user_pages_unlocked()
and replaces them with a gup_flags parameter to make the use of FOLL_FORCE
explicit in callers as use of this flag can result in surprising behaviour (and
hence bugs) within the mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 arch/mips/mm/gup.c |  2 +-
 arch/s390/mm/gup.c |  3 ++-
 arch/sh/mm/gup.c   |  3 ++-
 arch/sparc/mm/gup.c|  3 ++-
 arch/x86/mm/gup.c  |  2 +-
 drivers/media/pci/ivtv/ivtv-udma.c |  4 ++--
 drivers/media/pci/ivtv/ivtv-yuv.c  |  5 +++--
 drivers/scsi/st.c  |  5 ++---
 drivers/video/fbdev/pvr2fb.c   |  4 ++--
 include/linux/mm.h |  2 +-
 mm/gup.c   | 14 --
 mm/nommu.c | 11 ++-
 mm/util.c  |  3 ++-
 net/ceph/pagevec.c |  2 +-
 14 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/arch/mips/mm/gup.c b/arch/mips/mm/gup.c
index 42d124f..d8c3c15 100644
--- a/arch/mips/mm/gup.c
+++ b/arch/mips/mm/gup.c
@@ -287,7 +287,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages, 
int write,
pages += nr;
 
ret = get_user_pages_unlocked(start, (end - start) >> PAGE_SHIFT,
- write, 0, pages);
+ pages, write ? FOLL_WRITE : 0);
 
/* Have to be a bit careful with return values */
if (nr > 0) {
diff --git a/arch/s390/mm/gup.c b/arch/s390/mm/gup.c
index adb0c34..18d4107 100644
--- a/arch/s390/mm/gup.c
+++ b/arch/s390/mm/gup.c
@@ -266,7 +266,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, 
int write,
/* Try to get the remaining pages with get_user_pages */
start += nr << PAGE_SHIFT;
pages += nr;
-   ret = get_user_pages_unlocked(start, nr_pages - nr, write, 0, pages);
+   ret = get_user_pages_unlocked(start, nr_pages - nr, pages,
+ write ? FOLL_WRITE : 0);
/* Have to be a bit careful with return values */
if (nr > 0)
ret = (ret < 0) ? nr : ret + nr;
diff --git a/arch/sh/mm/gup.c b/arch/sh/mm/gup.c
index 40fa6c8..063c298 100644
--- a/arch/sh/mm/gup.c
+++ b/arch/sh/mm/gup.c
@@ -258,7 +258,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, 
int write,
pages += nr;
 
ret = get_user_pages_unlocked(start,
-   (end - start) >> PAGE_SHIFT, write, 0, pages);
+   (end - start) >> PAGE_SHIFT, pages,
+   write ? FOLL_WRITE : 0);
 
/* Have to be a bit careful with return values */
if (nr > 0) {
diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
index 4e06750..cd0e32b 100644
--- a/arch/sparc/mm/gup.c
+++ b/arch/sparc/mm/gup.c
@@ -238,7 +238,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, 
int write,
pages += nr;
 
ret = get_user_pages_unlocked(start,
-   (end - start) >> PAGE_SHIFT, write, 0, pages);
+   (end - start) >> PAGE_SHIFT, pages,
+   write ? FOLL_WRITE : 0);
 
/* Have to be a bit careful with return values */
if (nr > 0) {
diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
index b8b6a60..0d4fb3e 100644
--- a/arch/x86/mm/gup.c
+++ b/arch/x86/mm/gup.c
@@ -435,7 +435,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages, 
int write,
 
ret = get_user_pages_unlocked(start,
  (end - start) >> PAGE_SHIFT,
- write, 0, pages);
+ pages, write ? FOLL_WRITE : 0);
 
/* Have to be a bit careful with return values */
if (nr > 0) {
diff --git a/drivers/media/pci/ivtv/ivtv-udma.c 
b/drivers/media/pci/ivtv/ivtv-udma.c
index 4769469..2c9232e 100644
--- a/drivers/media/pci/ivtv/ivtv-udma.c
+++ b/drivers/media/pci/ivtv/ivtv-udma.c
@@ -124,8 +124,8 @@ int ivtv_udma_setup(struct ivtv *itv, unsigned long 
ivtv_dest_addr,
}
 
/* Get user pages for DMA Xfer */
-   err = get_user_pages_unlocked(user_dma.uaddr, user_dma.page_count, 0,
-   1, dma->map);
+   err = get_user_pages_unlocked(user_dma.uaddr, user_dma.page_count,
+   dma->map, FOLL_FORCE);
 
if (user_dma.page_count != err) {
IVTV_DEBUG_WARN("failed to map user pages, returned %d instead 
of %d\n",
diff --git a/drivers/media/pci/ivtv/ivtv-yuv.c 
b/drivers/media/pci/ivtv/ivtv-yuv.c
index b094054..f7299d3 100644
--- a/drivers/media/pci/ivtv/ivtv-yuv.c
+++ b/drivers/media/pci/ivtv/ivtv-yu

[PATCH 02/10] mm: remove write/force parameters from __get_user_pages_unlocked()

2016-10-12 Thread Lorenzo Stoakes 
This patch removes the write and force parameters from
__get_user_pages_unlocked() to make the use of FOLL_FORCE explicit in callers as
use of this flag can result in surprising behaviour (and hence bugs) within the
mm subsystem.

Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
---
 include/linux/mm.h |  3 +--
 mm/gup.c   | 17 +
 mm/nommu.c | 12 +---
 mm/process_vm_access.c |  7 +--
 virt/kvm/async_pf.c|  3 ++-
 virt/kvm/kvm_main.c| 11 ---
 6 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e9caec6..2db98b6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1285,8 +1285,7 @@ long get_user_pages_locked(unsigned long start, unsigned 
long nr_pages,
int write, int force, struct page **pages, int *locked);
 long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm,
   unsigned long start, unsigned long nr_pages,
-  int write, int force, struct page **pages,
-  unsigned int gup_flags);
+  struct page **pages, unsigned int gup_flags);
 long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
int write, int force, struct page **pages);
 int get_user_pages_fast(unsigned long start, int nr_pages, int write,
diff --git a/mm/gup.c b/mm/gup.c
index ba83942..3d620dd 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -865,17 +865,11 @@ EXPORT_SYMBOL(get_user_pages_locked);
  */
 __always_inline long __get_user_pages_unlocked(struct task_struct *tsk, struct 
mm_struct *mm,
   unsigned long start, unsigned 
long nr_pages,
-  int write, int force, struct 
page **pages,
-  unsigned int gup_flags)
+  struct page **pages, unsigned 
int gup_flags)
 {
long ret;
int locked = 1;
 
-   if (write)
-   gup_flags |= FOLL_WRITE;
-   if (force)
-   gup_flags |= FOLL_FORCE;
-
down_read(>mmap_sem);
ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL,
  , false, gup_flags);
@@ -905,8 +899,15 @@ EXPORT_SYMBOL(__get_user_pages_unlocked);
 long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
 int write, int force, struct page **pages)
 {
+   unsigned int flags = FOLL_TOUCH;
+
+   if (write)
+   flags |= FOLL_WRITE;
+   if (force)
+   flags |= FOLL_FORCE;
+
return __get_user_pages_unlocked(current, current->mm, start, nr_pages,
-write, force, pages, FOLL_TOUCH);
+pages, flags);
 }
 EXPORT_SYMBOL(get_user_pages_unlocked);
 
diff --git a/mm/nommu.c b/mm/nommu.c
index 95daf81..925dcc1 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -185,8 +185,7 @@ EXPORT_SYMBOL(get_user_pages_locked);
 
 long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm,
   unsigned long start, unsigned long nr_pages,
-  int write, int force, struct page **pages,
-  unsigned int gup_flags)
+  struct page **pages, unsigned int gup_flags)
 {
long ret;
down_read(>mmap_sem);
@@ -200,8 +199,15 @@ EXPORT_SYMBOL(__get_user_pages_unlocked);
 long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
 int write, int force, struct page **pages)
 {
+   unsigned int flags = 0;
+
+   if (write)
+   flags |= FOLL_WRITE;
+   if (force)
+   flags |= FOLL_FORCE;
+
return __get_user_pages_unlocked(current, current->mm, start, nr_pages,
-write, force, pages, 0);
+pages, flags);
 }
 EXPORT_SYMBOL(get_user_pages_unlocked);
 
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index 07514d4..be8dc8d 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -88,12 +88,16 @@ static int process_vm_rw_single_vec(unsigned long addr,
ssize_t rc = 0;
unsigned long max_pages_per_loop = PVM_MAX_KMALLOC_PAGES
/ sizeof(struct pages *);
+   unsigned int flags = FOLL_REMOTE;
 
/* Work out address and page range required */
if (len == 0)
return 0;
nr_pages = (addr + len - 1) / PAGE_SIZE - addr / PAGE_SIZE + 1;
 
+   if (vm_write)
+   flags |= FOLL_WRITE;
+
while (!rc && nr_pages && iov_iter_count(iter)) {
int pages = min(nr_pages, max_pages_per_loop);

[PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags

2016-10-12 Thread Lorenzo Stoakes 
This patch series adjusts functions in the get_user_pages* family such that
desired FOLL_* flags are passed as an argument rather than implied by flags.

The purpose of this change is to make the use of FOLL_FORCE explicit so it is
easier to grep for and clearer to callers that this flag is being used. The use
of FOLL_FORCE is an issue as it overrides missing VM_READ/VM_WRITE flags for the
VMA whose pages we are reading from/writing to, which can result in surprising
behaviour.

The patch series came out of the discussion around commit 38e0885, which
addressed a BUG_ON() being triggered when a page was faulted in with PROT_NONE
set but having been overridden by FOLL_FORCE. do_numa_page() was run on the
assumption the page _must_ be one marked for NUMA node migration as an actual
PROT_NONE page would have been dealt with prior to this code path, however
FOLL_FORCE introduced a situation where this assumption did not hold.

See https://marc.info/?l=linux-mm=147585445805166 for the patch proposal.

Lorenzo Stoakes (10):
  mm: remove write/force parameters from __get_user_pages_locked()
  mm: remove write/force parameters from __get_user_pages_unlocked()
  mm: replace get_user_pages_unlocked() write/force parameters with gup_flags
  mm: replace get_user_pages_locked() write/force parameters with gup_flags
  mm: replace get_vaddr_frames() write/force parameters with gup_flags
  mm: replace get_user_pages() write/force parameters with gup_flags
  mm: replace get_user_pages_remote() write/force parameters with gup_flags
  mm: replace __access_remote_vm() write parameter with gup_flags
  mm: replace access_remote_vm() write parameter with gup_flags
  mm: replace access_process_vm() write parameter with gup_flags

 arch/alpha/kernel/ptrace.c |  9 ++--
 arch/blackfin/kernel/ptrace.c  |  5 ++-
 arch/cris/arch-v32/drivers/cryptocop.c |  4 +-
 arch/cris/arch-v32/kernel/ptrace.c |  4 +-
 arch/ia64/kernel/err_inject.c  |  2 +-
 arch/ia64/kernel/ptrace.c  | 14 +++---
 arch/m32r/kernel/ptrace.c  | 15 ---
 arch/mips/kernel/ptrace32.c|  5 ++-
 arch/mips/mm/gup.c |  2 +-
 arch/powerpc/kernel/ptrace32.c |  5 ++-
 arch/s390/mm/gup.c |  3 +-
 arch/score/kernel/ptrace.c | 10 +++--
 arch/sh/mm/gup.c   |  3 +-
 arch/sparc/kernel/ptrace_64.c  | 24 +++
 arch/sparc/mm/gup.c|  3 +-
 arch/x86/kernel/step.c |  3 +-
 arch/x86/mm/gup.c  |  2 +-
 arch/x86/mm/mpx.c  |  5 +--
 arch/x86/um/ptrace_32.c|  3 +-
 arch/x86/um/ptrace_64.c|  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c|  7 ++-
 drivers/gpu/drm/etnaviv/etnaviv_gem.c  |  7 ++-
 drivers/gpu/drm/exynos/exynos_drm_g2d.c|  3 +-
 drivers/gpu/drm/i915/i915_gem_userptr.c|  6 ++-
 drivers/gpu/drm/radeon/radeon_ttm.c|  3 +-
 drivers/gpu/drm/via/via_dmablit.c  |  4 +-
 drivers/infiniband/core/umem.c |  6 ++-
 drivers/infiniband/core/umem_odp.c |  7 ++-
 drivers/infiniband/hw/mthca/mthca_memfree.c|  2 +-
 drivers/infiniband/hw/qib/qib_user_pages.c |  3 +-
 drivers/infiniband/hw/usnic/usnic_uiom.c   |  5 ++-
 drivers/media/pci/ivtv/ivtv-udma.c |  4 +-
 drivers/media/pci/ivtv/ivtv-yuv.c  |  5 ++-
 drivers/media/platform/omap/omap_vout.c|  2 +-
 drivers/media/v4l2-core/videobuf-dma-sg.c  |  7 ++-
 drivers/media/v4l2-core/videobuf2-memops.c |  6 ++-
 drivers/misc/mic/scif/scif_rma.c   |  3 +-
 drivers/misc/sgi-gru/grufault.c|  2 +-
 drivers/platform/goldfish/goldfish_pipe.c  |  3 +-
 drivers/rapidio/devices/rio_mport_cdev.c   |  3 +-
 drivers/scsi/st.c  |  5 +--
 .../interface/vchiq_arm/vchiq_2835_arm.c   |  3 +-
 .../vc04_services/interface/vchiq_arm/vchiq_arm.c  |  3 +-
 drivers/video/fbdev/pvr2fb.c   |  4 +-
 drivers/virt/fsl_hypervisor.c  |  4 +-
 fs/exec.c  |  9 +++-
 fs/proc/base.c | 19 +---
 include/linux/mm.h | 18 
 kernel/events/uprobes.c|  6 ++-
 kernel/ptrace.c| 16 ---
 mm/frame_vector.c  |  9 ++--
 mm/gup.c   | 50 ++
 mm/memory.c| 16