Re: [PATCH for-next V5 1/5] IB/uverbs: Fix reference counting usage of event files

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Mon, Jun 22, 2015 at 05:47:14PM +0300, Yishai Hadas wrote:
>  	fd_install(resp.async_fd, filp);
> @@ -386,6 +376,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
>  	return in_len;
>  
>  err_file:
> +	ib_uverbs_free_async_event_file(file);
>  	fput(filp);

This looks really weird. 

> +void ib_uverbs_free_async_event_file(struct ib_uverbs_file *file)
> +{
> +	kref_put(&file->async_file->ref, ib_uverbs_release_event_file);
> +	file->async_file = NULL;
> +}

So that put is supposed to pair with this get?

> +		uverbs_file->async_file = ev_file;
> +		kref_get(&uverbs_file->async_file->ref);

[..]

> +	fput(filp);
> +	uverbs_file->async_file = NULL;

But isn't it null?

Again again, WTF? async_file is a kref'd thing, if you copy or assign
to it you need to manipulate the kref, so the null assign should be
dropping the ref.

Whis looks good enough to remove ib_uverbs_free_async_event_file, if
the flip is created OK then the uverbs_file->async file can remain set
until the uverbs file is closed.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html