On Mon, Feb 11, 2019 at 05:38:00PM +0200, Leon Romanovsky wrote:
> @@ -295,19 +312,33 @@ EXPORT_SYMBOL(rdma_restrack_put);
>
> void rdma_restrack_del(struct rdma_restrack_entry *res)
> {
> - struct ib_device *dev;
> + struct ib_device *dev = res_to_dev(res);
> + struct xarray *xa;
>
> if (!res->valid)
> goto out;
>
> - dev = res_to_dev(res);
> + /*
> + * All objects except CM_ID set valid device immediately
> + * after new object is created, it means that for not valid
> + * objects will still have "dev".
> + *
> + * It is not the case for CM_ID, newly created object has
> + * this field set to NULL and it is set in _cma_attach_to_dev()
> + * only.
> + *
> + * Because we don't want to add any conditions on call
> + * to rdma_restrack_del(), the check below protects from
> + * NULL-dereference.
> + */
This comment makes no sense. valid means the restrack has been added
to the the device's xarray, so any situation that gives valid and a
NULL dev is a bug as we have an entry in the xarray that we can't
remove.
And the res_to_dev should still be after the valid check as res->type
could be garbage if the entry hasn't been inited yet.
and moving the res_to_dev up is not very safe, if valid isn't set we
shouldn't assume type is anything valid.
This seems better:
void rdma_restrack_del(struct rdma_restrack_entry *res)
{
struct rdma_restrack_root *rt;
struct ib_device *dev;
if (!res->valid)
goto out;
dev = res_to_dev(res);
if (WARN_ON(dev))
return;
rt = &dev->res[res->type];
if (WARN_ON(xa_cmpxchg(&rt->xa, res->id, res, NULL, GFP_KERNEL) != res))
return;
xa_erase(&rt->xa, res->id);
res->valid = false;
Jason
