On Wed, Jan 30, 2019 at 09:26:22PM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 30, 2019 at 12:49:06PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > Avoid direct access to internal restrack values by providing
> > special wrappers and update previous occurrence for
> > rdma_res_to_id(), which translated from restrack entry to CM_ID
> > and not to restrack ID.
> >
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/cma.c | 8 +++----
> > drivers/infiniband/core/core_priv.h | 2 +-
> > drivers/infiniband/core/uverbs_cmd.c | 8 +++----
> > drivers/infiniband/core/uverbs_std_types_cq.c | 2 +-
> > drivers/infiniband/core/verbs.c | 6 ++---
> > drivers/infiniband/hw/cxgb4/restrack.c | 2 +-
> > include/rdma/rdma_cm.h | 2 +-
> > include/rdma/restrack.h | 22 +++++++++++++++++++
> > 8 files changed, 37 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 83aa2ad0c27e..9570b98fdaeb 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -162,17 +162,17 @@ struct iw_cm_id *rdma_iw_cm_id(struct rdma_cm_id *id)
> > EXPORT_SYMBOL(rdma_iw_cm_id);
> >
> > /**
> > - * rdma_res_to_id() - return the rdma_cm_id pointer for this restrack.
> > + * rdma_res_to_cmid() - return the rdma_cm_id pointer for this restrack.
> > * @res: rdma resource tracking entry pointer
> > */
> > -struct rdma_cm_id *rdma_res_to_id(struct rdma_restrack_entry *res)
> > +struct rdma_cm_id *rdma_res_to_cmid(struct rdma_restrack_entry *res)
> > {
> > struct rdma_id_private *id_priv =
> > container_of(res, struct rdma_id_private, res);
> >
> > return &id_priv->id;
> > }
> > -EXPORT_SYMBOL(rdma_res_to_id);
> > +EXPORT_SYMBOL(rdma_res_to_cmid);
> >
> > static void cma_add_one(struct ib_device *device);
> > static void cma_remove_one(struct ib_device *device, void *client_data);
> > @@ -881,7 +881,7 @@ struct rdma_cm_id *__rdma_create_id(struct net *net,
> > return ERR_PTR(-ENOMEM);
> >
> > rdma_restrack_set_task(&id_priv->res, caller);
> > - id_priv->res.type = RDMA_RESTRACK_CM_ID;
> > + rdma_rt_set_type(&id_priv->res, RDMA_RESTRACK_CM_ID);
> > id_priv->state = RDMA_CM_IDLE;
> > id_priv->id.context = context;
> > id_priv->id.event_handler = event_handler;
> > diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
> > index 42a49982f66e..cda2db1add39 100644
> > +++ b/drivers/infiniband/core/core_priv.h
> > @@ -298,8 +298,8 @@ static inline struct ib_qp *_ib_create_qp(struct ib_device *dev,
> > * and more importantly they are created internaly by driver,
> > * see mlx5 create_dev_resources() as an example.
> > */
> > + rdma_rt_set_type(&qp->res, RDMA_RESTRACK_QP);
> > if (attr->qp_type < IB_QPT_XRC_INI) {
> > - qp->res.type = RDMA_RESTRACK_QP;
> > if (uobj)
> > rdma_restrack_uadd(&qp->res);
> > else
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index aa260cafbd85..4ac97a61d567 100644
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -262,7 +262,7 @@ static int ib_uverbs_get_context(struct uverbs_attr_bundle *attrs)
> >
> > fd_install(resp.async_fd, filp);
> >
> > - ucontext->res.type = RDMA_RESTRACK_CTX;
> > + rdma_rt_set_type(&ucontext->res, RDMA_RESTRACK_CTX);
> > rdma_restrack_uadd(&ucontext->res);
> >
> > /*
> > @@ -421,7 +421,7 @@ static int ib_uverbs_alloc_pd(struct uverbs_attr_bundle *attrs)
> > uobj->object = pd;
> > memset(&resp, 0, sizeof resp);
> > resp.pd_handle = uobj->id;
> > - pd->res.type = RDMA_RESTRACK_PD;
> > + rdma_rt_set_type(&pd->res, RDMA_RESTRACK_PD);
> > rdma_restrack_uadd(&pd->res);
> >
> > ret = uverbs_response(attrs, &resp, sizeof(resp));
> > @@ -737,7 +737,7 @@ static int ib_uverbs_reg_mr(struct uverbs_attr_bundle *attrs)
> > mr->dm = NULL;
> > mr->uobject = uobj;
> > atomic_inc(&pd->usecnt);
> > - mr->res.type = RDMA_RESTRACK_MR;
> > + rdma_rt_set_type(&mr->res, RDMA_RESTRACK_MR);
> > rdma_restrack_uadd(&mr->res);
> >
> > uobj->object = mr;
> > @@ -1015,7 +1015,7 @@ static struct ib_ucq_object *create_cq(struct uverbs_attr_bundle *attrs,
> > resp.base.cqe = cq->cqe;
> > resp.response_length = uverbs_response_length(attrs, sizeof(resp));
> >
> > - cq->res.type = RDMA_RESTRACK_CQ;
> > + rdma_rt_set_type(&cq->res, RDMA_RESTRACK_CQ);
> > rdma_restrack_uadd(&cq->res);
> >
> > ret = uverbs_response(attrs, &resp, sizeof(resp));
> > diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
> > index a59ea89e3f2b..6ca5d5016423 100644
> > +++ b/drivers/infiniband/core/uverbs_std_types_cq.c
> > @@ -125,7 +125,7 @@ static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
> > obj->uobject.object = cq;
> > obj->uobject.user_handle = user_handle;
> > atomic_set(&cq->usecnt, 0);
> > - cq->res.type = RDMA_RESTRACK_CQ;
> > + rdma_rt_set_type(&cq->res, RDMA_RESTRACK_CQ);
> > rdma_restrack_uadd(&cq->res);
> >
> > ret = uverbs_copy_to(attrs, UVERBS_ATTR_CREATE_CQ_RESP_CQE, &cq->cqe,
> > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> > index 3220fb42ecce..814cbc814cf4 100644
> > +++ b/drivers/infiniband/core/verbs.c
> > @@ -275,7 +275,7 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags,
> > mr_access_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
> > }
> >
> > - pd->res.type = RDMA_RESTRACK_PD;
> > + rdma_rt_set_type(&pd->res, RDMA_RESTRACK_PD);
> > rdma_restrack_set_task(&pd->res, caller);
> > rdma_restrack_kadd(&pd->res);
> >
> > @@ -1914,7 +1914,7 @@ struct ib_cq *__ib_create_cq(struct ib_device *device,
> > cq->event_handler = event_handler;
> > cq->cq_context = cq_context;
> > atomic_set(&cq->usecnt, 0);
> > - cq->res.type = RDMA_RESTRACK_CQ;
> > + rdma_rt_set_type(&cq->res, RDMA_RESTRACK_CQ);
> > rdma_restrack_set_task(&cq->res, caller);
> > rdma_restrack_kadd(&cq->res);
> > }
> > @@ -1997,7 +1997,7 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd,
> > mr->uobject = NULL;
> > atomic_inc(&pd->usecnt);
> > mr->need_inval = false;
> > - mr->res.type = RDMA_RESTRACK_MR;
> > + rdma_rt_set_type(&mr->res, RDMA_RESTRACK_MR);
> > rdma_restrack_kadd(&mr->res);
> > }
> >
> > diff --git a/drivers/infiniband/hw/cxgb4/restrack.c b/drivers/infiniband/hw/cxgb4/restrack.c
> > index 9a7520ee41e0..7224742f8b9a 100644
> > +++ b/drivers/infiniband/hw/cxgb4/restrack.c
> > @@ -198,7 +198,7 @@ union union_ep {
> > static int fill_res_ep_entry(struct sk_buff *msg,
> > struct rdma_restrack_entry *res)
> > {
> > - struct rdma_cm_id *cm_id = rdma_res_to_id(res);
> > + struct rdma_cm_id *cm_id = rdma_res_to_cmid(res);
> > struct nlattr *table_attr;
> > struct c4iw_ep_common *epcp;
> > struct c4iw_listen_ep *listen_ep = NULL;
> > diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
> > index 71f48cfdc24c..c1615e71ac0c 100644
> > +++ b/include/rdma/rdma_cm.h
> > @@ -427,6 +427,6 @@ void rdma_read_gids(struct rdma_cm_id *cm_id, union ib_gid *sgid,
> > union ib_gid *dgid);
> >
> > struct iw_cm_id *rdma_iw_cm_id(struct rdma_cm_id *cm_id);
> > -struct rdma_cm_id *rdma_res_to_id(struct rdma_restrack_entry *res);
> > +struct rdma_cm_id *rdma_res_to_cmid(struct rdma_restrack_entry *res);
> >
> > #endif /* RDMA_CM_H */
> > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> > index 92d11c35221a..c81e43d043cf 100644
> > +++ b/include/rdma/restrack.h
> > @@ -161,4 +161,26 @@ struct xarray *rdma_dev_to_xa(struct ib_device *dev,
> > enum rdma_restrack_type type);
> > void rdma_rt_read_lock(struct ib_device *dev, enum rdma_restrack_type type);
> > void rdma_rt_read_unlock(struct ib_device *dev, enum rdma_restrack_type type);
> > +
> > +/**
> > + * rdma_res_to_id() - Unique ID as seen by restrack
> > + * @res: resrouce to operate
> > + *
> > + * Return: unique ID
> > + */
> > +static inline u32 rdma_res_to_id(struct rdma_restrack_entry *res)
> > +{
> > + return res->id;
> > +}
> > +/**
> > + * rdma_rt_set_type() - Set restrack entry type,
> > + * prior to call rdma_restrack_add()
> > + * @res: resrouce to operate
> > + * @type: Actual type
> > + */
> > +static inline void rdma_rt_set_type(struct rdma_restrack_entry *res,
> > + enum rdma_restrack_type type)
> > +{
> > + res->type = type;
> > +}
>
> This seems a bit obfuscating, generally kernel code tries to avoid
> this sort of trivial accessor style, I thought
It is not really correct, kernel code distinguishes between driver code
and core code. Obfuscation is not allowed in drivers, but fine for core
core which needs to hide implementation from their users.
Such difference is due to need to refactor. You will need to refactor
core code in one place, but changes in drivers in many places.
If you are interested to feel the pain of obfuscated driver,
you are invited to do any major changes in bnxt_re driver.
>
> If there is some particular reason to do this it should be mentioned
> in the comment, and possibly this patch should be in a series with the
> patch that needs it.
This patch uses this simple function.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
