netdev - Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum offload with EH

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: <ab3e6312-cf67-47bb-b30f-d425f7914053@intel.com>
Date: Tue, 2 Jul 2024 12:31:01 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Tom Herbert <tom@...bertland.com>, <anthony.l.nguyen@...el.com>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <jesse.brandeburg@...el.com>,
	<cai.huoqing@...ux.dev>, <netdev@...r.kernel.org>, <felipe@...anda.io>,
	<justin.iurman@...ege.be>, Paul Greenwalt <paul.greenwalt@...el.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>
Subject: Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum
 offload with EH

On 7/1/24 21:55, Tom Herbert wrote:
> Several NICs would seem to support protocol specific TX checksum offload
> and allow for cases where an IPv6 packet contains extension headers.
> When deciding whether to offload a packet, ipv6_skip_exthdr is called
> to skip extension headers. The problem is that if a packet contains an
> IPv6 Routing Header then protocol specific checksum offload can't work,
> the destination IP address in the IPv6 header is not the same one that
> is used in the pseudo header for TCP or UDP. The correct address is
> derived from the last segment in the routing list (which itself might
> be obfuscated so that a device could even read it).

feels like there is a missing "not" after "could" - with it added, reads
fine (not a request to change, just being verbose about assumptions)

> 
> This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
> called in lieu of ipv6_skip_exthdr. If a routing header is present in
> a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
> zero, this is an indication to the driver that TX checksum offload
> is not viable and it should call skb_checksum_help instead of
> offloading the checksum.
> 
> The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
> to call ipv6_skip_exthdr_no_rthdr.
> 
> Testing: The code compiles, but is otherwise untested due to lack of
> NIC hardware. It would be appreciated if someone with access to the
> hardware could test.

we could test intel ones (except fm10k) via @Tony's tree

> 
> v2: Fixed uninitialized variable in exthdrs_core.c
> 
> Tom Herbert (7):
>    ipv6: Add ipv6_skip_exthdr_no_rthdr
>    i40e: Don't do TX csum offload with routing header present
>    iavf: Don't do TX csum offload with routing header present
>    ice: Don't do TX csum offload with routing header present

sidenote:
our HW is supporting (among others) a GCO check-summing mode described
as: "Checksum 16bit (TCP/UDP) with no pseudo Header", but we have not
yet provided patches for that, and I don't even know if this mode
will be used (CC @Paul)

>    idpf: Don't do TX csum offload with routing header present
>    hinic: Don't do TX csum offload with routing header present
>    fm10k: Don't do TX csum offload with routing header present
> 
>   drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
>   drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
>   drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28 +++++++++----------
>   include/net/ipv6.h                            | 17 +++++++++--
>   net/ipv6/exthdrs_core.c                       | 25 ++++++++++++-----
>   8 files changed, 98 insertions(+), 68 deletions(-)
> 

I have reviewed the patches and they conform to commit message/intent,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
(for the series)