FAILED: patch "[PATCH] drm/core: Do not preserve framebuffer on rmfb, v4." failed to apply to 4.6-stable tree

FAILED: patch "[PATCH] drm/core: Do not preserve framebuffer on rmfb, v4." failed to apply to 4.6-stable tree

[gregkh]

The patch below does not apply to the 4.6-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

>From f2d580b9a8149735cbc4b59c4a8df60173658140 Mon Sep 17 00:00:00 2001
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date: Wed, 4 May 2016 14:38:26 +0200
Subject: [PATCH] drm/core: Do not preserve framebuffer on rmfb, v4.

It turns out that preserving framebuffers after the rmfb call breaks
vmwgfx userspace. This was originally introduced because it was thought
nobody relied on the behavior, but unfortunately it seems there are
exceptions.

drm_framebuffer_remove may fail with -EINTR now, so a straight revert
is impossible. There is no way to remove the framebuffer from the lists
and active planes without introducing a race because of the different
locking requirements. Instead call drm_framebuffer_remove from a
workqueue, which is unaffected by signals.

Changes since v1:
- Add comment.
Changes since v2:
- Add fastpath for refcount = 1. (danvet)
Changes since v3:
- Rebased.
- Restore lastclose framebuffer removal too.

Cc: stable@vger.kernel.org #v4.4+
Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
Testcase: kms_rmfb_basic
References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Tested-by: Thomas Hellstrom <thellstrom@vmware.com> #v3
Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/6c63ca37-0e7e-ac7f-a6d2-c7822e3d611f@linux.intel.com

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a9c0a4348322..70f9c682d144 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3462,6 +3462,24 @@ int drm_mode_addfb2(struct drm_device *dev,
 	return 0;
 }
 
+struct drm_mode_rmfb_work {
+	struct work_struct work;
+	struct list_head fbs;
+};
+
+static void drm_mode_rmfb_work_fn(struct work_struct *w)
+{
+	struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
+
+	while (!list_empty(&arg->fbs)) {
+		struct drm_framebuffer *fb =
+			list_first_entry(&arg->fbs, typeof(*fb), filp_head);
+
+		list_del_init(&fb->filp_head);
+		drm_framebuffer_remove(fb);
+	}
+}
+
 /**
  * drm_mode_rmfb - remove an FB from the configuration
  * @dev: drm device for the ioctl
@@ -3502,12 +3520,29 @@ int drm_mode_rmfb(struct drm_device *dev,
 	list_del_init(&fb->filp_head);
 	mutex_unlock(&file_priv->fbs_lock);
 
-	/* we now own the reference that was stored in the fbs list */
-	drm_framebuffer_unreference(fb);
-
 	/* drop the reference we picked up in framebuffer lookup */
 	drm_framebuffer_unreference(fb);
 
+	/*
+	 * we now own the reference that was stored in the fbs list
+	 *
+	 * drm_framebuffer_remove may fail with -EINTR on pending signals,
+	 * so run this in a separate stack as there's no way to correctly
+	 * handle this after the fb is already removed from the lookup table.
+	 */
+	if (drm_framebuffer_read_refcount(fb) > 1) {
+		struct drm_mode_rmfb_work arg;
+
+		INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
+		INIT_LIST_HEAD(&arg.fbs);
+		list_add_tail(&fb->filp_head, &arg.fbs);
+
+		schedule_work(&arg.work);
+		flush_work(&arg.work);
+		destroy_work_on_stack(&arg.work);
+	} else
+		drm_framebuffer_unreference(fb);
+
 	return 0;
 
 fail_unref:
@@ -3657,7 +3692,6 @@ out_err1:
 	return ret;
 }
 
