Re: [PATCH v4 0/3] x86: modify_ldt improvement, test, and config option

From: Andy Lutomirski <luto@amacapital.net>
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: "security@kernel.org" <security@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	X86 ML <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	xen-devel <xen-devel@lists.xen.org>,
	Borislav Petkov <bp@alien8.de>, Jan Beulich <jbeulich@suse.com>,
	Sasha Levin <sasha.levin@oracle.com>
Subject: Re: [PATCH v4 0/3] x86: modify_ldt improvement, test, and config option
Date: Tue, 28 Jul 2015 21:26:35 -0700	[thread overview]
Message-ID: <CALCETrUje9Z6H9Eq7rDtnjr2KWQqAYCGMnyoF6ObTpZ+mcT3gA__39990.9208332703$1438144081$gmane$org@mail.gmail.com> (raw)
In-Reply-To: <55B841FF.2000102@oracle.com>

On Tue, Jul 28, 2015 at 8:01 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 07/28/2015 08:47 PM, Andrew Cooper wrote:
>>
>> On 29/07/2015 01:21, Andy Lutomirski wrote:
>>>
>>> On Tue, Jul 28, 2015 at 10:10 AM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>>
>>>> On 07/28/2015 01:07 PM, Andy Lutomirski wrote:
>>>>>
>>>>> On Tue, Jul 28, 2015 at 9:30 AM, Andrew Cooper
>>>>> <andrew.cooper3@citrix.com> wrote:
>>>>>>
>>>>>> I suspect that the set_ldt(NULL, 0) call hasn't reached Xen before
>>>>>> xen_free_ldt() is attempting to nab back the pages which Xen still has
>>>>>> mapped as an LDT.
>>>>>>
>>>>> I just instrumented it with yet more LSL instructions.  I'm pretty
>>>>> sure that set_ldt really is clearing at least LDT entry zero.
>>>>> Nonetheless the free_ldt call still oopses.
>>>>>
>>>> Yes, I added some instrumentation to the hypervisor and we definitely
>>>> set
>>>> LDT to NULL before failing.
>>>>
>>>> -boris
>>>
>>> Looking at map_ldt_shadow_page: what keeps shadow_ldt_mapcnt from
>>> getting incremented once on each CPU at the same time if both CPUs
>>> fault in the same shadow LDT page at the same time?
>>
>> Nothing, but that is fine.  If a page is in use in two vcpus LDTs, it is
>> expected to have a type refcount of 2.
>>
>>> Similarly, what
>>> keeps both CPUs from calling get_page_type at the same time and
>>> therefore losing track of the page type reference count?
>>
>> a cmpxchg() loop in the depths of __get_page_type().
>>
>>> I don't see why vmalloc or vm_unmap_aliases would have anything to do
>>> with this, though.
>
>
> So just for kicks I made lazy_max_pages() return 0 to free vmaps immediately
> and the problem went away.
>
> I also saw this warning, BTW:
>
> [  178.686542] ------------[ cut here ]------------
> [  178.686554] WARNING: CPU: 0 PID: 16440 at
> ./arch/x86/include/asm/mmu_context.h:96 load_mm_ldt+0x70/0x76()
> [  178.686558] DEBUG_LOCKS_WARN_ON(!irqs_disabled())

Whoops!  That should be checking preemptible(), not irqs_disabled().

--Andy