RE: [PATCH] crypto/ipsec_mb: Do not dequeue ops from ring after job flush.

["Cornu, Marcel D" <marcel.d.cornu@intel.com>]

> -----Original Message-----
> From: Karas, Krzysztof <krzysztof.karas@intel.com>
> Sent: Wednesday, September 27, 2023 1:41 PM
> To: Ji, Kai <kai.ji@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Cornu, Marcel D
> <marcel.d.cornu@intel.com>; Power, Ciara <ciara.power@intel.com>
> Cc: dev@dpdk.org; Karas, Krzysztof <krzysztof.karas@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] crypto/ipsec_mb: Do not dequeue ops from ring after job flush.
> 
> Previously it was possible to increment `processed_jobs` to a value greater than
> requested `nb_ops`, because after flushing at most `nb_ops` jobs the while loop
> continued, so `processed_jobs` could still be incremented and it was possible for
> this variable to be greater than `nb_ops`. If `ops` provided to the function were
> only `nb_ops` long, then the `aesni_mb_dequeue_burst()` would write to the
> memory outside of `ops` array.
> 
> Fixes: b50b8b5b38f8 ("crypto/ipsec_mb: use burst API in AESNI")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> 
> ---
>  drivers/crypto/ipsec_mb/pmd_aesni_mb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> index 9e298023d7..ff52bc85a4 100644
> --- a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> +++ b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> @@ -2056,7 +2056,7 @@ aesni_mb_dequeue_burst(void *queue_pair, struct
> rte_crypto_op **ops,
>  		uint16_t n = (nb_ops / burst_sz) ?
>  			burst_sz : nb_ops;
> 
> -		while (unlikely((IMB_GET_NEXT_BURST(mb_mgr, n, jobs)) < n)) {
> +		if (unlikely((IMB_GET_NEXT_BURST(mb_mgr, n, jobs)) < n)) {
>  			/*
>  			 * Not enough free jobs in the queue
>  			 * Flush n jobs until enough jobs available @@ -2074,6
> +2074,8 @@ aesni_mb_dequeue_burst(void *queue_pair, struct rte_crypto_op
> **ops,
>  					break;
>  				}
>  			}
> +			nb_ops -= nb_jobs;
This assumes the loop above completes without errors.
If post_process_mb_job() returns with an error it will break out of the loop and nb_ops will be decremented by the wrong value.
Maybe decrementing by 'i' would work better here.
> +			continue;
>  		}
> 
>  		/*
> --
> 2.34.1

Hi Krzysztof, I noticed that when I run the dpdk-test-crypto-perf application testing with imix, the number of failed enqueue ops increases vs without the patch.

Example command: dpdk-test-crypto-perf -l 4,5  --no-huge  --vdev="crypto_aesni_mb" -- --pool-sz 8192 --cipher-algo aes-cbc --cipher-key-sz 16 --optype cipher-only --cipher-iv-sz 16 --cipher-op encrypt --silent --buffer-sz 16,6144 --imix 99,1 --burst-sz 32

Could you try it and see if you get the same result?

Regards,
Marcel