RE: [PATCH V2 net-next] net: hns: Fix to conditionally convey RX checksum flag to stack

[Salil Mehta]

> -----Original Message-----
> From: Salil Mehta
> Sent: Thursday, December 01, 2016 12:09 PM
> To: 'David Miller'
> Cc: Zhuangyuzeng (Yisen); mehta.salil.lnk@xxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Linuxarm
> Subject: RE: [PATCH V2 net-next] net: hns: Fix to conditionally convey
> RX checksum flag to stack
> 
> > -----Original Message-----
> > From: David Miller [mailto:davem@xxxxxxxxxxxxx]
> > Sent: Wednesday, November 30, 2016 7:26 PM
> > To: Salil Mehta
> > Cc: Zhuangyuzeng (Yisen); mehta.salil.lnk@xxxxxxxxx;
> > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Linuxarm
> > Subject: Re: [PATCH V2 net-next] net: hns: Fix to conditionally
> convey
> > RX checksum flag to stack
> >
> > From: Salil Mehta <salil.mehta@xxxxxxxxxx>
> > Date: Tue, 29 Nov 2016 13:09:45 +0000
> >
> > > +	/* We only support checksum for IPv4,UDP(over IPv4 or IPv6),
> > TCP(over
> > > +	 * IPv4 or IPv6) and SCTP but we support many L3(IPv4, IPv6,
> > MPLS,
> > > +	 * PPPoE etc) and L4(TCP, UDP, GRE, SCTP, IGMP, ICMP etc.)
> > protocols.
> > > +	 * We want to filter out L3 and L4 protocols early on for which
> > checksum
> > > +	 * is not supported.
> >  ...
> > > +	 */
> > > +	l3id = hnae_get_field(flag, HNS_RXD_L3ID_M, HNS_RXD_L3ID_S);
> > > +	l4id = hnae_get_field(flag, HNS_RXD_L4ID_M, HNS_RXD_L4ID_S);
> > > +	if ((l3id != HNS_RX_FLAG_L3ID_IPV4) &&
> > > +	    ((l3id != HNS_RX_FLAG_L3ID_IPV6) ||
> > > +	     (l4id != HNS_RX_FLAG_L4ID_UDP)) &&
> > > +	    ((l3id != HNS_RX_FLAG_L3ID_IPV6) ||
> > > +	     (l4id != HNS_RX_FLAG_L4ID_TCP)) &&
> > > +	    (l4id != HNS_RX_FLAG_L4ID_SCTP))
> > > +		return;
> >
> > I have a hard time understanding this seemingly overcomplicated
> > check.
> >
> > It looks like if L3 is IPV4 it will accept any underlying L4
> protocol,
> > but is that what is really intended?  That doesn't match what this
> new
> > comment states.
> I agree that it is bit difficult to read. Earlier, I was banking on the
> register(mistakenly, its hardware implementation err ) to de-multiplex
> the checksum error type. The register supported indication of just IPv4
> Header Checksum Error as well (which meant it could carry any L4
> protocol).
> IPv6 does not have similar Header checksum error. Therefore, to check
> if it is just IPv4 Header checksum error or any supported L4 transport
> (UDP or TCP) error over IPv4 or IPv6 I had to bank upon above complex
> check
> 
> Below suggested solution check would have been insufficient for
> example, if packet had IPv4/IGMP and there was a checksum error in IPv4
> header.
> 
> Comment states:
> " We only support checksum for IPv4, UDP(over IPv4 or IPv6),
>   TCP(over IPv4 or IPv6) and SCTP"
>    1) Checksum of IPv4 (IPv4 header)
>    2) UDP(over IPv4 or IPv6)
>    3) TCP(over IPv4 or IPv6)
>    4) SCTP (over IPv4 or IPv6)*
> 
> (*) I should have put IPv4/IPv6 check for SCTP in the code
>     and made it clear in the comment as well?
> 
> >
> > My understanding is that the chip supports checksums for:
> >
> > 	UDP/IPV4
> > 	UDP/IPV6
> > 	TCP/IPV4
> > 	TCP/IPV6
> > 	SCTP/IPV4
> > 	SCTP/IPV6
> 
> Hardware also supports checksum of IPv4 Header.
> 
> >
> > So the simplest thing is to validate each level one at a time:
> >
> > 	if (l3 != IPV4 && l3 != IPV6)
> > 		return;
> > 	if (l4 != UDP && l4 != TCP && l4 != SCTP)
> > 		return;
> I guess above check will fail to catch cases like IPv4/IGMP, when there
> is a bad checksum in The IPv4 header.
> 
> But maybe now since we don't have any method to de-multiplex the kind
> of
> checksum error (cannot depend upon register) we can have below code
> re-arrangement:
> 
> hns_nic_rx_checksum() {
>       /* check supported L3 protocol */
> 	if (l3 != IPV4 && l3 != IPV6)
> 		return;
>       /* check if L3 protocols error */
>       if (l3e)
> 	 	return;
> 
>       /* check if the packets are fragmented */
> 	If (l3frags)
> 		Return;
> 
>       /* check supported L4 protocol */
>  	if (l4 != UDP && l4 != TCP && l4 != SCTP)
>  		return;
Hi David,
This logic might not do as well since IGMP packet with valid IP
Checksum will be bypassed here. We would want to set
CHECKSUM_UNNECESSARY for such IP packets as well.

It looks to me the cumbersome check in the PATCH V2 should
be retained.

>       /* check if any L4 protocol error */
>       if (l3e)
> 	 	return;
> 
>       /* packet with valid checksum - covey to stack */
>       skb->ip_summed = CHECKSUM_UNNECESSARY
> }

> 
> Hope I am not missing something here. Please correct my understanding
> if there is any gap here. Thanks!
> 
> Best regards
> Salil