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

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

[Krzysztof Karas]

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;
+			continue;
 		}
 
 		/*
-- 
2.34.1

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

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

[Akhil Goyal]

> Subject: [EXT] [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>
> 
Pablo/Kai,
Can you please review?

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

[Cornu, Marcel D]

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