-
 /**
  * drm_fb_release - remove and free the FBs on this file
  * @priv: drm file for the ioctl
@@ -3672,6 +3706,9 @@ out_err1:
 void drm_fb_release(struct drm_file *priv)
 {
 	struct drm_framebuffer *fb, *tfb;
+	struct drm_mode_rmfb_work arg;
+
+	INIT_LIST_HEAD(&arg.fbs);
 
 	/*
 	 * When the file gets released that means no one else can access the fb
@@ -3684,10 +3721,22 @@ void drm_fb_release(struct drm_file *priv)
 	 * at it any more.
 	 */
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-		list_del_init(&fb->filp_head);
+		if (drm_framebuffer_read_refcount(fb) > 1) {
+			list_move_tail(&fb->filp_head, &arg.fbs);
+		} else {
+			list_del_init(&fb->filp_head);
 
-		/* This drops the fpriv->fbs reference. */
-		drm_framebuffer_unreference(fb);
+			/* This drops the fpriv->fbs reference. */
+			drm_framebuffer_unreference(fb);
+		}
+	}
+
+	if (!list_empty(&arg.fbs)) {
+		INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
+
+		schedule_work(&arg.work);
+		flush_work(&arg.work);
+		destroy_work_on_stack(&arg.work);
 	}
 }
 

Re: FAILED: patch "[PATCH] drm/core: Do not preserve framebuffer on rmfb, v4." failed to apply to 4.6-stable tree

[Thomas Hellstrom]

Hi!

Anyone working on the backport?
I guess there already is a backported version, right?

Thanks,
Thomas


