netdev - RE: [RFC 2/2] ravb: Add Rx checksum offload support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Nov 2021 10:15:24 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Sergey Shtylyov <s.shtylyov@....ru>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        Biju Das <biju.das@...renesas.com>
Subject: RE: [RFC 2/2] ravb: Add Rx checksum offload support

Hi Sergey Shtylyov,

> Subject: Re: [RFC 2/2] ravb: Add Rx checksum offload support
> 
> Hello!
> 
> On 11/23/21 4:31 PM, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header checkum for IPV4 and
> > TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> >
> > This patch adds Rx checksum offload supported by TOE.
> >
> > Signed-off-by: Biju Das <biju.das.jz@...renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  4 +++
> >  drivers/net/ethernet/renesas/ravb_main.c | 31
> > ++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index a96552348e2d..d0e5eec0636e 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -44,6 +44,10 @@
> >  #define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
> >  #define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping
> */
> >
> > +/* GbEthernet TOE hardware checksum values */
> > +#define TOE_RX_CSUM_OK		0x0000
> > +#define TOE_RX_CSUM_UNSUPPORTED	0xFFFF
> 
>    These are hardly needed IMO.

OK.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index c2b92c6a6cd2..2533e3401593 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device
> *ndev)
> >  	}
> >  }
> >
> > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> > +	u32 csum_ip_hdr, csum_proto;
> 
>    Why u32 if both sums are 16-bit?
OK, will make it 16-bit.

> 
> > +	u8 *hw_csum;
> > +
> > +	/* The hardware checksum is contained in sizeof(__sum16) * 2 = 4
> bytes
> > +	 * appended to packet data. First 2 bytes is ip header csum and last
> > +	 * 2 bytes is protocol csum.
> > +	 */
> > +	if (unlikely(skb->len < sizeof(__sum16) * 2))
> > +		return;
> > +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> > +	csum_proto = csum_unfold((__force
> > +__sum16)get_unaligned_le16(hw_csum));
> > +
> > +	hw_csum = skb_tail_pointer(skb) - 2 * sizeof(__sum16);
> > +	csum_ip_hdr = csum_unfold((__force
> > +__sum16)get_unaligned_le16(hw_csum));
> > +
> > +	skb->ip_summed = CHECKSUM_NONE;
> > +	if (csum_proto == TOE_RX_CSUM_OK) {
> > +		if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr ==
> TOE_RX_CSUM_OK)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +		else if (skb->protocol == htons(ETH_P_IPV6) &&
> > +			 csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> 
>    Checksum is unsupported and you declare it unnecessary?


Do you mean takeout the check for unsupported headercsum for IPV6 and the code like one below?

If(!csum_proto) {
	if ((skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr) || skb->protocol == htons(ETH_P_IPV6))
		skb->ip_summed = CHECKSUM_UNNECESSARY;
}

Snippet from H/W manual for reception handling

(1) Reception Handling
The result of Checksum Calculation is attached to last 4 byte of Ethernet Frames like Figure 30.25. And then the 
handled frames are transferred to memory by DMAC. If the frame does not have checksum error at the part of IPv4 
Header or TCP/UDP/ICMP, the value of “0000h” is attached to each part as the result of Checksum Calculation. The 
case of Unsupported Frame, the value of “FFFFh” is attached. For example, if the part of IP Header is unsupported, 
“FFFFh” is set to both field of IPv4 Header and TCP/UDP/ICMP. The case of IPv6, IPv4 Header field is always set to 
“FFFFh”. 

 
> 
> > +	}
> 
>    Now where's a call to skb_trim()?

Currently I haven't seen any issue without using skb_trim.

OK, as you suggested, will check and add skb_trim to takeout the last 4bytes.

Regards,
Biju