[dpdk-dev] [PATCH 0/2] fixes for mlx5

[dpdk-dev] [PATCH 0/2] fixes for mlx5

[Yunjian Wang]

This series include two fixes patches for mlx5.

Yunjian Wang (2):
  net/mlx5: fix use after free when releasing tx queues
  net/mlx5: fix resource leak when releasing a drop action

 drivers/net/mlx5/mlx5_devx.c | 6 ++++++
 drivers/net/mlx5/mlx5_rxq.c  | 6 ------
 drivers/net/mlx5/mlx5_txq.c  | 3 ++-
 3 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.27.0

[dpdk-dev] [PATCH 1/2] net/mlx5: fix use after free when releasing tx queues

[Yunjian Wang]

The bonding slave remove function was calling the eth_dev_tx_queue_config
function, which frees dev->data->tx_queues, and then tries to free
priv->txqs[idx] in mlx5_txq_release function, which causes the heap use
after free issue. Add checks whether dev->data->tx_queues is not NULL.

Fixes: 94e257ec8ca ("net/mlx5: fix Rx/Tx queue checks")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 drivers/net/mlx5/mlx5_txq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index 0140f8b3b2..cb2c33a060 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -1198,7 +1198,8 @@ mlx5_txq_release(struct rte_eth_dev *dev, uint16_t idx)
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct mlx5_txq_ctrl *txq_ctrl;
 
-	if (priv->txqs == NULL || (*priv->txqs)[idx] == NULL)
+	if (dev->data->tx_queues == NULL || priv->txqs == NULL ||
+		(*priv->txqs)[idx] == NULL)
 		return 0;
 	txq_ctrl = container_of((*priv->txqs)[idx], struct mlx5_txq_ctrl, txq);
 	if (__atomic_sub_fetch(&txq_ctrl->refcnt, 1, __ATOMIC_RELAXED) > 1)
-- 
2.27.0

[dpdk-dev] [PATCH 2/2] net/mlx5: fix resource leak when releasing a drop action

[Yunjian Wang]

Currently, the resources for hrxq->action are allocated in
mlx5_devx_hrxq_new(). But it was not being freed when the
drop action was released in mlx5_devx_drop_action_destroy().
So, fix is to free the resources in mlx5_devx_tir_destroy().

Fixes: bc5bee028ebc ("net/mlx5: create drop queue using DevX")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 drivers/net/mlx5/mlx5_devx.c | 6 ++++++
 drivers/net/mlx5/mlx5_rxq.c  | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_devx.c b/drivers/net/mlx5/mlx5_devx.c
