On 20/05/2019 17:51, Leon Romanovsky wrote:
> On Mon, May 20, 2019 at 05:24:43PM +0300, Gal Pressman wrote:
>> 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?
>
> Sorry, but I didn't look deeply enough on QPs and MRs yet to make clear
> cut, but from what I have seen till now, the implementation looks like
> a disaster.
Is it possible that in the future the destroy flows will pass a valid kabi udata
from rdma-core? If so, these removed checks are needed.
Why not clear the udata struct before calling the driver?
>
>>
>>>
>>>>
>>>> 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.
>
> I don't think that it is worth of hassle. EFA just entered kernel,
That doesn't mean bugs shouldn't be fixed.
> rdma-core is not merged yet
Userspace library is independent of the kernel, once the provider is merged it
can be used with any kernel with EFA.
> and anyway no one can use EFA adapter in his home.
That's not true, open up an AWS account and spin up an EFA enabled instance, all
from your laptop at home. It's that simple :).
>
>>
>>>
>>>>
>>>>>
>>>>> 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
>>>>>
