Re: [PATCH 15/17] drm/atomic-helpers: functions for state duplicate/destroy/reset

From: Daniel Thompson <daniel.thompson@linaro.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 15/17] drm/atomic-helpers: functions for state duplicate/destroy/reset
Date: Mon, 03 Nov 2014 15:06:07 +0000	[thread overview]
Message-ID: <545799DF.2090505@linaro.org> (raw)
In-Reply-To: <20141103145338.GO26941@phenom.ffwll.local>

On 03/11/14 14:53, Daniel Vetter wrote:
> On Mon, Nov 03, 2014 at 02:45:28PM +0000, Daniel Thompson wrote:
>>> index 70bd67cf86e3..bd38df3cbe55 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -1429,7 +1429,7 @@ EXPORT_SYMBOL(drm_atomic_helper_set_config);
>>>  /**
>>>   * drm_atomic_helper_crtc_set_property - helper for crtc prorties
>>>   * @crtc: DRM crtc
>>> - * @prorty: DRM property
>>> + * @property: DRM property
>>
>> This looks like a bad fixup (should be in patch 11).
> 
> Indeed, will shuffle around.
> 
>>> +void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
>>> +{
>>> +	kfree(crtc->state);
>>> +	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
>>
>> This code looks semantically equivalent to a memset() although it may
>> result in a change to the pointer value. Is this code trying to flush
>> out uses-after-free?
>>
>> I can't find this free/alloc pattern in delivered code anywhere else in
>> the drm code base. Should this need to be replaced with memset() before
>> merging (or at least commenting)?
> 
> kfree is a nop when the argument is NULL, which is a crucial property of
> this - memset would oops on driver load.

Oops. Missed that (I think I misread who as assuming there was always
obj->state in the patch header).

Do you fancy making the comment "by freeing the state pointer and
allocating a new..." into "by freeing the state pointer (which may be
NULL) and allocating a new...".

If nothing else that means the documentation is richer than the code...

> Even neglecting this a memset imo doesn't blow up loudly enough if the
> driver subclasses the state structs (by adding more of it's driver private
> state at the end). Whereas underallocating tends to anger the slab
> poisoning code badly.
> 
> Finally it's really not just a memset, but a free + realloc. See the plane
> state, which also needs to drop a potential fb reference. Imo the explicit
> kfree+realloc makes that more obvious.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel