drm/gem: Check locking in drm_gem_object_unreference
Hello Daniel Vetter,
This is a semi-automatic email about new static checker warnings.
The patch 5771551c4877: "drm/gem: Check locking in
drm_gem_object_unreference" from Oct 15, 2015, leads to the following
Smatch complaint:
include/drm/drm_gem.h:147 drm_gem_object_unreference()
warn: variable dereferenced before check 'obj' (see line 145)
include/drm/drm_gem.h
144 {
145 WARN_ON(!mutex_is_locked(>dev->struct_mutex));
^^^
deref.
146
147 if (obj != NULL)
check.
148 kref_put(>refcount, drm_gem_object_free);
149 }
regards,
dan carpenter
[PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference
Hi
On Thu, Oct 15, 2015 at 10:55 AM, Daniel Vetter wrote:
> On Thu, Oct 15, 2015 at 10:36:08AM +0200, David Herrmann wrote:
>> Hi
>>
>> On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter
>> wrote:
>> > Pretty soon only some drivers will need dev->struct_mutex in their
>> > gem_free_object callbacks. Hence it's really important to make sure
>> > everything still keeps getting this right.
>> >
>> > Signed-off-by: Daniel Vetter
>> > ---
>> > include/drm/drm_gem.h | 2 ++
>> > 1 file changed, 2 insertions(+)
>> >
>> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> > index 7a592d7e398b..5b3754864fb0 100644
>> > --- a/include/drm/drm_gem.h
>> > +++ b/include/drm/drm_gem.h
>> > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
>> > static inline void
>> > drm_gem_object_unreference(struct drm_gem_object *obj)
>> > {
>> > + WARN_ON(!mutex_is_locked(>dev->struct_mutex));
>> > +
>>
>> This doesn't work. mutex_is_locked() is not context-aware, so it does
>> not check whether the holder is the current thread or someone else.
>> You *have* to use lockdep if you want negative lock checks.
>>
>> In other words, if some other thread holds the mutex in parallel to
>> this being called, you will trigger the WARN_ON.
>
> It's the other way round: If another thread holds the lock, but the caller
> doesn't, then we won't WARN: It's a false negative, not a false positive.
>
> And since lockdep is a serious hog and no one runs it I prefer this way
> round than the "perfect" lockdep_assert_hold.
>
> Not that in the unlocked variant we can't afford this mistake, so there we
> do rely upon the perfect lockdep locking tracking and use might_lock.
Gnah, you're right! I read this as assert, not WARN_ON.. *sigh* Sorry
for the noise.
David
[PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference
On Thu, Oct 15, 2015 at 10:36:08AM +0200, David Herrmann wrote:
> Hi
>
> On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter
> wrote:
> > Pretty soon only some drivers will need dev->struct_mutex in their
> > gem_free_object callbacks. Hence it's really important to make sure
> > everything still keeps getting this right.
> >
> > Signed-off-by: Daniel Vetter
> > ---
> > include/drm/drm_gem.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 7a592d7e398b..5b3754864fb0 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
> > static inline void
> > drm_gem_object_unreference(struct drm_gem_object *obj)
> > {
> > + WARN_ON(!mutex_is_locked(>dev->struct_mutex));
> > +
>
> This doesn't work. mutex_is_locked() is not context-aware, so it does
> not check whether the holder is the current thread or someone else.
> You *have* to use lockdep if you want negative lock checks.
>
> In other words, if some other thread holds the mutex in parallel to
> this being called, you will trigger the WARN_ON.
It's the other way round: If another thread holds the lock, but the caller
doesn't, then we won't WARN: It's a false negative, not a false positive.
And since lockdep is a serious hog and no one runs it I prefer this way
round than the "perfect" lockdep_assert_hold.
Not that in the unlocked variant we can't afford this mistake, so there we
do rely upon the perfect lockdep locking tracking and use might_lock.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference
Hi
On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter
wrote:
> Pretty soon only some drivers will need dev->struct_mutex in their
> gem_free_object callbacks. Hence it's really important to make sure
> everything still keeps getting this right.
>
> Signed-off-by: Daniel Vetter
> ---
> include/drm/drm_gem.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 7a592d7e398b..5b3754864fb0 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
> static inline void
> drm_gem_object_unreference(struct drm_gem_object *obj)
> {
> + WARN_ON(!mutex_is_locked(>dev->struct_mutex));
> +
This doesn't work. mutex_is_locked() is not context-aware, so it does
not check whether the holder is the current thread or someone else.
You *have* to use lockdep if you want negative lock checks.
In other words, if some other thread holds the mutex in parallel to
this being called, you will trigger the WARN_ON.
Thanks
David
> if (obj != NULL)
> kref_put(>refcount, drm_gem_object_free);
> }
> --
> 2.5.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference
Pretty soon only some drivers will need dev->struct_mutex in their
gem_free_object callbacks. Hence it's really important to make sure
everything still keeps getting this right.
Signed-off-by: Daniel Vetter
---
include/drm/drm_gem.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 7a592d7e398b..5b3754864fb0 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
static inline void
drm_gem_object_unreference(struct drm_gem_object *obj)
{
+ WARN_ON(!mutex_is_locked(>dev->struct_mutex));
+
if (obj != NULL)
kref_put(>refcount, drm_gem_object_free);
}
--
2.5.1
