On Wed, Nov 11, 2015 at 11:29:07AM +0100, Maarten Lankhorst wrote:

From: Maarten Lankhorst maarten.lankhorst@linux.intel.com

legacy_cursor_update was being set in restore_fbdev_mode_atomic which was probably unintended. Fix this by only setting it in the function that needs it.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

This oversight was introduced in

commit bbb1e52402b2a288b09ae37e8182599931c7e9df Author: Rob Clark robdclark@gmail.com Date: Tue Aug 25 15:35:58 2015 -0400

drm/fb-helper: atomic restore_fbdev_mode()...

With that added to the commit message:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_atomic_helper.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0c6f62168776..02d363ad35c9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1553,6 +1553,10 @@ retry: goto fail; }

• if (plane_state->crtc && (plane == plane->crtc->cursor))

• plane_state->state->legacy_cursor_update = true;
  

• ret = __drm_atomic_helper_disable_plane(plane, plane_state); if (ret != 0) goto fail;

@@ -1605,9 +1609,6 @@ int __drm_atomic_helper_disable_plane(struct drm_plane *plane, plane_state->src_h = 0; plane_state->src_w = 0;

• if (plane->crtc && (plane == plane->crtc->cursor))

• plane_state->state->legacy_cursor_update = true;
  

• return 0;

}

-- 2.1.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Nov 11, 2015 at 11:29:08AM +0100, Maarten Lankhorst wrote:

From: Maarten Lankhorst maarten.lankhorst@linux.intel.com

plane_mask should be cleared inside the retry loop, because it gets reset on every retry.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: stable@vger.kernel.org #v4.3

Nice catch, but a bit a terse commit message. We should add "Without this fix the plane->fb refcounting might get out of sync on retries, resulting in either leaked memory or use-after-free." With that:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_atomic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7bb3845d9974..0ac31b1ecb67 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1446,7 +1446,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, struct drm_plane *plane; struct drm_crtc *crtc; struct drm_crtc_state *crtc_state;

• unsigned plane_mask = 0;

• unsigned plane_mask; int ret = 0; unsigned int i, j;

@@ -1486,6 +1486,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);

retry:

• plane_mask = 0; copied_objs = 0; copied_props = 0;

-- 2.1.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Nov 11, 2015 at 11:29:09AM +0100, Maarten Lankhorst wrote:

From: Maarten Lankhorst maarten.lankhorst@linux.intel.com

This is useful for all the boilerplate code about cleaning old_fb.

drivers/gpu/drm/drm_atomic.c | 58 ++++++++++++++++++++++++++++++-------------- include/drm/drm_atomic.h | 3 +++ 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 0ac31b1ecb67..aeee083c7f95 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1432,6 +1432,45 @@ static int atomic_set_prop(struct drm_atomic_state *state, return ret; }

+/**

• • drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers.
• • @dev: drm device to check.
• • @plane_mask: plane mask for planes that were updated.
• • @ret: return value, can be -EDEADLK for a retry.
• • Before doing an update plane->old_fb is set to plane->fb,
• • but before dropping the locks old_fb needs to be set to NULL
• • and plane->fb updated. This is a common operation for each
• • atomic update, so this call is split off as a helper.
• */

+void drm_atomic_clean_old_fb(struct drm_device *dev,

• 	     unsigned plane_mask,
  

• 	     int ret)
  

A drm_atomic_prepare_old_fb inline to encapsulate the prep step would have been neat imo. Anyway: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

+{

• struct drm_plane *plane;
• /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping

• * locks (ie. while it is still safe to deref plane->state).  We
  

• * need to do this here because the driver entry points cannot
  

• * distinguish between legacy and atomic ioctls.
  

• */
  

• drm_for_each_plane_mask(plane, dev, plane_mask) {

• if (ret == 0) {
  

• 	struct drm_framebuffer *new_fb = plane->state->fb;
  

• 	if (new_fb)
  

• 		drm_framebuffer_reference(new_fb);
  

• 	plane->fb = new_fb;
  

• 	plane->crtc = plane->state->crtc;
  

• 	if (plane->old_fb)
  

• 		drm_framebuffer_unreference(plane->old_fb);
  

• }
  

• plane->old_fb = NULL;
  

• }

+} +EXPORT_SYMBOL(drm_atomic_clean_old_fb);

