RE: [PATCH hyperv-fixes] hv_netvsc: Fix IP header checksum for coalesced packets

[Haiyang Zhang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
> Sent: Saturday, February 23, 2019 11:46 AM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxxxxxx>
> Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; sashal@xxxxxxxxxx; linux-
> hyperv@xxxxxxxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>; olaf@xxxxxxxxx; vkuznets
> <vkuznets@xxxxxxxxxx>; davem@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH hyperv-fixes] hv_netvsc: Fix IP header checksum for
> coalesced packets
> 
> On Fri, 22 Feb 2019 18:25:03 +0000
> Haiyang Zhang <haiyangz@xxxxxxxxxxxxxxxxx> wrote:
> 
> > From: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> >
> > Incoming packets may have IP header checksum verified by the host.
> > They may not have IP header checksum computed after coalescing.
> > This patch re-compute the checksum when necessary, otherwise the
> > packets may be dropped, because Linux network stack always checks it.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > ---
> >  drivers/net/hyperv/netvsc_drv.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c index 256adbd044f5..cf4897043e83
> > 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -744,6 +744,14 @@ void netvsc_linkstatus_callback(struct net_device
> *net,
> >  	schedule_delayed_work(&ndev_ctx->dwork, 0);  }
> >
> > +static void netvsc_comp_ipcsum(struct sk_buff *skb) {
> > +	struct iphdr *iph = (struct iphdr *)skb->data;
> 
> Can you use iphdr(skb) here?
This skb is just allocated by netvsc, the skb->network_header is not set yet.

> 
> > +
> > +	iph->check = 0;
> > +	iph->check = ip_fast_csum(iph, iph->ihl); }
> > +
> >  static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
> >  					     struct netvsc_channel *nvchan)
> { @@ -770,9 +778,17 @@
> > static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
> >  	/* skb is already created with CHECKSUM_NONE */
> >  	skb_checksum_none_assert(skb);
> >
> > -	/*
> > -	 * In Linux, the IP checksum is always checked.
> > -	 * Do L4 checksum offload if enabled and present.
> > +	/* Incoming packets may have IP header checksum verified by the
> host.
> > +	 * They may not have IP header checksum computed after coalescing.
> > +	 * We compute it here if the flags are set, because on Linux, the IP
> > +	 * checksum is always checked.
> > +	 */
> > +	if (csum_info && csum_info->receive.ip_checksum_value_invalid &&
> > +	    csum_info->receive.ip_checksum_succeeded &&
> > +	    skb->protocol == htons(ETH_P_IP))
> > +		netvsc_comp_ipcsum(skb);
> 
> Does this still handle for coalesced and non-coalesced packets which are
> received with bad IP checksum?  My concern is that you are potentially
> correcting the checksum for a packet whose received checksum was bad.

Windows networking team told me that the flags above indicate host side 
already verified the checksum. Online doc is here:
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/indicating-coalesced-segments
If the NIC or miniport driver validates the TCP and IPv4 checksums but does not recompute them for the coalesced segment, it must set the TcpChecksumValueInvalid and IpChecksumValueInvalid flags in the NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO structure. Additionally, in this case the NIC or miniport driver may optionally zero out the TCP and IPv4 header checksum values in the segment.

The NIC and miniport driver must always set the IpChecksumSucceeded and TcpChecksumSucceeded flags in the NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO structure before indicating the coalesced segment.

Thanks,
- Haiyang