Re: [PATCH V1 for-next 6/7] IB/core: Add support for fd objects

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

On Sun, Feb 19, 2017 at 02:47:02PM +0200, Matan Barak wrote:

> >>
> >> All these callbacks are assumed to run when the context is alive.
> >
> > 'uboject->context' is touched outside the SRCU, eg in
> > uverbs_close_fd() - so it absolutely needs locking.
> >
> 
> uobject->context isn't protected by SRCU as intended.
> lookup_get_fd_uobject can't be executed concurrently with
> uverbs_close_fd, as uverbs_close_fd is called when the last
> reference of the fd is put and in that case fget called by
> lookup_get_fd_uobject would simply fail. Since fput and fget are
> atomically safe, this should be safe.

That is so tricky it needs a comment, but yes, that argument makes
sense for uverbs_close_fd().

But, you've just explained away every reason for this test:

+	if (f->f_op != fd_type->fops ||
+	    !uobject->context) {

It should probably be

	if (f->f_op != fd_type->fops) {
		fput(f);
		return ERR_PTR(-EBADF);
	}

	/* fget(id) ensures we are not currently running
	 * uverbs_close_fd, and the caller is expected to ensure
	 * that uverbs_cleanup_ucontext is never done while
	 * a call top lookup is possible. */
	WARN_ON(!uobject->context);
	if (!uobject->context) {
	        // Try to recover
		fput(f);
		return ERR_PTR(-EBADF);
	}

Or you need to explain where context = null comes from..

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