[PATCH] drm/msm/dp: Free resources after unregistering them

[PATCH] drm/msm/dp: Free resources after unregistering them

[Bjorn Andersson]

The DP component's unbind operation walks through the submodules to
unregister and clean things up. But if the unbind happens because the DP
controller itself is being removed, all the memory for those submodules
has just been freed.

Change the order of these operations to avoid the many use-after-free
that otherwise happens in this code path.

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bbb0550a022b..ebc84b8fddf8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1337,9 +1337,9 @@ static int dp_display_remove(struct platform_device *pdev)
 {
 	struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
 
+	component_del(&pdev->dev, &dp_display_comp_ops);
 	dp_display_deinit_sub_modules(dp);
 
-	component_del(&pdev->dev, &dp_display_comp_ops);
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
-- 
2.25.1

Re: [PATCH] drm/msm/dp: Free resources after unregistering them

[Dmitry Baryshkov]

On 13/06/2023 01:02, Bjorn Andersson wrote:
> The DP component's unbind operation walks through the submodules to
> unregister and clean things up. But if the unbind happens because the DP
> controller itself is being removed, all the memory for those submodules
> has just been freed.
> 
> Change the order of these operations to avoid the many use-after-free
> that otherwise happens in this code path.
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bbb0550a022b..ebc84b8fddf8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1337,9 +1337,9 @@ static int dp_display_remove(struct platform_device *pdev)
>   {
>   	struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
>   
> +	component_del(&pdev->dev, &dp_display_comp_ops);
>   	dp_display_deinit_sub_modules(dp);
>   
> -	component_del(&pdev->dev, &dp_display_comp_ops);
>   	platform_set_drvdata(pdev, NULL);

This matches more or less the order in dp_display_probe().

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

A note for the possible followup: the driver initializes DP debugfs from 
dpu_kms (ugh) by calling msm_dp_debugfs_init() -> dp_debug_get(). I 
think that dp_debug_put() in dp_display_deinit_sub_modules() does not 
look correct.

>   
>   	return 0;

-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm/dp: Free resources after unregistering them

[Dmitry Baryshkov]

On Mon, 12 Jun 2023 15:02:59 -0700, Bjorn Andersson wrote:
> The DP component's unbind operation walks through the submodules to
> unregister and clean things up. But if the unbind happens because the DP
> controller itself is being removed, all the memory for those submodules
> has just been freed.
> 
> Change the order of these operations to avoid the many use-after-free
> that otherwise happens in this code path.
> 
> [...]

Applied, thanks!

[1/1] drm/msm/dp: Free resources after unregistering them
      https://gitlab.freedesktop.org/lumag/msm/-/commit/fa0048a4b1fa

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>