Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161103AbaDTIEU (ORCPT ); Sun, 20 Apr 2014 04:04:20 -0400 Received: from mail-la0-f44.google.com ([209.85.215.44]:43174 "EHLO mail-la0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932110AbaDTIEE (ORCPT ); Sun, 20 Apr 2014 04:04:04 -0400 MIME-Version: 1.0 In-Reply-To: <1397960791-16320-3-git-send-email-davidlohr@hp.com> References: <1397960791-16320-1-git-send-email-davidlohr@hp.com> <1397960791-16320-3-git-send-email-davidlohr@hp.com> Date: Sun, 20 Apr 2014 10:04:02 +0200 X-Google-Sender-Auth: 4xkgZlq9SUcvdwLiFIrByfEPiUo Message-ID: Subject: Re: [PATCH 2/6] m68k: call find_vma with the mmap_sem held in sys_cacheflush() From: Geert Uytterhoeven To: Davidlohr Bueso Cc: Andrew Morton , zeus@gnu.org, Aswin Chandramouleeswaran , Linux MM , "linux-kernel@vger.kernel.org" , linux-m68k Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, On Sun, Apr 20, 2014 at 4:26 AM, Davidlohr Bueso 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 > Cc: Geert Uytterhoeven > Cc: linux-m68k@lists.linux-m68k.org > --- > 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 -- geert@linux-m68k.org 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/