diff mbox series

net: do fragmented headers check in non-debug build as well

Message ID 1590589976-2915-1-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net: do fragmented headers check in non-debug build as well |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing warning Testing issues
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

Andrew Rybchenko May 27, 2020, 2:32 p.m. UTC 
  Pseudo-header checksum calculation requires contiguous headers.
There is no any formal requirements on data location and mbuf
structure which could be used by the application.

Make corresponding check to be done in non-debug build as well
to avoid bad accesses, incorrect checksum caclculation and to
return appropriate error from Tx prepare.

Make no-offloads check more precise and do it in non-debug build
as well to avoid contiguous headers check and Tx prepare failure
if it is not actually required.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_net/rte_net.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Olivier Matz July 13, 2020, 1:35 p.m. UTC | #1 
Hi Andrew,

On Wed, May 27, 2020 at 03:32:56PM +0100, Andrew Rybchenko wrote:
> Pseudo-header checksum calculation requires contiguous headers.
> There is no any formal requirements on data location and mbuf
> structure which could be used by the application.
> 
> Make corresponding check to be done in non-debug build as well
> to avoid bad accesses, incorrect checksum caclculation and to

typo: caclculation -> calculation

> return appropriate error from Tx prepare.
> 
> Make no-offloads check more precise and do it in non-debug build
> as well to avoid contiguous headers check and Tx prepare failure
> if it is not actually required.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_net/rte_net.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index 1560ecfa46..1edc283a47 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -120,20 +120,17 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	struct rte_udp_hdr *udp_hdr;
>  	uint64_t inner_l3_offset = m->l2_len;
>  
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  	/*
>  	 * Does packet set any of available offloads?
>  	 * Mainly it is required to avoid fragmented headers check if
>  	 * no offloads are requested.
>  	 */
> -	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
>  		return 0;

Agree, the packet is modified only if one of these flag is set.

> -#endif
>  
>  	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>  
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  	/*
>  	 * Check if headers are fragmented.
>  	 * The check could be less strict depending on which offloads are
> @@ -142,7 +139,6 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	if (unlikely(rte_pktmbuf_data_len(m) <
>  		     inner_l3_offset + m->l3_len + m->l4_len))
>  		return -ENOTSUP;
> -#endif

Yes, despite the documentation of thus function says that used headers
should be in the first data segment of the mbuf, when it is used through
the ethdev tx_prepare() API there is no such requirement.

So yes, it looks safer to do these checks even if debug is off. They
will only be done when doing tx offload, so I guess it is ok in terms of
performance.

Maybe it is worth mentioning commit dfc6b2fd8da3 ("mbuf: remove Intel
offload checks from generic API") in the commit log?

>  
>  	if (ol_flags & PKT_TX_IPV4) {
>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
> -- 
> 2.17.1
> 

Acked-by: Olivier Matz <olivier.matz@6wind.com>
Andrew Rybchenko July 13, 2020, 2:22 p.m. UTC | #2 
Hi Olivier,

On 7/13/20 4:35 PM, Olivier Matz wrote:
> Hi Andrew,
>
> On Wed, May 27, 2020 at 03:32:56PM +0100, Andrew Rybchenko wrote:
>> Pseudo-header checksum calculation requires contiguous headers.
>> There is no any formal requirements on data location and mbuf
>> structure which could be used by the application.
>>
>> Make corresponding check to be done in non-debug build as well
>> to avoid bad accesses, incorrect checksum caclculation and to
> typo: caclculation -> calculation

Will fix in v2.

>> return appropriate error from Tx prepare.
>>
>> Make no-offloads check more precise and do it in non-debug build
>> as well to avoid contiguous headers check and Tx prepare failure
>> if it is not actually required.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  lib/librte_net/rte_net.h | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
>> index 1560ecfa46..1edc283a47 100644
>> --- a/lib/librte_net/rte_net.h
>> +++ b/lib/librte_net/rte_net.h
>> @@ -120,20 +120,17 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>>  	struct rte_udp_hdr *udp_hdr;
>>  	uint64_t inner_l3_offset = m->l2_len;
>>  
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	/*
>>  	 * Does packet set any of available offloads?
>>  	 * Mainly it is required to avoid fragmented headers check if
>>  	 * no offloads are requested.
>>  	 */
>> -	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
>>  		return 0;
> Agree, the packet is modified only if one of these flag is set.
>
>> -#endif
>>  
>>  	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
>>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>>  
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	/*
>>  	 * Check if headers are fragmented.
>>  	 * The check could be less strict depending on which offloads are
>> @@ -142,7 +139,6 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>>  	if (unlikely(rte_pktmbuf_data_len(m) <
>>  		     inner_l3_offset + m->l3_len + m->l4_len))
>>  		return -ENOTSUP;
>> -#endif
> Yes, despite the documentation of thus function says that used headers
> should be in the first data segment of the mbuf, when it is used through
> the ethdev tx_prepare() API there is no such requirement.
>
> So yes, it looks safer to do these checks even if debug is off. They
> will only be done when doing tx offload, so I guess it is ok in terms of
> performance.
>
> Maybe it is worth mentioning commit dfc6b2fd8da3 ("mbuf: remove Intel
> offload checks from generic API") in the commit log?

Yes, I think it could be useful will add a bit of history in v2.

>>  
>>  	if (ol_flags & PKT_TX_IPV4) {
>>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
>> -- 
>> 2.17.1
>>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks for the review,
Andrew.
diff mbox series

Patch

diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index 1560ecfa46..1edc283a47 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -120,20 +120,17 @@  rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	struct rte_udp_hdr *udp_hdr;
 	uint64_t inner_l3_offset = m->l2_len;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	/*
 	 * Does packet set any of available offloads?
 	 * Mainly it is required to avoid fragmented headers check if
 	 * no offloads are requested.
 	 */
-	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
+	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
 		return 0;
-#endif
 
 	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
 		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	/*
 	 * Check if headers are fragmented.
 	 * The check could be less strict depending on which offloads are
@@ -142,7 +139,6 @@  rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	if (unlikely(rte_pktmbuf_data_len(m) <
 		     inner_l3_offset + m->l3_len + m->l4_len))
 		return -ENOTSUP;
-#endif
 
 	if (ol_flags & PKT_TX_IPV4) {
 		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,