* [bug report] net/mxl5e: Add change profile method
@ 2021-02-05 12:45 Dan Carpenter
2021-02-10 6:12 ` Saeed Mahameed
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-02-05 12:45 UTC (permalink / raw)
To: saeedm, Feras Daoud; +Cc: linux-rdma
Hello Saeed Mahameed and Feras Daoud,
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
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.
5659
5660 err = mlx5e_netdev_attach_profile(priv, new_profile, new_ppriv);
5661 if (err) { /* roll back to original profile */
5662 netdev_warn(priv->netdev, "%s: new profile init failed, %d\n",
5663 __func__, err);
5664 goto rollback;
5665 }
5666
5667 return 0;
5668
5669 rollback:
5670 rollback_err = mlx5e_netdev_attach_profile(priv, orig_profile, orig_ppriv);
5671 if (rollback_err) {
5672 netdev_err(priv->netdev,
5673 "%s: failed to rollback to orig profile, %d\n",
5674 __func__, rollback_err);
5675 }
5676 return err;
5677 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] net/mxl5e: Add change profile method
2021-02-05 12:45 [bug report] net/mxl5e: Add change profile method Dan Carpenter
@ 2021-02-10 6:12 ` Saeed Mahameed
2021-02-10 7:02 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Saeed Mahameed @ 2021-02-10 6:12 UTC (permalink / raw)
To: Roi Dayan, dan.carpenter@oracle.com, Feras Daoud
Cc: linux-rdma@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] net/mxl5e: Add change profile method
2021-02-10 6:12 ` Saeed Mahameed
@ 2021-02-10 7:02 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-02-10 7:02 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: Roi Dayan, Feras Daoud, linux-rdma@vger.kernel.org
On Wed, Feb 10, 2021 at 06:12:58AM +0000, Saeed Mahameed wrote:
> 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 ?
>
This is a Smatch check but you have to have the cross function database
built. And the code is from 2018 so something must have improved in how
the cross function DB is working recently for it to generate this
warning... I'm not sure exactly what I improved or if I have pushed
that change yet...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-10 7:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-05 12:45 [bug report] net/mxl5e: Add change profile method Dan Carpenter
2021-02-10 6:12 ` Saeed Mahameed
2021-02-10 7:02 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox