| Message ID | 20240509112951.590184-2-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3d5918477f94e4c2f064567875c475468e264644 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mlx5 misc fixes | expand |
On Thu, May 09, 2024 at 02:29:47PM +0300, Tariq Toukan wrote: > From: Shay Drory <shayd@nvidia.com> > > mlx5e_suspend cleans resources only if netif_device_present() returns > true. However, mlx5e_resume changes the state of netif, via > mlx5e_nic_enable, only if reg_state == NETREG_REGISTERED. > In the below case, the above leads to NULL-ptr Oops[1] and memory > leaks: > > mlx5e_probe > _mlx5e_resume > mlx5e_attach_netdev > mlx5e_nic_enable <-- netdev not reg, not calling netif_device_attach() > register_netdev <-- failed for some reason. > ERROR_FLOW: > _mlx5e_suspend <-- netif_device_present return false, resources aren't freed :( > > Hence, clean resources in this case as well. > > [1] > BUG: kernel NULL pointer dereference, address: 0000000000000000 ... > Fixes: 2c3b5beec46a ("net/mlx5e: More generic netdev management API") > Signed-off-by: Shay Drory <shayd@nvidia.com> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> Hi, I think that this bug is caused by asymmetry in resource allocation/freeing such that there are cases where _mlx5e_suspend() doesn't unwind _mlx5e_resume(). It seems to me that asymmetry was introduced by the check for reg_state != NETREG_REGISTERED in mlx5e_nic_enable() by: 610e89e05c3f ("net/mlx5e: Don't sync netdev state when not registered") So perhaps that is a more appropriate commit for the Fixes tag. I do note that commit was a fix for: 26e59d8077a3 ("net/mlx5e: Implement mlx5e interface attach/detach callbacks") So perhaps a second fixes tag for that commit is also appropriate. Perhaps it's not important enough to revise things, I don't feel strongly about it, so feel free to add the following regardless. Reviewed-by: Simon Horman <horms@kernel.org> All that said, I do wonder if it would be better in the long run to implement things in such a way that there is symmetry in resource allocation / deallocation. Passing flags to control how much cleanup is performed does seem a bit awkward.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index 319930c04093..64497b6eebd3 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -6058,7 +6058,7 @@ static int mlx5e_resume(struct auxiliary_device *adev) return 0; } -static int _mlx5e_suspend(struct auxiliary_device *adev) +static int _mlx5e_suspend(struct auxiliary_device *adev, bool pre_netdev_reg) { struct mlx5e_dev *mlx5e_dev = auxiliary_get_drvdata(adev); struct mlx5e_priv *priv = mlx5e_dev->priv; @@ -6067,7 +6067,7 @@ static int _mlx5e_suspend(struct auxiliary_device *adev) struct mlx5_core_dev *pos; int i; - if (!netif_device_present(netdev)) { + if (!pre_netdev_reg && !netif_device_present(netdev)) { if (test_bit(MLX5E_STATE_DESTROYING, &priv->state)) mlx5_sd_for_each_dev(i, mdev, pos) mlx5e_destroy_mdev_resources(pos); @@ -6090,7 +6090,7 @@ static int mlx5e_suspend(struct auxiliary_device *adev, pm_message_t state) actual_adev = mlx5_sd_get_adev(mdev, adev, edev->idx); if (actual_adev) - err = _mlx5e_suspend(actual_adev); + err = _mlx5e_suspend(actual_adev, false); mlx5_sd_cleanup(mdev); return err; @@ -6157,7 +6157,7 @@ static int _mlx5e_probe(struct auxiliary_device *adev) return 0; err_resume: - _mlx5e_suspend(adev); + _mlx5e_suspend(adev, true); err_profile_cleanup: profile->cleanup(priv); err_destroy_netdev: @@ -6197,7 +6197,7 @@ static void _mlx5e_remove(struct auxiliary_device *adev) mlx5_core_uplink_netdev_set(mdev, NULL); mlx5e_dcbnl_delete_app(priv); unregister_netdev(priv->netdev); - _mlx5e_suspend(adev); + _mlx5e_suspend(adev, false); priv->profile->cleanup(priv); mlx5e_destroy_netdev(priv); mlx5e_devlink_port_unregister(mlx5e_dev);