diff mbox series

[2/2] drm/i915/fbdev: suspend HPD before fbdev unregistration

Message ID 20220623163813.1990801-3-andrzej.hajda@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: stop HPD workers before display driver unregister | expand

Commit Message

Andrzej Hajda June 23, 2022, 4:38 p.m. UTC 
HPD event after fbdev unregistration can cause registration of deferred
fbdev which will not be unregistered later, causing use-after-free.
To avoid it HPD handling should be suspended before fbdev unregistration.

It should fix following GPF:
[272.634530] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] PREEMPT SMP NOPTI
[272.634536] CPU: 0 PID: 6030 Comm: i915_selftest Tainted: G     U            5.18.0-rc5-CI_DRM_11603-g12dccf4f5eef+ #1
[272.634541] Hardware name: Intel Corporation Raptor Lake Client Platform/RPL-S ADP-S DDR5 UDIMM CRB, BIOS RPLSFWI1.R00.2397.A01.2109300731 09/30/2021
[272.634545] RIP: 0010:fb_do_apertures_overlap.part.14+0x26/0x60
...
[272.634582] Call Trace:
[272.634583]  <TASK>
[272.634585]  do_remove_conflicting_framebuffers+0x59/0xa0
[272.634589]  remove_conflicting_framebuffers+0x2d/0xc0
[272.634592]  remove_conflicting_pci_framebuffers+0xc8/0x110
[272.634595]  drm_aperture_remove_conflicting_pci_framebuffers+0x52/0x70
[272.634604]  i915_driver_probe+0x63a/0xdd0 [i915]

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5329
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5510
Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev.c | 44 +++++++++++-----------
 1 file changed, 23 insertions(+), 21 deletions(-)

Comments

Murthy, Arun R June 29, 2022, 5:38 a.m. UTC | #1 
>  void intel_fbdev_unregister(struct drm_i915_private *dev_priv)  {
>       struct intel_fbdev *ifbdev = dev_priv->fbdev; @@ -573,6 +594,8 @@
> void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
>       if (!ifbdev)
>               return;
>
> +     intel_fbdev_hpd_set_suspend(dev_priv,
> FBINFO_STATE_SUSPENDED);
> +

Instead of intel_fbdev_hpd_set_suspend(), will intel_fbdev_set_suspend() make more sense?
If intel_fbdev_set_suspend() is called, then the below cancel_work_sync() may not be required.

>       cancel_work_sync(&dev_priv->fbdev_suspend_work);
>       if (!current_is_async())
>               intel_fbdev_sync(ifbdev);

Thanks and Regards,
Arun R Murthy
--------------------
Andrzej Hajda June 29, 2022, 8:22 a.m. UTC | #2 
Thanks for comments,


On 29.06.2022 07:38, Murthy, Arun R wrote:
>>   void intel_fbdev_unregister(struct drm_i915_private *dev_priv)  {
>>        struct intel_fbdev *ifbdev = dev_priv->fbdev; @@ -573,6 +594,8 @@
>> void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
>>        if (!ifbdev)
>>                return;
>>
>> +     intel_fbdev_hpd_set_suspend(dev_priv,
>> FBINFO_STATE_SUSPENDED);
>> +
> 
> Instead of intel_fbdev_hpd_set_suspend(), will intel_fbdev_set_suspend() make more sense?
> If intel_fbdev_set_suspend() is called, then the below cancel_work_sync() may not be required.


It does more than I needed (calls drm_fb_helper_set_suspend), but it 
shouldn't hurt. I will try this approach.

Regards
Andrzej

> 
>>        cancel_work_sync(&dev_priv->fbdev_suspend_work);
>>        if (!current_is_async())
>>                intel_fbdev_sync(ifbdev);
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 221336178991f0..113d6a8d65c872 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -566,6 +566,27 @@  static void intel_fbdev_sync(struct intel_fbdev *ifbdev)
 	ifbdev->cookie = 0;
 }
 
+/* Suspends/resumes fbdev processing of incoming HPD events. When resuming HPD
+ * processing, fbdev will perform a full connector reprobe if a hotplug event
+ * was received while HPD was suspended.
+ */
+static void intel_fbdev_hpd_set_suspend(struct drm_i915_private *i915, int state)
+{
+	struct intel_fbdev *ifbdev = i915->fbdev;
+	bool send_hpd = false;
+
+	mutex_lock(&ifbdev->hpd_lock);
+	ifbdev->hpd_suspended = state == FBINFO_STATE_SUSPENDED;
+	send_hpd = !ifbdev->hpd_suspended && ifbdev->hpd_waiting;
+	ifbdev->hpd_waiting = false;
+	mutex_unlock(&ifbdev->hpd_lock);
+
+	if (send_hpd) {
+		drm_dbg_kms(&i915->drm, "Handling delayed fbcon HPD event\n");
+		drm_fb_helper_hotplug_event(&ifbdev->helper);
+	}
+}
+
 void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
 {
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
@@ -573,6 +594,8 @@  void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
 	if (!ifbdev)
 		return;
 
+	intel_fbdev_hpd_set_suspend(dev_priv, FBINFO_STATE_SUSPENDED);
+
 	cancel_work_sync(&dev_priv->fbdev_suspend_work);
 	if (!current_is_async())
 		intel_fbdev_sync(ifbdev);
@@ -590,27 +613,6 @@  void intel_fbdev_fini(struct drm_i915_private *dev_priv)
 	intel_fbdev_destroy(ifbdev);
 }
 
-/* Suspends/resumes fbdev processing of incoming HPD events. When resuming HPD
- * processing, fbdev will perform a full connector reprobe if a hotplug event
- * was received while HPD was suspended.
- */
-static void intel_fbdev_hpd_set_suspend(struct drm_i915_private *i915, int state)
-{
-	struct intel_fbdev *ifbdev = i915->fbdev;
-	bool send_hpd = false;
-
-	mutex_lock(&ifbdev->hpd_lock);
-	ifbdev->hpd_suspended = state == FBINFO_STATE_SUSPENDED;
-	send_hpd = !ifbdev->hpd_suspended && ifbdev->hpd_waiting;
-	ifbdev->hpd_waiting = false;
-	mutex_unlock(&ifbdev->hpd_lock);
-
-	if (send_hpd) {
-		drm_dbg_kms(&i915->drm, "Handling delayed fbcon HPD event\n");
-		drm_fb_helper_hotplug_event(&ifbdev->helper);
-	}
-}
-
 void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);