| Message ID | 20220610092924.754942-33-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Fix hotplug for vc4 | expand |
Am 10.06.22 um 11:28 schrieb Maxime Ripard: > The vc4_dsi structure is currently allocated through a device-managed > allocation. This can lead to use-after-free issues however in the unbinding > path since the DRM entities will stick around, but the underlying structure > has been freed. > > However, we can't just fix it by using a DRM-managed allocation like we did > for the other drivers since the DSI case is a bit more intricate. > > Indeed, the structure will be allocated at probe time, when we don't have a > DRM device yet, to be able to register the DSI bus driver. We will then > reuse it at bind time to register our KMS entities in the framework. > > In order to work around both constraints, we can use a kref to track the > users of the structure (DSI host, and KMS), and then put our structure when > the DSI host will have been unregistered, and through a DRM-managed action > that will execute once we won't need the KMS entities anymore. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_dsi.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > index 10533a2a41b3..282537f27b8e 100644 > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > @@ -510,6 +510,8 @@ struct vc4_dsi { > struct vc4_encoder encoder; > struct mipi_dsi_host dsi_host; > > + struct kref kref; > + > struct platform_device *pdev; > > struct drm_bridge *bridge; > @@ -1479,6 +1481,15 @@ vc4_dsi_init_phy_clocks(struct vc4_dsi *dsi) > dsi->clk_onecell); > } > > +static void vc4_dsi_release(struct kref *kref); > + > +static void vc4_dsi_put(struct drm_device *drm, void *ptr) > +{ > + struct vc4_dsi *dsi = ptr; > + > + kref_put(&dsi->kref, &vc4_dsi_release); > +} > + > static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -1488,6 +1499,12 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) > dma_cap_mask_t dma_mask; > int ret; > > + kref_get(&dsi->kref); > + > + ret = drmm_add_action_or_reset(drm, vc4_dsi_put, dsi); > + if (ret) > + return ret; > + > dsi->variant = of_device_get_match_data(dev); > > INIT_LIST_HEAD(&dsi->bridge_chain); > @@ -1642,16 +1659,25 @@ static const struct component_ops vc4_dsi_ops = { > .unbind = vc4_dsi_unbind, > }; > > +static void vc4_dsi_release(struct kref *kref) > +{ > + struct vc4_dsi *dsi = > + container_of(kref, struct vc4_dsi, kref); > + > + kfree(dsi); > +} > + > static int vc4_dsi_dev_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct vc4_dsi *dsi; > > - dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); > + dsi = kzalloc(sizeof(*dsi), GFP_KERNEL); > if (!dsi) > return -ENOMEM; > dev_set_drvdata(dev, dsi); > > + kref_init(&dsi->kref); > dsi->pdev = pdev; > dsi->dsi_host.ops = &vc4_dsi_host_ops; > dsi->dsi_host.dev = dev; > @@ -1666,6 +1692,7 @@ static int vc4_dsi_dev_remove(struct platform_device *pdev) > struct vc4_dsi *dsi = dev_get_drvdata(dev); > > mipi_dsi_host_unregister(&dsi->dsi_host); > + kref_put(&dsi->kref, &vc4_dsi_release); Maybe vc4_dsi_put() ? > return 0; > } >
Am 20.06.22 um 12:55 schrieb Thomas Zimmermann: > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard: >> The vc4_dsi structure is currently allocated through a device-managed >> allocation. This can lead to use-after-free issues however in the >> unbinding >> path since the DRM entities will stick around, but the underlying >> structure >> has been freed. >> >> However, we can't just fix it by using a DRM-managed allocation like >> we did >> for the other drivers since the DSI case is a bit more intricate. >> >> Indeed, the structure will be allocated at probe time, when we don't >> have a >> DRM device yet, to be able to register the DSI bus driver. We will then >> reuse it at bind time to register our KMS entities in the framework. >> >> In order to work around both constraints, we can use a kref to track the >> users of the structure (DSI host, and KMS), and then put our structure >> when >> the DSI host will have been unregistered, and through a DRM-managed >> action >> that will execute once we won't need the KMS entities anymore. >> >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >> --- >> drivers/gpu/drm/vc4/vc4_dsi.c | 29 ++++++++++++++++++++++++++++- >> 1 file changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c >> b/drivers/gpu/drm/vc4/vc4_dsi.c >> index 10533a2a41b3..282537f27b8e 100644 >> --- a/drivers/gpu/drm/vc4/vc4_dsi.c >> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c >> @@ -510,6 +510,8 @@ struct vc4_dsi { >> struct vc4_encoder encoder; >> struct mipi_dsi_host dsi_host; >> + struct kref kref; >> + >> struct platform_device *pdev; >> struct drm_bridge *bridge; >> @@ -1479,6 +1481,15 @@ vc4_dsi_init_phy_clocks(struct vc4_dsi *dsi) >> dsi->clk_onecell); >> } >> +static void vc4_dsi_release(struct kref *kref); >> + >> +static void vc4_dsi_put(struct drm_device *drm, void *ptr) >> +{ >> + struct vc4_dsi *dsi = ptr; >> + >> + kref_put(&dsi->kref, &vc4_dsi_release); >> +} >> + >> static int vc4_dsi_bind(struct device *dev, struct device *master, >> void *data) >> { >> struct platform_device *pdev = to_platform_device(dev); >> @@ -1488,6 +1499,12 @@ static int vc4_dsi_bind(struct device *dev, >> struct device *master, void *data) >> dma_cap_mask_t dma_mask; >> int ret; >> + kref_get(&dsi->kref); >> + >> + ret = drmm_add_action_or_reset(drm, vc4_dsi_put, dsi); >> + if (ret) >> + return ret; >> + >> dsi->variant = of_device_get_match_data(dev); >> INIT_LIST_HEAD(&dsi->bridge_chain); >> @@ -1642,16 +1659,25 @@ static const struct component_ops vc4_dsi_ops = { >> .unbind = vc4_dsi_unbind, >> }; >> +static void vc4_dsi_release(struct kref *kref) >> +{ >> + struct vc4_dsi *dsi = >> + container_of(kref, struct vc4_dsi, kref); >> + >> + kfree(dsi); >> +} >> + >> static int vc4_dsi_dev_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct vc4_dsi *dsi; >> - dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); >> + dsi = kzalloc(sizeof(*dsi), GFP_KERNEL); >> if (!dsi) >> return -ENOMEM; >> dev_set_drvdata(dev, dsi); >> + kref_init(&dsi->kref); >> dsi->pdev = pdev; >> dsi->dsi_host.ops = &vc4_dsi_host_ops; >> dsi->dsi_host.dev = dev; >> @@ -1666,6 +1692,7 @@ static int vc4_dsi_dev_remove(struct >> platform_device *pdev) >> struct vc4_dsi *dsi = dev_get_drvdata(dev); >> mipi_dsi_host_unregister(&dsi->dsi_host); >> + kref_put(&dsi->kref, &vc4_dsi_release); > > Maybe vc4_dsi_put() ? No, wait. That's the release function. It's confusing. I'd rename vc4_dsi_put() to vc4_dsi_release_action() and wrap those kref_get/kref_put calls in small helpers named vc4_dsi_get/vc4_dsi_put. That's more aligned to the usual naming conventions, I'd say. Best regards Thomas > >> return 0; >> } >
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 10533a2a41b3..282537f27b8e 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -510,6 +510,8 @@ struct vc4_dsi { struct vc4_encoder encoder; struct mipi_dsi_host dsi_host; + struct kref kref; + struct platform_device *pdev; struct drm_bridge *bridge; @@ -1479,6 +1481,15 @@ vc4_dsi_init_phy_clocks(struct vc4_dsi *dsi) dsi->clk_onecell); } +static void vc4_dsi_release(struct kref *kref); + +static void vc4_dsi_put(struct drm_device *drm, void *ptr) +{ + struct vc4_dsi *dsi = ptr; + + kref_put(&dsi->kref, &vc4_dsi_release); +} + static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); @@ -1488,6 +1499,12 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) dma_cap_mask_t dma_mask; int ret; + kref_get(&dsi->kref); + + ret = drmm_add_action_or_reset(drm, vc4_dsi_put, dsi); + if (ret) + return ret; + dsi->variant = of_device_get_match_data(dev); INIT_LIST_HEAD(&dsi->bridge_chain); @@ -1642,16 +1659,25 @@ static const struct component_ops vc4_dsi_ops = { .unbind = vc4_dsi_unbind, }; +static void vc4_dsi_release(struct kref *kref) +{ + struct vc4_dsi *dsi = + container_of(kref, struct vc4_dsi, kref); + + kfree(dsi); +} + static int vc4_dsi_dev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct vc4_dsi *dsi; - dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); + dsi = kzalloc(sizeof(*dsi), GFP_KERNEL); if (!dsi) return -ENOMEM; dev_set_drvdata(dev, dsi); + kref_init(&dsi->kref); dsi->pdev = pdev; dsi->dsi_host.ops = &vc4_dsi_host_ops; dsi->dsi_host.dev = dev; @@ -1666,6 +1692,7 @@ static int vc4_dsi_dev_remove(struct platform_device *pdev) struct vc4_dsi *dsi = dev_get_drvdata(dev); mipi_dsi_host_unregister(&dsi->dsi_host); + kref_put(&dsi->kref, &vc4_dsi_release); return 0; }
The vc4_dsi structure is currently allocated through a device-managed allocation. This can lead to use-after-free issues however in the unbinding path since the DRM entities will stick around, but the underlying structure has been freed. However, we can't just fix it by using a DRM-managed allocation like we did for the other drivers since the DSI case is a bit more intricate. Indeed, the structure will be allocated at probe time, when we don't have a DRM device yet, to be able to register the DSI bus driver. We will then reuse it at bind time to register our KMS entities in the framework. In order to work around both constraints, we can use a kref to track the users of the structure (DSI host, and KMS), and then put our structure when the DSI host will have been unregistered, and through a DRM-managed action that will execute once we won't need the KMS entities anymore. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_dsi.c | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)