Re: [PATCH v2 2/2] crypto/ipsec_mb: fix QP release in secondary

[Moses Young <mosesyyoung@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4104 bytes --]

On 5/7/2025 11:25 PM, Ji, Kai wrote:

> Hi Yangming,
>
> PID check is implemented here:
> https://github.com/DPDK/dpdk/blob/75f179ebe347b6098cf3af26d3d3b7168fe3fe24/drivers/crypto/ipsec_mb/ipsec_mb_ops.c#L376 
> <https://github.com/DPDK/dpdk/blob/75f179ebe347b6098cf3af26d3d3b7168fe3fe24/drivers/crypto/ipsec_mb/ipsec_mb_ops.c#L376>
>
> Can you share the steps to re-produce the error ?
>
> Regards
>
> Kai
>
> ------------------------------------------------------------------------
> *From:* Yang Ming
> *Sent:* Thursday, April 24, 2025 15:26
> *To:* dev@dpdk.org
> *Subject:* Re: [PATCH v2 2/2] crypto/ipsec_mb: fix QP release in 
> secondary
>
> Hi,
>
> On 2025/4/7 13:25, Yang Ming wrote:
> > From: myang <ming.1.yang@nokia-sbell.com>
> >
> > When a secondary process tries to release a queue pair (QP) that
> > does not belong to it, error logs occur:
> > CRYPTODEV: ipsec_mb_ipc_request() line 373: Unable to release
> > qp_id=0
> > EAL: Message data is too long
> > EAL: Fail to handle message: ipsec_mb_mp_msg
> > EAL: Fail to recv reply for request /tmp/dpdk/l2hi/mp_socket:
> > ipsec_mb_mp_msg
> >
> > This patch ensures that a secondary process only frees a QP if
> > it actually owns it, preventing conflicts and resolving the
> > issue.
> >
> > Signed-off-by: myang <ming.1.yang@nokia-sbell.com>
> > ---
> >   drivers/crypto/ipsec_mb/ipsec_mb_ops.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c 
> b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> > index 910efb1a97..50ee140ccd 100644
> > --- a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> > +++ b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
> > @@ -138,6 +138,7 @@ int
> >   ipsec_mb_qp_release(struct rte_cryptodev *dev, uint16_t qp_id)
> >   {
> >        struct ipsec_mb_qp *qp = dev->data->queue_pairs[qp_id];
> > +     uint16_t process_id = (uint16_t)getpid();
> >
> >        if (!qp)
> >                return 0;
> > @@ -152,8 +153,10 @@ ipsec_mb_qp_release(struct rte_cryptodev *dev, 
> uint16_t qp_id)
> >                rte_free(qp);
> >                dev->data->queue_pairs[qp_id] = NULL;
> >        } else { /* secondary process */
> > -             return ipsec_mb_secondary_qp_op(dev->data->dev_id, qp_id,
> > -                                     NULL, 0, 
> RTE_IPSEC_MB_MP_REQ_QP_FREE);
> > +             if (qp->qp_used_by_pid == process_id)
> > +                     return ipsec_mb_secondary_qp_op(dev->data->dev_id,
> > +                                             qp_id, NULL, 0,
> > + RTE_IPSEC_MB_MP_REQ_QP_FREE);
> >        }
> >        return 0;
> >   }
>
> Hi Experts,
>
> Is there any chance to review and accept this patch?
>
> Brs,
>
> Yang Ming
>
Hi,

David. Thanks for your feedback. I will Cc maintainers in next version.

Kai, Thanks for your feedback. Here's our test steps after applying the 
previous patch called "eal: prevent socket closure before MP sync" in 
this serious:
1. Start the primary process: Run the DPDK primary process with the 
IPsec MB crypto device.
2. Launch the secondary process: Start a DPDK secondary process using 
the same device parameters.
3. Exit the secondary normally.

On secondary exit, error logs show below as my commit log's description:
CRYPTODEV: ipsec_mb_ipc_request() line 373: Unable to release qp_id=0
EAL: Message data is too long
EAL: Fail to handle message: ipsec_mb_mp_msg
EAL: Fail to recv reply for request /tmp/dpdk/l2hi/mp_socket: 
ipsec_mb_mp_msg

This message corresponds exactly to the PID check in the code you 
referenced:
if (qp->qp_used_by_pid != req_param->process_id) {
     CDEV_LOG_ERR("Unable to release qp_id=%d", qp_id);
     goto out;
}

In our view, this log indicates an abnormal condition: a secondary 
process should be able to release its own QPs without triggering an 
error during normal shutdown.

Thanks for your help.

Best,
Yang Ming

[-- Attachment #2: Type: text/html, Size: 7996 bytes --]