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

From: Ingo Molnar
Date: Tue Feb 09 2016 - 07:46:59 EST



* 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