| Message ID | 4bd5c6655c5970ac30adb254a1f09f4f5e992158.1666159448.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESEND,net] net/mlx5e: Cleanup MACsec uninitialization routine | expand |
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.
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
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.
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); }