lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20161019075903.GP29967@quack2.suse.cz> Date: Wed, 19 Oct 2016 09:59:03 +0200 From: Jan Kara <jack@...e.cz> To: Lorenzo Stoakes <lstoakes@...il.com> Cc: linux-mm@...ck.org, Linus Torvalds <torvalds@...ux-foundation.org>, Jan Kara <jack@...e.cz>, Hugh Dickins <hughd@...gle.com>, Dave Hansen <dave.hansen@...ux.intel.com>, Rik van Riel <riel@...hat.com>, Mel Gorman <mgorman@...hsingularity.net>, Andrew Morton <akpm@...ux-foundation.org>, adi-buildroot-devel@...ts.sourceforge.net, ceph-devel@...r.kernel.org, dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org, kvm@...r.kernel.org, linux-alpha@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-cris-kernel@...s.com, linux-fbdev@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-ia64@...r.kernel.org, linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, linux-mips@...ux-mips.org, linux-rdma@...r.kernel.org, linux-s390@...r.kernel.org, linux-samsung-soc@...r.kernel.org, linux-scsi@...r.kernel.org, linux-security-module@...r.kernel.org, linux-sh@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, netdev@...r.kernel.org, sparclinux@...r.kernel.org, x86@...nel.org Subject: Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags 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@...il.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 > -- Jan Kara <jack@...e.com> SUSE Labs, CR
Powered by blists - more mailing lists