linux-kernel - Re: [PATCH v2] HV: properly delay KVP packets when negotiation is in progress

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bmssyo87.fsf@vitty.brq.redhat.com>
Date:   Thu, 23 Mar 2017 17:03:36 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Long Li <longli@...rosoft.com>
Cc:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "devel\@linuxdriverproject.org" <devel@...uxdriverproject.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable\@vger.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v2] HV: properly delay KVP packets when negotiation is in progress

Long Li <longli@...rosoft.com> writes:

> The host may send multiple negotiation packets (due to timeout) before 
> the KVP user-mode daemon is connected. We need to defer processing  
> those packets until the daemon is negotiated and connected. It's okay
> for guest to respond to all negotiation packets.
>
> In addition, the host may send multiple staged KVP requests as soon as
> negotiation is done. We need to properly process those packets using 
> one tasklet for exclusive access to ring buffer.
>
> This patch is based on the work of Nick Meier 
> <Nick.Meier@...rosoft.com>
>
> The patch v2 has incorporated suggestion from Vitaly Kuznetsov 
> <vkuznets@...hat.com>.
>
> Signed-off-by: Long Li <longli@...rosoft.com>
> ---
>  drivers/hv/hv_kvp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index de26371..845b70b 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -113,7 +113,7 @@ static void kvp_poll_wrapper(void *channel)
>  {
>  	/* Transaction is finished, reset the state here to avoid races. */
>  	kvp_transaction.state = HVUTIL_READY;
> -	hv_kvp_onchannelcallback(channel);
> +	tasklet_schedule(&((struct vmbus_channel*)channel)->callback_event);
>  }

There is one more function in the code which calls
hv_kvp_onchannelcallback():

static void kvp_host_handshake_func(struct work_struct *dummy)
{
	hv_poll_channel(kvp_transaction.recv_channel, hv_kvp_onchannelcallback);
}

we can't replace hv_kvp_onchannelcallback with kvp_poll_wrapper here as
we don't want to reset kvp_transaction.state but it seems this should
also get updated, e.g. hv_poll_channel() here can be replaced with the 
direct 

 tasklet_schedule(&((struct vmbus_channel*)channel)->callback_event);

call. This will ensure hv_kvp_onchannelcallback() calls are always
serialized.

>
>  static void kvp_register_done(void)
> @@ -628,16 +628,17 @@ void hv_kvp_onchannelcallback(void *context)
>  		     NEGO_IN_PROGRESS,
>  		     NEGO_FINISHED} host_negotiatied = NEGO_NOT_STARTED;
>
> -	if (host_negotiatied == NEGO_NOT_STARTED &&
> -	    kvp_transaction.state < HVUTIL_READY) {
> +	if (kvp_transaction.state < HVUTIL_READY) {
>  		/*
>  		 * If userspace daemon is not connected and host is asking
>  		 * us to negotiate we need to delay to not lose messages.
>  		 * This is important for Failover IP setting.
>  		 */
> -		host_negotiatied = NEGO_IN_PROGRESS;
> -		schedule_delayed_work(&kvp_host_handshake_work,
> +		if (host_negotiatied == NEGO_NOT_STARTED) {
> +			host_negotiatied = NEGO_IN_PROGRESS;
> +			schedule_delayed_work(&kvp_host_handshake_work,
>  				      HV_UTIL_NEGO_TIMEOUT * HZ);
> +		}
>  		return;
>  	}
>  	if (kvp_transaction.state > HVUTIL_READY)
> @@ -705,6 +706,7 @@ void hv_kvp_onchannelcallback(void *context)
>  				       VM_PKT_DATA_INBAND, 0);
>
>  		host_negotiatied = NEGO_FINISHED;
> +		hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
>  	}
>
>  }

-- 
  Vitaly