netdev - Re: [PATCH net] ibmveth: Remove condition to recompute TCP header checksum.

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: <ff64d1f2-cef5-20a9-c263-3e0e562d411a@intel.com>
Date: Fri, 29 Sep 2023 10:29:22 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Nick Child <nnac123@...ux.ibm.com>, <netdev@...r.kernel.org>
CC: <dwilder@...ibm.com>, <wilder@...ibm.com>, <pradeeps@...ux.vnet.ibm.com>,
	<nick.child@....com>
Subject: Re: [PATCH net] ibmveth: Remove condition to recompute TCP header
 checksum.



On 9/26/2023 2:42 PM, Nick Child wrote:
> From: David Wilder <dwilder@...ibm.com>
> 
> In some OVS environments the TCP pseudo header checksum may need to be
> recomputed. Currently this is only done when the interface instance is
> configured for "Trunk Mode". We found the issue also occurs in some
> Kubernetes environments, these environments do not use "Trunk Mode",
> therefor the condition is removed.
> 
> Performance tests with this change show only a fractional decrease in
> throughput (< 0.2%).
> 
> Fixes: 7525de2516fb ("ibmveth: Set CHECKSUM_PARTIAL if NULL TCP CSUM.")
> Signed-off-by: David Wilder <dwilder@...ibm.com>
> Reviewed-by: Nick Child <nnac123@...ux.ibm.com>
> ---
> Hello, I (Nick Child) am submitting on behalf of the
> author (David Wilder) since he is having patch submission issues.
> Apologies for any inconvenience.
> 

I think you're supposed to add your own Signed-off-by when sending
patches on behalf of another author, but its clear who the author is
with the From line, and you called it out here too. Thanks! Just a note
for future submissions on behalf of others. From
Documentation/process/submitting-patches.rst:


> Any further SoBs (Signed-off-by:'s) following the author's SoB are from
> people handling and transporting the patch, but were not involved in its
> development. SoB chains should reflect the **real** route a patch took
> as it was propagated to the maintainers and ultimately to Linus, with
> the first SoB entry signalling primary authorship of a single author.
>> When to use Acked-by:, Cc:, and Co-developed-by:
> ------------------------------------------------
> 
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path.
> 

I don't think this should require a resend, but its good to note for the
future :)

Patch looks good. I assume there isn't a different "fast" check that can
be done to determine if we need the recomputation, and it doesn't hit
the performance too badly. The simplification and ensuring that no one
else hits this error in the future is worth the slight cost I think.
Correctness comes before speed.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

> 
>  drivers/net/ethernet/ibm/ibmveth.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 113fcb3e353e..748ee25cee8d 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1303,24 +1303,23 @@ static void ibmveth_rx_csum_helper(struct sk_buff *skb,
>  	 * the user space for finding a flow. During this process, OVS computes
>  	 * checksum on the first packet when CHECKSUM_PARTIAL flag is set.
>  	 *
> -	 * So, re-compute TCP pseudo header checksum when configured for
> -	 * trunk mode.
> +	 * So, re-compute TCP pseudo header checksum.
>  	 */
> +
>  	if (iph_proto == IPPROTO_TCP) {
>  		struct tcphdr *tcph = (struct tcphdr *)(skb->data + iphlen);
> +
>  		if (tcph->check == 0x0000) {
>  			/* Recompute TCP pseudo header checksum  */
> -			if (adapter->is_active_trunk) {
> -				tcphdrlen = skb->len - iphlen;
> -				if (skb_proto == ETH_P_IP)
> -					tcph->check =
> -					 ~csum_tcpudp_magic(iph->saddr,
> -					iph->daddr, tcphdrlen, iph_proto, 0);
> -				else if (skb_proto == ETH_P_IPV6)
> -					tcph->check =
> -					 ~csum_ipv6_magic(&iph6->saddr,
> -					&iph6->daddr, tcphdrlen, iph_proto, 0);
> -			}
> +			tcphdrlen = skb->len - iphlen;
> +			if (skb_proto == ETH_P_IP)
> +				tcph->check =
> +				 ~csum_tcpudp_magic(iph->saddr,
> +				iph->daddr, tcphdrlen, iph_proto, 0);
> +			else if (skb_proto == ETH_P_IPV6)
> +				tcph->check =
> +				 ~csum_ipv6_magic(&iph6->saddr,
> +				&iph6->daddr, tcphdrlen, iph_proto, 0);
>  			/* Setup SKB fields for checksum offload */
>  			skb_partial_csum_set(skb, iphlen,
>  					     offsetof(struct tcphdr, check));