Re: [PATCH 02/11] mm,migration: Do not try to migrate unmapped anonymous pages

[Minchan Kim <minchan.kim@gmail.com>]

On Mon, Mar 15, 2010 at 3:44 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com>
>> Thanks for detail explanation, Kame.
>> But it can't understand me enough, Sorry.
>>
>> Mel said he met "use-after-free errors in anon_vma".
>> So added the check in unmap_and_move.
>>
>> if (PageAnon(page)) {
>>  ....
>>  if (!page_mapcount(page))
>>    goto uncharge;
>>  rcu_read_lock();
>>
>> My concern what protects racy mapcount of the page?
>> For example,
>>
>> CPU A                                 CPU B
>> unmap_and_move
>> page_mapcount check pass    zap_pte_range
>> <-- some stall -->                   pte_lock
>> <-- some stall -->                   page_remove_rmap(map_count is zero!)
>> <-- some stall -->                   pte_unlock
>> <-- some stall -->                   anon_vma_unlink
>> <-- some stall -->                   anon_vma free !!!!
>> rcu_read_lock
>> anon_vma has gone!!
>>
>> I think above scenario make error "use-after-free", again.
>> What prevent above scenario?
>>
> I think this patch is not complete.
> I guess this patch in [1/11] is trigger for the race.
> ==
> +
> +       /* Drop an anon_vma reference if we took one */
> +       if (anon_vma && atomic_dec_and_lock(&anon_vma->migrate_refcount, &anon_vma->lock)) {
> +               int empty = list_empty(&anon_vma->head);
> +               spin_unlock(&anon_vma->lock);
> +               if (empty)
> +                       anon_vma_free(anon_vma);
> +       }
> ==
> If my understainding in above is correct, this "modify" freed anon_vma.
> Then, use-after-free happens. (In old implementation, there are no refcnt,
> so, there is no use-after-free ops.)
>

I agree.
Let's wait Mel's response.

>
> So, what I can think of now is a patch like following is necessary.
>
> ==
> static inline struct anon_vma *anon_vma_alloc(void)
> {
>        struct anon_vma *anon_vma;
>        anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL);
>        atomic_set(&anon_vma->refcnt, 1);
> }
>
> void anon_vma_free(struct anon_vma *anon_vma)
> {
>        /*
>         * This called when anon_vma is..
>         * - anon_vma->vma_list becomes empty.
>         * - incremetned refcnt while migration, ksm etc.. is dropped.
>         * - allocated but unused.
>         */
>        if (atomic_dec_and_test(&anon_vma->refcnt))
>                kmem_cache_free(anon_vma_cachep, anon_vma);
> }
> ==
> Then all things will go simple.
> Overhead is concern but list_empty() helps us much.

When they made things complicated without atomic_op,
there was reasonable reason, I think. :)

My opinion depends on you and server guys(Hugh, Rik, Andrea Arcangeli and so on)


>
> Thanks,
> -Kame
>
>
>
>
>



-- 
Kind regards,
Minchan Kim