Re: [patch] mmap: feed back correct prev vma when finding vma

[Hillf Danton <dhillf@gmail.com>]

On Tue, Aug 14, 2012 at 10:17 AM, Hugh Dickins <hughd@google.com> wrote:
> [PATCH] mm: replace find_vma_prepare by clearer find_vma_links
>
> People get confused by find_vma_prepare(), because it doesn't care about
> what it returns in its output args, when its callers won't be interested.
>
> Clarify by passing in end-of-range address too, and returning failure if
> any existing vma overlaps the new range: instead of returning an ambiguous
> vma which most callers then must check.  find_vma_links() is a clearer name.
>
> This does revert 2.6.27's dfe195fb79e88 ("mm: fix uninitialized variables
> for find_vma_prepare callers"), but it looks like gcc 4.3.0 was one of
> those releases too eager to shout about uninitialized variables: only
> copy_vma() warns with 4.5.1 and 4.7.1, which a BUG on error silences.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: Benny Halevy <bhalevy@tonian.com>


Acked-by: Hillf Danton <dhillf@gmail.com>

>  mm/mmap.c |   45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
>
> --- 3.6-rc1/mm/mmap.c   2012-08-03 08:31:27.064842271 -0700
> +++ linux/mm/mmap.c     2012-08-13 12:23:35.862895633 -0700
> @@ -356,17 +356,14 @@ void validate_mm(struct mm_struct *mm)
>  #define validate_mm(mm) do { } while (0)
>  #endif
>
> -static struct vm_area_struct *
> -find_vma_prepare(struct mm_struct *mm, unsigned long addr,
> -               struct vm_area_struct **pprev, struct rb_node ***rb_link,
> -               struct rb_node ** rb_parent)
> +static int find_vma_links(struct mm_struct *mm, unsigned long addr,
> +               unsigned long end, struct vm_area_struct **pprev,
> +               struct rb_node ***rb_link, struct rb_node **rb_parent)
>  {
> -       struct vm_area_struct * vma;
> -       struct rb_node ** __rb_link, * __rb_parent, * rb_prev;
> +       struct rb_node **__rb_link, *__rb_parent, *rb_prev;
>

Just a nitpick, we could further cut a couple of lines if
rb_prev is replaced by vma.

>         __rb_link = &mm->mm_rb.rb_node;
>         rb_prev = __rb_parent = NULL;
> -       vma = NULL;
>
>         while (*__rb_link) {
>                 struct vm_area_struct *vma_tmp;
> @@ -375,9 +372,9 @@ find_vma_prepare(struct mm_struct *mm, u
>                 vma_tmp = rb_entry(__rb_parent, struct vm_area_struct, vm_rb);
>
>                 if (vma_tmp->vm_end > addr) {
> -                       vma = vma_tmp;
> -                       if (vma_tmp->vm_start <= addr)
> -                               break;
> +                       /* Fail if an existing vma overlaps the area */
> +                       if (vma_tmp->vm_start < end)
> +                               return -ENOMEM;
>                         __rb_link = &__rb_parent->rb_left;
>                 } else {
>                         rb_prev = __rb_parent;
> @@ -390,7 +387,7 @@ find_vma_prepare(struct mm_struct *mm, u
>                 *pprev = rb_entry(rb_prev, struct vm_area_struct, vm_rb);
>         *rb_link = __rb_link;
>         *rb_parent = __rb_parent;
> -       return vma;
> +       return 0;
>  }
>