RE: [PATCH v2 2/5] crypto/ipsec_mb: fix sessionless cleanup

["Power, Ciara" <ciara.power@intel.com>]

Hi Pablo,

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Thursday 15 September 2022 13:39
> To: Power, Ciara <ciara.power@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>
> Cc: dev@dpdk.org; Ji, Kai <kai.ji@intel.com>; Mrozowicz, SlawomirX
> <slawomirx.mrozowicz@intel.com>
> Subject: RE: [PATCH v2 2/5] crypto/ipsec_mb: fix sessionless cleanup
> 
> Hi Ciara,
> 
> > -----Original Message-----
> > From: Power, Ciara <ciara.power@intel.com>
> > Sent: Thursday, August 25, 2022 3:29 PM
> > To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Cc: dev@dpdk.org; Ji, Kai <kai.ji@intel.com>; Power, Ciara
> > <ciara.power@intel.com>; Mrozowicz, SlawomirX
> > <slawomirx.mrozowicz@intel.com>
> > Subject: [PATCH v2 2/5] crypto/ipsec_mb: fix sessionless cleanup
> >
> > Currently, for a sessionless op, the session created is reset before
> > being put back into the mempool. This causes issues as the object
> > isn't correctly released into the mempool.
> >
> > Fixes: c68d7aa354f6 ("crypto/aesni_mb: use architecture independent
> > macros")
> > Fixes: b3bbd9e5f265 ("cryptodev: support device independent sessions")
> > Fixes: f16662885472 ("crypto/ipsec_mb: add chacha_poly PMD")
> > Cc: roy.fan.zhang@intel.com
> > Cc: slawomirx.mrozowicz@intel.com
> > Cc: kai.ji@intel.com
> >
> > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > ---
> >  drivers/crypto/ipsec_mb/pmd_aesni_mb.c    | 4 ----
> >  drivers/crypto/ipsec_mb/pmd_chacha_poly.c | 4 ----
> >  drivers/crypto/ipsec_mb/pmd_kasumi.c      | 5 -----
> >  drivers/crypto/ipsec_mb/pmd_snow3g.c      | 4 ----
> >  drivers/crypto/ipsec_mb/pmd_zuc.c         | 4 ----
> >  5 files changed, 21 deletions(-)
> >
> > diff --git a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> > b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> > index 6d5d3ce8eb..944fce0261 100644
> > --- a/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> > +++ b/drivers/crypto/ipsec_mb/pmd_aesni_mb.c
> > @@ -1770,10 +1770,6 @@ post_process_mb_job(struct ipsec_mb_qp *qp,
> > IMB_JOB *job)
> >
> >  	/* Free session if a session-less crypto op */
> >  	if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
> > -		memset(sess, 0, sizeof(struct aesni_mb_session));
> > -		memset(op->sym->session, 0,
> > -
> 
> This will leave some info leftover, so it may cause a problem if this object is
> reused? Is this memset clearing mempool object header and that's the reason
> why it cannot be released properly?
> Maybe Fan/Kai/Slawomir will know more on this.
[CP] 
Yes, I believe this would leave data leftover, my initial solution was incorrect.

I have sent a v3 fix which takes a different approach, after debugging the issue a little more.
I found the sessionless tests were reusing data in old session objects from previous session testcases,
which had not been reset before being put back into the mempool.
Once that reset was added, the sessionless tests failed due to session->nb_drivers being 0 - this was due
to the value never being set for sessionless operations. Instead of pulling from the mempool directly,
I added a call to sym_session_create(), which pulls from the mempool, and also sets values such as nb_drivers.

These changes can be seen here: https://patchwork.dpdk.org/project/dpdk/patch/20220921125036.9104-3-ciara.power@intel.com/

Thanks for the review.