Re: [PATCH 02/10] Drivers: hv: utils: run polling callback always in interrupt context

[Vitaly Kuznetsov]

"K. Y. Srinivasan" <kys@xxxxxxxxxxxxx> writes:

> From: Olaf Hering <olaf@xxxxxxxxx>
>
> All channel interrupts are bound to specific VCPUs in the guest
> at the point channel is created. While currently, we invoke the
> polling function on the correct CPU (the CPU to which the channel
> is bound to) in some cases we may run the polling function in
> a non-interrupt context. This  potentially can cause an issue as the
> polling function can be interrupted by the channel callback function.
> Fix the issue by running the polling function on the appropriate CPU
> at interrupt level. Additional details of the issue being addressed by
> this patch are given below:
>
> Currently hv_fcopy_onchannelcallback is called from interrupts and also
> via the ->write function of hv_utils. Since the used global variables to
> maintain state are not thread safe the state can get out of sync.
> This affects the variable state as well as the channel inbound buffer.
>
> As suggested by KY adjust hv_poll_channel to always run the given
> callback on the cpu which the channel is bound to. This avoids the need
> for locking because all the util services are single threaded and only
> one transaction is active at any given point in time.
>
> Additionally, remove the context variable, they will always be the same as
> recv_channel.
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> ---
>  drivers/hv/hv_fcopy.c     |   37 +++++++++++++------------------------
>  drivers/hv/hv_kvp.c       |   28 ++++++++++------------------
>  drivers/hv/hv_snapshot.c  |   29 +++++++++++------------------
>  drivers/hv/hyperv_vmbus.h |    6 +-----
>  4 files changed, 35 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index bbdec50..4eab465 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -51,7 +51,6 @@ static struct {
>  	struct hv_fcopy_hdr  *fcopy_msg; /* current message */
>  	struct vmbus_channel *recv_channel; /* chn we got the request */
>  	u64 recv_req_id; /* request ID. */
> -	void *fcopy_context; /* for the channel callback */
>  } fcopy_transaction;
>
>  static void fcopy_respond_to_host(int error);
> @@ -67,6 +66,13 @@ static struct hvutil_transport *hvt;
>   */
>  static int dm_reg_value;
>
> +static void fcopy_poll_wrapper(void *channel)
> +{
> +	/* Transaction is finished, reset the state here to avoid races. */
> +	fcopy_transaction.state = HVUTIL_READY;
> +	hv_fcopy_onchannelcallback(channel);
> +}
> +
>  static void fcopy_timeout_func(struct work_struct *dummy)
>  {
>  	/*
> @@ -74,13 +80,7 @@ static void fcopy_timeout_func(struct work_struct *dummy)
>  	 * process the pending transaction.
>  	 */
>  	fcopy_respond_to_host(HV_E_FAIL);
> -
> -	/* Transaction is finished, reset the state. */
> -	if (fcopy_transaction.state > HVUTIL_READY)
> -		fcopy_transaction.state = HVUTIL_READY;
> -
> -	hv_poll_channel(fcopy_transaction.fcopy_context,
> -			hv_fcopy_onchannelcallback);
> +	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
>  }
>
>  static int fcopy_handle_handshake(u32 version)
> @@ -108,9 +108,9 @@ static int fcopy_handle_handshake(u32 version)
>  		return -EINVAL;
>  	}
>  	pr_debug("FCP: userspace daemon ver. %d registered\n", version);
> +	/* Forward state for hv_fcopy_onchannelcallback */
>  	fcopy_transaction.state = HVUTIL_READY;
> -	hv_poll_channel(fcopy_transaction.fcopy_context,
> -			hv_fcopy_onchannelcallback);
> +	hv_poll_channel(fcopy_transaction.recv_channel, fcopy_poll_wrapper);
>  	return 0;
>  }
>
> @@ -227,15 +227,8 @@ void hv_fcopy_onchannelcallback(void *context)
>  	int util_fw_version;
>  	int fcopy_srv_version;
>
> -	if (fcopy_transaction.state > HVUTIL_READY) {
> -		/*
> -		 * We will defer processing this callback once
> -		 * the current transaction is complete.
> -		 */
> -		fcopy_transaction.fcopy_context = context;
> +	if (fcopy_transaction.state > HVUTIL_READY)
>  		return;
> -	}
> -	fcopy_transaction.fcopy_context = NULL;
>
>  	vmbus_recvpacket(channel, recv_buffer, PAGE_SIZE * 2, &recvlen,
>  			 &requestid);
> @@ -295,9 +288,6 @@ static int fcopy_on_msg(void *msg, int len)
>  	if (fcopy_transaction.state == HVUTIL_DEVICE_INIT)
>  		return fcopy_handle_handshake(*val);
>
> -	if (fcopy_transaction.state != HVUTIL_USERSPACE_REQ)
> -		return -EINVAL;
> -

This particular change seems unrelated and I'm unsure it's safe to
remove this check. It is meant to protect against daemon screwing the
protocol and writing to the device when it wasn't requested for an
action. It is correct to propagate -EINVAL in this case. Or am I missing
something and the check is redundant now?

Thanks,

[...]

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/