Re: [PATCH v4 1/2] x86/mem_sharing: make fork_reset more configurable

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Apr 26, 2022, 4:33 AM Julien Grall <julien@xxxxxxx> wrote:

Hi,

On 26/04/2022 09:17, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 11:24:37AM -0400, Tamas K Lengyel wrote:
>> On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>>>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>>>> index 84cf52636b..d26a6699fc 100644
>>>> --- a/xen/common/vm_event.c
>>>> +++ b/xen/common/vm_event.c
>>>> @@ -28,6 +28,11 @@
>>>>   #include <asm/p2m.h>
>>>>   #include <asm/monitor.h>
>>>>   #include <asm/vm_event.h>
>>>> +
>>>> +#ifdef CONFIG_MEM_SHARING
>>>> +#include <asm/mem_sharing.h>
>>>> +#endif
>>>> +
>>>>   #include <xsm/xsm.h>
>>>>   #include <public/hvm/params.h>
>>>>
>>>> @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>>>>               if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
>>>>                   p2m_mem_paging_resume(d, &rsp);
>>>>   #endif
>>>> +#ifdef CONFIG_MEM_SHARING
>>>> +            if ( mem_sharing_is_fork(d) )
>>>> +            {
>>>> +                bool reset_state = rsp.flags & VM_EVENT_FLAG_RESET_FORK_STATE;
>>>> +                bool reset_mem = rsp.flags & VM_EVENT_FLAG_RESET_FORK_MEMORY;
>>>> +
>>>> +                if ( reset_state || reset_mem )
>>>> +                    ASSERT(!mem_sharing_fork_reset(d, reset_state, reset_mem));
>>>
>>> Might be appropriate to destroy the domain in case fork reset fails?
>>> ASSERT will only help in debug builds.
>>
>> No, I would prefer not destroying the domain here. If it ever becomes
>> necessary the right way would be to introduce a new monitor event to
>> signal an error and let the listener decide what to do. At the moment
>> I don't see that being necessary as there are no known scenarios where
>> we would be able to setup a fork but fail to reset is.
>
> My concern for raising this was what would happen on non-debug
> builds if mem_sharing_fork_reset() failed, and hence my request to
> crash the domain.  I would have used something like:
>
> if ( (reset_state || reset_mem) &&
>       mem_sharing_fork_reset(d, reset_state, reset_mem) )
> {
>      ASSERT_UNREACHABLE();
>      domain_crash(d);
>      break;
> }
>
> But if you and other vm_event maintainers are fine with the current
> approach and don't think it's a problem that's OK with me.

The current approach is actually not correct. On production build,
ASSERT() will turn to NOP. IOW mem_sharing_fork_reset() *will* not be
called.

So the call needs to move outside of the ASSERT() and use a construct
similar to what you suggested:

if ( .... && mem_sharing_fork_reset(...) )
{
   ASSERT_UNREACHABLE();
   break;
}

Ah, good call. Thanks!

Tamas