Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall <julien@xen.org>]

(+ Some AWS folks)

Hi Juergen,

On 22/09/2021 17:34, Juergen Gross wrote:
> On 22.09.21 12:23, Julien Grall wrote:
>> Hi Roger,
>>
>> On 22/09/2021 14:58, Roger Pau Monné wrote:
>>> On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>>>> Failure to map the shared ring and thus establish a xenstore
>>>>> connection with a domain shouldn't prevent the "@introduceDomain"
>>>>> watch from firing, likewise with "@releaseDomain".
>>>>>
>>>>> In order to handle such events properly xenstored should keep track of
>>>>> the domains even if the shared communication ring cannot be mapped.
>>>>> This was partially the case due to the restore mode, which needs to
>>>>> handle domains that have been destroyed between the save and restore
>>>>> period. This patch extends on the previous limited support of
>>>>> temporary adding a domain without a valid interface ring, and modifies
>>>>> check_domains to keep domains without an interface on the list.
>>>>>
>>>>> Handling domains without a valid shared ring is required in order to
>>>>> support domain without a grant table, since the lack of grant table
>>>>> will prevent the mapping of the xenstore ring grant reference.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>> oxenstored will need a similar treatment once grant mapping is used
>>>>> there. For the time being it still works correctly because it uses
>>>>> foreign memory to map the shared ring, and that will work in the
>>>>> absence of grant tables on the domain.
>>>>> ---
>>>>> Changes since v1:
>>>>>    - New in this version.
>>>>> ---
>>>>>    tools/xenstore/xenstored_domain.c | 30 
>>>>> ++++++++++++++++++------------
>>>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>>> b/tools/xenstore/xenstored_domain.c
>>>>> index 9fb78d5772..150c6f082e 100644
>>>>> --- a/tools/xenstore/xenstored_domain.c
>>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>>> @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>>        XENSTORE_RING_IDX cons, prod;
>>>>> +    if (!intf) {
>>>>> +        errno = ENODEV;
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>>        /* Must read indexes once, and before anything else, and 
>>>>> verified. */
>>>>>        cons = intf->rsp_cons;
>>>>>        prod = intf->rsp_prod;
>>>>> @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, 
>>>>> void *data, unsigned int len)
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>>        XENSTORE_RING_IDX cons, prod;
>>>>> +    if (!intf) {
>>>>> +        errno = ENODEV;
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>>        /* Must read indexes once, and before anything else, and 
>>>>> verified. */
>>>>>        cons = intf->req_cons;
>>>>>        prod = intf->req_prod;
>>>>> @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection 
>>>>> *conn)
>>>>>    {
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>> +    if (!intf)
>>>>> +        return false;
>>>>> +
>>>>
>>>> Rather than adding an extra check, how about taking advantage of 
>>>> is_ignore?
>>>
>>> Right, I just need to change the order in conn_can_read and
>>> conn_can_write so that the is_ignored check is performed before the
>>> can_{read,write} handler is called.
>>>
>>>>>        return ((intf->rsp_prod - intf->rsp_cons) != 
>>>>> XENSTORE_RING_SIZE);
>>>>>    }
>>>>> @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection 
>>>>> *conn)
>>>>>    {
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>> -    if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
>>>>> +    if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit 
>>>>> < 0) ||
>>>>> +        !intf)
>>>>>            return false;
>>>>>        return (intf->req_cons != intf->req_prod);
>>>>> @@ -268,14 +282,7 @@ void check_domains(void)
>>>>>                    domain->shutdown = true;
>>>>>                    notify = 1;
>>>>>                }
>>>>> -            /*
>>>>> -             * On Restore, we may have been unable to remap the
>>>>> -             * interface and the port. As we don't know whether
>>>>> -             * this was because of a dying domain, we need to
>>>>> -             * check if the interface and port are still valid.
>>>>> -             */
>>>>> -            if (!dominfo.dying && domain->port &&
>>>>> -                domain->interface)
>>>>> +            if (!dominfo.dying)
>>>>>                    continue;
>>>>
>>>> This is mostly a revert on "tools/xenstore: handle dying domains in 
>>>> live
>>>> update". However, IIRC, this check was necessary to release the 
>>>> connection
>>>> if the domain has died in the middle of Live-Update.
>>>
>>> But if the domain has died in the middle of live update
>>> get_domain_info will return false, and thus the code won't get here.
>>
>> Hmmm... I think I am mixing up a few things... I went through the 
>> original discussion (it was on the security ML) to find out why I 
>> wrote the patch like that. When going through the archives, I noticed 
>> that I provided a different version of this patch (see [1]) because 
>> there was some issue with the check here (I wrote that it would lead 
>> to zombie domain, but don't have the rationale :().
>>
>> Juergen, I don't seem to find the reason why the patch was not 
>> replaced as we discussed on the security ML. Do you remember why?
> 
> Sorry, no, I don't.
> 
> You did send the new version for V6 of the LU series, but it seems at
> least in V9 you commented on the patch requesting that a comment just
> in the section being different between the two variants to be removed.
> 
> So either we both overlooked the new variant not having gone in, or we
> agreed to use the old version (e.g. in a security meeting). In my IRC
> logs I couldn't find anything either (the only mentioning of that patch
> was before V6 of the series was sent, and that was before you sending
> the new one as a reply to V6).
> 
>> Assuming this was a mistake, could someone take care of sending an 
>> update? If not, I could do it when I am back in October.
>>
>> For the archives, the issues would appear when shutting down a domain 
>> during Live-Update.
> 
> Hmm, IIRC you did quite some extensive testing of LU and didn't find
> any problem in the final version.

I did extensive testing with early series but I can't remember whether I 
tested the shutdown reproducer with the latest series.

> 
> Are you sure there really is a problem?

I thought a bit more and looked at the code (I don't have access to a 
test machine at the moment). I think there is indeed a problem.

Some watchers of @releaseDomain (such as xenconsoled) will only remove a 
domain from their internal state when the domain is actually dead.

This is based on dominfo.dying which is only set when all the resources 
are relinquished and waiting for the other domains to release any 
resources for that domain.

The problem is Xenstore may fail to map the interface or the event 
channel long before the domain is actually dead. With the current check, 
we would free the allocated structure and therefore send @releaseDomain 
too early. So daemon like xenconsoled, would never cleanup for the 
domain and leave a zombie domain. Well... until the next @releaseDomain 
(or @introduceDomain for Xenconsoled) AFAICT.

The revised patch is meant to solve it by just ignoring the connection. 
With that approach, we would correctly notify watches when the domain is 
dead.

Cheers,

-- 
Julien Grall