| Message ID | 20231117091655.872426-2-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Convert to platform remove callback | expand |
On 17/11/2023 11:16, Uwe Kleine-König wrote: > Returning early from .remove() with an error code still results in the > driver unbinding the device. So the driver core ignores the returned error > code and the resources that were not freed are never catched up. In > combination with devm this also often results in use-after-free bugs. > > In case of the am65-cpsw-nuss driver there is an error path, but it's never > taken because am65_cpts_resume() never fails (which however might be > another problem). Still make this explicit and drop the early return in > exchange for an error message (that is more useful than the error the > driver core emits when .remove() returns non-zero). > > This prepares changing am65_cpsw_nuss_remove() to return void. > > Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > index ece9f8df98ae..960cb3fa0754 100644 > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev) > > common = dev_get_drvdata(dev); > > - ret = pm_runtime_resume_and_get(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); Unrelated to this patch but I see pm_runtime_resume_and_get() used at multiple places in this driver. Would it be wise to replace them all with pm_runtime_get_sync()? > if (ret < 0) > - return ret; > + /* am65_cpts_resume() doesn't fail, so handling ret < 0 is only > + * for the sake of completeness. > + */ > + dev_err(dev, "runtime resume failed (%pe)\n", ERR_PTR(ret)); > > am65_cpsw_unregister_devlink(common); > am65_cpsw_unregister_notifiers(common);
Hello, On Fri, Nov 17, 2023 at 11:51:56AM +0200, Roger Quadros wrote: > On 17/11/2023 11:16, Uwe Kleine-König wrote: > > Returning early from .remove() with an error code still results in the > > driver unbinding the device. So the driver core ignores the returned error > > code and the resources that were not freed are never catched up. In > > combination with devm this also often results in use-after-free bugs. > > > > In case of the am65-cpsw-nuss driver there is an error path, but it's never > > taken because am65_cpts_resume() never fails (which however might be > > another problem). Still make this explicit and drop the early return in > > exchange for an error message (that is more useful than the error the > > driver core emits when .remove() returns non-zero). > > > > This prepares changing am65_cpsw_nuss_remove() to return void. > > > > Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > > index ece9f8df98ae..960cb3fa0754 100644 > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > > @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev) > > > > common = dev_get_drvdata(dev); > > > > - ret = pm_runtime_resume_and_get(&pdev->dev); > > + ret = pm_runtime_get_sync(&pdev->dev); > > Unrelated to this patch but I see pm_runtime_resume_and_get() > used at multiple places in this driver. > > Would it be wise to replace them all with pm_runtime_get_sync()? No, in general pm_runtime_resume_and_get() is the variant that is easier to use because it drops the usage count in the error path. Here however the error isn't simply propagated and so pm_runtime_get_sync() is better. Best regards Uwe
On 17/11/2023 11:16, Uwe Kleine-König wrote: > Returning early from .remove() with an error code still results in the > driver unbinding the device. So the driver core ignores the returned error > code and the resources that were not freed are never catched up. In > combination with devm this also often results in use-after-free bugs. > > In case of the am65-cpsw-nuss driver there is an error path, but it's never > taken because am65_cpts_resume() never fails (which however might be > another problem). Still make this explicit and drop the early return in > exchange for an error message (that is more useful than the error the > driver core emits when .remove() returns non-zero). > > This prepares changing am65_cpsw_nuss_remove() to return void. > > Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > index ece9f8df98ae..960cb3fa0754 100644 > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev) > > common = dev_get_drvdata(dev); > > - ret = pm_runtime_resume_and_get(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); > if (ret < 0) > - return ret; > + /* am65_cpts_resume() doesn't fail, so handling ret < 0 is only > + * for the sake of completeness. > + */ > + dev_err(dev, "runtime resume failed (%pe)\n", ERR_PTR(ret)); If the pm_runtime_get_sync() call fails then am65_cpts_release()->am65_cpts_disable() will cause a bus error as we are accessing the module with its power domain turned off. So, the am65_cpts_disable() call needs to be avoided in the pm_runtime_get_sync() failure path. > > am65_cpsw_unregister_devlink(common); > am65_cpsw_unregister_notifiers(common);
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index ece9f8df98ae..960cb3fa0754 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev) common = dev_get_drvdata(dev); - ret = pm_runtime_resume_and_get(&pdev->dev); + ret = pm_runtime_get_sync(&pdev->dev); if (ret < 0) - return ret; + /* am65_cpts_resume() doesn't fail, so handling ret < 0 is only + * for the sake of completeness. + */ + dev_err(dev, "runtime resume failed (%pe)\n", ERR_PTR(ret)); am65_cpsw_unregister_devlink(common); am65_cpsw_unregister_notifiers(common);
Returning early from .remove() with an error code still results in the driver unbinding the device. So the driver core ignores the returned error code and the resources that were not freed are never catched up. In combination with devm this also often results in use-after-free bugs. In case of the am65-cpsw-nuss driver there is an error path, but it's never taken because am65_cpts_resume() never fails (which however might be another problem). Still make this explicit and drop the early return in exchange for an error message (that is more useful than the error the driver core emits when .remove() returns non-zero). This prepares changing am65_cpsw_nuss_remove() to return void. Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)