[PATCH v10 3/6] RDMA/uverbs: Add uverbs command for dma-buf based MR registration

[Xiong, Jianxin]

> -----Original Message-----
> From: Jason Gunthorpe <jgg at ziepe.ca>
> Sent: Thursday, November 12, 2020 4:34 PM
> To: Xiong, Jianxin <jianxin.xiong at intel.com>
> Cc: linux-rdma at vger.kernel.org; dri-devel at lists.freedesktop.org; Doug Ledford <dledford at redhat.com>; Leon Romanovsky
> <leon at kernel.org>; Sumit Semwal <sumit.semwal at linaro.org>; Christian Koenig <christian.koenig at amd.com>; Vetter, Daniel
> <daniel.vetter at intel.com>
> Subject: Re: [PATCH v10 3/6] RDMA/uverbs: Add uverbs command for dma-buf based MR registration
> 
> On Tue, Nov 10, 2020 at 01:41:14PM -0800, Jianxin Xiong wrote:
> > +	mr = pd->device->ops.reg_user_mr_dmabuf(pd, offset, length, virt_addr,
> > +						fd, access_flags,
> > +						&attrs->driver_udata);
> > +	if (IS_ERR(mr))
> > +		return PTR_ERR(mr);
> > +
> > +	mr->device = pd->device;
> > +	mr->pd = pd;
> > +	mr->type = IB_MR_TYPE_USER;
> > +	mr->uobject = uobj;
> > +	atomic_inc(&pd->usecnt);
> > +
> > +	uobj->object = mr;
> > +
> > +	uverbs_finalize_uobj_create(attrs,
> > +UVERBS_ATTR_REG_DMABUF_MR_HANDLE);
> > +
> > +	ret = uverbs_copy_to(attrs, UVERBS_ATTR_REG_DMABUF_MR_RESP_LKEY,
> > +			     &mr->lkey, sizeof(mr->lkey));
> > +	if (ret)
> > +		goto err_dereg;
> > +
> > +	ret = uverbs_copy_to(attrs, UVERBS_ATTR_REG_DMABUF_MR_RESP_RKEY,
> > +			     &mr->rkey, sizeof(mr->rkey));
> > +	if (ret)
> > +		goto err_dereg;
> > +
> > +	return 0;
> > +
> > +err_dereg:
> > +	ib_dereg_mr_user(mr, uverbs_get_cleared_udata(attrs));
> 
> This isn't how the error handling works with
> uverbs_finalize_uobj_create() - you just return the error code and the caller deals with destroying the fully initialized HW object properly.
> Calling ib_dereg_mr_user() here will crash the kernel.
> 
> Check the other uses for an example.
> 

Will fix.

> Again I must ask what the plan is for testing as even a basic set of pyverbs sanity tests would catch this.
> 
> I've generally been insisting that all new uABI needs testing support in rdma-core. This *might* be the exception but I want to hear a really
> good reason why we can't have a test first...
> 

So far I have been using a user space test that focuses on basic functionality and limited parameter checking (e.g. incorrect addr, length, flags). This specific error path happens
to be untested. Will work more on the test front to increase the code coverage.

> Jason