From: Julien Grall <jgrall@amazon.com>
call_delayed() is currently assuming that conn->in is NULL when
handling delayed request. However, the connection is not paused.
Therefore new request can be processed and conn->in may be non-NULL
if we have only received a partial request.
Furthermore, as we overwrite conn->in, the current partial request
will not be transferred. This will result to corrupt the connection.
Rather than updating conn->in, stash the LU request in lu_status and
let each callback for delayed request to update conn->in when
necessary.
To keep a sane interface, the code to write the "OK" response the
LU request is moved in xenstored_core.c.
Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request")
Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live update")
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
This is fixing bugs from two separate commits. I couldn't figure out
how to split in two patches without breaking bisection.
---
tools/xenstore/xenstored_control.c | 41 ++++++++++++++++++++++++++++--
tools/xenstore/xenstored_control.h | 3 +++
tools/xenstore/xenstored_core.c | 17 +++----------
3 files changed, 46 insertions(+), 15 deletions(-)
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index d08a2b961432..7acc2d134f9f 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -50,6 +50,9 @@ struct live_update {
/* For verification the correct connection is acting. */
struct connection *conn;
+ /* Pointer to the command used to request LU */
+ struct buffered_data *in;
+
#ifdef __MINIOS__
void *kernel;
unsigned int kernel_size;
@@ -100,6 +103,7 @@ static const char *lu_begin(struct connection *conn)
if (!lu_status)
return "Allocation failure.";
lu_status->conn = conn;
+ lu_status->in = conn->in;
talloc_set_destructor(lu_status, lu_destroy);
return NULL;
@@ -110,11 +114,34 @@ struct connection *lu_get_connection(void)
return lu_status ? lu_status->conn : NULL;
}
+unsigned int lu_write_response(FILE *fp)
+{
+ struct xsd_sockmsg msg;
+
+ assert(lu_status);
+
+ msg = lu_status->in->hdr.msg;
+
+ msg.len = sizeof("OK");
+ if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
+ return 0;
+ if (fp && fwrite("OK", msg.len, 1, fp) != 1)
+ return 0;
+
+ return sizeof(msg) + msg.len;
+}
+
#else
struct connection *lu_get_connection(void)
{
return NULL;
}
+
+unsigned int lu_write_response(FILE *fp)
+{
+ /* Unsupported */
+ return 0;
+}
#endif
struct cmd_s {
@@ -658,6 +685,8 @@ static bool do_lu_start(struct delayed_request *req)
{
time_t now = time(NULL);
const char *ret;
+ struct buffered_data *saved_in;
+ struct connection *conn = lu_status->conn;
if (!lu_check_lu_allowed()) {
if (now < lu_status->started_at + lu_status->timeout)
@@ -668,8 +697,9 @@ static bool do_lu_start(struct delayed_request *req)
}
}
+ assert(req->in == lu_status->in);
/* Dump out internal state, including "OK" for live update. */
- ret = lu_dump_state(req->in, lu_status->conn);
+ ret = lu_dump_state(req->in, conn);
if (!ret) {
/* Perform the activation of new binary. */
ret = lu_activate_binary(req->in);
@@ -677,7 +707,14 @@ static bool do_lu_start(struct delayed_request *req)
/* We will reach this point only in case of failure. */
out:
- send_reply(lu_status->conn, XS_CONTROL, ret, strlen(ret) + 1);
+ /*
+ * send_reply() will send the response for conn->in. Save the current
+ * conn->in and restore it afterwards.
+ */
+ saved_in = conn->in;
+ conn->in = req->in;
+ send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
+ conn->in = saved_in;
talloc_free(lu_status);
return true;
diff --git a/tools/xenstore/xenstored_control.h b/tools/xenstore/xenstored_control.h
index 6842b8d88760..27d7f19e4b7f 100644
--- a/tools/xenstore/xenstored_control.h
+++ b/tools/xenstore/xenstored_control.h
@@ -20,3 +20,6 @@ int do_control(struct connection *conn, struct buffered_data *in);
void lu_read_state(void);
struct connection *lu_get_connection(void);
+
+/* Write the "OK" response for the live-update command */
+unsigned int lu_write_response(FILE *fp);
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 607187361d84..41b26d7094c8 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -272,15 +272,10 @@ static int undelay_request(void *_req)
static void call_delayed(struct connection *conn, struct delayed_request *req)
{
- assert(conn->in == NULL);
- conn->in = req->in;
-
if (req->func(req)) {
undelay_request(req);
talloc_set_destructor(req, NULL);
}
-
- conn->in = NULL;
}
int delay_request(struct connection *conn, struct buffered_data *in,
@@ -2375,7 +2370,7 @@ const char *dump_state_buffered_data(FILE *fp, const struct connection *c,
struct buffered_data *out, *in = c->in;
bool partial = true;
- if (in && c != lu_get_connection()) {
+ if (in) {
len = in->inhdr ? in->used : sizeof(in->hdr);
if (fp && fwrite(&in->hdr, len, 1, fp) != 1)
return "Dump read data error";
@@ -2416,16 +2411,12 @@ const char *dump_state_buffered_data(FILE *fp, const struct connection *c,
/* Add "OK" for live-update command. */
if (c == lu_get_connection()) {
- struct xsd_sockmsg msg = c->in->hdr.msg;
+ unsigned int rc = lu_write_response(fp);
- msg.len = sizeof("OK");
- if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
+ if (!rc)
return "Dump buffered data error";
- len += sizeof(msg);
- if (fp && fwrite("OK", msg.len, 1, fp) != 1)
- return "Dump buffered data error";
- len += msg.len;
+ len += rc;
}
if (sc)
--
2.17.1
On 16.06.21 16:43, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> call_delayed() is currently assuming that conn->in is NULL when
> handling delayed request. However, the connection is not paused.
> Therefore new request can be processed and conn->in may be non-NULL
> if we have only received a partial request.
>
> Furthermore, as we overwrite conn->in, the current partial request
> will not be transferred. This will result to corrupt the connection.
>
> Rather than updating conn->in, stash the LU request in lu_status and
> let each callback for delayed request to update conn->in when
> necessary.
>
> To keep a sane interface, the code to write the "OK" response the
> LU request is moved in xenstored_core.c.
>
> Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request")
> Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live update")
> Signed-off-by: Julien Grall <jgrall@amazon.com>
With dropping the conn parameter from call_delayed as already
mentioned by Luca you can add my:
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
On 24.06.21 09:32, Juergen Gross wrote:
> On 16.06.21 16:43, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> call_delayed() is currently assuming that conn->in is NULL when
>> handling delayed request. However, the connection is not paused.
>> Therefore new request can be processed and conn->in may be non-NULL
>> if we have only received a partial request.
>>
>> Furthermore, as we overwrite conn->in, the current partial request
>> will not be transferred. This will result to corrupt the connection.
>>
>> Rather than updating conn->in, stash the LU request in lu_status and
>> let each callback for delayed request to update conn->in when
>> necessary.
>>
>> To keep a sane interface, the code to write the "OK" response the
>> LU request is moved in xenstored_core.c.
>>
>> Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution
>> of a xenstore request")
>> Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live
>> update")
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> With dropping the conn parameter from call_delayed as already
> mentioned by Luca you can add my:
Oh, please drop my request to delete the conn parameter, as it is being
used in patch 4 again.
>
> Reviewed-by: Juergen Gross <jgross@suse.com>
This stands, of course.
Juergen
> On 24 Jun 2021, at 08:34, Juergen Gross <jgross@suse.com> wrote:
>
> On 24.06.21 09:32, Juergen Gross wrote:
>> On 16.06.21 16:43, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> call_delayed() is currently assuming that conn->in is NULL when
>>> handling delayed request. However, the connection is not paused.
>>> Therefore new request can be processed and conn->in may be non-NULL
>>> if we have only received a partial request.
>>>
>>> Furthermore, as we overwrite conn->in, the current partial request
>>> will not be transferred. This will result to corrupt the connection.
>>>
>>> Rather than updating conn->in, stash the LU request in lu_status and
>>> let each callback for delayed request to update conn->in when
>>> necessary.
>>>
>>> To keep a sane interface, the code to write the "OK" response the
>>> LU request is moved in xenstored_core.c.
>>>
>>> Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution
>
>>> of a xenstore request")
>>> Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live update")
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> With dropping the conn parameter from call_delayed as already
>> mentioned by Luca you can add my:
>
> Oh, please drop my request to delete the conn parameter, as it is being
> used in patch 4 again.
>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
>
> This stands, of course.
Hi Juergen,
I’m sorry but I don’t see when the parameter is used, in patch 4 we have this call:
line 2344:
if (delayed_requests) {
list_for_each_entry(conn, &connections, list) {
struct delayed_request *req, *tmp;
list_for_each_entry_safe(req, tmp,
&conn->delayed, list)
call_delayed(conn, req);
}
}
But call_delayed is still this one:
Line 273:
static void call_delayed(struct connection *conn, struct delayed_request *req)
{
if (req->func(req)) {
undelay_request(req);
talloc_set_destructor(req, NULL);
}
}
Am I missing something?
Cheers,
Luca
>
>
> Juergen
> <OpenPGP_0xB0DE9DD628BF132F.asc>
On 24.06.21 09:56, Luca Fancellu wrote:
>
>
>> On 24 Jun 2021, at 08:34, Juergen Gross <jgross@suse.com> wrote:
>>
>> On 24.06.21 09:32, Juergen Gross wrote:
>>> On 16.06.21 16:43, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> call_delayed() is currently assuming that conn->in is NULL when
>>>> handling delayed request. However, the connection is not paused.
>>>> Therefore new request can be processed and conn->in may be non-NULL
>>>> if we have only received a partial request.
>>>>
>>>> Furthermore, as we overwrite conn->in, the current partial request
>>>> will not be transferred. This will result to corrupt the connection.
>>>>
>>>> Rather than updating conn->in, stash the LU request in lu_status and
>>>> let each callback for delayed request to update conn->in when
>>>> necessary.
>>>>
>>>> To keep a sane interface, the code to write the "OK" response the
>>>> LU request is moved in xenstored_core.c.
>>>>
>>>> Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution
>>
>>>> of a xenstore request")
>>>> Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live
update")
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> With dropping the conn parameter from call_delayed as already
>>> mentioned by Luca you can add my:
>>
>> Oh, please drop my request to delete the conn parameter, as it is being
>> used in patch 4 again.
>>
>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>
>> This stands, of course.
>
> Hi Juergen,
>
> I’m sorry but I don’t see when the parameter is used, in patch 4 we have this call:
>
> line 2344:
> if (delayed_requests) {
> list_for_each_entry(conn, &connections, list) {
> struct delayed_request *req, *tmp;
>
> list_for_each_entry_safe(req, tmp,
> &conn->delayed, list)
> call_delayed(conn, req);
> }
> }
>
> But call_delayed is still this one:
>
> Line 273:
> static void call_delayed(struct connection *conn, struct delayed_request *req)
> {
> if (req->func(req)) {
> undelay_request(req);
> talloc_set_destructor(req, NULL);
> }
> }
>
> Am I missing something?
Hmm, I seem to have mixed up something.
Yes, you are right. So off with the conn parameter (again).
Juergen
> On 16 Jun 2021, at 15:43, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> call_delayed() is currently assuming that conn->in is NULL when
> handling delayed request. However, the connection is not paused.
> Therefore new request can be processed and conn->in may be non-NULL
> if we have only received a partial request.
>
> Furthermore, as we overwrite conn->in, the current partial request
> will not be transferred. This will result to corrupt the connection.
>
> Rather than updating conn->in, stash the LU request in lu_status and
> let each callback for delayed request to update conn->in when
> necessary.
>
> To keep a sane interface, the code to write the "OK" response the
> LU request is moved in xenstored_core.c.
>
> Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request")
> Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live update")
> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> ----
>
> This is fixing bugs from two separate commits. I couldn't figure out
> how to split in two patches without breaking bisection.
> ---
> tools/xenstore/xenstored_control.c | 41 ++++++++++++++++++++++++++++--
> tools/xenstore/xenstored_control.h | 3 +++
> tools/xenstore/xenstored_core.c | 17 +++----------
> 3 files changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
> index d08a2b961432..7acc2d134f9f 100644
> --- a/tools/xenstore/xenstored_control.c
> +++ b/tools/xenstore/xenstored_control.c
> @@ -50,6 +50,9 @@ struct live_update {
> /* For verification the correct connection is acting. */
> struct connection *conn;
>
> + /* Pointer to the command used to request LU */
> + struct buffered_data *in;
> +
> #ifdef __MINIOS__
> void *kernel;
> unsigned int kernel_size;
> @@ -100,6 +103,7 @@ static const char *lu_begin(struct connection *conn)
> if (!lu_status)
> return "Allocation failure.";
> lu_status->conn = conn;
> + lu_status->in = conn->in;
> talloc_set_destructor(lu_status, lu_destroy);
>
> return NULL;
> @@ -110,11 +114,34 @@ struct connection *lu_get_connection(void)
> return lu_status ? lu_status->conn : NULL;
> }
>
> +unsigned int lu_write_response(FILE *fp)
> +{
> + struct xsd_sockmsg msg;
> +
> + assert(lu_status);
> +
> + msg = lu_status->in->hdr.msg;
> +
> + msg.len = sizeof("OK");
> + if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
> + return 0;
> + if (fp && fwrite("OK", msg.len, 1, fp) != 1)
> + return 0;
> +
> + return sizeof(msg) + msg.len;
> +}
> +
> #else
> struct connection *lu_get_connection(void)
> {
> return NULL;
> }
> +
> +unsigned int lu_write_response(FILE *fp)
> +{
> + /* Unsupported */
> + return 0;
> +}
> #endif
>
> struct cmd_s {
> @@ -658,6 +685,8 @@ static bool do_lu_start(struct delayed_request *req)
> {
> time_t now = time(NULL);
> const char *ret;
> + struct buffered_data *saved_in;
> + struct connection *conn = lu_status->conn;
>
> if (!lu_check_lu_allowed()) {
> if (now < lu_status->started_at + lu_status->timeout)
> @@ -668,8 +697,9 @@ static bool do_lu_start(struct delayed_request *req)
> }
> }
>
> + assert(req->in == lu_status->in);
> /* Dump out internal state, including "OK" for live update. */
> - ret = lu_dump_state(req->in, lu_status->conn);
> + ret = lu_dump_state(req->in, conn);
> if (!ret) {
> /* Perform the activation of new binary. */
> ret = lu_activate_binary(req->in);
> @@ -677,7 +707,14 @@ static bool do_lu_start(struct delayed_request *req)
>
> /* We will reach this point only in case of failure. */
> out:
> - send_reply(lu_status->conn, XS_CONTROL, ret, strlen(ret) + 1);
> + /*
> + * send_reply() will send the response for conn->in. Save the current
> + * conn->in and restore it afterwards.
> + */
> + saved_in = conn->in;
> + conn->in = req->in;
> + send_reply(conn, XS_CONTROL, ret, strlen(ret) + 1);
> + conn->in = saved_in;
> talloc_free(lu_status);
>
> return true;
> diff --git a/tools/xenstore/xenstored_control.h b/tools/xenstore/xenstored_control.h
> index 6842b8d88760..27d7f19e4b7f 100644
> --- a/tools/xenstore/xenstored_control.h
> +++ b/tools/xenstore/xenstored_control.h
> @@ -20,3 +20,6 @@ int do_control(struct connection *conn, struct buffered_data *in);
> void lu_read_state(void);
>
> struct connection *lu_get_connection(void);
> +
> +/* Write the "OK" response for the live-update command */
> +unsigned int lu_write_response(FILE *fp);
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 607187361d84..41b26d7094c8 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -272,15 +272,10 @@ static int undelay_request(void *_req)
>
> static void call_delayed(struct connection *conn, struct delayed_request *req)
Here the conn parameter is not needed anymore, or am I missing something?
Cheers,
Luca
> {
> - assert(conn->in == NULL);
> - conn->in = req->in;
> -
> if (req->func(req)) {
> undelay_request(req);
> talloc_set_destructor(req, NULL);
> }
> -
> - conn->in = NULL;
> }
>
> int delay_request(struct connection *conn, struct buffered_data *in,
> @@ -2375,7 +2370,7 @@ const char *dump_state_buffered_data(FILE *fp, const struct connection *c,
> struct buffered_data *out, *in = c->in;
> bool partial = true;
>
> - if (in && c != lu_get_connection()) {
> + if (in) {
> len = in->inhdr ? in->used : sizeof(in->hdr);
> if (fp && fwrite(&in->hdr, len, 1, fp) != 1)
> return "Dump read data error";
> @@ -2416,16 +2411,12 @@ const char *dump_state_buffered_data(FILE *fp, const struct connection *c,
>
> /* Add "OK" for live-update command. */
> if (c == lu_get_connection()) {
> - struct xsd_sockmsg msg = c->in->hdr.msg;
> + unsigned int rc = lu_write_response(fp);
>
> - msg.len = sizeof("OK");
> - if (fp && fwrite(&msg, sizeof(msg), 1, fp) != 1)
> + if (!rc)
> return "Dump buffered data error";
> - len += sizeof(msg);
> - if (fp && fwrite("OK", msg.len, 1, fp) != 1)
>
> - return "Dump buffered data error";
> - len += msg.len;
> + len += rc;
> }
>
> if (sc)
> --
> 2.17.1
>
>
Hi Luca, On 21/06/2021 10:55, Luca Fancellu wrote: >> diff --git a/tools/xenstore/xenstored_control.h b/tools/xenstore/xenstored_control.h >> index 6842b8d88760..27d7f19e4b7f 100644 >> --- a/tools/xenstore/xenstored_control.h >> +++ b/tools/xenstore/xenstored_control.h >> @@ -20,3 +20,6 @@ int do_control(struct connection *conn, struct buffered_data *in); >> void lu_read_state(void); >> >> struct connection *lu_get_connection(void); >> + >> +/* Write the "OK" response for the live-update command */ >> +unsigned int lu_write_response(FILE *fp); >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index 607187361d84..41b26d7094c8 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -272,15 +272,10 @@ static int undelay_request(void *_req) >> >> static void call_delayed(struct connection *conn, struct delayed_request *req) > > Here the conn parameter is not needed anymore, or am I missing something? The parameter is now unused. I will drop it. Cheers, -- Julien Grall
© 2016 - 2026 Red Hat, Inc.