Re: [PATCH] drm: fix drm_framebuffer cleanup.

[Inki Dae <inki.dae@xxxxxxxxxxx>]

2012/11/9 Prathyush K <prathyush@xxxxxxxxxxxx>

On Fri, Nov 9, 2012 at 1:09 PM, Inki Dae <inki.dae@xxxxxxxxxxx> 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.

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.

I am confused regarding this. If crtc points to second frame buffer, then the dma is accessing the memory region

of the second framebuffer. Why can't we free the first framebuffer and its gem buffer?

With this patch, there is no way to free a framebuffer (which has been set to a crtc), without disabling

the crtc.

Consider this example:

I have three framebuffers (0, 1, 2) and I set them to a crtc A one by one. So with your patch,

fb[0]->crtc = A

fb[1]->crtc = A

fb[2]->crtc = A

After this, I am using only framebuffer 0 and 1 i.e. 0 and 1 are being page flipped.

Now, I want to remove framebuffer 2.

fb[2]->crtc = A .. so while removing the framebuffer, we will end up disabling the crtc

which is not correct.

I think, there should be an additional interface to unset a fb to a crtc. That way, we can

reset the crtc inside a framebuffer so that it can be freed if not in use.

i.e. fb[2]->crtc = NULL;

Right, thank you for comments. There is my missing point. how about adding fb->crtc = NULL like below then?

int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file *file_priv) {
        ....
        fb->crtc = NULL;
        fb->funcs->destroy(fb);
out:
        ...
}

With this, when user requested rmfb to remove fb[2], fb[2]'s crtc becomes NULL so the fb would be freed without disabling crtc.

Thanks,
Inki Dae

 

 

 

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@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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