* [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine
@ 2022-10-19 6:06 Leon Romanovsky
2022-10-19 6:27 ` Saeed Mahameed
2022-10-19 22:44 ` Jakub Kicinski
0 siblings, 2 replies; 4+ messages in thread
From: Leon Romanovsky @ 2022-10-19 6:06 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: Leon Romanovsky, Dan Carpenter, Emeel Hakim, Eric Dumazet, 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>
---
Resend: https://lore.kernel.org/all/b43b1c5aadd5cfdcd2e385ce32693220331700ba.1665645548.git.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] 4+ messages in thread* Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-19 6:06 [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine Leon Romanovsky
@ 2022-10-19 6:27 ` Saeed Mahameed
2022-10-19 6:41 ` Leon Romanovsky
2022-10-19 22:44 ` Jakub Kicinski
1 sibling, 1 reply; 4+ messages in thread
From: Saeed Mahameed @ 2022-10-19 6:27 UTC (permalink / raw)
To: Leon Romanovsky
Cc: David S . Miller, Jakub Kicinski, Leon Romanovsky, Dan Carpenter,
Emeel Hakim, Eric Dumazet, netdev, Paolo Abeni, Raed Salem,
Tariq Toukan
On 19 Oct 09:06, 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>
>---
>Resend: https://lore.kernel.org/all/b43b1c5aadd5cfdcd2e385ce32693220331700ba.1665645548.git.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;
>-
Tariq was right, we need this check, the same priv can be resurrected
after cleanup to be used in switchdev representor profile, where
capabilities are not guaranteed to be the same as NIC netdev, so you will
end up using a garbage macsec.
Also we don't submit cleanups to net to avoid porting unnecessary changes
and new bugs to -rc.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-19 6:27 ` Saeed Mahameed
@ 2022-10-19 6:41 ` Leon Romanovsky
0 siblings, 0 replies; 4+ messages in thread
From: Leon Romanovsky @ 2022-10-19 6:41 UTC (permalink / raw)
To: Saeed Mahameed
Cc: David S . Miller, Jakub Kicinski, Dan Carpenter, Emeel Hakim,
Eric Dumazet, netdev, Paolo Abeni, Raed Salem, Tariq Toukan
On Tue, Oct 18, 2022 at 11:27:10PM -0700, Saeed Mahameed wrote:
> On 19 Oct 09:06, 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>
> > ---
> > Resend: https://lore.kernel.org/all/b43b1c5aadd5cfdcd2e385ce32693220331700ba.1665645548.git.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;
> > -
>
> Tariq was right, we need this check, the same priv can be resurrected
> after cleanup to be used in switchdev representor profile, where
> capabilities are not guaranteed to be the same as NIC netdev, so you will
> end up using a garbage macsec.
Not really, we are checking that device supports MACsec with
mlx5e_is_macsec_device() in every entry call where is a chance do
not have priv->macsec. If device doesn't support, the relevant ops
won't be installed (mlx5e_macsec_build_netdev) and nothing will call
uninitialized MACsec.
The NULL assignment is purely anti-pattern where you hide use-after-free
access.
>
> Also we don't submit cleanups to net to avoid porting unnecessary changes
> and new bugs to -rc.
It is something that was added in previous cycle. There is no
backporting yet.
Thanks
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine
2022-10-19 6:06 [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine Leon Romanovsky
2022-10-19 6:27 ` Saeed Mahameed
@ 2022-10-19 22:44 ` Jakub Kicinski
1 sibling, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2022-10-19 22:44 UTC (permalink / raw)
To: Leon Romanovsky
Cc: David S . Miller, Leon Romanovsky, Dan Carpenter, Emeel Hakim,
Eric Dumazet, netdev, Paolo Abeni, Raed Salem, Saeed Mahameed,
Tariq Toukan
On Wed, 19 Oct 2022 09:06:43 +0300 Leon Romanovsky wrote:
> The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
^
NULL? _/
> doesn't support MACsec (priv->macsec will be NULL) together with useless
s/together with/. While at it delete/
> comment line, assignment and extra blank lines.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-10-19 22:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-19 6:06 [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine Leon Romanovsky
2022-10-19 6:27 ` Saeed Mahameed
2022-10-19 6:41 ` Leon Romanovsky
2022-10-19 22:44 ` 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).