[PATCH] drm: fix drm_framebuffer cleanup.

[Ville Syrjälä]

On Fri, Nov 09, 2012 at 11:04:58PM +0900, Inki Dae wrote:
> 2012/11/9 Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> > On Fri, Nov 09, 2012 at 09:41:19PM +0900, Inki Dae wrote:
> > > 2012/11/9 Ville Syrjälä <ville.syrjala at 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.
> > > >
> > > >
> > > At least, when drm is released, the crtc to framebuffer that was recently
> > > used for scanout can be disabled correctly.
> >
> > Let's take this scenario:
> >
> > setcrtc(crtc0, fb0)
> >  -> crtc0.fb = fb0, fb0.crtc = crtc0
> > page_flip(crtc0, fb1)
> >  -> crtc0.fb = fb1, fb1.crtc = crtc0
> > rmfb(fb0)
> >  -> ?
> >
> > The rmfb() will now disable crtc0 because fb0.crtc == crtc0, but
> > let's consider this just a bug and ignore it for now. So let's assume
> > that crtc0 won't be disabled when we call rmfb(fb0).
> >
> >
> Ok, Please assume that my patch includes the below codes. when we call
> rmfb(fb0), fb0->crtc is NULL. so fb0 will be freed without disabling crtc.
> 
> int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file
> *file_priv) {
> ....
> fb->crtc = NULL;
> fb->funcs->destroy(fb);
> out:
> ...
> }

As stated above, I already assumed that.

> > The real question is, how would your patch help? You can't free fb0
> > until the page flip has occured, and your patch doesn't track page
> > flips, so how can it do anything useful?
> >
> > > > 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.
> > > >
> > > >
> > > It looks like that you missed my previous comment. Plaese, see the
> > previous
> > > comment. And that was already pointed out by Prathyush. fb.crtc will be
> > > cleared at drm_mode_rmfb(). With this, is there my missing point?  I
> > really
> > > wonder that with this patch, drm framwork could be faced with any
> > problem.
> >
> > If you clear the fb.crtc pointer before destroying the fb, then you
> > never disable any crtcs in response to removing a framebuffer.
> > That's not what we want when the fb is the current fb for the crtc.
> >
> 
> Right, there is my missing point. How about this then? Couldn't  we check
> this fb is last one or not? And if last one, it makes fb->crtc keep as is.

That won't help, It would just make the behaviour of your code
identical to the behaviour of the current code.

> > > > 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.
> > > >
> > > >
> > > >
> > >
> > > And it seems like that your saying is that each specific drivers
> > > should resolve this issue(access to invalid memory region) tracking the
> > > pending page flip. But I think that this issue may be a missing point drm
> > > framework missed so specific drivers is processing this instead. And with
> > > this patch, couldn't some codes you mentioned be removed from specific
> > > drivers? because it doesn't need to track the pending page flips and
> > > relevant things anymore.
> > >
> > > There may be my missing point. I'd welcome to any comments and advices.
> >
> > If you don't track the page flips somehow, you can't safely free the
> > previous buffer. It's that simple. You can of course make some of
> > that code generic enough to be shared between drivers. That is
> > actually what the drm_flip mechanism that I wrote for atomic page
> > flips is; a reasonably generic helper for tracking page flips.
> >
> >
> Thanks for comments. Right, now Exynos driver doesn't consider tracking
> page flips yet. This is my TODO. Anyway couldn't we improve this patch so
> that drm framework considers such thing?

No, because it depends on hardware specific information. The drm core
code doesn't have a crystall ball, so it can't just magically know when
a page flip completes.

> I think with this patch, we could avoid invald memory access issue without
> tracking page flips. In this case, pended page flips would be just ignored.

I still don't understand how the patch is supposed to help. If you make
the proposed change to rmfb() so that it doesn't disable the crtc when
you remove a non-current fb, then this would happen:

setcrtc(crtc0, fb0)
 -> crtc0.fb = fb0, fb0.crtc = crtc0
pageflip(crtc0, fb1);
 -> crtc0.fb = fb1, fb1.crtc = crtc0
rmfb(fb0);
 -> fb0.crtc = NULL;
    destroy(fb0);

That is exactly the same behaviour we have today, so you still get
the invalid memory access to fb0 if the page flip hasn't occured yet.
And that means the fb.crtc pointer is entirely pointless.

-- 
Ville Syrjälä
Intel OTC