lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Mar 2010 16:11:31 +0900 From: Minchan Kim <minchan.kim@...il.com> To: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> Cc: Mel Gorman <mel@....ul.ie>, Andrew Morton <akpm@...ux-foundation.org>, Andrea Arcangeli <aarcange@...hat.com>, Christoph Lameter <cl@...ux-foundation.org>, Adam Litke <agl@...ibm.com>, Avi Kivity <avi@...hat.com>, David Rientjes <rientjes@...gle.com>, KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>, Rik van Riel <riel@...hat.com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org, Hugh Dickins <hugh.dickins@...cali.co.uk> Subject: Re: [PATCH 02/11] mm,migration: Do not try to migrate unmapped anonymous pages On Mon, Mar 15, 2010 at 3:44 PM, KAMEZAWA Hiroyuki <kamezawa.hiroyu@...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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists