linux-kernel - Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF6AEGuZb1nhMJwc+k-cwPTPLHTiO1PkHcpGN899Yq3pEMUwkA@mail.gmail.com>
Date:	Thu, 1 Sep 2011 20:18:04 -0500
From:	Rob Clark <robdclark@...il.com>
To:	Inki Dae <inki.dae@...sung.com>
Cc:	airlied@...ux.ie, dri-devel@...ts.freedesktop.org,
	sw0312.kim@...sung.com, linux-kernel@...r.kernel.org,
	kyungmin.park@...sung.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210.

On Thu, Sep 1, 2011 at 8:06 AM, Inki Dae <inki.dae@...sung.com> wrote:
>> >> > +struct samsung_drm_gem_obj *
>> >> > +               find_samsung_drm_gem_object(struct drm_file
> *file_priv,
>> >> > +                       struct drm_device *dev, unsigned int handle)
>> >> > +{
>> >> > +       struct drm_gem_object *gem_obj;
>> >> > +
>> >> > +       gem_obj = drm_gem_object_lookup(dev, file_priv, handle);
>> >> > +       if (!gem_obj) {
>> >> > +               DRM_LOG_KMS("a invalid gem object not registered to
>> >> lookup.\n");
>> >> > +               return NULL;
>> >> > +       }
>> >> > +
>> >> > +       /**
>> >> > +        * unreference refcount of the gem object.
>> >> > +        * at drm_gem_object_lookup(), the gem object was referenced.
>> >> > +        */
>> >> > +       drm_gem_object_unreference(gem_obj);
>> >>
>> >> this doesn't seem right, to drop the reference before you use the
>> >> buffer elsewhere..
>> >>
>> > No, see drm_gem_object_lookup fxn. at this function, if there is a
>> object
>> > found then drm_gem_object_reference is called to increase refcount of
>> this
>> > object. if there is any missing point, give me any comment please. thank
>> > you.
>>
>>
>> Right, but I think there is a reason it takes a reference... so that
>> the object doesn't get free'd from under your feet.  So pattern
>> should, I think, be:
>>
>>   obj = lookup(...);
>>   ... do stuff w/ obj ...
>>   unreference(obj)
>>
>> so the caller who is using the looked up obj should unref it when done
>>
>> Instead, you have:
>>
>>   obj = lookup(...);
>>   unreference(obj);
>>   ... do stuff w/ obj ...
>>
>>
>
> Generally right, but in this case, it is just used to get specific gem
> object through find_samsung_drm_gem_object() so doesn't reference this gem
> object anywhere.
> therefore reference and unreference should be done within
> find_samsung_drm_gem_object(). if there is any point I missed then let me
> know please. thank you.
>

Still, it seems like find_samsung_drm_gem_object() is encouraging the
wrong usage-pattern, even if it works fine today because you know
somewhere else is holding a reference to the object.  Later if you
expand your use of GEM objects, this fxn might come back to bite you.
There is a good reason that drm_gem_object_lookup() takes a reference
to the object, and it feels wrong to intentionally subvert that.

(I'm perfectly willing to be overridden on the subject.. there are
plenty of folks on this list who have been doing the GEM thing longer
than I have.  But it just seems better to use APIs like
drm_gem_object_lookup() the way they were intended.)

BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/