| Message ID | 1491881502-24357-3-git-send-email-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 11, 2017 at 11:31:42AM +0800, Jeffy Chen wrote: > We are freeing all framebuffers in drm_mode_config_cleanup without > sync the drm_file's fbs list. > > So if someone try to unbind drm before release drm dev fd, the fbs > list would remain some invalid fb references. And that would cause > crash later in drm_fb_release. > > Add a sanity check to prevent that. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> This feels like duct-tape. The problem is that when we unplug a drm device, we don't properly clean this up. I think we should first clean up all the drm files (and make sure all ioctl and anything else completed), before we proceed further in the driver cleanup. Like I said, fixing unplug is going to be serious amounts of work, not sure you really want to do this just for a pure debug use-cases. -Daniel > > --- > > Changes in v7: > Update commit message. > > Changes in v6: None > Changes in v5: None > Changes in v2: None > > drivers/gpu/drm/drm_framebuffer.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index e8f9c13..03c1632 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv) > { > struct drm_framebuffer *fb, *tfb; > struct drm_mode_rmfb_work arg; > + struct drm_minor *minor = priv->minor; > + struct drm_device *dev = minor->dev; > + > + if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs))) > + return; > > INIT_LIST_HEAD(&arg.fbs); > > -- > 2.1.4 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On 04/12/2017 02:36 PM, Daniel Vetter wrote: > On Tue, Apr 11, 2017 at 11:31:42AM +0800, Jeffy Chen wrote: >> We are freeing all framebuffers in drm_mode_config_cleanup without >> sync the drm_file's fbs list. >> >> So if someone try to unbind drm before release drm dev fd, the fbs >> list would remain some invalid fb references. And that would cause >> crash later in drm_fb_release. >> >> Add a sanity check to prevent that. >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > This feels like duct-tape. The problem is that when we unplug a drm > device, we don't properly clean this up. I think we should first clean up > all the drm files (and make sure all ioctl and anything else completed), > before we proceed further in the driver cleanup. > > Like I said, fixing unplug is going to be serious amounts of work, not > sure you really want to do this just for a pure debug use-cases. > -Daniel right, and it's ok to drop this 2 patches, the rests are already enough for the testing :) > >> >> --- >> >> Changes in v7: >> Update commit message. >> >> Changes in v6: None >> Changes in v5: None >> Changes in v2: None >> >> drivers/gpu/drm/drm_framebuffer.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >> index e8f9c13..03c1632 100644 >> --- a/drivers/gpu/drm/drm_framebuffer.c >> +++ b/drivers/gpu/drm/drm_framebuffer.c >> @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv) >> { >> struct drm_framebuffer *fb, *tfb; >> struct drm_mode_rmfb_work arg; >> + struct drm_minor *minor = priv->minor; >> + struct drm_device *dev = minor->dev; >> + >> + if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs))) >> + return; >> >> INIT_LIST_HEAD(&arg.fbs); >> >> -- >> 2.1.4 >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index e8f9c13..03c1632 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -583,6 +583,11 @@ void drm_fb_release(struct drm_file *priv) { struct drm_framebuffer *fb, *tfb; struct drm_mode_rmfb_work arg; + struct drm_minor *minor = priv->minor; + struct drm_device *dev = minor->dev; + + if (WARN_ON(!dev->mode_config.num_fb && !list_empty(&priv->fbs))) + return; INIT_LIST_HEAD(&arg.fbs);
We are freeing all framebuffers in drm_mode_config_cleanup without sync the drm_file's fbs list. So if someone try to unbind drm before release drm dev fd, the fbs list would remain some invalid fb references. And that would cause crash later in drm_fb_release. Add a sanity check to prevent that. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v7: Update commit message. Changes in v6: None Changes in v5: None Changes in v2: None drivers/gpu/drm/drm_framebuffer.c | 5 +++++ 1 file changed, 5 insertions(+)