[PATCH] drm: fix drm_framebuffer cleanup.

[Inki Dae]

2012/11/9 Rob Clark <robdclark at gmail.com>

> On Fri, Nov 9, 2012 at 1:39 AM, Inki Dae <inki.dae at samsung.com> wrote:
> > This patch fixes access issue to invalid memory region.
> >
> > crtc had only one drm_framebuffer object so when framebuffer
> > cleanup was requested after page flip, it'd try to disable
> > hardware overlay to current crtc.
> > But if current crtc points to another drm_framebuffer,
> > This may induce invalid memory access.
>
> btw, this should instead be done by holding a ref to the GEM
> object(s)..  or these days you can increment the reference count on
> the fb and let the fb hold ref's to the GEM object(s) (which makes it
> a bit easier to deal with multi-planar formats)
>
>
Rob, let's discuss that again after you read my latest comment. Please, see
my latest comment.

Thanks,
Inki Dae


> BR,
> -R
>
> > Assume that some application are doing page flip with two
> > drm_framebuffer objects. At this time, if second drm_framebuffer
> > is set to current crtc and the cleanup to first drm_framebuffer
> > is requested once drm release is requested, then first
> > drm_framebuffer would be cleaned up without disabling
> > hardware overlay because current crtc points to second
> > drm_framebuffer. After that, gem buffer to first drm_framebuffer
> > would be released and this makes dma access invalid memory region.
> >
> > This patch adds drm_framebuffer to drm_crtc structure as member
> > and makes drm_framebuffer_cleanup function check if fb->crtc is
> > same as desired fb. And also when setcrtc and pageflip are
> > requested, it makes each drm_framebuffer point to current crtc.
> >
> > Signed-off-by: Inki Dae <inki.dae at samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c |    7 ++++---
> >  include/drm/drm_crtc.h     |    1 +
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index ef1b221..5c04bd4 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -386,7 +386,7 @@ void drm_framebuffer_remove(struct drm_framebuffer
> *fb)
> >
> >         /* remove from any CRTC */
> >         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > -               if (crtc->fb == fb) {
> > +               if (fb->crtc == crtc) {
> >                         /* should turn off the crtc */
> >                         memset(&set, 0, sizeof(struct drm_mode_set));
> >                         set.crtc = crtc;
> > @@ -2027,6 +2027,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void
> *data,
> >         set.mode = mode;
> >         set.connectors = connector_set;
> >         set.num_connectors = crtc_req->count_connectors;
> > +       fb->crtc = crtc;
> >         set.fb = fb;
> >         ret = crtc->funcs->set_config(&set);
> >
> > @@ -3635,8 +3636,8 @@ int drm_mode_page_flip_ioctl(struct drm_device
> *dev,
> >                         spin_unlock_irqrestore(&dev->event_lock, flags);
> >                         kfree(e);
> >                 }
> > -       }
> > -
> > +       } else
> > +               fb->crtc = crtc;
> >  out:
> >         mutex_unlock(&dev->mode_config.mutex);
> >         return ret;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 3fa18b7..92889be 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -256,6 +256,7 @@ struct drm_framebuffer {
> >         struct kref refcount;
> >         struct list_head head;
> >         struct drm_mode_object base;
> > +       struct drm_crtc *crtc;
> >         const struct drm_framebuffer_funcs *funcs;
> >         unsigned int pitches[4];
> >         unsigned int offsets[4];
> > --
> > 1.7.4.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121110/c3f49ec2/attachment-0001.html>