From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753496AbbLKNwh (ORCPT ); Fri, 11 Dec 2015 08:52:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56542 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751687AbbLKNwg (ORCPT ); Fri, 11 Dec 2015 08:52:36 -0500 From: Vitaly Kuznetsov To: Haiyang Zhang Cc: davem@davemloft.net, netdev@vger.kernel.org, olaf@aepfle.de, jasowang@redhat.com, driverdev-devel@linuxdriverproject.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-Send Data field References: <1449778775-14404-1-git-send-email-haiyangz@microsoft.com> <877fklyxiu.fsf@vitty.brq.redhat.com> Date: Fri, 11 Dec 2015 14:52:32 +0100 In-Reply-To: <877fklyxiu.fsf@vitty.brq.redhat.com> (Vitaly Kuznetsov's message of "Fri, 11 Dec 2015 13:34:17 +0100") Message-ID: <87vb85xfbz.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Vitaly Kuznetsov writes: > Haiyang Zhang 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 >> Reviewed-by: K. Y. Srinivasan >> --- >> 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)? Oh, my bad, it was me who was looking at the wrong branch... Sorry for the noise. > >> 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. This patch will need some changes but the suggestion still stands: let's untangle sending data and control messages. -- Vitaly