On Mon, Mar 15, 2010 at 3:44 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@xxxxxxxxxxxxxx>
>> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href