* [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine
@ 2022-10-13 7:21 Leon Romanovsky
2022-10-13 10:03 ` Tariq Toukan
2022-10-18 10:26 ` Leon Romanovsky
0 siblings, 2 replies; 5+ messages in thread
From: Leon Romanovsky @ 2022-10-13 7:21 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: Leon Romanovsky, Dan Carpenter, Emeel Hakim, Eric Dumazet,
linux-rdma, netdev, Paolo Abeni, Raed Salem, Saeed Mahameed,
Tariq Toukan
From: Leon Romanovsky <leonro@nvidia.com>
The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
doesn't support MACsec (priv->macsec will be NULL) together with useless
comment line, assignment and extra blank lines.
Fix everything in one patch.
Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
.../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 41970067917b..4331235b21ee 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -1846,25 +1846,16 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
{
struct mlx5e_macsec *macsec = priv->macsec;
- struct mlx5_core_dev *mdev = macsec->mdev;
+ struct mlx5_core_dev *mdev = priv->mdev;
if (!macsec)
return;
mlx5_notifier_unregister(mdev, &macsec->nb);
-
mlx5e_macsec_fs_cleanup(macsec->macsec_fs);
-
- /* Cleanup workqueue */
destroy_workqueue(macsec->wq);
-
mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
-
- priv->macsec = NULL;
-
rhashtable_destroy(&macsec->sci_hash);
-
mutex_destroy(&macsec->lock);
-
kfree(macsec);
}
--
2.37.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-13 7:21 [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine Leon Romanovsky
@ 2022-10-13 10:03 ` Tariq Toukan
2022-10-13 10:14 ` Leon Romanovsky
2022-10-18 10:26 ` Leon Romanovsky
1 sibling, 1 reply; 5+ messages in thread
From: Tariq Toukan @ 2022-10-13 10:03 UTC (permalink / raw)
To: Leon Romanovsky, David S . Miller, Jakub Kicinski
Cc: Leon Romanovsky, Dan Carpenter, Emeel Hakim, Eric Dumazet,
linux-rdma, netdev, Paolo Abeni, Raed Salem, Saeed Mahameed,
Tariq Toukan
On 10/13/2022 10:21 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
>
> The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
> doesn't support MACsec (priv->macsec will be NULL) together with useless
> comment line, assignment and extra blank lines.
>
> Fix everything in one patch.
>
> Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index 41970067917b..4331235b21ee 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -1846,25 +1846,16 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
> void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
> {
> struct mlx5e_macsec *macsec = priv->macsec;
> - struct mlx5_core_dev *mdev = macsec->mdev;
> + struct mlx5_core_dev *mdev = priv->mdev;
>
simply defer the mdev calculation to be after the early return, trying
to keep this macsec function as independent as possible.
> if (!macsec)
> return;
>
> mlx5_notifier_unregister(mdev, &macsec->nb);
> -
> mlx5e_macsec_fs_cleanup(macsec->macsec_fs);
> -
> - /* Cleanup workqueue */
> destroy_workqueue(macsec->wq);
> -
> mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
> -
> - priv->macsec = NULL;
> -
Why remove this assignment?
It protects against accessing freed memory, for instance when querying
the macsec stats, see
drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_stats.c
> rhashtable_destroy(&macsec->sci_hash);
> -
> mutex_destroy(&macsec->lock);
> -
> kfree(macsec);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-13 10:03 ` Tariq Toukan
@ 2022-10-13 10:14 ` Leon Romanovsky
0 siblings, 0 replies; 5+ messages in thread
From: Leon Romanovsky @ 2022-10-13 10:14 UTC (permalink / raw)
To: Tariq Toukan
Cc: David S . Miller, Jakub Kicinski, Dan Carpenter, Emeel Hakim,
Eric Dumazet, linux-rdma, netdev, Paolo Abeni, Raed Salem,
Saeed Mahameed, Tariq Toukan
On Thu, Oct 13, 2022 at 01:03:43PM +0300, Tariq Toukan wrote:
>
>
> On 10/13/2022 10:21 AM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
> > doesn't support MACsec (priv->macsec will be NULL) together with useless
> > comment line, assignment and extra blank lines.
> >
> > Fix everything in one patch.
> >
> > Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
> > 1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > index 41970067917b..4331235b21ee 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > @@ -1846,25 +1846,16 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
> > void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
> > {
> > struct mlx5e_macsec *macsec = priv->macsec;
> > - struct mlx5_core_dev *mdev = macsec->mdev;
> > + struct mlx5_core_dev *mdev = priv->mdev;
>
> simply defer the mdev calculation to be after the early return, trying to
> keep this macsec function as independent as possible.
It is done to keep _cleanup symmetrical to _init one. The function
should operate on same priv->mdev as was used there without any relation
to macsec->mdev. Of course, it is the same pointer, but it is better to have
same code.
>
> > if (!macsec)
> > return;
> > mlx5_notifier_unregister(mdev, &macsec->nb);
> > -
> > mlx5e_macsec_fs_cleanup(macsec->macsec_fs);
> > -
> > - /* Cleanup workqueue */
> > destroy_workqueue(macsec->wq);
> > -
> > mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
> > -
> > - priv->macsec = NULL;
> > -
>
> Why remove this assignment?
>
> It protects against accessing freed memory, for instance when querying the
> macsec stats, see
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_stats.c
You can't and shouldn't access anything related to MACsec after call to
mlx5e_macsec_cleanup(). If you do so, you are already in trouble as you
don't have any locking protection from stat access and cleanup.
So no, it doesn't protect from accessing freed memory. It is just
anti-pattern of hiding bugs related to unlocked concurrent accesses
and wrong release flows. Don't do it.
Thanks
>
> > rhashtable_destroy(&macsec->sci_hash);
> > -
> > mutex_destroy(&macsec->lock);
> > -
> > kfree(macsec);
> > }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-13 7:21 [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine Leon Romanovsky
2022-10-13 10:03 ` Tariq Toukan
@ 2022-10-18 10:26 ` Leon Romanovsky
2022-10-18 18:04 ` Jakub Kicinski
1 sibling, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2022-10-18 10:26 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet
Cc: Dan Carpenter, Emeel Hakim, linux-rdma, netdev, Raed Salem,
Saeed Mahameed, Tariq Toukan
On Thu, Oct 13, 2022 at 10:21:00AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
>
> The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
> doesn't support MACsec (priv->macsec will be NULL) together with useless
> comment line, assignment and extra blank lines.
>
> Fix everything in one patch.
>
> Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
Gentle ping.
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-18 10:26 ` Leon Romanovsky
@ 2022-10-18 18:04 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2022-10-18 18:04 UTC (permalink / raw)
To: Leon Romanovsky
Cc: David S . Miller, Paolo Abeni, Eric Dumazet, Dan Carpenter,
Emeel Hakim, linux-rdma, netdev, Raed Salem, Saeed Mahameed,
Tariq Toukan
On Tue, 18 Oct 2022 13:26:09 +0300 Leon Romanovsky wrote:
> On Thu, Oct 13, 2022 at 10:21:00AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
> > doesn't support MACsec (priv->macsec will be NULL) together with useless
> > comment line, assignment and extra blank lines.
> >
> > Fix everything in one patch.
> >
> > Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>
> Gentle ping.
Just repost. I'm guessing that the maintainer who marked it as Changes
Requested wasn't sure if Tariq was convinced and decided to err on the
side of caution. If you repost and Tariq doesn't disagree again the
case will be clear.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-18 18:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-13 7:21 [PATCH net] net/mlx5e: Cleanup MACsec uninitialization routine Leon Romanovsky
2022-10-13 10:03 ` Tariq Toukan
2022-10-13 10:14 ` Leon Romanovsky
2022-10-18 10:26 ` Leon Romanovsky
2022-10-18 18:04 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).