Re: [PATCH] drm: fix drm_framebuffer cleanup.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 09, 2012 at 04:39:30PM +0900, Inki Dae 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.
> 
> This patch adds drm_framebuffer to drm_crtc structure as member

That is exactly the reverse of what you're doing in the patch.
We already have crtc.fb, and you're adding fb.crtc.

> 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.

Looks like you're just setting the crtc.fb and fb.crtc pointers to
exactly mirror each other in the page flip ioctl. That can't fix
anything.

First of all multiple crtcs can scan out from the same fb, so a single
fb.crtc pointer is clearly not enough to represent the relationship
between fbs and crtcs.

Also you're not clearing the fb.crtc pointer anywhere, so now
destroying any framebuffer that was once used for scanout, would disable
some crtc.

So it looks like you're making things worse, not better.

Anyway I'll just nack the whole idea. What you need to do is track the
pending page flips, and make sure the old buffer is not freed too early.
Just grab a reference to the old gem object when issuing the page flip,
and unref it when you're sure the flip has occured. Or you could use the
new drm_framebuffer refcount, but personally I don't see much point in
that when you already have the gem object refcount at your disposal.

-- 
Ville Syrjälä
Intel OTC