The current code to unregister our CEC device needs to be undone manually when we remove the HDMI driver.
Since the CEC framework will allocate its main structure, and will defer its deallocation to when the last user will have closed it, we don't really need to take any particular measure to prevent any use-after-free and can thus use any managed action.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 95 ++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8f71f5a5e4ce..402bfde3b5fe 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2541,6 +2541,14 @@ static const struct cec_adap_ops vc4_hdmi_cec_adap_ops = { .adap_transmit = vc4_hdmi_cec_adap_transmit, };
+static void vc4_hdmi_cec_release(void *ptr) +{ + struct vc4_hdmi *vc4_hdmi = ptr; + + cec_unregister_adapter(vc4_hdmi->cec_adap); + vc4_hdmi->cec_adap = NULL; +} + static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { struct cec_connector_info conn_info; @@ -2576,75 +2584,75 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) vc4_hdmi_cec_update_clk_div(vc4_hdmi);
if (vc4_hdmi->variant->external_irq_controller) { - ret = request_threaded_irq(platform_get_irq_byname(pdev, "cec-rx"), - vc4_cec_irq_handler_rx_bare, - vc4_cec_irq_handler_rx_thread, 0, - "vc4 hdmi cec rx", vc4_hdmi); + ret = devm_request_threaded_irq(dev, platform_get_irq_byname(pdev, "cec-rx"), + vc4_cec_irq_handler_rx_bare, + vc4_cec_irq_handler_rx_thread, 0, + "vc4 hdmi cec rx", vc4_hdmi); if (ret) goto err_delete_cec_adap;
- ret = request_threaded_irq(platform_get_irq_byname(pdev, "cec-tx"), - vc4_cec_irq_handler_tx_bare, - vc4_cec_irq_handler_tx_thread, 0, - "vc4 hdmi cec tx", vc4_hdmi); + ret = devm_request_threaded_irq(dev, platform_get_irq_byname(pdev, "cec-tx"), + vc4_cec_irq_handler_tx_bare, + vc4_cec_irq_handler_tx_thread, 0, + "vc4 hdmi cec tx", vc4_hdmi); if (ret) - goto err_remove_cec_rx_handler; + goto err_delete_cec_adap; } else { spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
- ret = request_threaded_irq(platform_get_irq(pdev, 0), - vc4_cec_irq_handler, - vc4_cec_irq_handler_thread, 0, - "vc4 hdmi cec", vc4_hdmi); + ret = devm_request_threaded_irq(dev, platform_get_irq(pdev, 0), + vc4_cec_irq_handler, + vc4_cec_irq_handler_thread, 0, + "vc4 hdmi cec", vc4_hdmi); if (ret) goto err_delete_cec_adap; }
ret = cec_register_adapter(vc4_hdmi->cec_adap, &pdev->dev); if (ret < 0) - goto err_remove_handlers; + goto err_delete_cec_adap; + + /* + * NOTE: Strictly speaking, we should probably use a DRM-managed + * registration there to avoid removing the CEC adapter by the + * time the DRM driver doesn't have any user anymore. + * + * However, the CEC framework already cleans up the CEC adapter + * only when the last user has closed its file descriptor, so we + * don't need to handle it in DRM. + * + * By the time the device-managed hook is executed, we will give + * up our reference to the CEC adapter and therefore don't + * really care when it's actually freed. + * + * There's still a problematic sequence: if we unregister our + * CEC adapter, but the userspace keeps a handle on the CEC + * adapter but not the DRM device for some reason. In such a + * case, our vc4_hdmi structure will be freed, but the + * cec_adapter structure will have a dangling pointer to what + * used to be our HDMI controller. If we get a CEC call at that + * moment, we could end up with a use-after-free. Fortunately, + * the CEC framework already handles this too, by calling + * cec_is_registered() in cec_ioctl() and cec_poll(). + */ + ret = devm_add_action_or_reset(dev, vc4_hdmi_cec_release, vc4_hdmi); + if (ret) + return ret;
return 0;
-err_remove_handlers: - if (vc4_hdmi->variant->external_irq_controller) - free_irq(platform_get_irq_byname(pdev, "cec-tx"), vc4_hdmi); - else - free_irq(platform_get_irq(pdev, 0), vc4_hdmi); - -err_remove_cec_rx_handler: - if (vc4_hdmi->variant->external_irq_controller) - free_irq(platform_get_irq_byname(pdev, "cec-rx"), vc4_hdmi); - err_delete_cec_adap: cec_delete_adapter(vc4_hdmi->cec_adap);
return ret; } - -static void vc4_hdmi_cec_exit(struct vc4_hdmi *vc4_hdmi) -{ - struct platform_device *pdev = vc4_hdmi->pdev; - - if (vc4_hdmi->variant->external_irq_controller) { - free_irq(platform_get_irq_byname(pdev, "cec-rx"), vc4_hdmi); - free_irq(platform_get_irq_byname(pdev, "cec-tx"), vc4_hdmi); - } else { - free_irq(platform_get_irq(pdev, 0), vc4_hdmi); - } - - cec_unregister_adapter(vc4_hdmi->cec_adap); -} #else static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) { return 0; } - -static void vc4_hdmi_cec_exit(struct vc4_hdmi *vc4_hdmi) {}; - #endif
static int vc4_hdmi_build_regset(struct vc4_hdmi *vc4_hdmi, @@ -2967,7 +2975,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
ret = vc4_hdmi_audio_init(vc4_hdmi); if (ret) - goto err_free_cec; + goto err_free_hotplug;
vc4_debugfs_add_file(drm, variant->debugfs_name, vc4_hdmi_debugfs_regs, @@ -2977,8 +2985,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
return 0;
-err_free_cec: - vc4_hdmi_cec_exit(vc4_hdmi); err_free_hotplug: vc4_hdmi_hotplug_exit(vc4_hdmi); err_put_runtime_pm: @@ -3019,7 +3025,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, kfree(vc4_hdmi->hdmi_regset.regs); kfree(vc4_hdmi->hd_regset.regs);
- vc4_hdmi_cec_exit(vc4_hdmi); vc4_hdmi_hotplug_exit(vc4_hdmi);
pm_runtime_disable(dev);