Hi David, On Sun, Apr 20, 2014 at 4:26 AM, Davidlohr Bueso <[email protected]> wrote: > Performing vma lookups without taking the mm->mmap_sem is asking > for trouble. While doing the search, the vma in question can be > modified or even removed before returning to the caller. Take the > lock (shared) in order to avoid races while iterating through > the vmacache and/or rbtree.
Thanks for your patch! > This patch is completely *untested*. > > Signed-off-by: Davidlohr Bueso <[email protected]> > Cc: Geert Uytterhoeven <[email protected]> > Cc: [email protected] > --- > arch/m68k/kernel/sys_m68k.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/arch/m68k/kernel/sys_m68k.c b/arch/m68k/kernel/sys_m68k.c > index 3a480b3..d2263a0 100644 > --- a/arch/m68k/kernel/sys_m68k.c > +++ b/arch/m68k/kernel/sys_m68k.c > @@ -376,7 +376,6 @@ cache_flush_060 (unsigned long addr, int scope, int > cache, unsigned long len) > asmlinkage int > sys_cacheflush (unsigned long addr, int scope, int cache, unsigned long len) > { > - struct vm_area_struct *vma; > int ret = -EINVAL; > > if (scope < FLUSH_SCOPE_LINE || scope > FLUSH_SCOPE_ALL || > @@ -389,16 +388,23 @@ sys_cacheflush (unsigned long addr, int scope, int > cache, unsigned long len) > if (!capable(CAP_SYS_ADMIN)) > goto out; > } else { > + struct vm_area_struct *vma; > + bool invalid; > + > + /* Check for overflow. */ > + if (addr + len < addr) > + goto out; > + > /* > * Verify that the specified address region actually belongs > * to this process. > */ > - vma = find_vma (current->mm, addr); > ret = -EINVAL; > - /* Check for overflow. */ > - if (addr + len < addr) > - goto out; > - if (vma == NULL || addr < vma->vm_start || addr + len > > vma->vm_end) > + down_read(¤t->mm->mmap_sem); > + vma = find_vma(current->mm, addr); > + invalid = !vma || addr < vma->vm_start || addr + len > > vma->vm_end; > + up_read(¤t->mm->mmap_sem); > + if (invalid) > goto out; > } Shouldn't the up_read() be moved to the end of the function? The vma may still be modified or destroyed between the call to find_vma(), and the actual cache flush? Am I missing something? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
