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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Roger.

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