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?
Also, if passing NULL udata is expected (why?) we have a bigger problem here as
existing code will cause NULL dereference.
>
> 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
>
