| Message ID | 20210616144324.31652-4-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstored: Bug fixes + Improve Live-Update | expand |
> 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 > >
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
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,
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)