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

From: Haiyang Zhang
Date: Fri Dec 11 2015 - 10:57:39 EST




> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx]
> Sent: Friday, December 11, 2015 8:53 AM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> jasowang@xxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] hv_netvsc: Fix race condition on Multi-
> Send Data field
>
> Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:
>
> > Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> Reviewed-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> >> ---
> >> 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