Re: [Xen-devel] [PATCH 3/3] x86/mm-locks: apply a bias to lock levels for current domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.12.18 at 10:58, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Dec 20, 2018 at 02:16:15AM -0700, Jan Beulich wrote:
>> >>> On 20.12.18 at 10:05, <roger.pau@xxxxxxxxxx> wrote:
>> > On Wed, Dec 19, 2018 at 06:10:04PM +0000, George Dunlap wrote:
>> >> On 12/19/18 2:59 PM, Roger Pau Monné wrote:
>> >> >> Using 'current' means that potential deadlocks which would now cause a
>> >> >> BUG() won't anymore.  I'm fine with not adding extra protections that
>> >> >> aren't there now; but I don't want to remove protections that are.
>> >> > 
>> >> > The lock ordering enforcement is still kept as-is, but Xen is allowed
>> >> > to lock the caller mm locks in the right order (be it privileged or
>> >> > not) after having locked a subject domain ones also in the correct
>> >> > order.
>> >> 
>> >> Suppose we have mm_lock_x, with value 8, and mm_lock_y with value 16.
>> >> 
>> >> Suppose we have H1, which grabs current->mm_lock_x and tgt->mm_lock_y.
>> > 
>> > With my proposal H1 won't be a valid lock sequence, since
>> > current->mm_lock_x level > tgt->mm_lock_y level.
>> > 
>> > With your proposal H1 would be valid depending on whether 'current' is
>> > Dom0 or not. If current == Dom0, H1 is invalid, since
>> > current->mm_lock_x level > tgt->mm_lock_y level. If current != Dom0
>> > then the H1 sequence would be valid.
>> > 
>> > I think this is not ideal, since the correctness of the H1 sequence
>> > with your proposal depends on whether the caller is Dom0 or an
>> > unprivileged domain. With my proposal there's no such difference, and
>> > the validity of sequences doesn't depend on whether the caller is
>> > privileged or not, which makes it easier to reason about.
>> > 
>> >> And suppose we have H2, which grabs current->mm_lock_y, and 
>> >> tgt->mm_lock_x.
>> > 
>> > H2 won't be a valid locking sequence with my proposal, since
>> > current->mm_lock_y level > tgt->mm_lock_x level, so the H2 sequence
>> > would be invalid and always trigger the BUG.
>> > 
>> > With my proposal all current domain mm locks always have a level
>> > higher than any target mm locks.
>> > 
>> >> 
>> >> And suppose domA calls H1 on domB at the same time that domB calls H2 on
>> >> domA.  We could have the following sequence:
>> >> 
>> >> * H1: grab A->mm_lock_x
>> >> * H2: grab B->mm_lock_y
>> >> * H1: wait on B->mm_lock_y
>> >> * H2: wait on A->mm_lock_x #DEADLOCK
>> >> 
>> >> With the current mm lock checking, H2 would cause a BUG() the first time
>> >> it was called.
>> >> 
>> >> With my "special privilege only" boosting you'd also get a BUG() in at
>> >> least one of the invocations.
>> >> 
>> >> With your "current bias" patch, no BUG() would be encountered; we'd only
>> >> discover the deadlock once a live server had actually deadlocked.
>> >> 
>> >> This is what I'm talking about -- with "boost dom0", you have a global
>> >> order to the locks.  It goes:
>> >> 
>> >> 1-8: All domU locks (in the order listed in mm-locks.h)
>> >> 9-16: All dom0 locks
>> >> 
>> >> Thus we know for certain that there can't be a caller / callee deadlock
>> >> that's not detected.  With your patch, there isn't a global order: the
>> >> order changes based on who made the hypercall, so it's more difficult to
>> >> reason about whether there's a deadlock or not.
>> >> 
>> >> So, do H1 and H2 exist right now?  I think probably not, but I can't
>> >> immediately say.  Will such a pair *never* exist?  I don't think I can
>> >> guarantee that either.  That's why I want something to check.
>> > 
>> > I'm fine with this proposal, it's just that if it's safe for Dom0 to
>> > pick any other domain mm lock and then any Dom0 mm lock it should also
>> > be safe for any domain and not Dom0 only.
>> 
>> Since I'm not sure whether this was implied here: "Any" is of
>> course wrong here, or else there'd be ABBA deadlock potential
>> between such two arbitrary domains. I suppose you still mean
>> "any domain controlling another domain", and for such a domain
>> to only acquire locks of the domain it controls.
> 
> Yes, the last "any" should be "any domain controlling another domain",
> but it's not the job of the mm lock checker to assert whether a domain
> is controlling another domain or not.

Well, that depends on the model we pick.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel