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]
Message-ID: <OS0PR01MB5922BCF31F520F8F975606B286B19@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date:   Thu, 7 Oct 2021 20:09:27 +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:     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

Hi Sergey,


Thanks for the feedback.

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/7/21 8:49 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);
> >>>>
> >>>>    Not 32-bit? The manual says the IP checksum is stored in the
> >>>> first
> >>>> 2 bytes.
> >>>
> >>> It is 16 bit. It is on last 2 bytes.
> 
>    The IP checksum is at the 1st 2 bytes of the overall 4-byte checksum
> (coming after the packet payload), no?

Sorry, I got confused with your question earlier. Now it is clear for me.

I agree the checksum part is stored in last 4bytes. Of this, the first 2 bytes IPV4 checksum
and last 2 bytes TCP/UDP/ICMP checksum.

> 
> >>    So you're saying the manual is wrong?
> >
> > I am not sure which manual you are referring here.
> >
> > I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
> 
>    Same here.
> 
> [...]
> 
> > 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?

Regards,
Biju
> 
> >>>>> +
> >>>>> +	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 *.
> 
>    Note that this 'u16' halfword can be unaligned, that's why the current
> code uses get_unaligned_le16().
> 
> >>>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>>>> +	else
> >>>>> +		skb->ip_summed = CHECKSUM_NONE;
> >>>>
> >>>>   So the TCP/UDP/ICMP checksums are not dealt with? Why enable them
> >> then?
> >>>
> >>> If last 2bytes is zero, means there is no checksum error w.r.to
> >> TCP/UDP/ICMP checksums.
> >>
> >>    Why checksum them independently then?
> >
> > It is a hardware feature.
> 
>    Switchable, isn't it?

> 
> > Regards,
> > Biju
> 
> [...]
> 
> MBR, Sergey