int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1577,24 +1616,7 @@ retry: }

out:

• /* if succeeded, fixup legacy plane crtc/fb ptrs before dropping

• * locks (ie. while it is still safe to deref plane->state).  We
  

• * need to do this here because the driver entry points cannot
  

• * distinguish between legacy and atomic ioctls.
  

• */
  

• drm_for_each_plane_mask(plane, dev, plane_mask) {

• if (ret == 0) {
  

• 	struct drm_framebuffer *new_fb = plane->state->fb;
  

• 	if (new_fb)
  

• 		drm_framebuffer_reference(new_fb);
  

• 	plane->fb = new_fb;
  

• 	plane->crtc = plane->state->crtc;
  

• 	if (plane->old_fb)
  

• 		drm_framebuffer_unreference(plane->old_fb);
  

• }
  

• plane->old_fb = NULL;
  

• }

• drm_atomic_clean_old_fb(dev, plane_mask, ret);

  if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) { /*

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index e67aeac2aee0..4b74c97d297a 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -136,6 +136,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,

void drm_atomic_legacy_backoff(struct drm_atomic_state *state);

+void +drm_atomic_clean_old_fb(struct drm_device *dev, unsigned plane_mask, int ret);

int __must_check drm_atomic_check_only(struct drm_atomic_state *state); int __must_check drm_atomic_commit(struct drm_atomic_state *state); int __must_check drm_atomic_async_commit(struct drm_atomic_state *state); -- 2.1.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Wed, Nov 11, 2015 at 11:29:11AM +0100, Maarten Lankhorst wrote:

From: Maarten Lankhorst maarten.lankhorst@linux.intel.com

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

Needs "Don't touch plane->old_fb/fb without having the right locks held." like the previous patch in the commit message. With that for patches 4&5:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_fb_helper.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index abd50863506e..69cbab5e5c81 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1227,7 +1227,9 @@ static int pan_display_atomic(struct fb_var_screeninfo *var, struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_atomic_state *state;

• struct drm_plane *plane; int i, ret;

• unsigned plane_mask;

  state = drm_atomic_state_alloc(dev); if (!state)

@@ -1235,19 +1237,22 @@ static int pan_display_atomic(struct fb_var_screeninfo *var,

state->acquire_ctx = dev->mode_config.acquire_ctx; retry:

• plane_mask = 0; for(i = 0; i < fb_helper->crtc_count; i++) { struct drm_mode_set *mode_set;

  mode_set = &fb_helper->crtc_info[i].mode_set;

• mode_set->crtc->primary->old_fb = mode_set->crtc->primary->fb;
  

• mode_set->x = var->xoffset; mode_set->y = var->yoffset;

  ret = __drm_atomic_helper_set_config(mode_set, state); if (ret != 0) goto fail;

• plane = mode_set->crtc->primary;
  

• plane_mask |= drm_plane_index(plane);
  

• plane->old_fb = plane->fb;
  

  }

  ret = drm_atomic_commit(state);

@@ -1259,26 +1264,7 @@ retry:

fail:

• for(i = 0; i < fb_helper->crtc_count; i++) {

• struct drm_mode_set *mode_set;
  

• struct drm_plane *plane;
  

• mode_set = &fb_helper->crtc_info[i].mode_set;
  

• plane = mode_set->crtc->primary;
  

• if (ret == 0) {
  

• 	struct drm_framebuffer *new_fb = plane->state->fb;
  

• 	if (new_fb)
  

• 		drm_framebuffer_reference(new_fb);
  

• 	plane->fb = new_fb;
  

• 	plane->crtc = plane->state->crtc;
  

• 	if (plane->old_fb)
  

• 		drm_framebuffer_unreference(plane->old_fb);
  

• }
  

• plane->old_fb = NULL;
  

• }

• drm_atomic_clean_old_fb(dev, plane_mask, ret);

  if (ret == -EDEADLK) goto backoff;

-- 2.1.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel