| Message ID | 20230201101559.15529-16-johan+linaro@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | interconnect: fix racy provider registration | expand |
On 01/02/2023 11:15, Johan Hovold wrote: > The current interconnect provider registration interface is inherently > racy as nodes are not added until the after adding the provider. This > can specifically cause racing DT lookups to trigger a NULL-pointer > deference when either a NULL pointer or not fully initialised node is > returned from exynos_generic_icc_xlate(). > > Switch to using the new API where the provider is not registered until > after it has been fully initialised. > > Fixes: 2f95b9d5cf0b ("interconnect: Add generic interconnect driver for Exynos SoCs") > Cc: stable@vger.kernel.org # 5.11 > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/interconnect/samsung/exynos.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c > index e70665899482..72e42603823b 100644 > --- a/drivers/interconnect/samsung/exynos.c > +++ b/drivers/interconnect/samsung/exynos.c > @@ -98,12 +98,13 @@ static int exynos_generic_icc_remove(struct platform_device *pdev) > struct exynos_icc_priv *priv = platform_get_drvdata(pdev); > struct icc_node *parent_node, *node = priv->node; > > + icc_provider_deregister(&priv->provider); > + > parent_node = exynos_icc_get_parent(priv->dev->parent->of_node); > if (parent_node && !IS_ERR(parent_node)) > icc_link_destroy(node, parent_node); > > icc_nodes_remove(&priv->provider); > - icc_provider_del(&priv->provider); > > return 0; > } > @@ -132,15 +133,11 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) > provider->inter_set = true; > provider->data = priv; > > - ret = icc_provider_add(provider); > - if (ret < 0) > - return ret; > + icc_provider_init(provider); > > icc_node = icc_node_create(pdev->id); > - if (IS_ERR(icc_node)) { > - ret = PTR_ERR(icc_node); > - goto err_prov_del; > - } > + if (IS_ERR(icc_node)) > + return PTR_ERR(icc_node); > > priv->node = icc_node; > icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn", > @@ -171,14 +168,17 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) > goto err_pmqos_del; > } > > + ret = icc_provider_register(provider); > + if (ret < 0) > + goto err_pmqos_del; If I understand correctly there is no need for icc_link_destroy() in error path here, right? Even in case of probe retry (defer or whatever reason) - the link will be removed with icc_nodes_remove()? Best regards, Krzysztof
diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c index e70665899482..72e42603823b 100644 --- a/drivers/interconnect/samsung/exynos.c +++ b/drivers/interconnect/samsung/exynos.c @@ -98,12 +98,13 @@ static int exynos_generic_icc_remove(struct platform_device *pdev) struct exynos_icc_priv *priv = platform_get_drvdata(pdev); struct icc_node *parent_node, *node = priv->node; + icc_provider_deregister(&priv->provider); + parent_node = exynos_icc_get_parent(priv->dev->parent->of_node); if (parent_node && !IS_ERR(parent_node)) icc_link_destroy(node, parent_node); icc_nodes_remove(&priv->provider); - icc_provider_del(&priv->provider); return 0; } @@ -132,15 +133,11 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) provider->inter_set = true; provider->data = priv; - ret = icc_provider_add(provider); - if (ret < 0) - return ret; + icc_provider_init(provider); icc_node = icc_node_create(pdev->id); - if (IS_ERR(icc_node)) { - ret = PTR_ERR(icc_node); - goto err_prov_del; - } + if (IS_ERR(icc_node)) + return PTR_ERR(icc_node); priv->node = icc_node; icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn", @@ -171,14 +168,17 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) goto err_pmqos_del; } + ret = icc_provider_register(provider); + if (ret < 0) + goto err_pmqos_del; + return 0; err_pmqos_del: dev_pm_qos_remove_request(&priv->qos_req); err_node_del: icc_nodes_remove(provider); -err_prov_del: - icc_provider_del(provider); + return ret; }
The current interconnect provider registration interface is inherently racy as nodes are not added until the after adding the provider. This can specifically cause racing DT lookups to trigger a NULL-pointer deference when either a NULL pointer or not fully initialised node is returned from exynos_generic_icc_xlate(). Switch to using the new API where the provider is not registered until after it has been fully initialised. Fixes: 2f95b9d5cf0b ("interconnect: Add generic interconnect driver for Exynos SoCs") Cc: stable@vger.kernel.org # 5.11 Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/interconnect/samsung/exynos.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)