[dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well

[dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well

[Andrew Rybchenko]

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

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 *,
-- 
2.17.1

Re: [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well

[Olivier Matz]

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>

Re: [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well

[Andrew Rybchenko]

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.

[dpdk-dev] [PATCH v2] net: do fragmented headers check in non-debug build as well

[Andrew Rybchenko]

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.

Since

commit dfc6b2fd8da3 ("mbuf: remove Intel offload checks from generic API")

fragmented headers checks are done inside
rte_net_intel_cksum_flags_prepare() in RTE_LIBRTE_ETHDEV_DEBUG build
because it is moved from rte_validate_tx_offload() which is called
under debug only.

Make corresponding check to be done in non-debug build as well
to avoid bad accesses, incorrect checksum calculation 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>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
v2:
 - fix typo in description
 - mention dfc6b2fd8da3 in the description

 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;
-#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 *,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] net: do fragmented headers check in non-debug build as well

[Ferruh Yigit]

On 7/13/2020 3:22 PM, 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.
> 
> Since
> 
> commit dfc6b2fd8da3 ("mbuf: remove Intel offload checks from generic API")
> 
> fragmented headers checks are done inside
> rte_net_intel_cksum_flags_prepare() in RTE_LIBRTE_ETHDEV_DEBUG build
> because it is moved from rte_validate_tx_offload() which is called
> under debug only.
> 
> Make corresponding check to be done in non-debug build as well
> to avoid bad accesses, incorrect checksum calculation 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>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied to dpdk-next-net/master, thanks.