[PATCH 32/64] drm/vc4: dsi: Fix the driver structure lifetime

[Thomas Zimmermann]

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 at 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;
>>   }
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220620/0ba7bcb0/attachment.sig>