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

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxxxx>
> > > > ---
> > > > 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.

Are you sure there really is a problem?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature