From: George Dunlap <george.dunlap@cloud.com>
Commit fc2b57c9a ("xenstored: send an evtchn notification on
introduce_domain") introduced the sending of an event channel to the
guest when first introduced, so that dom0less domains waiting for the
connection would know that xenstore was ready to use.
Unfortunately, it was introduced in introduce_domain(), which 1) is
called by other functions, where such functionality is unneeded, and
2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
introduces a race condition, whereby if xenstored is delayed, a domain
can wake up, send messages to the buffer, only to have them deleted by
xenstore before finishing its processing of the XS_INTRODUCE message.
Move the connect-and-notfy call into do_introduce() instead, after the
domain_conn_rest(); predicated on the state being in the
XENSTORE_RECONNECT state.
(We don't need to check for "restoring", since that value is always
passed as "false" from do_domain_introduce()).
Also take the opportunity to add a missing wmb barrier after resetting
the indexes of the ring in domain_conn_reset.
This change will also remove an extra event channel notification for
dom0 (because the notification is now done by do_introduce which is not
called for dom0.) The extra dom0 event channel notification was only
introduced by fc2b57c9a and was never present before. It is not needed
because dom0 is the one to tell xenstored the connection parameters, so
dom0 has to know that the ring page is setup correctly by the time
xenstored starts looking at it. It is dom0 that performs the ring page
init.
Signed-off-by: George Dunlap <george.dunlap@cloud.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
CC: jgross@suse.com
CC: julien@xen.org
CC: wl@xen.org
---
tools/xenstored/domain.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index a6cd199fdc..f6ef37856c 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
domain->interface->req_cons = domain->interface->req_prod = 0;
domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
+ xen_wmb();
}
/*
@@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
/* Now domain belongs to its connection. */
talloc_steal(domain->conn, domain);
- if (!restore) {
- /* Notify the domain that xenstore is available */
- interface->connection = XENSTORE_CONNECTED;
- xenevtchn_notify(xce_handle, domain->port);
- }
-
if (!is_master_domain && !restore)
fire_special_watches("@introduceDomain");
} else {
@@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection *conn,
domain_conn_reset(domain);
+ if (domain->interface != NULL &&
+ domain->interface->connection == XENSTORE_RECONNECT) {
+ /* Notify the domain that xenstore is available */
+ domain->interface->connection = XENSTORE_CONNECTED;
+ xenevtchn_notify(xce_handle, domain->port);
+ }
+
send_ack(conn, XS_INTRODUCE);
return 0;
--
2.25.1On 14.10.2023 01:06, Stefano Stabellini wrote:
> From: George Dunlap <george.dunlap@cloud.com>
>
> Commit fc2b57c9a ("xenstored: send an evtchn notification on
> introduce_domain") introduced the sending of an event channel to the
> guest when first introduced, so that dom0less domains waiting for the
> connection would know that xenstore was ready to use.
>
> Unfortunately, it was introduced in introduce_domain(), which 1) is
> called by other functions, where such functionality is unneeded, and
> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
> introduces a race condition, whereby if xenstored is delayed, a domain
> can wake up, send messages to the buffer, only to have them deleted by
> xenstore before finishing its processing of the XS_INTRODUCE message.
>
> Move the connect-and-notfy call into do_introduce() instead, after the
> domain_conn_rest(); predicated on the state being in the
> XENSTORE_RECONNECT state.
>
> (We don't need to check for "restoring", since that value is always
> passed as "false" from do_domain_introduce()).
>
> Also take the opportunity to add a missing wmb barrier after resetting
> the indexes of the ring in domain_conn_reset.
>
> This change will also remove an extra event channel notification for
> dom0 (because the notification is now done by do_introduce which is not
> called for dom0.) The extra dom0 event channel notification was only
> introduced by fc2b57c9a and was never present before. It is not needed
> because dom0 is the one to tell xenstored the connection parameters, so
> dom0 has to know that the ring page is setup correctly by the time
> xenstored starts looking at it. It is dom0 that performs the ring page
> init.
>
> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Should this have had a Fixes: tag thus marking it to pick up for
backport?
Jan
On Wed, 17 Oct 2023, Jan Beulich wrote:
>
> On 14.10.2023 01:06, Stefano Stabellini wrote:
> > From: George Dunlap <george.dunlap@cloud.com>
> >
> > Commit fc2b57c9a ("xenstored: send an evtchn notification on
> > introduce_domain") introduced the sending of an event channel to the
> > guest when first introduced, so that dom0less domains waiting for the
> > connection would know that xenstore was ready to use.
> >
> > Unfortunately, it was introduced in introduce_domain(), which 1) is
> > called by other functions, where such functionality is unneeded, and
> > 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
> > introduces a race condition, whereby if xenstored is delayed, a domain
> > can wake up, send messages to the buffer, only to have them deleted by
> > xenstore before finishing its processing of the XS_INTRODUCE message.
> >
> > Move the connect-and-notfy call into do_introduce() instead, after the
> > domain_conn_rest(); predicated on the state being in the
> > XENSTORE_RECONNECT state.
> >
> > (We don't need to check for "restoring", since that value is always
> > passed as "false" from do_domain_introduce()).
> >
> > Also take the opportunity to add a missing wmb barrier after resetting
> > the indexes of the ring in domain_conn_reset.
> >
> > This change will also remove an extra event channel notification for
> > dom0 (because the notification is now done by do_introduce which is not
> > called for dom0.) The extra dom0 event channel notification was only
> > introduced by fc2b57c9a and was never present before. It is not needed
> > because dom0 is the one to tell xenstored the connection parameters, so
> > dom0 has to know that the ring page is setup correctly by the time
> > xenstored starts looking at it. It is dom0 that performs the ring page
> > init.
> >
> > Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>
> Should this have had a Fixes: tag thus marking it to pick up for
> backport?
Sorry this was committed already
On 18.10.2023 23:31, Stefano Stabellini wrote:
> On Wed, 17 Oct 2023, Jan Beulich wrote:
>>
>> On 14.10.2023 01:06, Stefano Stabellini wrote:
>>> From: George Dunlap <george.dunlap@cloud.com>
>>>
>>> Commit fc2b57c9a ("xenstored: send an evtchn notification on
>>> introduce_domain") introduced the sending of an event channel to the
>>> guest when first introduced, so that dom0less domains waiting for the
>>> connection would know that xenstore was ready to use.
>>>
>>> Unfortunately, it was introduced in introduce_domain(), which 1) is
>>> called by other functions, where such functionality is unneeded, and
>>> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
>>> introduces a race condition, whereby if xenstored is delayed, a domain
>>> can wake up, send messages to the buffer, only to have them deleted by
>>> xenstore before finishing its processing of the XS_INTRODUCE message.
>>>
>>> Move the connect-and-notfy call into do_introduce() instead, after the
>>> domain_conn_rest(); predicated on the state being in the
>>> XENSTORE_RECONNECT state.
>>>
>>> (We don't need to check for "restoring", since that value is always
>>> passed as "false" from do_domain_introduce()).
>>>
>>> Also take the opportunity to add a missing wmb barrier after resetting
>>> the indexes of the ring in domain_conn_reset.
>>>
>>> This change will also remove an extra event channel notification for
>>> dom0 (because the notification is now done by do_introduce which is not
>>> called for dom0.) The extra dom0 event channel notification was only
>>> introduced by fc2b57c9a and was never present before. It is not needed
>>> because dom0 is the one to tell xenstored the connection parameters, so
>>> dom0 has to know that the ring page is setup correctly by the time
>>> xenstored starts looking at it. It is dom0 that performs the ring page
>>> init.
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>>
>> Should this have had a Fixes: tag thus marking it to pick up for
>> backport?
>
> Sorry this was committed already
That's why I used "have had". I still need an answer to the question
though; your reply only hints towards "yes".
Jan
On Thu, 19 Oct 2023, Jan Beulich wrote:
> On 18.10.2023 23:31, Stefano Stabellini wrote:
> > On Wed, 17 Oct 2023, Jan Beulich wrote:
> >>
> >> On 14.10.2023 01:06, Stefano Stabellini wrote:
> >>> From: George Dunlap <george.dunlap@cloud.com>
> >>>
> >>> Commit fc2b57c9a ("xenstored: send an evtchn notification on
> >>> introduce_domain") introduced the sending of an event channel to the
> >>> guest when first introduced, so that dom0less domains waiting for the
> >>> connection would know that xenstore was ready to use.
> >>>
> >>> Unfortunately, it was introduced in introduce_domain(), which 1) is
> >>> called by other functions, where such functionality is unneeded, and
> >>> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
> >>> introduces a race condition, whereby if xenstored is delayed, a domain
> >>> can wake up, send messages to the buffer, only to have them deleted by
> >>> xenstore before finishing its processing of the XS_INTRODUCE message.
> >>>
> >>> Move the connect-and-notfy call into do_introduce() instead, after the
> >>> domain_conn_rest(); predicated on the state being in the
> >>> XENSTORE_RECONNECT state.
> >>>
> >>> (We don't need to check for "restoring", since that value is always
> >>> passed as "false" from do_domain_introduce()).
> >>>
> >>> Also take the opportunity to add a missing wmb barrier after resetting
> >>> the indexes of the ring in domain_conn_reset.
> >>>
> >>> This change will also remove an extra event channel notification for
> >>> dom0 (because the notification is now done by do_introduce which is not
> >>> called for dom0.) The extra dom0 event channel notification was only
> >>> introduced by fc2b57c9a and was never present before. It is not needed
> >>> because dom0 is the one to tell xenstored the connection parameters, so
> >>> dom0 has to know that the ring page is setup correctly by the time
> >>> xenstored starts looking at it. It is dom0 that performs the ring page
> >>> init.
> >>>
> >>> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> >>
> >> Should this have had a Fixes: tag thus marking it to pick up for
> >> backport?
> >
> > Sorry this was committed already
>
> That's why I used "have had". I still need an answer to the question
> though; your reply only hints towards "yes".
Yes, I would add the Fixes tag
+Henry for release ack
On Fri, 13 Oct 2023, Stefano Stabellini wrote:
> From: George Dunlap <george.dunlap@cloud.com>
>
> Commit fc2b57c9a ("xenstored: send an evtchn notification on
> introduce_domain") introduced the sending of an event channel to the
> guest when first introduced, so that dom0less domains waiting for the
> connection would know that xenstore was ready to use.
>
> Unfortunately, it was introduced in introduce_domain(), which 1) is
> called by other functions, where such functionality is unneeded, and
> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
> introduces a race condition, whereby if xenstored is delayed, a domain
> can wake up, send messages to the buffer, only to have them deleted by
> xenstore before finishing its processing of the XS_INTRODUCE message.
>
> Move the connect-and-notfy call into do_introduce() instead, after the
> domain_conn_rest(); predicated on the state being in the
> XENSTORE_RECONNECT state.
>
> (We don't need to check for "restoring", since that value is always
> passed as "false" from do_domain_introduce()).
>
> Also take the opportunity to add a missing wmb barrier after resetting
> the indexes of the ring in domain_conn_reset.
>
> This change will also remove an extra event channel notification for
> dom0 (because the notification is now done by do_introduce which is not
> called for dom0.) The extra dom0 event channel notification was only
> introduced by fc2b57c9a and was never present before. It is not needed
> because dom0 is the one to tell xenstored the connection parameters, so
> dom0 has to know that the ring page is setup correctly by the time
> xenstored starts looking at it. It is dom0 that performs the ring page
> init.
>
> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> CC: jgross@suse.com
> CC: julien@xen.org
> CC: wl@xen.org
> ---
> tools/xenstored/domain.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index a6cd199fdc..f6ef37856c 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
>
> domain->interface->req_cons = domain->interface->req_prod = 0;
> domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
> + xen_wmb();
> }
>
> /*
> @@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
> /* Now domain belongs to its connection. */
> talloc_steal(domain->conn, domain);
>
> - if (!restore) {
> - /* Notify the domain that xenstore is available */
> - interface->connection = XENSTORE_CONNECTED;
> - xenevtchn_notify(xce_handle, domain->port);
> - }
> -
> if (!is_master_domain && !restore)
> fire_special_watches("@introduceDomain");
> } else {
> @@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection *conn,
>
> domain_conn_reset(domain);
>
> + if (domain->interface != NULL &&
> + domain->interface->connection == XENSTORE_RECONNECT) {
> + /* Notify the domain that xenstore is available */
> + domain->interface->connection = XENSTORE_CONNECTED;
> + xenevtchn_notify(xce_handle, domain->port);
> + }
> +
> send_ack(conn, XS_INTRODUCE);
>
> return 0;
> --
> 2.25.1
>
Hi Stefano,
> On Oct 17, 2023, at 07:14, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> +Henry for release ack
Thanks, I think we did have Juergen for reviewing this patch so:
Release-acked-by: Henry Wang <Henry.Wang@arm.com>
Kind regards,
Henry
>
>
> On Fri, 13 Oct 2023, Stefano Stabellini wrote:
>> From: George Dunlap <george.dunlap@cloud.com>
>>
>> Commit fc2b57c9a ("xenstored: send an evtchn notification on
>> introduce_domain") introduced the sending of an event channel to the
>> guest when first introduced, so that dom0less domains waiting for the
>> connection would know that xenstore was ready to use.
>>
>> Unfortunately, it was introduced in introduce_domain(), which 1) is
>> called by other functions, where such functionality is unneeded, and
>> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
>> introduces a race condition, whereby if xenstored is delayed, a domain
>> can wake up, send messages to the buffer, only to have them deleted by
>> xenstore before finishing its processing of the XS_INTRODUCE message.
>>
>> Move the connect-and-notfy call into do_introduce() instead, after the
>> domain_conn_rest(); predicated on the state being in the
>> XENSTORE_RECONNECT state.
>>
>> (We don't need to check for "restoring", since that value is always
>> passed as "false" from do_domain_introduce()).
>>
>> Also take the opportunity to add a missing wmb barrier after resetting
>> the indexes of the ring in domain_conn_reset.
>>
>> This change will also remove an extra event channel notification for
>> dom0 (because the notification is now done by do_introduce which is not
>> called for dom0.) The extra dom0 event channel notification was only
>> introduced by fc2b57c9a and was never present before. It is not needed
>> because dom0 is the one to tell xenstored the connection parameters, so
>> dom0 has to know that the ring page is setup correctly by the time
>> xenstored starts looking at it. It is dom0 that performs the ring page
>> init.
>>
>> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> CC: jgross@suse.com
>> CC: julien@xen.org
>> CC: wl@xen.org
>> ---
>> tools/xenstored/domain.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>> index a6cd199fdc..f6ef37856c 100644
>> --- a/tools/xenstored/domain.c
>> +++ b/tools/xenstored/domain.c
>> @@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
>>
>> domain->interface->req_cons = domain->interface->req_prod = 0;
>> domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
>> + xen_wmb();
>> }
>>
>> /*
>> @@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
>> /* Now domain belongs to its connection. */
>> talloc_steal(domain->conn, domain);
>>
>> - if (!restore) {
>> - /* Notify the domain that xenstore is available */
>> - interface->connection = XENSTORE_CONNECTED;
>> - xenevtchn_notify(xce_handle, domain->port);
>> - }
>> -
>> if (!is_master_domain && !restore)
>> fire_special_watches("@introduceDomain");
>> } else {
>> @@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection *conn,
>>
>> domain_conn_reset(domain);
>>
>> + if (domain->interface != NULL &&
>> + domain->interface->connection == XENSTORE_RECONNECT) {
>> + /* Notify the domain that xenstore is available */
>> + domain->interface->connection = XENSTORE_CONNECTED;
>> + xenevtchn_notify(xce_handle, domain->port);
>> + }
>> +
>> send_ack(conn, XS_INTRODUCE);
>>
>> return 0;
>> --
>> 2.25.1
>>
On 14.10.23 01:06, Stefano Stabellini wrote:
> From: George Dunlap <george.dunlap@cloud.com>
>
> Commit fc2b57c9a ("xenstored: send an evtchn notification on
> introduce_domain") introduced the sending of an event channel to the
> guest when first introduced, so that dom0less domains waiting for the
> connection would know that xenstore was ready to use.
>
> Unfortunately, it was introduced in introduce_domain(), which 1) is
> called by other functions, where such functionality is unneeded, and
> 2) after the main XS_INTRODUCE call, calls domain_conn_reset(). This
> introduces a race condition, whereby if xenstored is delayed, a domain
> can wake up, send messages to the buffer, only to have them deleted by
> xenstore before finishing its processing of the XS_INTRODUCE message.
>
> Move the connect-and-notfy call into do_introduce() instead, after the
> domain_conn_rest(); predicated on the state being in the
> XENSTORE_RECONNECT state.
>
> (We don't need to check for "restoring", since that value is always
> passed as "false" from do_domain_introduce()).
>
> Also take the opportunity to add a missing wmb barrier after resetting
> the indexes of the ring in domain_conn_reset.
>
> This change will also remove an extra event channel notification for
> dom0 (because the notification is now done by do_introduce which is not
> called for dom0.) The extra dom0 event channel notification was only
> introduced by fc2b57c9a and was never present before. It is not needed
> because dom0 is the one to tell xenstored the connection parameters, so
> dom0 has to know that the ring page is setup correctly by the time
> xenstored starts looking at it. It is dom0 that performs the ring page
> init.
>
> Signed-off-by: George Dunlap <george.dunlap@cloud.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> CC: jgross@suse.com
> CC: julien@xen.org
> CC: wl@xen.org
Two nits below, with those fixed:
Reviewed-by: Juergen Gross <jgross@suse.com>
> ---
> tools/xenstored/domain.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index a6cd199fdc..f6ef37856c 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -923,6 +923,7 @@ static void domain_conn_reset(struct domain *domain)
>
> domain->interface->req_cons = domain->interface->req_prod = 0;
> domain->interface->rsp_cons = domain->interface->rsp_prod = 0;
> + xen_wmb();
> }
>
> /*
> @@ -988,12 +989,6 @@ static struct domain *introduce_domain(const void *ctx,
> /* Now domain belongs to its connection. */
> talloc_steal(domain->conn, domain);
>
> - if (!restore) {
> - /* Notify the domain that xenstore is available */
> - interface->connection = XENSTORE_CONNECTED;
> - xenevtchn_notify(xce_handle, domain->port);
> - }
> -
> if (!is_master_domain && !restore)
> fire_special_watches("@introduceDomain");
> } else {
> @@ -1033,6 +1028,13 @@ int do_introduce(const void *ctx, struct connection *conn,
>
> domain_conn_reset(domain);
>
> + if (domain->interface != NULL &&
> + domain->interface->connection == XENSTORE_RECONNECT) {
Identation should be fixed (both tested conditions at the same position).
> + /* Notify the domain that xenstore is available */
Please add a "." at the end of the sentence while you are moving this line.
> + domain->interface->connection = XENSTORE_CONNECTED;
> + xenevtchn_notify(xce_handle, domain->port);
> + }
> +
> send_ack(conn, XS_INTRODUCE);
>
> return 0;
Juergen
On Mon, Oct 16, 2023 at 8:38 AM Juergen Gross <jgross@suse.com> wrote: > On 14.10.23 01:06, Stefano Stabellini wrote: > > + /* Notify the domain that xenstore is available */ > > Please add a "." at the end of the sentence while you are moving this line. CODING_STYLE explicitly allows the omission of a '.' when a comment is only a single sentence. More generally, in this context in English at least, a comment seems to me more like a sign (which should not have punctuation) than a normal English sentence (which does). As such, to me the period actually seems wrong. In any case, CODING_STYLE was modified to avoid having to spend time arguing about this sort of thing. :-) -George
On 16.10.23 09:45, George Dunlap wrote: > On Mon, Oct 16, 2023 at 8:38 AM Juergen Gross <jgross@suse.com> wrote: >> On 14.10.23 01:06, Stefano Stabellini wrote: >>> + /* Notify the domain that xenstore is available */ >> >> Please add a "." at the end of the sentence while you are moving this line. > > CODING_STYLE explicitly allows the omission of a '.' when a comment is > only a single sentence. > > More generally, in this context in English at least, a comment seems > to me more like a sign (which should not have punctuation) than a > normal English sentence (which does). As such, to me the period > actually seems wrong. > > In any case, CODING_STYLE was modified to avoid having to spend time > arguing about this sort of thing. :-) I won't fight for the period. :-) Maybe I've just read too many kernel reviews asking for it. Juergen
© 2016 - 2024 Red Hat, Inc.