* re: drm/atomic: track bitmask of planes attached to crtc
@ 2014-11-27 6:41 Dan Carpenter
2014-11-27 9:55 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-11-27 6:41 UTC (permalink / raw)
To: robdclark; +Cc: dri-devel
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()
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.
369 crtc_state = drm_atomic_get_crtc_state(plane_state->state,
370 plane_state->crtc);
371 if (WARN_ON(IS_ERR(crtc_state)))
372 return PTR_ERR(crtc_state);
373
374 crtc_state->plane_mask &= ~(1 << drm_plane_index(plane));
375 }
376
377 plane_state->crtc = crtc;
378
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drm/atomic: track bitmask of planes attached to crtc 2014-11-27 6:41 drm/atomic: track bitmask of planes attached to crtc Dan Carpenter @ 2014-11-27 9:55 ` Daniel Vetter 2014-11-27 15:54 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2014-11-27 9:55 UTC (permalink / raw) To: Dan Carpenter; +Cc: dri-devel 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. > > 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? > > 369 crtc_state = drm_atomic_get_crtc_state(plane_state->state, > 370 plane_state->crtc); > 371 if (WARN_ON(IS_ERR(crtc_state))) > 372 return PTR_ERR(crtc_state); > 373 > 374 crtc_state->plane_mask &= ~(1 << drm_plane_index(plane)); > 375 } > 376 > 377 plane_state->crtc = crtc; > 378 > > regards, > dan carpenter > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drm/atomic: track bitmask of planes attached to crtc 2014-11-27 9:55 ` Daniel Vetter @ 2014-11-27 15:54 ` Dan Carpenter 2014-11-27 17:11 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-11-27 15:54 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drm/atomic: track bitmask of planes attached to crtc 2014-11-27 15:54 ` Dan Carpenter @ 2014-11-27 17:11 ` Daniel Vetter 2014-11-27 20:04 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2014-11-27 17:11 UTC (permalink / raw) To: Dan Carpenter; +Cc: dri-devel 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drm/atomic: track bitmask of planes attached to crtc 2014-11-27 17:11 ` Daniel Vetter @ 2014-11-27 20:04 ` Dan Carpenter 2014-11-28 10:39 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-11-27 20:04 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel On Thu, Nov 27, 2014 at 06:11:30PM +0100, Daniel Vetter wrote: > 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. If you do: if (WARN_ON(xxx)) return -ESOMETHING; Then that's important because it affects code flow and Smatch does the right thing, but if it's: WARN_ON(xxx); then Smatch ignores that. I guess I could hack it so WARN_ON() was treated like BUG_ON()... > > 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? Oh. The checker doesn't complain, that was just me looking at the code. I see my mistake now. regards, dan carpenter _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drm/atomic: track bitmask of planes attached to crtc 2014-11-27 20:04 ` Dan Carpenter @ 2014-11-28 10:39 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2014-11-28 10:39 UTC (permalink / raw) To: Dan Carpenter; +Cc: dri-devel On Thu, Nov 27, 2014 at 9:04 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Thu, Nov 27, 2014 at 06:11:30PM +0100, Daniel Vetter wrote: >> 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. > > If you do: > > if (WARN_ON(xxx)) > return -ESOMETHING; > > Then that's important because it affects code flow and Smatch does the > right thing, but if it's: > > WARN_ON(xxx); > > then Smatch ignores that. I guess I could hack it so WARN_ON() was > treated like BUG_ON()... I think even the if above should be treated like a BUG_ON for analysis, since something definitely went wrong that should have been. If the checker treats it as normal control flow it might not see bugs which are there (since it probably assumes that the worst-case recovery code is normal flow when it's just to make sure we can get useable bug reports instead of "machine hung"). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-28 10:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-27 6:41 drm/atomic: track bitmask of planes attached to crtc Dan Carpenter 2014-11-27 9:55 ` Daniel Vetter 2014-11-27 15:54 ` Dan Carpenter 2014-11-27 17:11 ` Daniel Vetter 2014-11-27 20:04 ` Dan Carpenter 2014-11-28 10:39 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.