On 06/04/2016 10:48 PM, gregkh@linuxfoundation.org wrote:
> The patch below does not apply to the 4.6-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From f2d580b9a8149735cbc4b59c4a8df60173658140 Mon Sep 17 00:00:00 2001
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date: Wed, 4 May 2016 14:38:26 +0200
> Subject: [PATCH] drm/core: Do not preserve framebuffer on rmfb, v4.
>
> It turns out that preserving framebuffers after the rmfb call breaks
> vmwgfx userspace. This was originally introduced because it was thought
> nobody relied on the behavior, but unfortunately it seems there are
> exceptions.
>
> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> is impossible. There is no way to remove the framebuffer from the lists
> and active planes without introducing a race because of the different
> locking requirements. Instead call drm_framebuffer_remove from a
> workqueue, which is unaffected by signals.
>
> Changes since v1:
> - Add comment.
> Changes since v2:
> - Add fastpath for refcount = 1. (danvet)
> Changes since v3:
> - Rebased.
> - Restore lastclose framebuffer removal too.
>
> Cc: stable@vger.kernel.org #v4.4+
> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> Testcase: kms_rmfb_basic
> References: https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_dri-2Ddevel_2016-2DMarch_102876.html&d=CwIDAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=_TXmx4O5hjjbk-6pJRf-nlguy-dnLkXDcekA-7U-chk&s=LPwVyEguJf4N297fPwPsa53AcXIxjgdUs7C2t6UyRjc&e= 
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Tested-by: Thomas Hellstrom <thellstrom@vmware.com> #v3
> Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Link: https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.freedesktop.org_patch_msgid_6c63ca37-2D0e7e-2Dac7f-2Da6d2-2Dc7822e3d611f-40linux.intel.com&d=CwIDAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=_TXmx4O5hjjbk-6pJRf-nlguy-dnLkXDcekA-7U-chk&s=T9QASJXimZqvBbndSjYyAhr5-uIz7i_SBiyjr3ZBUig&e= 
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index a9c0a4348322..70f9c682d144 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3462,6 +3462,24 @@ int drm_mode_addfb2(struct drm_device *dev,
>  	return 0;
>  }
>  
> +struct drm_mode_rmfb_work {
> +	struct work_struct work;
> +	struct list_head fbs;
> +};
> +
> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> +{
> +	struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> +
> +	while (!list_empty(&arg->fbs)) {
> +		struct drm_framebuffer *fb =
> +			list_first_entry(&arg->fbs, typeof(*fb), filp_head);
> +
> +		list_del_init(&fb->filp_head);
> +		drm_framebuffer_remove(fb);
> +	}
> +}
> +
>  /**
>   * drm_mode_rmfb - remove an FB from the configuration
>   * @dev: drm device for the ioctl
> @@ -3502,12 +3520,29 @@ int drm_mode_rmfb(struct drm_device *dev,
>  	list_del_init(&fb->filp_head);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> -	/* we now own the reference that was stored in the fbs list */
> -	drm_framebuffer_unreference(fb);
> -
>  	/* drop the reference we picked up in framebuffer lookup */
>  	drm_framebuffer_unreference(fb);
>  
> +	/*
> +	 * we now own the reference that was stored in the fbs list
> +	 *
> +	 * drm_framebuffer_remove may fail with -EINTR on pending signals,
> +	 * so run this in a separate stack as there's no way to correctly
> +	 * handle this after the fb is already removed from the lookup table.
> +	 */
> +	if (drm_framebuffer_read_refcount(fb) > 1) {
> +		struct drm_mode_rmfb_work arg;
> +
> +		INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
> +		INIT_LIST_HEAD(&arg.fbs);
> +		list_add_tail(&fb->filp_head, &arg.fbs);
> +
> +		schedule_work(&arg.work);
> +		flush_work(&arg.work);
> +		destroy_work_on_stack(&arg.work);
> +	} else
> +		drm_framebuffer_unreference(fb);
> +
>  	return 0;
>  
>  fail_unref:
> @@ -3657,7 +3692,6 @@ out_err1:
>  	return ret;
>  }
>  
> -
>  /**
>   * drm_fb_release - remove and free the FBs on this file
>   * @priv: drm file for the ioctl
> @@ -3672,6 +3706,9 @@ out_err1:
>  void drm_fb_release(struct drm_file *priv)
>  {
>  	struct drm_framebuffer *fb, *tfb;
> +	struct drm_mode_rmfb_work arg;
> +
> +	INIT_LIST_HEAD(&arg.fbs);
>  
>  	/*
>  	 * When the file gets released that means no one else can access the fb
> @@ -3684,10 +3721,22 @@ void drm_fb_release(struct drm_file *priv)
>  	 * at it any more.
>  	 */
>  	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> -		list_del_init(&fb->filp_head);
> +		if (drm_framebuffer_read_refcount(fb) > 1) {
> +			list_move_tail(&fb->filp_head, &arg.fbs);
> +		} else {
> +			list_del_init(&fb->filp_head);
>  
> -		/* This drops the fpriv->fbs reference. */
> -		drm_framebuffer_unreference(fb);
> +			/* This drops the fpriv->fbs reference. */
> +			drm_framebuffer_unreference(fb);
> +		}
> +	}
> +
> +	if (!list_empty(&arg.fbs)) {
> +		INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
> +
> +		schedule_work(&arg.work);
> +		flush_work(&arg.work);
> +		destroy_work_on_stack(&arg.work);
>  	}
>  }
>  
>

Re: FAILED: patch "[PATCH] drm/core: Do not preserve framebuffer on rmfb, v4." failed to apply to 4.6-stable tree

[Greg KH]

On Thu, Jun 16, 2016 at 10:51:39AM +0200, Thomas Hellstrom wrote:
> Hi!
> 
> Anyone working on the backport?
> I guess there already is a backported version, right?

There is?  Where?

Re: FAILED: patch "[PATCH] drm/core: Do not preserve framebuffer on rmfb, v4." failed to apply to 4.6-stable tree

[Maarten Lankhorst]

Op 22-06-16 om 07:13 schreef Greg KH:
> On Thu, Jun 16, 2016 at 10:51:39AM +0200, Thomas Hellstrom wrote:
>> Hi!
>>
>> Anyone working on the backport?
>> I guess there already is a backported version, right?
> There is?  Where?
>
There was a private email from Hans de Goede to stable@vger.kernel.org, but also to you and daniel vetter.

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