netdev - Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field

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: <877fklyxiu.fsf@vitty.brq.redhat.com>
Date:	Fri, 11 Dec 2015 13:34:17 +0100
From:	Vitaly Kuznetsov <vkuznets@...hat.com>
To:	Haiyang Zhang <haiyangz@...rosoft.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org, olaf@...fle.de,
	jasowang@...hat.com, driverdev-devel@...uxdriverproject.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field

Haiyang Zhang <haiyangz@...rosoft.com> writes:

> In commit 2a04ae8acb14 ("hv_netvsc: remove locking in netvsc_send()"), the
> locking for MSD (Multi-Send Data) field was removed. This could cause a
> race condition between RNDIS control messages and data packets processing,
> because these two types of traffic are not synchronized.
> This patch fixes this issue by sending control messages out directly
> without reading MSD field.
>
> Signed-off-by: Haiyang Zhang <haiyangz@...rosoft.com>
> Reviewed-by: K. Y. Srinivasan <kys@...rosoft.com>
> ---
>  drivers/net/hyperv/netvsc.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 02bab9a..059fc52 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -867,6 +867,14 @@ int netvsc_send(struct hv_device *device,
>  	packet->send_buf_index = NETVSC_INVALID_INDEX;
>  	packet->cp_partial = false;
>  
> +	/* Send control message directly without accessing msd (Multi-Send
> +	 * Data) field which may be changed during data packet processing.
> +	 */
> +	if (!skb) {
> +		cur_send = packet;
> +		goto send_now;
> +	}
> +

Is is supposed to be applied on top of some other patches? It doesn't
compile on top of current net-next:

drivers/net/hyperv/netvsc.c: In function ‘netvsc_send’:
drivers/net/hyperv/netvsc.c:865:7: error: ‘skb’ undeclared (first use in
this function)
  if (!skb) {
         ^
         
Did you mean to check rndis_msg instead (as skb is not defined here)?

>  	msdp = &net_device->msd[q_idx];
>  
>  	/* batch packets in send buffer if possible */
> @@ -939,6 +947,7 @@ int netvsc_send(struct hv_device *device,
>  		}
>  	}
>  
> +send_now:
>  	if (cur_send)
>  		ret = netvsc_send_pkt(cur_send, net_device, pb, skb);

I suppose we untangle these two pathes completely: let
rndis_filter_send_request() call netvsc_send_pkt() directly. Please see
my patch attached (note: it should be split in 3 patches if
submitted). If you like the idea I can send it.

-- 
  Vitaly


View attachment "untangle.patch" of type "text/x-patch" (4045 bytes)