On Mon, May 20, 2019 at 03:39:26PM +0300, Gal Pressman wrote:
> On 20/05/2019 9:54, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > There are two possible execution contexts of destroy flows in EFA.
> > One is normal flow where user explicitly asked for object release
> > and another error unwinding.
> >
> > In normal scenario, RDMA/core will ensure that udata is supplied
> > according to KABI contract, for now it means no udata at all.
> >
> > In unwind flow, the EFA driver will receive uncleared udata from
> > numerous *_create_*() calls, but won't release those resources
> > due to extra checks.
>
> Thanks for the fix Leon, a few questions:
>
> Some of the unwind flows pass NULL udata and others an uncleared udata (is it
> really uncleared or is it actually the create udata?), what are we considering
> as the expected behavior? Isn't passing an uncleared udata the bug here?
It is a matter of unwind sequence, if IB/core did something after
driver created some object, it will need to call to destroy of this
object too. So I don't think that it is the bug.
And yes, it is not applicable for all flows, the one which caused me to
write this patch is failure in ib_uverbs_reg_mr(), which will call to
ib_dereg_mr_user(mr, &attrs->driver_udata);
and attrs->driver_udata is valid there.
>
> Also, if passing NULL udata is expected (why?) we have a bigger problem here as
> existing code will cause NULL dereference.
Not anymore, the destroy paths are not relying on udata now.
>
> >
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/hw/efa/efa_verbs.c | 24 ------------------------
> > 1 file changed, 24 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
> > index 6d6886c9009f..4999a74cee24 100644
> > --- a/drivers/infiniband/hw/efa/efa_verbs.c
> > +++ b/drivers/infiniband/hw/efa/efa_verbs.c
> > @@ -436,12 +436,6 @@ void efa_dealloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
> > struct efa_dev *dev = to_edev(ibpd->device);
> > struct efa_pd *pd = to_epd(ibpd);
> >
> > - if (udata->inlen &&
> > - !ib_is_udata_cleared(udata, 0, udata->inlen)) {
> > - ibdev_dbg(&dev->ibdev, "Incompatible ABI params\n");
> > - return;
> > - }
> > -
> > ibdev_dbg(&dev->ibdev, "Dealloc pd[%d]\n", pd->pdn);
> > efa_pd_dealloc(dev, pd->pdn);
> > }
> > @@ -459,12 +453,6 @@ int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
> > struct efa_qp *qp = to_eqp(ibqp);
> > int err;
> >
> > - if (udata->inlen &&
> > - !ib_is_udata_cleared(udata, 0, udata->inlen)) {
> > - ibdev_dbg(&dev->ibdev, "Incompatible ABI params\n");
> > - return -EINVAL;
> > - }
> > -
> > ibdev_dbg(&dev->ibdev, "Destroy qp[%u]\n", ibqp->qp_num);
> > err = efa_destroy_qp_handle(dev, qp->qp_handle);
> > if (err)
> > @@ -865,12 +853,6 @@ int efa_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
> > struct efa_cq *cq = to_ecq(ibcq);
> > int err;
> >
> > - if (udata->inlen &&
> > - !ib_is_udata_cleared(udata, 0, udata->inlen)) {
> > - ibdev_dbg(&dev->ibdev, "Incompatible ABI params\n");
> > - return -EINVAL;
> > - }
> > -
> > ibdev_dbg(&dev->ibdev,
> > "Destroy cq[%d] virt[0x%p] freed: size[%lu], dma[%pad]\n",
> > cq->cq_idx, cq->cpu_addr, cq->size, &cq->dma_addr);
> > @@ -1556,12 +1538,6 @@ int efa_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> > struct efa_mr *mr = to_emr(ibmr);
> > int err;
> >
> > - if (udata->inlen &&
> > - !ib_is_udata_cleared(udata, 0, udata->inlen)) {
> > - ibdev_dbg(&dev->ibdev, "Incompatible ABI params\n");
> > - return -EINVAL;
> > - }
> > -
> > ibdev_dbg(&dev->ibdev, "Deregister mr[%d]\n", ibmr->lkey);
> >
> > if (mr->umem) {
> > --
> > 2.20.1
> >
