On 20/05/2019 16:10, Leon Romanovsky wrote:
> 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.
Right, but is it really valid? The udata in/out buffers in that case are
actually the create buffers and the driver has no way of telling. I think a
better approach is to clear the udata before calling the driver as done in
normal destroy flow.
Also, create_qp flow for example calls ib_destroy_qp on failure which passes
NULL udata, the different flows are not consistent and I don't see a reason why
they shouldn't be?
>
>>
>> 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.
This patch solves it, I'm thinking we need another patch for for-rc to prevent
panic.
>
>>
>>>
>>> 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
>>>
