On Sat, Sep 26, 2020 at 01:19:35PM +0300, Leon Romanovsky wrote:
> @@ -449,7 +453,10 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> ret = ib_dev->ops.alloc_pd(pd, &attrs->driver_udata);
> if (ret)
> goto err_alloc;
> - rdma_restrack_add(&pd->res);
> +
> + ret = rdma_restrack_add(&pd->res);
> + if (ret)
> + goto err_restrack;
>
> uobj->object = pd;
> uobj_finalize_uobj_create(uobj, attrs);
> @@ -457,6 +464,9 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> resp.pd_handle = uobj->id;
> return uverbs_response(attrs, &resp, sizeof(resp));
>
> +err_restrack:
> + ib_dev->ops.dealloc_pd(pd, &attrs->driver_udata);
There can be no failure between allocating the HW object and calling
uobj_finalize_uobj_create(). That was the whole point of that scheme.
It is really important that be kept.
Now that destroys are allowed to fail we aboslutely cannot have any
open coded destroys like this *anywhere* in the uobject system.
I think you need to go back to a model where the xarray allocation can
fail but we can still call del, otherwise the error unwind is a
complete nightmare.
This also has problems like this:
int ib_dereg_mr_user(struct ib_mr *mr, struct ib_udata *udata)
{
rdma_restrack_del(&mr->res);
ret = mr->device->ops.dereg_mr(mr, udata);
if (!ret) {
With the uobject destroy retry scheme restrack_del will be called
multiple times.
I think this is pretty simple to solve, write del as this:
void rdma_restrack_del(struct rdma_restrack_entry *res)
{
struct ib_device *dev = res_to_dev(res);
struct rdma_restrack_entry *old;
struct rdma_restrack_root *rt;
if (WARN_ON(!dev))
return
rt = &dev->res[res->type];
if (xa_cmpxchg(&rt->xa, res->id, res, NULL, GFP_KERNEL) != res)
return;
rdma_restrack_put(res);
wait_for_completion(&res->comp);
}
There is no need to do the wait_for_completion if we didn't change the
xarray.
Jason
