Re: [Xen-devel] [PATCH] x86/mm: Add debug code to detect illegal page_lock and put_page_type ordering

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 01/24/2018 01:04 PM, Andrew Cooper wrote:
> On 24/01/18 12:48, George Dunlap wrote:
>> The fix for XSA-242 depends on the same cpu never calling
>> _put_page_type() while holding a page_lock() for that page.  By having
>> no locking discipline between pages, the current code also assumes
>> that we will never lock two pages on the same cpu.
>>
>> Add a check to debug builds to verify that both of these are true.
>>
>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> 
> Is there any requirement or expectation that we only ever lock a single
> page at once?

Every page effectively has its own lock, and AFAICT there is no locking
discipline on pages.  So if a single pcpu ever locks two pages we have
the potential for a classic deadlock situation:
    P1: Lock A
    P2: Lock B
    P1: Spin-and-wait for lock B
    P2: Spin-and-wait for lock A #

> If yes, then this check needs to be more than just a debug check.  If
> not, then the code as presented may falsely explode.

Well there may come a day when we do grab two locks on a single pcpu,
but in such a way as there can never be a circular dependency.
Technically that would be a false positive, but I would argue that the
ASSERT() will still be doing its job, alerting us to the fact that we
need to think carefully about the new circumstances, update the
documentation, and update the assert.

Personally I'm happy to just document the requirement and invoke an
ASSERT().  If we want to be more rigorous about making sure there are no
triggerable deadlocks, one option might be to have page_lock() return 0
if this cpu already holds another lock.  (Haven't looked at that in detail.)

One of the things I'm more worried about is the requirement ever to call
put_page_type() on a page if you already hold the page_lock() for that
page.  Jan argued that when called together they "generally" act on
different pages, but that's not the kind of word you normally want to
use when you're talking about security issues.

> Independently of this, it would probably be cleaner to split these out
> into predicates to avoid all the ifdefary.

Yes, I wasn't happy with those either -- I'll take Jan's suggestion
unless you have a better one.

 -George

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