* 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.