[PATCH] drm: fix drm_framebuffer cleanup.

[Ville Syrjälä]

On Sat, Nov 10, 2012 at 01:50:53AM +0900, Inki Dae wrote:
> 2012/11/9 Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> > 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.
> >
> >
> I don't think so. let's see it again. below is my idea AND the reason I
> posted this patch.
> Original codes,
> gem alloc(gem0);
> -> gem0 refcount = 1
> gem0 mmap
> -> gem0 refcount = 2
> gem alloc(gem1);
> -> gem1 refcount =1
> gem1 mmap
> -> gem1 refcount =2
> addfb(fb0, gem0);
> -> gem0 refcount=3
> addfb(fb1,gem1);
> -> gem1 refcount = 3
> setcrtc(fb0, crtc0)
> -> crtc0.fb = fb0
> pageflip(crtc0, fb1);
> -> crtc0.fb = fb1.
> and pageflip is repeated
> 
> close(gem0)
> -> gem0 refcount = 2
> close(gem1)
> ->gem1 refcount = 2
> munmap(gem0)
> ->gem0 refcount = 1
> munmap(gem1)
> ->gem1 refcount = 1
> 
> close(drm)
> 1. fb release
> -> check if crtc->fb is same as fb0 but current crtc is pointing to fb1
> 2. so free fb0 without disabling current crtc.
> -> gem0 refcount = 0 so released. At this time, dma access invalid memory
> region unfortunately* *if the dma is accessing gem0.
> 
> 
> 
> With my patch,
> ...
> setcrtc(fb0, crtc0)
> -> crtc0.fb = fb0, fb0.crtc = crtc0
> pageflip(crtc0, fb1);
> -> crtc0.fb = fb1, fb1.crtc = crtc0.
> and pageflip is repeated
> 
> close(gem0)
> -> gem0 refcount = 2
> close(gem1)
> ->gem1 refcount = 2
> munmap(gem0)
> ->gem0 refcount = 1
> munmap(gem1)
> ->gem1 refcount = 1
> close(drm)
> 1. fb release
> -> fb0->crtc is same as crtc so disable crtc (dma stop)

No, that's wrong. The current fb is fb1, so destroying fb0 must not
disable the crtc.

> 2. and free fb0.
> -> gem0 refcount = 0 so released. We can avoid invalid memory access
> because the dma was stopped.
> In addition, one thing you pointed should be considered like below,
> 1. When rmfb is called, if fb is last one then it sets fb->crtc to NULL.
> - the fb is cleaned up with disabling crtc.
> 
> That's all and the issue I faced with.
> 
> I'd happy to give me any comments, opinions.
> 
> Thanks,
> Inki Dae

-- 
Ville Syrjälä
Intel OTC