linux-kernel - Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct file

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: <20160628080221.GB25424@nuc-i3427.alporthouse.com>
Date:	Tue, 28 Jun 2016 09:02:21 +0100
From:	Chris Wilson <chris@...is-wilson.co.uk>
To:	Gustavo Padovan <gustavo@...ovan.org>
Cc:	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	Daniel Stone <daniels@...labora.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Rob Clark <robdclark@...il.com>,
	Greg Hackmann <ghackmann@...gle.com>,
	John Harrison <John.C.Harrison@...el.com>,
	laurent.pinchart@...asonboard.com, seanpaul@...gle.com,
	marcheu@...gle.com, m.chehab@...sung.com,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Gustavo Padovan <gustavo.padovan@...labora.co.uk>,
	Christian König <christian.koenig@....com>
Subject: Re: [RFC v2 3/3] dma-buf/sync_file: rework fence storage in struct
 file

On Mon, Jun 27, 2016 at 04:29:22PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> 
> Create sync_file->fence to abstract the type of fence we are using for
> each sync_file. If only one fence is present we use a normal struct fence
> but if there is more fences to be added to the sync_file a fence_array
> is created.
> 
> This change cleans up sync_file a bit. We don't need to have sync_file_cb
> array anymore. Instead, as we always have  one fence, only one fence
> callback is registered per sync_file.
> 
> Cc: Chris Wilson <chris@...is-wilson.co.uk>
> Cc: Christian König <christian.koenig@....com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@...labora.co.uk>
> ---
> @@ -76,21 +76,19 @@ struct sync_file *sync_file_create(struct fence *fence)
>  {
>  	struct sync_file *sync_file;
>  
> -	sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]));
> +	sync_file = sync_file_alloc();
>  	if (!sync_file)
>  		return NULL;
>  
> -	sync_file->num_fences = 1;
> +	sync_file->fence = fence;
> +
>  	atomic_set(&sync_file->status, 1);

sync_file->status => fence_is_signaled(sync_file->fence);

Both should just be an atomic read, except fence_is_signaled() will then
do a secondary poll.

>  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
>  		 fence->ops->get_driver_name(fence),
>  		 fence->ops->get_timeline_name(fence), fence->context,
>  		 fence->seqno);
>  
> -	sync_file->cbs[0].fence = fence;
> -	sync_file->cbs[0].sync_file = sync_file;
> -	if (fence_add_callback(fence, &sync_file->cbs[0].cb,
> -			       fence_check_cb_func))
> +	if (fence_add_callback(fence, &sync_file->cb, fence_check_cb_func))
>  		atomic_dec(&sync_file->status);
>  
>  	return sync_file;
> @@ -121,14 +119,42 @@ err:
>  	return NULL;
>  }
>  
> -static void sync_file_add_pt(struct sync_file *sync_file, int *i,
> -			     struct fence *fence)
> +static int sync_file_set_fence(struct sync_file *sync_file,
> +			       struct fence **fences, int num_fences)
>  {
> -	sync_file->cbs[*i].fence = fence;
> -	sync_file->cbs[*i].sync_file = sync_file;
> +	struct fence_array *array;
> +
> +	if (num_fences == 1) {
> +		sync_file->fence = fences[0];

This steals the references.

> +	} else {
> +		array = fence_array_create(num_fences, fences,
> +					   fence_context_alloc(1), 1, false);

This creates a reference.

When we call fence_put(sync_fence->fence) we release a reference we
never owned if num_fences == 1.

> +	struct fence **fences, **a_fences, **b_fences;
> +	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
>  
> +	a_fences = get_fences(a, &a_num_fences);
> +	b_fences = get_fences(b, &b_num_fences);
> +	num_fences = a_num_fences + b_num_fences;
> +
> +	fences = kcalloc(num_fences, sizeof(**fences), GFP_KERNEL);

Just sizeof(*fences) (you want to allocate an array of pointers, not an
array of fence structs).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre