> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: Friday, December 11, 2015 8:53 AM > To: Haiyang Zhang <haiya...@microsoft.com> > Cc: da...@davemloft.net; net...@vger.kernel.org; o...@aepfle.de; > jasow...@redhat.com; driverdev-de...@linuxdriverproject.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi- > Send Data field > > Vitaly Kuznetsov <vkuzn...@redhat.com> writes: > > > Haiyang Zhang <haiya...@microsoft.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 <haiya...@microsoft.com> > >> Reviewed-by: K. Y. Srinivasan <k...@microsoft.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)? > > 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.
Thanks for the suggestion. I still prefer fixing this bug in the netvsc_send(), and keep the common entry point for sending data & control packets. So, the total lines of code is smaller. And, when we add more pre-processing steps for send in the future, we won't need to change multiple places. Thanks, - Haiyang
