netdev - Re: [PATCH RFC net-next 01/34] idpf: reuse libie's definitions of parsed ptype structures

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: <9e8e8b47-24ab-4947-8f22-f2a07c2549ca@intel.com>
Date: Mon, 8 Jan 2024 17:04:16 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
	Michal Kubiak <michal.kubiak@...el.com>, Larysa Zaremba
	<larysa.zaremba@...el.com>, Alexei Starovoitov <ast@...nel.org>, "Daniel
 Borkmann" <daniel@...earbox.net>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC net-next 01/34] idpf: reuse libie's definitions of
 parsed ptype structures

From: Willem De Bruijn <willemdebruijn.kernel@...il.com>
Date: Wed, 27 Dec 2023 10:43:34 -0500

> Alexander Lobakin wrote:
>> idpf's in-kernel parsed ptype structure is almost identical to the one
>> used in the previous Intel drivers, which means it can be converted to
>> use libie's definitions and even helpers. The only difference is that
>> it doesn't use a constant table, rather than one obtained from the
>> device.

[...]

>>  static void idpf_rx_singleq_csum(struct idpf_queue *rxq, struct sk_buff *skb,
>> -				 struct idpf_rx_csum_decoded *csum_bits,
>> -				 u16 ptype)
>> +				 struct idpf_rx_csum_decoded csum_bits,
>> +				 struct libie_rx_ptype_parsed parsed)
>>  {
>> -	struct idpf_rx_ptype_decoded decoded;
>>  	bool ipv4, ipv6;
>>  
>>  	/* check if Rx checksum is enabled */
>> -	if (unlikely(!(rxq->vport->netdev->features & NETIF_F_RXCSUM)))
>> +	if (!libie_has_rx_checksum(rxq->vport->netdev, parsed))
>>  		return;
>>  
>>  	/* check if HW has decoded the packet and checksum */
>> -	if (unlikely(!(csum_bits->l3l4p)))
>> +	if (unlikely(!csum_bits.l3l4p))
>>  		return;
>>  
>> -	decoded = rxq->vport->rx_ptype_lkup[ptype];
>> -	if (unlikely(!(decoded.known && decoded.outer_ip)))
>> +	if (unlikely(parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_L2))
>>  		return;
>>  
>> -	ipv4 = IDPF_RX_PTYPE_TO_IPV(&decoded, IDPF_RX_PTYPE_OUTER_IPV4);
>> -	ipv6 = IDPF_RX_PTYPE_TO_IPV(&decoded, IDPF_RX_PTYPE_OUTER_IPV6);
>> +	ipv4 = parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_IPV4;
>> +	ipv6 = parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_IPV6;
>>  
>>  	/* Check if there were any checksum errors */
>> -	if (unlikely(ipv4 && (csum_bits->ipe || csum_bits->eipe)))
>> +	if (unlikely(ipv4 && (csum_bits.ipe || csum_bits.eipe)))
>>  		goto checksum_fail;
>>  
>>  	/* Device could not do any checksum offload for certain extension
>>  	 * headers as indicated by setting IPV6EXADD bit
>>  	 */
>> -	if (unlikely(ipv6 && csum_bits->ipv6exadd))
>> +	if (unlikely(ipv6 && csum_bits.ipv6exadd))
>>  		return;
>>  
>>  	/* check for L4 errors and handle packets that were not able to be
>>  	 * checksummed due to arrival speed
>>  	 */
>> -	if (unlikely(csum_bits->l4e))
>> +	if (unlikely(csum_bits.l4e))
>>  		goto checksum_fail;
>>  
>> -	if (unlikely(csum_bits->nat && csum_bits->eudpe))
>> +	if (unlikely(csum_bits.nat && csum_bits.eudpe))
>>  		goto checksum_fail;
>>  
>>  	/* Handle packets that were not able to be checksummed due to arrival
>>  	 * speed, in this case the stack can compute the csum.
>>  	 */
>> -	if (unlikely(csum_bits->pprs))
>> +	if (unlikely(csum_bits.pprs))
>>  		return;
>>  
>>  	/* If there is an outer header present that might contain a checksum
>>  	 * we need to bump the checksum level by 1 to reflect the fact that
>>  	 * we are indicating we validated the inner checksum.
>>  	 */
>> -	if (decoded.tunnel_type >= IDPF_RX_PTYPE_TUNNEL_IP_GRENAT)
>> +	if (parsed.tunnel_type >= LIBIE_RX_PTYPE_TUNNEL_IP_GRENAT)
>>  		skb->csum_level = 1;
>>  
>> -	/* Only report checksum unnecessary for ICMP, TCP, UDP, or SCTP */
>> -	switch (decoded.inner_prot) {
>> -	case IDPF_RX_PTYPE_INNER_PROT_ICMP:
>> -	case IDPF_RX_PTYPE_INNER_PROT_TCP:
>> -	case IDPF_RX_PTYPE_INNER_PROT_UDP:
>> -	case IDPF_RX_PTYPE_INNER_PROT_SCTP:
>> -		skb->ip_summed = CHECKSUM_UNNECESSARY;
>> -		return;
>> -	default:
>> -		return;
>> -	}
>> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +	return;
> 
> Is it intentional to change from CHECKSUM_NONE to CHECKSUM_UNNECESSARY
> in the default case?

The basic logic wasn't changed. libie_has_rx_checksum() checks if the
protocol can be checksummed by HW at the beginning of the function
instead of the end (why calculate and check all this if the proto is not
supported?).

> 
> I suppose so, as idpf_rx_csum (the splitq equivalent) does the same
> (bar CHECKSUM_COMPLETE depending on descriptor bit).

Thanks,
Olek