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] [day] [month] [year] [list]

Message-ID: <c48e2250-3fd1-dd0c-d187-6efc33609d15@omp.ru>
Date:   Sat, 27 Nov 2021 00:27:09 +0300
From:   Sergey Shtylyov <s.shtylyov@....ru>
To:     Biju Das <biju.das.jz@...renesas.com>,
        "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

Hello!

On 11/25/21 1:15 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
[...]
>>> 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
[...]
>>> +	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?

   No, I meant that the code looks strange. 

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

   I think this statement doesn't make sense, unless the TOE itself doesn't check the protocol ID in
the Ethernet frame (which it should). It also seems (after reading <linux/skbuff.h>) that enabling
IPv4 header checksum calculation is pointless -- you don't have the means to report it anyway...
You may want to set skb-csum_level though (not sure if that's not already 0).

> 		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.

   We do it for EtherAVB, why not do it for GbEther?

> Regards,
> Biju

MBR, Sergey