Re: [PATCH v2] xenbus: track caller request id

[Juergen Gross]

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;
>