| Message ID | 20161013002020.3062-9-lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <lstoakes@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 accordingly (it is much simpler than searching for all get_user_pages() users and extracting from parameter lists what they actually pass as 'force' argument). Sure it makes somewhat more visible to callers of access_remote_vm() that they get FOLL_FORCE semantics but OTOH it also opens a space for issues where a caller of access_remote_vm() actually wants FOLL_FORCE (and currently all of them want it) and just mistakenly does not set it. All in all I'd prefer to keep access_remote_vm() and friends as is... Honza > --- > 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(&mm->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, &page, &vma); > + gup_flags, &page, &vma); > 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(&mm->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 >
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 <lstoakes@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? Anyway I would rather see the flag explicit and used at more places than hidden behind a helper function.
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 <lstoakes@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&m=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.
On Wed 19-10-16 09:40:45, Lorenzo Stoakes wrote: > 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 <lstoakes@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 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
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.
On Wed 19-10-16 10:06:46, Lorenzo Stoakes wrote: > 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. I do agree. Making them explicit will help to clean them up later, should there be a need.
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(&mm->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, &page, &vma); + gup_flags, &page, &vma); 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(&mm->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;
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 <lstoakes@gmail.com> --- mm/memory.c | 23 +++++++++++++++-------- mm/nommu.c | 9 ++++++--- 2 files changed, 21 insertions(+), 11 deletions(-)