Re: drm/atomic: track bitmask of planes attached to crtc

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: drm/atomic: track bitmask of planes attached to crtc
Date: Thu, 27 Nov 2014 18:54:03 +0300	[thread overview]
Message-ID: <20141127155403.GB4911@mwanda> (raw)
In-Reply-To: <20141127095502.GX32117@phenom.ffwll.local>

On Thu, Nov 27, 2014 at 10:55:02AM +0100, Daniel Vetter wrote:
> On Thu, Nov 27, 2014 at 09:41:13AM +0300, Dan Carpenter wrote:
> > Hello Rob Clark,
> > 
> > The patch 1ed2f34b4cc0: "drm/atomic: track bitmask of planes attached
> > to crtc" from Nov 21, 2014, leads to the following static checker
> > warning:
> > 
> > 	drivers/gpu/drm/drm_atomic.c:368 drm_atomic_set_crtc_for_plane()
> > 	error: 'plane_state' dereferencing possible ERR_PTR()
> 
> Hm yeah that shouldn't ever happen when callers use this correctly. But a
> WARN_ON would be good I guess. I'll add it.
> 

It could fail because of allocation failures.  But maybe this is a boot
time thing?  Normally dereferencing an ERR_PTR() is easy enough to debug
and static checkers just ignore WARN_ONs.  I am ambivalent.

> > 
> > drivers/gpu/drm/drm_atomic.c
> >    360  int
> >    361  drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> >    362                                struct drm_plane *plane, struct drm_crtc *crtc)
> >    363  {
> >    364          struct drm_plane_state *plane_state =
> >                                         ^^^^^^^^^^^^^
> >    365                          drm_atomic_get_plane_state(state, plane);
> >                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >    366          struct drm_crtc_state *crtc_state;
> >    367  
> >    368          if (plane_state->crtc) {
> >                     ^^^^^^^^^^^^^^^^^
> > Missing IS_ERR() check.
> > 
> > Also drm_atomic_get_plane_state() has poor error handling.  In
> > drm_atomic_get_plane_state(), if the call to drm_atomic_get_plane_state()
> > fails then it leaks memory.
> 
> Where does it leak memory exactly?

drivers/gpu/drm/drm_atomic.c
   249  
   250          plane_state = plane->funcs->atomic_duplicate_state(plane);

This is a kmemdup().

   251          if (!plane_state)
   252                  return ERR_PTR(-ENOMEM);
   253  
   254          state->plane_states[index] = plane_state;
   255          state->planes[index] = plane;
   256          plane_state->state = state;
   257  
   258          DRM_DEBUG_KMS("Added [PLANE:%d] %p state to %p\n",
   259                        plane->base.id, plane_state, state);
   260  
   261          if (plane_state->crtc) {
   262                  struct drm_crtc_state *crtc_state;
   263  
   264                  crtc_state = drm_atomic_get_crtc_state(state,
   265                                                         plane_state->crtc);
   266                  if (IS_ERR(crtc_state))
   267                          return ERR_CAST(crtc_state);

We leak if we return here.

   268          }
   269  
   270          return plane_state;

regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel