On Mon, May 20, 2019 at 07:54:57PM +0300, Gal Pressman wrote:
> 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.
In such case, kernel modules will be updated. We are not supporting out-of-tree modules.
> Why not clear the udata struct before calling the driver?
What exactly are you trying to solve?
>
> >
> >>
> >>>
> >>>>
> >>>> 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.
Go for it.
>
> > 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.
I was under impression that Amazon controls OS stack, so unclear to me
how I can install bad kernel version on AWS hypervisor.
>
> > 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 :).
Can I get two adapters to install in my server not in AWS?
>
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>> 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
> >>>>>
