This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


[Xen-devel] re: [PATCH] mem_sharing: fix race condition of nominate and



Sent: Jui-Hao Chiang [mailto:juihaochiang@xxxxxxxxx]
Date: 2011
17 14:02
To: Tim Deegan
CC: tinnycloud; xen-devel@xxxxxxxxxxxxxxxxxxx
Sub: Re: [PATCH] mem_sharing: fix race condition of nominate and unshare



Oops, I forgot again.
After this change, unshare() has a potential problem of deadlock for shr_lock and p2m_lock with different locking order.
Assume two CPUs do the following
CPU1: hvm_hap_nested_page_fault() => unshare() => p2m_change_type() (locking order: shr_lock, p2m_lock)
CPU2: p2m_teardown() => unshare() (locking order: p2m_lock, shr_lock)
When CPU1 grabs shr_lock and CPU2 grabs p2m_lock, they deadlock later.

So it seems better to fix the following rules
(1) Fix locking order: p2m_lock ---> shr_lock
(2) Any function in mem_sharing, if modifying/checking p2m entry is necessary, it must hold p2m_lock and then shr_lock. Later on, when changing p2m entries, don't call any p2m function which locks p2m again

So for p2m functions, it seems better to provide some functions which don't call p2m_lock again.
What do you think? If that's ok, I will do it in this way.

Hmm,  after looking it deeper, I summarize as the following
(1) It seems all the users of shr_lock, nominate/share/unshare, will check/modify p2m type.
- nominate: p2m_change_type()
- share: set_shared_p2m_entry()
- unshare: set_shared_p2m_entry() and p2m_change_type()
(2) The functions which call unshare()
- hvm_hap_nested_page_fault(): I don't see any p2m_lock holded
- p2m_tear_down(): hold p2m_lock
- gfn_to_mfn_unshare(): I don't see any p2m_lock holded


Thank for sharing the lock info.

I’ve go through the code too.


1.       mem_sharing_unshare_page() has the routine  called from gfn_to_mfn_unshare, which is called by gnttab_transfer  

Since no bug report on grant_table right now, so I think this is safe for now

Also  p2m_tear_down è mem_sharing_unshare_page() , its flag is MEM_SHARING_DESTROY_GFN, and won’t has the chance to

call set_shared_p2m_entry()


2.       as for p2m_change_type(), I found in other place is it called lock free, so it is safe too

3.       set_shared_p2m_entry() which call set_p2m_entry() is not in p2m_lock, and I found in other code set_p2m_entry is called in p2m_lock,

so here I think it is a problem


So I think at least set_p2m_entry should be put into p2m_lock.

I’ll do more investigation base on this.


One of the solution is to
(a) Simply replace shr_lock with p2m_lock.
(b) In unshare(), apply the following: if (!p2m_locked_by_me(p2m)) call p2m_lock, otherwise, don't lock it.
(c) p2m_change_type() and set_shared_p2m_entry() are pretty similar, we can merge the functionality into one function, which does NOT take p2m_lock, and keep the original p2m_change_type() unchanged.

Correct me if wrong.


Xen-devel mailing list
<Prev in Thread] Current Thread [Next in Thread>