On 27/07/2022 13:53, Jan Beulich wrote:
> On 27.07.2022 14:46, Andrew Cooper wrote:
>> On 26/07/2022 17:04, Jan Beulich wrote:
>>> --- 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?
Erm pass.
We still don't have a sensible construct for ASSERT_OR_DOMAIN_CRASH(),
or a well-thought-through piece of guidance.
Probably keep the ASSERT() ? Almost all state checking in the shadow
code is via asserts.
Mostly what I'm concerned by is it feeling weird to have one path which
only domain crashes, and one path which only asserts.
~Andrew
