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

[tinnycloud <tinnycloud@xxxxxxxxxxx>]

 

 

Sent: Jui-Hao Chiang [mailto:juihaochiang@xxxxxxxxx]
Date: 2011年1月7日 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.

Bests,
Jui-Hao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel