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 <[email protected]>
> Sent: Friday, September 23, 2022 12:32
> To: [email protected]
> Cc: Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>;
> Slava Ovsiienko <[email protected]>; Dmitry Kozlyuk
> <[email protected]>; Huangshaozhang <[email protected]>;
> [email protected]
> 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: [email protected]
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; Huangshaozhang <[email protected]>;
> > wangyunjian <[email protected]>; [email protected]
> > 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: [email protected]
> >
> > Signed-off-by: Yunjian Wang <[email protected]>
> > ---
> >  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

Reply via email to