> -----Original Message----- > From: Jason Gunthorpe <jgg@xxxxxxxx> > Sent: Thursday, November 12, 2020 4:34 PM > To: Xiong, Jianxin <jianxin.xiong@xxxxxxxxx> > Cc: linux-rdma@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky > <leon@xxxxxxxxxx>; Sumit Semwal <sumit.semwal@xxxxxxxxxx>; Christian Koenig <christian.koenig@xxxxxxx>; Vetter, Daniel > <daniel.vetter@xxxxxxxxx> > 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
