lkml.org 
[lkml]   [2012]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] fix bug introduced in "mm: simplify find_vma_prev()"
(3/6/12 1:57 PM), Mikulas Patocka wrote:
>
>
> On Sun, 4 Mar 2012, Linus Torvalds wrote:
>
>> On Sun, Mar 4, 2012 at 4:52 PM, Mikulas Patocka<mpatocka@redhat.com> wrote:
>>>
>>> This patch fixes a bug introduced in "mm: simplify find_vma_prev()". You
>>> can apply this, or alternatively revert the original patch.
>>
>> Hmm.
>>
>> I realize the patch is a bug-fix, but I do wonder if we shouldn't look
>> at alternatives.
>>
>> In particular, how about we just change the rule to be clearer, and
>> make the rule be:
>>
>> - no more find_vma_prev() AT ALL.
>>
>> - callers get turned into "find_vma()" and then we have a
>> "vma_prev()", which basically does your thing.
>>
>> - many of the callers seem to be interested in "prev" *only* if they
>> found a vma. For example, the do_mlock() kind of logic seems to be
>> common:
>>
>> vma = find_vma_prev(current->mm, start,&prev);
>> if (!vma || vma->vm_start> start)
>> return -ENOMEM;
>>
>> which implies that the current find_vma_prev() semantics are really
>> wrong, because they force us to do extra work in this case where we
>> really really don't care about the result.
>>
>> So we could do an entirely mechanical conversion of
>>
>> vma = find_vma_prev(mm, address,&prev_vma);
>>
>> into
>>
>> vma = find_vma(mm, address);
>> prev_vma = vma_prev(vma);
>>
>> and then after we've done that conversion, it looks like the bulk of
>> them will not care about "prev_vma" if vma is NULL, so they can then
>> be replaced with a pure
>>
>> prev_vma = vma->vm_prev;
>>
>> instead. Leaving just the (few) cases that may care about the
>> "previous vma may be the last vma of the VM" case.
>>
>> Hmm? And we'd get away from that horrible "find_vma_prev()" interface
>> that uses pointers to vma pointers for return values. Ugh. There only
>> seems to be nine callers in the whole kernel.
>>
>> Linus
>
> You can try this to remove most users of find_vma_prev (only three are
> left --- those dealing with up-growing stack). But the patch should be
> delayed until the next merge window.
>
> Mikulas
>

Thank you, Mikulas for finding and fixing the bug. And sorry for the long
delay. I was offlined a while.

Following is the replacing last three caller of find_vma_prev. oh, I use
find_last_vma() instead vma_prev(vma) because parisc need last vma search
when Milulas's original testcase. Does this make sense to you? if yes, we
can remove find_vma_prev() completely.



=========================
Subject: [PATCH] mm: introduce find_last_vma()

Cc: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
arch/ia64/mm/fault.c | 20 +++++++++++++-------
arch/parisc/mm/fault.c | 6 +++---
include/linux/mm.h | 3 +++
mm/mmap.c | 32 +++++++++++++++++++++++++++++++-
4 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 20b3593..27ca30c 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -112,18 +112,16 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re

down_read(&mm->mmap_sem);

- vma = find_vma_prev(mm, address, &prev_vma);
- if (!vma && !prev_vma )
- goto bad_area;
+ vma = find_vma(mm, address);

- /*
- * find_vma_prev() returns vma such that address < vma->vm_end or NULL
+ /*
+ * find_vma() returns vma such that address < vma->vm_end or NULL
*
* May find no vma, but could be that the last vm area is the
* register backing store that needs to expand upwards, in
- * this case vma will be null, but prev_vma will ne non-null
+ * this case vma will be null and a stack need to be expanded.
*/
- if (( !vma && prev_vma ) || (address < vma->vm_start) )
+ if (!vma || (address < vma->vm_start))
goto check_expansion;

good_area:
@@ -177,6 +175,14 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
return;

check_expansion:
+ if (vma)
+ prev_vma = vma->vm_prev;
+ else {
+ prev_vma = find_last_vma(mm);
+ if (!prev_vma)
+ goto bad_area;
+ }
+
if (!(prev_vma && (prev_vma->vm_flags & VM_GROWSUP) && (address == prev_vma->vm_end))) {
if (!vma)
goto bad_area;
diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index 18162ce..7b5a466 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -170,7 +170,7 @@ int fixup_exception(struct pt_regs *regs)
void do_page_fault(struct pt_regs *regs, unsigned long code,
unsigned long address)
{
- struct vm_area_struct *vma, *prev_vma;
+ struct vm_area_struct *vma;
struct task_struct *tsk = current;
struct mm_struct *mm = tsk->mm;
unsigned long acc_type;
@@ -180,7 +180,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
goto no_context;

down_read(&mm->mmap_sem);
- vma = find_vma_prev(mm, address, &prev_vma);
+ vma = find_vma(mm, address);
if (!vma || address < vma->vm_start)
goto check_expansion;
/*
@@ -222,7 +222,7 @@ good_area:
return;

check_expansion:
- vma = prev_vma;
+ vma = vma ? vma->vm_prev : find_last_vma(mm);
if (vma && (expand_stack(vma, address) == 0))
goto good_area;

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 17b27cd..d758818 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1463,6 +1463,9 @@ extern int expand_upwards(struct vm_area_struct *vma, unsigned long address);

/* Look up the first VMA which satisfies addr < vm_end, NULL if none. */
extern struct vm_area_struct * find_vma(struct mm_struct * mm, unsigned long addr);
+#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
+extern struct vm_area_struct *find_last_vma(struct mm_struct *mm);
+#endif
extern struct vm_area_struct * find_vma_prev(struct mm_struct * mm, unsigned long addr,
struct vm_area_struct **pprev);

diff --git a/mm/mmap.c b/mm/mmap.c
index 22e1a0b..3e9c186 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1605,6 +1605,32 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)

EXPORT_SYMBOL(find_vma);

+#if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64)
+/* Look up the last VMA, NULL if mm have no vma. */
+struct vm_area_struct *find_last_vma(struct mm_struct *mm)
+{
+ struct vm_area_struct *vma = NULL;
+ struct rb_node *rb_node;
+
+ BUG_ON(!mm);
+
+ rb_node = mm->mm_rb.rb_node;
+ while (rb_node) {
+ vma = rb_entry(rb_node, struct vm_area_struct, vm_rb);
+ rb_node = rb_node->rb_right;
+ }
+
+ /*
+ * This function is mainly used for a page fault. Thus recording vma
+ * may improve the performance.
+ */
+ if (vma)
+ mm->mmap_cache = vma;
+
+ return vma;
+}
+#endif
+
/*
* Same as find_vma, but also return a pointer to the previous VMA in *pprev.
* Note: pprev is set to NULL when return value is NULL.
@@ -1789,9 +1815,13 @@ find_extend_vma(struct mm_struct *mm, unsigned long addr)
struct vm_area_struct *vma, *prev;

addr &= PAGE_MASK;
- vma = find_vma_prev(mm, addr, &prev);
+ vma = find_vma(mm, addr);
if (vma && (vma->vm_start <= addr))
return vma;
+
+ prev = vma->vm_prev;
+ if (!prev)
+ prev = find_last_vma(mm);
if (!prev || expand_stack(prev, addr))
return NULL;
if (prev->vm_flags & VM_LOCKED) {
--
1.7.1



\
 
 \ /
  Last update: 2012-03-07 01:29    [W:2.133 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site