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

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