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 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.

> > Using is_control_domain will limit the usage of paging_log_dirty_op to
> > Dom0 (because that's the only domain that has is_privileged set,
> > there's no way for the toolstack to create a domain with is_privileged
> > set), no other paging domain will be able to use such hypercall
> > without triggering the lock level check. Is that really what we
> > want?
> >
> > I assume that under a distributed Xen system it would be possible to
> > issue this hypercall from a domain different that Dom0 if it's given
> > the right privileges?
> 
> I take it you mean a "disaggregated" system?  This is what Andy was
> talking about with XSM.  As I said to him, at the moment, this will
> _already_ fail, so it's not a regression.
> 
> I'm not willing to open up new potential holes for deadlocks,
> particularly for a hypothetical use case that nobody has asked for and
> doesn't work upstream at the moment.
> 
> If you want to make the calls succeed under a more "peer-to-peer"
> arrangement, you're going to have to come up with something else:
> Change the order of the mm locks, or grab the caller's p2m lock before
> grabbing the paging lock, or refactoring what's covered by what, or
> something.
> 
> Or, we can fix what's actually broken at the moment, and fix other
> things when we encounter them.

I'm fine with only boosting locks that belong to a privileged domain,
TBH the code is mostly the same, and we can always change the boosting
easily when required.

Thanks, Roger.

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