On Fri, 2021-02-05 at 15:45 +0300, Dan Carpenter wrote:
> Hello Saeed Mahameed and Feras Daoud,
>
Hi Dan, thanks for the report, adding Roi the owner of this change.
> I'm not exactly sure if I should blame commit c4d7eb57687f:
> "net/mxl5e:
> Add change profile method" fomr Mar 22, 2020 or commit 182570b26223
> ("net/mlx5e: Gather common netdev init/cleanup functionality in one
> place") from Oct 2, 2018.
>
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5658
> mlx5e_netdev_change_profile() warn: 'priv->htb.qos_sq_stats' double
> freed
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5658
> mlx5e_netdev_change_profile() warn: 'priv->scratchpad.cpumask' double
> freed
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5789 mlx5e_probe()
> warn: 'priv->htb.qos_sq_stats' double freed
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5789 mlx5e_probe()
> warn: 'priv->scratchpad.cpumask' double freed
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5802 mlx5e_remove()
> warn: 'priv->htb.qos_sq_stats' double freed
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c:5802 mlx5e_remove()
> warn: 'priv->scratchpad.cpumask' double freed
> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:1317
> mlx5e_vport_rep_unload() warn: 'priv->htb.qos_sq_stats' double freed
> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:1317
> mlx5e_vport_rep_unload() warn: 'priv->scratchpad.cpumask' double
> freed
If I may ask,
What is this report ? Coverity ? static checker ? or runtime checks ?
>
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> 5639 int mlx5e_netdev_change_profile(struct mlx5e_priv *priv,
> 5640 const struct mlx5e_profile
> *new_profile, void *new_ppriv)
> 5641 {
> 5642 unsigned int new_max_nch = mlx5e_calc_max_nch(priv,
> new_profile);
> 5643 const struct mlx5e_profile *orig_profile = priv-
> >profile;
> 5644 void *orig_ppriv = priv->ppriv;
> 5645 int err, rollback_err;
> 5646
> 5647 /* sanity */
> 5648 if (new_max_nch != priv->max_nch) {
> 5649 netdev_warn(priv->netdev,
> 5650 "%s: Replacing profile with
> different max channles\n",
> 5651 __func__);
> 5652 return -EINVAL;
> 5653 }
> 5654
> 5655 /* cleanup old profile */
> 5656 mlx5e_detach_netdev(priv);
> 5657 priv->profile->cleanup(priv);
>
> The problem is that mlx5i_pkey_cleanup() calls mlx5e_priv_cleanup().
>
> 5658 mlx5e_priv_cleanup(priv);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> And then it gets called again here.
impossible, mlx5i would never call mlx5e_netdev_change_profile, but
anyway i see the issue with the error flow if this function fail after
mlx5e_priv_cleanup, then we remove the driver, we will actually end up
with double free, since this function is not supposed to free priv-
>fields and then just abort..
this function must guarantee that the priv is still intact after it
returns. We will handle.
