Revert "drm/core: Preserve the framebuffer after removing it."

Submitted by Hans de Goede on May 19, 2016, 3:15 p.m.

Details

Message ID 1463670901-31153-1-git-send-email-hdegoede@redhat.com
State New
Headers show
Series "Revert "drm/core: Preserve the framebuffer after removing it."" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Hans de Goede May 19, 2016, 3:15 p.m.
This reverts commit 13803132818c ("drm/core: Preserve the framebuffer
after removing it.").

This commit assumes that going through drm_framebuffer_remove() is not
necessary because "the fbdev code or any system compositor should restore
the planes anyway so there's no need to do it twice". But this is not true
for secondary GPUs / slave outputs.

This revert fixes the dgpu no longer suspending on laptops with
switchable graphics after an external output which is connected
to the dgpu has been used.

And it fixes the WARN_ON to detect drm_framebuffer leaks in
drm_mode_config_cleanup() triggering when unplugging an USB displaylink
device; or when rmmod-ing the secondary GPU kms driver on laptops with
switchable-graphics.

Also this part of the reverted commit's commit-msg: "The old fb_id is
zero'd, so there's no danger of being able to restore the fb from fb_id."
is no longer true, the zero-ing does not happen until drm_framebuffer_free
gets called, which does not happen until the last ref is dropped, so
if a crtc's primary->fb is still pointing to this fb, the id will not
get zero'd and userspace could potentially gain access to the removed
fb again.

Cc: stable@vger.kenrel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_crtc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e08f962..15f5cd7 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3474,7 +3474,7 @@  int drm_mode_rmfb(struct drm_device *dev,
 	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
 
-	drm_framebuffer_unreference(fb);
+	drm_framebuffer_remove(fb);
 
 	return 0;
 
@@ -3656,8 +3656,8 @@  void drm_fb_release(struct drm_file *priv)
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
 		list_del_init(&fb->filp_head);
 
-		/* This drops the fpriv->fbs reference. */
-		drm_framebuffer_unreference(fb);
+		/* This will also drop the fpriv->fbs reference. */
+		drm_framebuffer_remove(fb);
 	}
 }
 

Comments

On Thu, May 19, 2016 at 5:15 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> This reverts commit 13803132818c ("drm/core: Preserve the framebuffer
> after removing it.").
>
> This commit assumes that going through drm_framebuffer_remove() is not
> necessary because "the fbdev code or any system compositor should restore
> the planes anyway so there's no need to do it twice". But this is not true
> for secondary GPUs / slave outputs.
>
> This revert fixes the dgpu no longer suspending on laptops with
> switchable graphics after an external output which is connected
> to the dgpu has been used.
>
> And it fixes the WARN_ON to detect drm_framebuffer leaks in
> drm_mode_config_cleanup() triggering when unplugging an USB displaylink
> device; or when rmmod-ing the secondary GPU kms driver on laptops with
> switchable-graphics.
>
> Also this part of the reverted commit's commit-msg: "The old fb_id is
> zero'd, so there's no danger of being able to restore the fb from fb_id."
> is no longer true, the zero-ing does not happen until drm_framebuffer_free
> gets called, which does not happen until the last ref is dropped, so
> if a crtc's primary->fb is still pointing to this fb, the id will not
> get zero'd and userspace could potentially gain access to the removed
> fb again.
>
> Cc: stable@vger.kenrel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

We have a proper fix in drm-next:

commit f2d580b9a8149735cbc4b59c4a8df60173658140
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Wed May 4 14:38:26 2016 +0200

    drm/core: Do not preserve framebuffer on rmfb, v4.

That thing took forever to get merged since no one seemed to have
cared and bothered with a tested-by. But it's on its way to stable
kernels now.
-Daniel
Hi,

On 19-05-16 17:33, Daniel Vetter wrote:
> On Thu, May 19, 2016 at 5:15 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> This reverts commit 13803132818c ("drm/core: Preserve the framebuffer
>> after removing it.").
>>
>> This commit assumes that going through drm_framebuffer_remove() is not
>> necessary because "the fbdev code or any system compositor should restore
>> the planes anyway so there's no need to do it twice". But this is not true
>> for secondary GPUs / slave outputs.
>>
>> This revert fixes the dgpu no longer suspending on laptops with
>> switchable graphics after an external output which is connected
>> to the dgpu has been used.
>>
>> And it fixes the WARN_ON to detect drm_framebuffer leaks in
>> drm_mode_config_cleanup() triggering when unplugging an USB displaylink
>> device; or when rmmod-ing the secondary GPU kms driver on laptops with
>> switchable-graphics.
>>
>> Also this part of the reverted commit's commit-msg: "The old fb_id is
>> zero'd, so there's no danger of being able to restore the fb from fb_id."
>> is no longer true, the zero-ing does not happen until drm_framebuffer_free
>> gets called, which does not happen until the last ref is dropped, so
>> if a crtc's primary->fb is still pointing to this fb, the id will not
>> get zero'd and userspace could potentially gain access to the removed
>> fb again.
>>
>> Cc: stable@vger.kenrel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> We have a proper fix in drm-next:
>
> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Wed May 4 14:38:26 2016 +0200
>
>     drm/core: Do not preserve framebuffer on rmfb, v4.
>
> That thing took forever to get merged since no one seemed to have
> cared and bothered with a tested-by. But it's on its way to stable
> kernels now.

Ah, excellent, the important thing is that this gets fixed :)

Regards,

Hans