On 02/02/18 18:42, Joao Martins wrote:
> Commit fd8aa9095a95 ("xen: optimize xenbus driver for multiple concurrent
> xenstore accesses") optimized xenbus concurrent accesses but in doing so
> broke UABI of /dev/xen/xenbus. Through /dev/xen/xenbus applications are in
> charge of xenbus message exchange with the correct header and body. Now,
> after the mentioned commit the replies received by application will no
> longer have the header req_id echoed back as it was on request (see
> specification below for reference), because that particular field is being
> overwritten by kernel.
>
> struct xsd_sockmsg
> {
> uint32_t type; /* XS_??? */
> uint32_t req_id;/* Request identifier, echoed in daemon's response. */
> uint32_t tx_id; /* Transaction id (0 if not related to a transaction). */
> uint32_t len; /* Length of data following this. */
>
> /* Generally followed by nul-terminated string(s). */
> };
>
> Before there was only one request at a time so req_id could simply be
> forwarded back and forth. To allow simultaneous requests we need a
> different req_id for each message thus kernel keeps a monotonic increasing
> counter for this field and is written on every request irrespective of
> userspace value.
>
> Forwarding again the req_id on userspace requests is not a solution because
> we would open the possibility of userspace-generated req_id colliding with
> kernel ones. So this patch instead takes another route which is to
> artificially keep user req_id while keeping the xenbus logic as is. We do
> that by saving the original req_id before xs_send(), use the private kernel
> counter as req_id and then once reply comes and was validated, we restore
> back the original req_id.
>
> Cc: <stable@xxxxxxxxxxxxxxx> # 4.11
> Fixes: fd8aa9095a ("xen: optimize xenbus driver for multiple concurrent xenstore accesses")
> Reported-by: Bhavesh Davda <bhavesh.davda@xxxxxxxxxx>
> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
Juergen
> ---
> Here's a link to a unit test (https://pastebin.com/2q51j2sR) where req_id of
> reply and response are being asserted each request. Without this patch
> the assert will fail (e.g. try it with `./xswire_reqid_test name`). But
> on <= v4.10 or >= v4.11 with the fix above, it will print domain name 10
> times.
>
> Changes since v1:
> * Adjust commit message
> (Comments from Juergen on IRC)
> * Unilateraly save/restore req_id and remove xs_request_is_user()
> * Initialize req_id for kernel callers
> ---
> drivers/xen/xenbus/xenbus.h | 1 +
> drivers/xen/xenbus/xenbus_comms.c | 1 +
> drivers/xen/xenbus/xenbus_xs.c | 3 +++
> 3 files changed, 5 insertions(+)
>
> diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h
> index 149c5e7efc89..092981171df1 100644
> --- a/drivers/xen/xenbus/xenbus.h
> +++ b/drivers/xen/xenbus/xenbus.h
> @@ -76,6 +76,7 @@ struct xb_req_data {
> struct list_head list;
> wait_queue_head_t wq;
> struct xsd_sockmsg msg;
> + uint32_t caller_req_id;
> enum xsd_sockmsg_type type;
> char *body;
> const struct kvec *vec;
> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
> index 5b081a01779d..d239fc3c5e3d 100644
> --- a/drivers/xen/xenbus/xenbus_comms.c
> +++ b/drivers/xen/xenbus/xenbus_comms.c
> @@ -309,6 +309,7 @@ static int process_msg(void)
> goto out;
>
> if (req->state == xb_req_state_wait_reply) {
> + req->msg.req_id = req->caller_req_id;
> req->msg.type = state.msg.type;
> req->msg.len = state.msg.len;
> req->body = state.body;
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index 3e59590c7254..3f3b29398ab8 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -227,6 +227,8 @@ static void xs_send(struct xb_req_data *req, struct xsd_sockmsg *msg)
> req->state = xb_req_state_queued;
> init_waitqueue_head(&req->wq);
>
> + /* Save the caller req_id and restore it later in the reply */
> + req->caller_req_id = req->msg.req_id;
> req->msg.req_id = xs_request_enter(req);
>
> mutex_lock(&xb_write_mutex);
> @@ -310,6 +312,7 @@ static void *xs_talkv(struct xenbus_transaction t,
> req->num_vecs = num_vecs;
> req->cb = xs_wake_up;
>
> + msg.req_id = 0;
> msg.tx_id = t.id;
> msg.type = type;
> msg.len = 0;
>