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: <91496a94-1648-b69d-e014-65868aca3a78@intel.com>
Date: Wed, 3 Jul 2024 07:20:41 -0700
From: "Greenwalt, Paul" <paul.greenwalt@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>, 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>, Alexander Lobakin <aleksander.lobakin@...el.com>,
	"Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum
 offload with EH



On 7/2/2024 3:31 AM, Przemek Kitszel wrote:
> 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)
> 

We will be adding support for GCO "Checksum 16 with pseudo Headers" to
the ice driver. It will be off by default.

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