netdev - Re: [RFC 07/12] ravb: Fillup ravb_rx

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:   Fri, 8 Oct 2021 20:59:50 +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:     Sergei Shtylyov <sergei.shtylyov@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Sergey Shtylyov <s.shtylyov@...russia.ru>,
        "Adam Ford" <aford173@...il.com>, Andrew Lunn <andrew@...n.ch>,
        Yuusuke Ashizuka <ashiduka@...itsu.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>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        Biju Das <biju.das@...renesas.com>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

On 10/8/21 9:46 AM, Biju Das wrote:

[...]
>>>>>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
>>>>>>>>
>>>>>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
>>>>>>>> consistent with the naming convention used in sh_eth driver.
>>>>>>>>
>>>>>>>> Signed-off-by: Biju Das <biju.das.jz@...renesas.com>
>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>> <prabhakar.mahadev-lad.rj@...renesas.com>[...]
>>>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> index 37164a983156..42573eac82b9 100644
>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
>>>>>>>> net_device
>>>>>>> *ndev)
>>>>>>>>  	}
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>>>>>>> +	u8 *hw_csum;
>>>>>>>> +
>>>>>>>> +	/* The hardware checksum is contained in sizeof(__sum16) (2)
>>> bytes
>>>>>>>> +	 * appended to packet data
>>>>>>>> +	 */
>>>>>>>> +	if (unlikely(skb->len < sizeof(__sum16)))
>>>>>>>> +		return;
>>>>>>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
[...]
>>>> Please check the section 30.5.6.1 checksum calculation handling> And
>>>> figure 30.25 the field of checksum attaching field
>>>
>>>    I have.
>>>
>>>> Also see Table 30.17 for checksum values for non-error conditions.
>>>
>>>> TCP/UDP/ICPM checksum is at last 2bytes.
>>>
>>>    What are you arguing with then? :-)
>>>    My point was that your code fetched the TCP/UDP/ICMP checksum ISO
>>> the IP checksum because it subtracts sizeof(__sum16), while should
>>> probably subtract sizeof(__wsum)
>>
>> Agreed. My code missed IP4 checksum result. May be we need to extract 2
>> checksum info from last 4 bytes.  First checksum(2bytes) is IP4 header
>> checksum and next checksum(2 bytes)  for TCP/UDP/ICMP and use this info
>> finding the non error case mentioned in  Table 30.17.
>>
>> For eg:-
>> IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and "0x0000"
>> TCP/UDP/ICMP CSUM value
>>
>> IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and "0x0000"
>> TCP/UDP/ICMP CSUM value
>>
>> Do you agree?

> What I meant here is some thing like below, please let me know if you have any issues with
> this, otherwise I would like to send the patch with below changes.
> 
> Further improvements can happen later.
> 
> Please let me know.
> 
> +/* Hardware checksum status */
> +#define IPV4_RX_CSUM_OK                0x00000000
> +#define IPV6_RX_CSUM_OK                0xFFFF0000

   Mhm, this should prolly come from the IP headers...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bbb42e5328e4..d9201fbbd472 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  
>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>  {
> -       u16 *hw_csum;
> +       u32 csum_result;

   This is not against the patch currently under investigation. :-)

> +       u8 *hw_csum;
>  
>         /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>          * appended to packet data
>          */
> -       if (unlikely(skb->len < sizeof(__sum16)))
> +       if (unlikely(skb->len < sizeof(__wsum)))

   I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here
covered by that, not a 32-bit sum...

>                 return;
> -       hw_csum = (u16*)(skb_tail_pointer(skb) - sizeof(__sum16));
> +       hw_csum = skb_tail_pointer(skb) - sizeof(__wsum);
> +       csum_result = get_unaligned_le32(hw_csum);
>  
> -       if (*hw_csum == 0)
> +       if (csum_result == IPV4_RX_CSUM_OK || csum_result == IPV6_RX_CSUM_OK)

   I don't think there's a hard-and-fast way to differentiate the valid packet just from
the 2 16-bit checksums...

[...]
>>>>>>>> +
>>>>>>>> +	if (*hw_csum == 0)
>>>>>>>
>>>>>>>    You only check the 1st byte, not the full checksum!
>>>>>>
>>>>>> As I said earlier, "0" value on last 16 bit, means no checksum
>> error.
>>>>>
>>>>>    How's that? 'hw_csum' is declared as 'u8 *'!
>>>>
>>>> It is my mistake, which will be taken care in the next patch by
>>>> using
>>> u16 *.

   That won't do it, I'm afraid...

   From an IRC discuassion on IRC we concluded that we don't need to check the checksum's
value, we just need to store it for the upper layers to catch the invalid sums...

[...]

MBR, Sergey