Re: [PATCH 01/31] mm, gup: introduce concept of "foreign" get_user_pages()

[Ingo Molnar]

* Dave Hansen <dave@xxxxxxxx> wrote:

> 
> OK, so I've fixed up my build process to _actually_ build the
> nommu code.
> 
> One of Vlastimil's comments made me go dig back in to the uprobes
> code's use of get_user_pages().  I decided to change both of them
> to be "foreign" accesses.
> 
> This also fixes the nommu breakage that Vlastimil noted last time.
> 
> Srikar, I'd appreciate if you can have a look at the uprobes.c
> modifications, especially the comment.  I don't think this will
> change any behavior, but I want to make sure the comment is
> accurate.
> 
> ---
> 
> From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> 
> For protection keys, we need to understand whether protections
> should be enforced in software or not.  In general, we enforce
> protections when working on our own task, but not when on others.
> We call these "current" and "foreign" operations.
> 
> This patch introduces a new get_user_pages() variant:
> 
> 	get_user_pages_foreign()
> 
> We modify the vanilla get_user_pages() so it can no longer be
> used on mm/tasks other than 'current/current->mm', which is by
> far the most common way it is called.  Using it makes a few of
> the call sites look a bit nicer.
> 
> In other words, get_user_pages_foreign() is a replacement for
> when get_user_pages() is called on non-current tsk/mm.
> 
> This also switches get_user_pages_(un)locked() over to be like
> get_user_pages() and not take a tsk/mm.  There is no
> get_user_pages_foreign_(un)locked().  If someone wants that
> behavior they just have to use "__" variant and pass in
> FOLL_FOREIGN explicitly.
> 
> The uprobes is_trap_at_addr() location holds mmap_sem and
> calls get_user_pages(current->mm) on an instruction address.  This
> makes it a pretty unique gup caller.  Being an instruction access
> and also really originating from the kernel (vs. the app), I opted
> to consider this a 'foreign' access where protection keys will not
> be enforced.
> 
> Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Acked-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> Cc: jack@xxxxxxx
> ---
> 
>  b/arch/cris/arch-v32/drivers/cryptocop.c        |    8 ---
>  b/arch/ia64/kernel/err_inject.c                 |    3 -
>  b/arch/mips/mm/gup.c                            |    3 -
>  b/arch/s390/mm/gup.c                            |    4 -
>  b/arch/sh/mm/gup.c                              |    2 
>  b/arch/sparc/mm/gup.c                           |    2 
>  b/arch/x86/mm/gup.c                             |    2 
>  b/arch/x86/mm/mpx.c                             |    4 -
>  b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |    3 -
>  b/drivers/gpu/drm/etnaviv/etnaviv_gem.c         |    2 
>  b/drivers/gpu/drm/i915/i915_gem_userptr.c       |    2 
>  b/drivers/gpu/drm/radeon/radeon_ttm.c           |    3 -
>  b/drivers/gpu/drm/via/via_dmablit.c             |    3 -
>  b/drivers/infiniband/core/umem.c                |    2 
>  b/drivers/infiniband/core/umem_odp.c            |    8 +--
>  b/drivers/infiniband/hw/mthca/mthca_memfree.c   |    3 -
>  b/drivers/infiniband/hw/qib/qib_user_pages.c    |    3 -
>  b/drivers/infiniband/hw/usnic/usnic_uiom.c      |    2 
>  b/drivers/media/pci/ivtv/ivtv-udma.c            |    4 -
>  b/drivers/media/pci/ivtv/ivtv-yuv.c             |   10 +---
>  b/drivers/media/v4l2-core/videobuf-dma-sg.c     |    3 -
>  b/drivers/misc/mic/scif/scif_rma.c              |    2 
>  b/drivers/misc/sgi-gru/grufault.c               |    3 -
>  b/drivers/scsi/st.c                             |    2 
>  b/drivers/staging/rdma/ipath/ipath_user_pages.c |    3 -
>  b/drivers/video/fbdev/pvr2fb.c                  |    4 -
>  b/drivers/virt/fsl_hypervisor.c                 |    5 --
>  b/fs/exec.c                                     |    8 ++-
>  b/include/linux/mm.h                            |   21 +++++----
>  b/kernel/events/uprobes.c                       |   10 +++-
>  b/mm/frame_vector.c                             |    2 
>  b/mm/gup.c                                      |   52 +++++++++++++++---------
>  b/mm/ksm.c                                      |    2 
>  b/mm/memory.c                                   |    2 
>  b/mm/mempolicy.c                                |    6 +-
>  b/mm/nommu.c                                    |   30 ++++++++-----
>  b/mm/process_vm_access.c                        |   11 +++--
>  b/mm/util.c                                     |    4 -
>  b/net/ceph/pagevec.c                            |    2 
>  b/security/tomoyo/domain.c                      |    9 +++-
>  b/virt/kvm/async_pf.c                           |    7 ++-
>  b/virt/kvm/kvm_main.c                           |   10 ++--
>  42 files changed, 148 insertions(+), 123 deletions(-)

So this patch conflicts with recent upstream changes:

  patching file drivers/scsi/st.c
  can't find file to patch at input line 463

mind respinning it against v4.5-rc3 or so?

Also, please split this into three patches:

 - one patch adds the _foreign() GUP variant and applies it to code that uses it
   on remote tasks.

 - introduce the new get_user_pages() but also add macros so that both 8-parameter 
   and 7-parameter variants work without breaking the build. We can remove the 
   compatibility wrapping on v4.6 or so.

 - the third will be a large but trivial patch, which will change 8-parameter GUP 
   usage to 7-parameter usage.

... this should reduce the pain from the GUP interface change churn.

Agreed?

Thanks,

	Ingo