On Thu, Nov 27, 2014 at 06:54:03PM +0300, Dan Carpenter wrote:
> 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.
Well there rules are that you need to acquire the plane_state first. We're
now respinning the interfaces a bit to again make sure that's done by
requiring callers to directly pass in the plane_state.
btw not sure whether checker should just look through WARN_ON, we have
lots of places where we've historically screwed up and added a WARN_ON +
early return to make sure we'll in the future somewhat recover. This is
really important for gfx since at boot-up (due to fbcon locking bonghits)
the entire intial modeset is run with console_lock held. And that's a few
10k lines of code depending upon platform :(
So we absolutely have to handle failures robustely, but if checkers assume
that it's ok to pass crap caught by WARN_ONs around then that's might
reduce checker usefulness quite a bit.
Just an aside really
> > >
> > > 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().
Another aside: it'll soon be more once a few drivers with atomic support
have merged. But fundamentally they'll all still need to do at least the
kmemdup.
> 251 if (!plane_state)
> 252 return ERR_PTR(-ENOMEM);
> 253
> 254 state->plane_states[index] = plane_state;
This statement here should make sure that drm_atomic_state_free cleans
everthing up. So I still don't see a leak ... where does the checker see
one?
> 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.
Note that the atomic stuff is using wait/wound mutexes, so bailing out
with -EDEADLK into the slowpath is an expected path. Hence why we tend to
keep all the allocs around until we eventually get rid of them in one
spot.
-Daniel
>
> 268 }
> 269
> 270 return plane_state;
>
> regards,
> dan carpenter
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
