Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>, Roger Pau Monne <roger.pau@citrix.com>,
	George Dunlap <George.Dunlap@citrix.com>,
	"Tim (Xen.org)" <tim@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/8] x86/shadow: properly handle get_page() failing
Date: Wed, 27 Jul 2022 14:53:10 +0200	[thread overview]
Message-ID: <f803480d-4c31-1465-4aa7-c8f0bdc11c4c@suse.com> (raw)
In-Reply-To: <6c5b5339-f645-4214-bc53-2b7ead004367@citrix.com>

On 27.07.2022 14:46, Andrew Cooper wrote:
> On 26/07/2022 17:04, Jan Beulich wrote:
>> We should not blindly (in a release build) insert the new entry in the
>> hash if a reference to the guest page cannot be obtained, or else an
>> excess reference would be put when removing the hash entry again. Crash
>> the domain in that case instead. The sole caller doesn't further care
>> about the state of the guest page: All it does is return the
>> corresponding shadow page (which was obtained successfully before) to
>> its caller.
>>
>> To compensate we further need to adjust hash removal: Since the shadow
>> page already has had its backlink set, domain cleanup code would try to
>> destroy the shadow, and hence still cause a put_page() without
>> corresponding get_page(). Leverage that the failed get_page() leads to
>> no hash insertion, making shadow_hash_delete() no longer assume it will
>> find the requested entry. Instead return back whether the entry was
>> found. This way delete_shadow_status() can avoid calling put_page() in
>> the problem scenario.
>>
>> For the other caller of shadow_hash_delete() simply reinstate the
>> otherwise dropped assertion at the call site.
>>
>> While touching the conditionals in {set,delete}_shadow_status() anyway,
>> also switch around their two pre-existing parts, to have the cheap one
>> first (frequently allowing to avoid evaluation of the expensive - due to
>> evaluate_nospec() - one altogether).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although with
> some observations and a suggestion.

Thanks.

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
>>      SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
>>                     gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
>>      ASSERT(mfn_to_page(smfn)->u.sh.head);
>> -    shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
>> +    if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
>> +        ASSERT_UNREACHABLE();
> 
> I'd recommend crashing the domain here too.  I know the we've already
> got the return value we want, but this represents corruption in the
> shadow pagetable metadata, and I highly doubt it is safe to let the
> guest continue to execute in such circumstances.

I'm happy to add or convert, but please clarify: DYM in addition to
the assertion or in place of it?

Jan