diff mbox series

drm/msm/dp: Free resources after unregistering them

Message ID 20230612220259.1884381-1-quic_bjorande@quicinc.com
State New
Headers
Series drm/msm/dp: Free resources after unregistering them |

Commit Message

Bjorn Andersson June 12, 2023, 10:02 p.m. UTC 
  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(-)

Comments

Dmitry Baryshkov June 15, 2023, 10:17 a.m. UTC | #1 
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;
Dmitry Baryshkov June 15, 2023, 11:31 a.m. UTC | #2 
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,
diff mbox series

Patch

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;