netdev - RE: [PATCH net-next v2 1/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:
 <TYCPR01MB112695FD89B16249FD803D1E7867D2@TYCPR01MB11269.jpnprd01.prod.outlook.com>
Date: Tue, 30 Jan 2024 17:00:09 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Sergey Shtylyov <s.shtylyov@....ru>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
CC: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Yoshihiro Shimoda
	<yoshihiro.shimoda.uh@...esas.com>, Wolfram Sang
	<wsa+renesas@...g-engineering.com>, nikita.yoush
	<nikita.yoush@...entembedded.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>, Prabhakar Mahadev Lad
	<prabhakar.mahadev-lad.rj@...renesas.com>, biju.das.au
	<biju.das.au@...il.com>
Subject: RE: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support

Hi Sergey Shtylyov,

Thanks for the feedback.

> >>    Don't we need to set skb->csum_level?
> >
> > As per my knowledge, it is not needed. I may be wrong. Why do you
> > think it is needed?
> 
>  *   CHECKSUM_UNNECESSARY is applicable to following protocols:
>  *     TCP: IPv6 and IPv4.
>  *     UDP: IPv4 and IPv6. A device may apply CHECKSUM_UNNECESSARY to a
>  *       zero UDP checksum for either IPv4 or IPv6, the networking stack
>  *       may perform further validation in this case.
>  *     GRE: only if the checksum is present in the header.
>  *     SCTP: indicates the CRC in SCTP header has been validated.
>  *     FCOE: indicates the CRC in FC frame has been validated.
>  *
>  *   skb->csum_level indicates the number of consecutive checksums found
> in
>  *   the packet minus one that have been verified as CHECKSUM_UNNECESSARY.
>  *   For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
>  *   and a device is able to verify the checksums for UDP (possibly zero),
>  *   GRE (checksum flag is set) and TCP, skb->csum_level would be set to
>  *   two. If the device were only able to verify the UDP checksum and not
>  *   GRE, either because it doesn't support GRE checksum or because GRE
>  *   checksum is bad, skb->csum_level would be set to zero (TCP checksum
> is
>  *   not considered in this case).
> 
>    It would seem we should set this field to 1 if the TCP/UDP checksum was
> successfully verified?

I guess it is for encapsulated packets. For just IP and UDP/TCP
Checksum it is not required.

See

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/3com/3c59x.c#L2669
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/amazon/ena/ena_netdev.c#L1626
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/alx/main.c#L272
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/am65-cpsw-nuss.c#L711

> 
> >> [...]
> >>> @@ -2518,6 +2593,8 @@ static const struct ravb_hw_info gbeth_hw_info =
> {
> >>>  	.emac_init = ravb_emac_init_gbeth,
> >>>  	.gstrings_stats = ravb_gstrings_stats_gbeth,
> >>>  	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
> >>> +	.net_hw_features = NETIF_F_RXCSUM,
> >>> +	.net_features = NETIF_F_RXCSUM,
> >>
> >>    What about NETIF_F_IP_CSUM, BTW?
> >
> > Why is it needed? Can you please clarify?
> 
>    Ignore me -- this one seems to be used for the TX path.
>    I just had to learn how the checksum offloading works while reviewing
> your patches... :-)

OK.

For other comments. I will test and respond.

Cheers,
Biju