index 6886ae1f22..38a4a7c802 100644
--- a/drivers/net/mlx5/mlx5_devx.c
+++ b/drivers/net/mlx5/mlx5_devx.c
@@ -907,6 +907,12 @@ mlx5_devx_hrxq_new(struct rte_eth_dev *dev, struct mlx5_hrxq *hrxq,
 static void
 mlx5_devx_tir_destroy(struct mlx5_hrxq *hrxq)
 {
+#if defined(HAVE_IBV_FLOW_DV_SUPPORT) || !defined(HAVE_INFINIBAND_VERBS_H)
+	if (hrxq->hws_flags)
+		mlx5dr_action_destroy(hrxq->action);
+	else
+		mlx5_glue->destroy_flow_action(hrxq->action);
+#endif
 	claim_zero(mlx5_devx_cmd_destroy(hrxq->tir));
 }
 
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index eaf23d0df4..e518fe9bfd 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2861,12 +2861,6 @@ __mlx5_hrxq_remove(struct rte_eth_dev *dev, struct mlx5_hrxq *hrxq)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 
-#ifdef HAVE_IBV_FLOW_DV_SUPPORT
-	if (hrxq->hws_flags)
-		mlx5dr_action_destroy(hrxq->action);
-	else
-		mlx5_glue->destroy_flow_action(hrxq->action);
-#endif
 	priv->obj_ops.hrxq_destroy(hrxq);
 	if (!hrxq->standalone) {
 		mlx5_ind_table_obj_release(dev, hrxq->ind_table,
-- 
2.27.0

[dpdk-dev] [PATCH v2 0/2] fixes for mlx5

[Yunjian Wang]

This series include two fixes patches for mlx5.

---
v2:
   * update patch 2/2 code styles

Yunjian Wang (2):
  net/mlx5: fix use after free when releasing tx queues
  net/mlx5: fix resource leak when releasing a drop action

 drivers/net/mlx5/mlx5_devx.c | 7 +++++++
 drivers/net/mlx5/mlx5_rxq.c  | 6 ------
 drivers/net/mlx5/mlx5_txq.c  | 3 ++-
 3 files changed, 9 insertions(+), 7 deletions(-)

-- 
2.27.0

[dpdk-dev] [PATCH v2 1/2] net/mlx5: fix use after free when releasing tx queues

[Yunjian Wang]

The bonding slave remove function was calling the eth_dev_tx_queue_config
function, which frees dev->data->tx_queues, and then tries to free
priv->txqs[idx] in mlx5_txq_release function, which causes the heap use
after free issue. Add checks whether dev->data->tx_queues is not NULL.

Fixes: 94e257ec8ca ("net/mlx5: fix Rx/Tx queue checks")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 drivers/net/mlx5/mlx5_txq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index 0140f8b3b2..cb2c33a060 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -1198,7 +1198,8 @@ mlx5_txq_release(struct rte_eth_dev *dev, uint16_t idx)
 	struct mlx5_priv *priv = dev->data->dev_private;
 	struct mlx5_txq_ctrl *txq_ctrl;
 
-	if (priv->txqs == NULL || (*priv->txqs)[idx] == NULL)
+	if (dev->data->tx_queues == NULL || priv->txqs == NULL ||
+		(*priv->txqs)[idx] == NULL)
 		return 0;
 	txq_ctrl = container_of((*priv->txqs)[idx], struct mlx5_txq_ctrl, txq);
 	if (__atomic_sub_fetch(&txq_ctrl->refcnt, 1, __ATOMIC_RELAXED) > 1)
-- 
2.27.0

[dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when releasing a drop action

[Yunjian Wang]

Currently, the resources for hrxq->action are allocated in
mlx5_devx_hrxq_new(). But it was not being freed when the
drop action was released in mlx5_devx_drop_action_destroy().
So, fix is to free the resources in mlx5_devx_tir_destroy().

Fixes: bc5bee028ebc ("net/mlx5: create drop queue using DevX")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 drivers/net/mlx5/mlx5_devx.c | 7 +++++++
 drivers/net/mlx5/mlx5_rxq.c  | 6 ------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_devx.c b/drivers/net/mlx5/mlx5_devx.c
index 6886ae1f22..09c8856f05 100644
--- a/drivers/net/mlx5/mlx5_devx.c
+++ b/drivers/net/mlx5/mlx5_devx.c
@@ -907,6 +907,13 @@ mlx5_devx_hrxq_new(struct rte_eth_dev *dev, struct mlx5_hrxq *hrxq,
 static void
 mlx5_devx_tir_destroy(struct mlx5_hrxq *hrxq)
 {
+#if defined(HAVE_IBV_FLOW_DV_SUPPORT) || !defined(HAVE_INFINIBAND_VERBS_H)
+	if (hrxq->hws_flags)
+		mlx5dr_action_destroy(hrxq->action);
+	else
+		mlx5_flow_os_destroy_flow_action(hrxq->action);
+	hrxq->action = NULL;
+#endif
 	claim_zero(mlx5_devx_cmd_destroy(hrxq->tir));
 }
 
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index eaf23d0df4..e518fe9bfd 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -2861,12 +2861,6 @@ __mlx5_hrxq_remove(struct rte_eth_dev *dev, struct mlx5_hrxq *hrxq)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 
-#ifdef HAVE_IBV_FLOW_DV_SUPPORT
-	if (hrxq->hws_flags)
-		mlx5dr_action_destroy(hrxq->action);
-	else
-		mlx5_glue->destroy_flow_action(hrxq->action);
-#endif
 	priv->obj_ops.hrxq_destroy(hrxq);
 	if (!hrxq->standalone) {
 		mlx5_ind_table_obj_release(dev, hrxq->ind_table,
-- 
2.27.0

RE: [dpdk-dev] [PATCH v2 0/2] fixes for mlx5

[wangyunjian]

Friendly ping.

> -----Original Message-----
> From: wangyunjian
> Sent: Tuesday, August 23, 2022 2:45 PM
> To: dev@dpdk.org
> Cc: matan@nvidia.com; rasland@nvidia.com; viacheslavo@nvidia.com;
> dkozlyuk@nvidia.com; Huangshaozhang <huangshaozhang@huawei.com>;
> wangyunjian <wangyunjian@huawei.com>
> Subject: [dpdk-dev] [PATCH v2 0/2] fixes for mlx5
> 
> This series include two fixes patches for mlx5.
> 
> ---
> v2:
>    * update patch 2/2 code styles
> 
> Yunjian Wang (2):
>   net/mlx5: fix use after free when releasing tx queues
>   net/mlx5: fix resource leak when releasing a drop action
> 
>  drivers/net/mlx5/mlx5_devx.c | 7 +++++++  drivers/net/mlx5/mlx5_rxq.c  |
> 6 ------  drivers/net/mlx5/mlx5_txq.c  | 3 ++-
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> --
> 2.27.0

RE: [dpdk-dev] [PATCH v2 1/2] net/mlx5: fix use after free when releasing tx queues

[wangyunjian]

Friendly ping.

> -----Original Message-----
> From: wangyunjian
> Sent: Tuesday, August 23, 2022 2:46 PM
> To: dev@dpdk.org
> Cc: matan@nvidia.com; rasland@nvidia.com; viacheslavo@nvidia.com;
> dkozlyuk@nvidia.com; Huangshaozhang <huangshaozhang@huawei.com>;
> wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 1/2] net/mlx5: fix use after free when releasing
> tx queues
> 
> The bonding slave remove function was calling the eth_dev_tx_queue_config
> function, which frees dev->data->tx_queues, and then tries to free
> priv->txqs[idx] in mlx5_txq_release function, which causes the heap use
> after free issue. Add checks whether dev->data->tx_queues is not NULL.
> 
> Fixes: 94e257ec8ca ("net/mlx5: fix Rx/Tx queue checks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  drivers/net/mlx5/mlx5_txq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c index
> 0140f8b3b2..cb2c33a060 100644
> --- a/drivers/net/mlx5/mlx5_txq.c
> +++ b/drivers/net/mlx5/mlx5_txq.c
> @@ -1198,7 +1198,8 @@ mlx5_txq_release(struct rte_eth_dev *dev, uint16_t
> idx)
>  	struct mlx5_priv *priv = dev->data->dev_private;
>  	struct mlx5_txq_ctrl *txq_ctrl;
> 
> -	if (priv->txqs == NULL || (*priv->txqs)[idx] == NULL)
> +	if (dev->data->tx_queues == NULL || priv->txqs == NULL ||
> +		(*priv->txqs)[idx] == NULL)
>  		return 0;
>  	txq_ctrl = container_of((*priv->txqs)[idx], struct mlx5_txq_ctrl, txq);
>  	if (__atomic_sub_fetch(&txq_ctrl->refcnt, 1, __ATOMIC_RELAXED) > 1)
> --
> 2.27.0

RE: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when releasing a drop action

[wangyunjian]

Friendly ping.

> -----Original Message-----
> From: wangyunjian
> Sent: Tuesday, August 23, 2022 2:46 PM
> To: dev@dpdk.org
> Cc: matan@nvidia.com; rasland@nvidia.com; viacheslavo@nvidia.com;
> dkozlyuk@nvidia.com; Huangshaozhang <huangshaozhang@huawei.com>;
> wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when releasing a
> drop action
> 
> Currently, the resources for hrxq->action are allocated in
> mlx5_devx_hrxq_new(). But it was not being freed when the drop action was
> released in mlx5_devx_drop_action_destroy().
> So, fix is to free the resources in mlx5_devx_tir_destroy().
> 
> Fixes: bc5bee028ebc ("net/mlx5: create drop queue using DevX")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  drivers/net/mlx5/mlx5_devx.c | 7 +++++++  drivers/net/mlx5/mlx5_rxq.c  |
> 6 ------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_devx.c b/drivers/net/mlx5/mlx5_devx.c index
> 6886ae1f22..09c8856f05 100644
> --- a/drivers/net/mlx5/mlx5_devx.c
> +++ b/drivers/net/mlx5/mlx5_devx.c
> @@ -907,6 +907,13 @@ mlx5_devx_hrxq_new(struct rte_eth_dev *dev,
> struct mlx5_hrxq *hrxq,  static void  mlx5_devx_tir_destroy(struct
> mlx5_hrxq *hrxq)  {
> +#if defined(HAVE_IBV_FLOW_DV_SUPPORT)
> || !defined(HAVE_INFINIBAND_VERBS_H)
> +	if (hrxq->hws_flags)
> +		mlx5dr_action_destroy(hrxq->action);
> +	else
> +		mlx5_flow_os_destroy_flow_action(hrxq->action);
> +	hrxq->action = NULL;
> +#endif
>  	claim_zero(mlx5_devx_cmd_destroy(hrxq->tir));
>  }
> 
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index
> eaf23d0df4..e518fe9bfd 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -2861,12 +2861,6 @@ __mlx5_hrxq_remove(struct rte_eth_dev *dev,
> struct mlx5_hrxq *hrxq)  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
> 
> -#ifdef HAVE_IBV_FLOW_DV_SUPPORT
> -	if (hrxq->hws_flags)
> -		mlx5dr_action_destroy(hrxq->action);
> -	else
> -		mlx5_glue->destroy_flow_action(hrxq->action);
> -#endif
>  	priv->obj_ops.hrxq_destroy(hrxq);
>  	if (!hrxq->standalone) {
>  		mlx5_ind_table_obj_release(dev, hrxq->ind_table,
> --
> 2.27.0

RE: [dpdk-dev] [PATCH v2 1/2] net/mlx5: fix use after free when releasing tx queues

[Slava Ovsiienko]

Hi, Yunjian

Could you, please, tell more details about problematic scenario?
In bonding slave? It is not fully clean for me how mlx5_txq_release
frees priv->txqs[idx] (BTW NULL is OK to free, it is safe).
We have check for NULL here:
> > -	if (priv->txqs == NULL || (*priv->txqs)[idx] == NULL)

priv->txq is internal objects managed by PMD, dev->data->tx_queues
are DPDK-wide ones. Theoretically it might happen when DPDK objects
are created and internals are not, and vice versa. So, checking 
for existence of external objects in the routine that manages internals
does not look so reasonable. Internal queue object management is based
on the atomic reference counter and, generally speaking, should not depend
on externals.

With best regards,
Slava 

> -----Original Message-----
> From: wangyunjian <wangyunjian@huawei.com>
> Sent: Friday, September 23, 2022 12:32
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Dmitry Kozlyuk
> <dkozlyuk@nvidia.com>; Huangshaozhang <huangshaozhang@huawei.com>;
> stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 1/2] net/mlx5: fix use after free when
> releasing tx queues
> 
> Friendly ping.
> 
> > -----Original Message-----
> > From: wangyunjian
> > Sent: Tuesday, August 23, 2022 2:46 PM
> > To: dev@dpdk.org
> > Cc: matan@nvidia.com; rasland@nvidia.com; viacheslavo@nvidia.com;
> > dkozlyuk@nvidia.com; Huangshaozhang <huangshaozhang@huawei.com>;
> > wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 1/2] net/mlx5: fix use after free when
> > releasing tx queues
> >
> > The bonding slave remove function was calling the
> > eth_dev_tx_queue_config function, which frees dev->data->tx_queues,
> > and then tries to free
> > priv->txqs[idx] in mlx5_txq_release function, which causes the heap
> > priv->use
> > after free issue. Add checks whether dev->data->tx_queues is not NULL.
> >
> > Fixes: 94e257ec8ca ("net/mlx5: fix Rx/Tx queue checks")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> >  drivers/net/mlx5/mlx5_txq.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
> > index
> > 0140f8b3b2..cb2c33a060 100644
> > --- a/drivers/net/mlx5/mlx5_txq.c
> > +++ b/drivers/net/mlx5/mlx5_txq.c
> > @@ -1198,7 +1198,8 @@ mlx5_txq_release(struct rte_eth_dev *dev,
> > uint16_t
> > idx)
> >  	struct mlx5_priv *priv = dev->data->dev_private;
> >  	struct mlx5_txq_ctrl *txq_ctrl;
> >
> > -	if (priv->txqs == NULL || (*priv->txqs)[idx] == NULL)
> > +	if (dev->data->tx_queues == NULL || priv->txqs == NULL ||
> > +		(*priv->txqs)[idx] == NULL)
> >  		return 0;
> >  	txq_ctrl = container_of((*priv->txqs)[idx], struct mlx5_txq_ctrl,
> txq);
> >  	if (__atomic_sub_fetch(&txq_ctrl->refcnt, 1, __ATOMIC_RELAXED) > 1)
> > --
> > 2.27.0

RE: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when releasing a drop action

[Slava Ovsiienko]

Hi, Yunjian

We have drop action in mlx5.
To create/destroy drop action we have 
mlx5_drop_action_create/ mlx5_drop_action_destroy common routines.

As PMD supports operation either over FW (Verbs in rdma_core) or SW/HW steering
with DevX objects the "virtual" methods are used for objects:

priv->obj_ops.drop_action_create(dev) - create drop action
   a) mlx5_ibv_drop_action_create()- use Verbs (dv-flow_en == 0)
   b) mlx5_devx_drop_action_create() - use DevX (dv-flow_en != 0)

priv->obj_ops.drop_action_destroy(dev) - destroy drop action
   a) mlx5_ibv_drop_action_destroy()- use Verbs (dv-flow_en == 0)
   b) mlx5_devx_drop_action_destroy() - use DevX (dv-flow_en != 0)


Let's consider DevX case.
mlx5_devx_drop_action_create()
    mlx5_rxq_devx_obj_drop_create()
    mlx5_devx_ind_table_new()
    mlx5_devx_hrxq_new()
         xxx_create_dest_tir()
         xxx_action_create_dest_tir()


mlx5_devx_drop_action_destroy()
    /* It seems action destroy is missing here */
    mlx5_devx_tir_destroy()
    mlx5_devx_ind_table_destroy()
    mlx5_rxq_devx_obj_drop_release()

So, yes, I agree. There is missing action destroy, it seems we have leakage.

Let's look at 
__mlx5_hrxq_remove()
   destroy_action()
   priv->obj_ops.hrxq_destroy();


And there we have a problem.
obj_ops.hrxq_destroy() - not always mlx5_devx_tir_destroy() is called.
It might be mlx5_ibv_qp_destroy() (for Verbas) and patch would not work.

So, instead of fixing __mlx5_hrxq_remove() and mlx5_devx_tir_destroy()
I would consider patching:
- mlx5_devx_drop_action_destroy()
- mlx5_ibv_drop_action_destroy
by adding action desatroying.

With best regards,
Slava


> -----Original Message-----
> From: wangyunjian <wangyunjian@huawei.com>
> Sent: Friday, September 23, 2022 12:32
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Dmitry Kozlyuk
> <dkozlyuk@nvidia.com>; Huangshaozhang <huangshaozhang@huawei.com>;
> stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when
> releasing a drop action
> 
> Friendly ping.
> 
> > -----Original Message-----
> > From: wangyunjian
> > Sent: Tuesday, August 23, 2022 2:46 PM
> > To: dev@dpdk.org
> > Cc: matan@nvidia.com; rasland@nvidia.com; viacheslavo@nvidia.com;
> > dkozlyuk@nvidia.com; Huangshaozhang <huangshaozhang@huawei.com>;
> > wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when
> > releasing a drop action
> >
> > Currently, the resources for hrxq->action are allocated in
> > mlx5_devx_hrxq_new(). But it was not being freed when the drop action
> > was released in mlx5_devx_drop_action_destroy().
> > So, fix is to free the resources in mlx5_devx_tir_destroy().
> >
> > Fixes: bc5bee028ebc ("net/mlx5: create drop queue using DevX")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> >  drivers/net/mlx5/mlx5_devx.c | 7 +++++++  drivers/net/mlx5/mlx5_rxq.c
> > |
> > 6 ------
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_devx.c
> > b/drivers/net/mlx5/mlx5_devx.c index
> > 6886ae1f22..09c8856f05 100644
> > --- a/drivers/net/mlx5/mlx5_devx.c
> > +++ b/drivers/net/mlx5/mlx5_devx.c
> > @@ -907,6 +907,13 @@ mlx5_devx_hrxq_new(struct rte_eth_dev *dev,
> > struct mlx5_hrxq *hrxq,  static void  mlx5_devx_tir_destroy(struct
> > mlx5_hrxq *hrxq)  {
> > +#if defined(HAVE_IBV_FLOW_DV_SUPPORT)
> > || !defined(HAVE_INFINIBAND_VERBS_H)
> > +	if (hrxq->hws_flags)
> > +		mlx5dr_action_destroy(hrxq->action);
> > +	else
> > +		mlx5_flow_os_destroy_flow_action(hrxq->action);
> > +	hrxq->action = NULL;
> > +#endif
> >  	claim_zero(mlx5_devx_cmd_destroy(hrxq->tir));
> >  }
> >
> > diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> > index eaf23d0df4..e518fe9bfd 100644
> > --- a/drivers/net/mlx5/mlx5_rxq.c
> > +++ b/drivers/net/mlx5/mlx5_rxq.c
> > @@ -2861,12 +2861,6 @@ __mlx5_hrxq_remove(struct rte_eth_dev *dev,
> > struct mlx5_hrxq *hrxq)  {
> >  	struct mlx5_priv *priv = dev->data->dev_private;
> >
> > -#ifdef HAVE_IBV_FLOW_DV_SUPPORT
> > -	if (hrxq->hws_flags)
> > -		mlx5dr_action_destroy(hrxq->action);
> > -	else
> > -		mlx5_glue->destroy_flow_action(hrxq->action);
> > -#endif
> >  	priv->obj_ops.hrxq_destroy(hrxq);
> >  	if (!hrxq->standalone) {
> >  		mlx5_ind_table_obj_release(dev, hrxq->ind_table,
> > --
> > 2.27.0

RE: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when releasing a drop action

[wangyunjian]

I agree with you. Can you fix it?

Thanks
        Yunjian

> -----Original Message-----
> From: Slava Ovsiienko [mailto:viacheslavo@nvidia.com]
> Sent: Tuesday, September 27, 2022 3:36 AM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; Dmitry Kozlyuk <dkozlyuk@nvidia.com>;
> Huangshaozhang <huangshaozhang@huawei.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when
> releasing a drop action
> 
> Hi, Yunjian
> 
> We have drop action in mlx5.
> To create/destroy drop action we have
> mlx5_drop_action_create/ mlx5_drop_action_destroy common routines.
> 
> As PMD supports operation either over FW (Verbs in rdma_core) or SW/HW
> steering with DevX objects the "virtual" methods are used for objects:
> 
> priv->obj_ops.drop_action_create(dev) - create drop action
>    a) mlx5_ibv_drop_action_create()- use Verbs (dv-flow_en == 0)
>    b) mlx5_devx_drop_action_create() - use DevX (dv-flow_en != 0)
> 
> priv->obj_ops.drop_action_destroy(dev) - destroy drop action
>    a) mlx5_ibv_drop_action_destroy()- use Verbs (dv-flow_en == 0)
>    b) mlx5_devx_drop_action_destroy() - use DevX (dv-flow_en != 0)
> 
> 
> Let's consider DevX case.
> mlx5_devx_drop_action_create()
>     mlx5_rxq_devx_obj_drop_create()
>     mlx5_devx_ind_table_new()
>     mlx5_devx_hrxq_new()
>          xxx_create_dest_tir()
>          xxx_action_create_dest_tir()
> 
> 
> mlx5_devx_drop_action_destroy()
>     /* It seems action destroy is missing here */
>     mlx5_devx_tir_destroy()
>     mlx5_devx_ind_table_destroy()
>     mlx5_rxq_devx_obj_drop_release()
> 
> So, yes, I agree. There is missing action destroy, it seems we have leakage.
> 
> Let's look at
> __mlx5_hrxq_remove()
>    destroy_action()
>    priv->obj_ops.hrxq_destroy();
> 
> 
> And there we have a problem.
> obj_ops.hrxq_destroy() - not always mlx5_devx_tir_destroy() is called.
> It might be mlx5_ibv_qp_destroy() (for Verbas) and patch would not work.
> 
> So, instead of fixing __mlx5_hrxq_remove() and mlx5_devx_tir_destroy()
> I would consider patching:
> - mlx5_devx_drop_action_destroy()
> - mlx5_ibv_drop_action_destroy
> by adding action desatroying.
> 
> With best regards,
> Slava
> 
> 
> > -----Original Message-----
> > From: wangyunjian <wangyunjian@huawei.com>
> > Sent: Friday, September 23, 2022 12:32
> > To: dev@dpdk.org
> > Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>;
> > Slava Ovsiienko <viacheslavo@nvidia.com>; Dmitry Kozlyuk
> > <dkozlyuk@nvidia.com>; Huangshaozhang
> <huangshaozhang@huawei.com>;
> > stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when
> > releasing a drop action
> >
> > Friendly ping.
> >
> > > -----Original Message-----
> > > From: wangyunjian
> > > Sent: Tuesday, August 23, 2022 2:46 PM
> > > To: dev@dpdk.org
> > > Cc: matan@nvidia.com; rasland@nvidia.com; viacheslavo@nvidia.com;
> > > dkozlyuk@nvidia.com; Huangshaozhang <huangshaozhang@huawei.com>;
> > > wangyunjian <wangyunjian@huawei.com>; stable@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix resource leak when
> > > releasing a drop action
> > >
> > > Currently, the resources for hrxq->action are allocated in
> > > mlx5_devx_hrxq_new(). But it was not being freed when the drop action
> > > was released in mlx5_devx_drop_action_destroy().
> > > So, fix is to free the resources in mlx5_devx_tir_destroy().
> > >
> > > Fixes: bc5bee028ebc ("net/mlx5: create drop queue using DevX")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_devx.c | 7 +++++++  drivers/net/mlx5/mlx5_rxq.c
> > > |
> > > 6 ------
> > >  2 files changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_devx.c
> > > b/drivers/net/mlx5/mlx5_devx.c index
> > > 6886ae1f22..09c8856f05 100644
> > > --- a/drivers/net/mlx5/mlx5_devx.c
> > > +++ b/drivers/net/mlx5/mlx5_devx.c
> > > @@ -907,6 +907,13 @@ mlx5_devx_hrxq_new(struct rte_eth_dev *dev,
> > > struct mlx5_hrxq *hrxq,  static void  mlx5_devx_tir_destroy(struct
> > > mlx5_hrxq *hrxq)  {
> > > +#if defined(HAVE_IBV_FLOW_DV_SUPPORT)
> > > || !defined(HAVE_INFINIBAND_VERBS_H)
> > > +	if (hrxq->hws_flags)
> > > +		mlx5dr_action_destroy(hrxq->action);
> > > +	else
> > > +		mlx5_flow_os_destroy_flow_action(hrxq->action);
> > > +	hrxq->action = NULL;
> > > +#endif
> > >  	claim_zero(mlx5_devx_cmd_destroy(hrxq->tir));
> > >  }
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> > > index eaf23d0df4..e518fe9bfd 100644
> > > --- a/drivers/net/mlx5/mlx5_rxq.c
> > > +++ b/drivers/net/mlx5/mlx5_rxq.c
> > > @@ -2861,12 +2861,6 @@ __mlx5_hrxq_remove(struct rte_eth_dev *dev,
> > > struct mlx5_hrxq *hrxq)  {
> > >  	struct mlx5_priv *priv = dev->data->dev_private;
> > >
> > > -#ifdef HAVE_IBV_FLOW_DV_SUPPORT
> > > -	if (hrxq->hws_flags)
> > > -		mlx5dr_action_destroy(hrxq->action);
> > > -	else
> > > -		mlx5_glue->destroy_flow_action(hrxq->action);
> > > -#endif
> > >  	priv->obj_ops.hrxq_destroy(hrxq);
> > >  	if (!hrxq->standalone) {
> > >  		mlx5_ind_table_obj_release(dev, hrxq->ind_table,
> > > --
> > > 2.27.0