linux-kernel - Re: [PATCH 05/13] drm: provide device-refcount

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-next>] [day] [month] [year] [list]

Message-ID: <CAKMK7uEbHUL5=CiRqcCh7nBRAirOB4tdSwiz6YPHNiThVujA8A@mail.gmail.com>
Date:	Wed, 12 Feb 2014 17:26:57 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	Dave Airlie <airlied@...il.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 05/13] drm: provide device-refcount

On Wed, Feb 12, 2014 at 3:44 PM, David Herrmann <dh.herrmann@...il.com> wrote:
>>> +/**
>>> + * drm_dev_ref - Take reference of a DRM device
>>> + * @dev: device to take reference of or NULL
>>> + *
>>> + * This increases the ref-count of @dev by one. You *must* already own a
>>> + * reference when calling this. Use drm_dev_unref() to drop this reference
>>> + * again.
>>> + *
>>> + * This function never fails. However, this function does not provide *any*
>>> + * guarantee whether the device is alive or running. It only provides a
>>> + * reference to the object and the memory associated with it.
>>> + */
>>> +void drm_dev_ref(struct drm_device *dev)
>>> +{
>>> +     if (dev)
>>
>> This check here (and below in the unref code) look funny. What's the
>> reason for it? Trying to grab/drop a ref on a NULL pointer sounds like a
>> pretty serious bug to me. This is in contrast to kfree(NULL) which imo
>> makes sense - freeing nothing is a legitimate operation imo.
>
> I added it mainly to simplify cleanup-code paths. You can then just
> call unref() and set it to NULL regardless whether you actually hold a
> reference or not. For ref() I don't really care but I think the
> NULL-test doesn't hurt either.
>
> I copied this behavior from get_device() and put_device(), btw.
> Similar to these functions, I think a lot more will go wrong if the
> NULL pointer is not intentional. Imo, ref-counting on a NULL object
> just means "no object", so it shouldn't do anything.

My fear with this kind of magic is that someone accidentally exchanges
the pointer clearing to NULL (or assignement when grabbing a ref) with
the unref/ref call and then we have a very subtle bug at hand. If we
don't accept NULL objects the failure will be much more obvious.

The entire kernel kobject stuff is very consistent about this, but I
couldn't find a reason for it - all the NULL checks predate git
history. Greg can you please shed some lights on best practice here
and whether my fears are justified given your experience with shoddy
drivers in general?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
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/