x86/mm lock order enforcement

[Jan Beulich <jbeulich@xxxxxxxx>]

Hello,

the issue pointed out in [1] (and hence the proposed fix in a reply) is
actually pointing out a bigger problem, addressing of which may also
invalidate that proposed fix.

First, fundamentally any code path reaching p2m_pt_set_entry() (i.e. in
particular any caller of p2m_set_entry()) is liable to hit the same
lock order violation if it holds _any_ of the locks numbered higher than
the p2m one in mm-locks.h, due to write_p2m_entry()'s call to
p2m_flush_nestedp2m().

Similarly I think ept_set_entry() -> ept_sync_domain() ->
ept_sync_domain_prepare() -> p2m_flush_nestedp2m() is affected.

But what's imo worse - there's no enforcement (and afaics not even a
rule written down) when it comes to the many p2m locks associated with
a single domain: The host p2m has one, and every altp2m as well as every
nested p2m have one, too. _check_lock_level() only checks whether the
already held lock has a strictly lower number in the scheme, so all of
these p2m locks are only put in just two separate groups, the members of
which are then all equal right now. The two groups are altp2m (all the
altp2m locks) and the rest (host p2m and all nested p2ms). Within a
group the various locks can be freely acquired in any order without the
resulting issue getting pointed out.

A similar issue looks to have been recognized for mem-sharing's per-
page locking: These also all fall into a single group. share_pages()
has special logic to guarantee uniform order of acquires. (I don't
think the comment in mm-locks.h is quite right there, though: "We
enforce order here against the p2m lock, which is taken after the
page_lock to change the gfn's p2m entry." To me this contradicts both
the code in share_pages() and the numbering in the scheme.)

A first step here might be to have _check_lock_level() consider not
only the level, but also the involved lock. Since recursive acquires
are frequently intended, the two involved levels being equal would
only be permitted if it is the same lock that gets used.

Otoh it may be somehow implicit that all of the altp2m and similarly
all of the nested p2m locks may not nest with one another, in which
case it might be good enough to separate the nested p2m ones from the
host p2m one (just like the altp2m ones have their own group). If so,
I'm afraid I would see no way for myself to determine where in the
numbering scheme they should be placed - them currently sharing
position with the host p2m lock suggests that any lock numbered
higher may already get acquired somewhere with a nested p2m lock held.
Yet to address the original report they would need to go above of at
least the PoD lock.

Thoughts anyone?

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2021-09/msg02231.html