From: Leon Romanovsky <[email protected]>
Hi,
The "QP allocation" series shows clearly how convoluted the create QP
flow and especially in XRC_TGT flows, where it called to kernel verb
just to pass some parameters as NULL to the user create QP verb.
This series is a small step to make clean XRC_TGT flow by providing
more clean user/kernel create QP verb separation.
It is based on the "QP allocation" series.
Thanks
Leon Romanovsky (7):
RDMA/mlx5: Delete not-available udata check
RDMA/core: Delete duplicated and unreachable code
RDMA/core: Remove protection from wrong in-kernel API usage
RDMA/core: Reorganize create QP low-level functions
RDMA/core: Configure selinux QP during creation
RDMA/core: Properly increment and decrement QP usecnts
RDMA/core: Create clean QP creations interface for uverbs
drivers/infiniband/core/core_priv.h | 67 ++----
drivers/infiniband/core/uverbs_cmd.c | 30 +--
drivers/infiniband/core/uverbs_std_types_qp.c | 28 +--
drivers/infiniband/core/verbs.c | 216 ++++++++++++------
drivers/infiniband/hw/mlx5/qp.c | 3 -
include/rdma/ib_verbs.h | 8 +-
6 files changed, 166 insertions(+), 186 deletions(-)
--
2.31.1
From: Leon Romanovsky <[email protected]>
The ib_create_named_qp() is kernel verb and no kernel users exist that
use XRC_INI QP. Hence such QP path is not reachable. In addition, delete
duplicated assignments of QP attributes from the initialization structure.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/core_priv.h | 1 +
drivers/infiniband/core/verbs.c | 22 ++++------------------
2 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index 5dfa1190e3ea..cc54d74930d6 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -342,6 +342,7 @@ _ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
qp->rwq_ind_tbl = attr->rwq_ind_tbl;
qp->event_handler = attr->event_handler;
qp->port = attr->port_num;
+ qp->qp_context = attr->qp_context;
spin_lock_init(&qp->mr_lock);
INIT_LIST_HEAD(&qp->rdma_mrs);
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 89c6987cb5eb..635642a3ecbc 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1257,28 +1257,14 @@ struct ib_qp *ib_create_named_qp(struct ib_pd *pd,
return xrc_qp;
}
- qp->event_handler = qp_init_attr->event_handler;
- qp->qp_context = qp_init_attr->qp_context;
- if (qp_init_attr->qp_type == IB_QPT_XRC_INI) {
- qp->recv_cq = NULL;
- qp->srq = NULL;
- } else {
- qp->recv_cq = qp_init_attr->recv_cq;
- if (qp_init_attr->recv_cq)
- atomic_inc(&qp_init_attr->recv_cq->usecnt);
- qp->srq = qp_init_attr->srq;
- if (qp->srq)
- atomic_inc(&qp_init_attr->srq->usecnt);
- }
-
- qp->send_cq = qp_init_attr->send_cq;
- qp->xrcd = NULL;
+ if (qp_init_attr->recv_cq)
+ atomic_inc(&qp_init_attr->recv_cq->usecnt);
+ if (qp->srq)
+ atomic_inc(&qp_init_attr->srq->usecnt);
atomic_inc(&pd->usecnt);
if (qp_init_attr->send_cq)
atomic_inc(&qp_init_attr->send_cq->usecnt);
- if (qp_init_attr->rwq_ind_tbl)
- atomic_inc(&qp->rwq_ind_tbl->usecnt);
if (qp_init_attr->cap.max_rdma_ctxs) {
ret = rdma_rw_init_mrs(qp, qp_init_attr);
--
2.31.1
From: Leon Romanovsky <[email protected]>
The ib_create_named_qp() is kernel verb that is not used for user
supplied attributes. In such case, it is ULP responsibility to provide
valid QP attributes.
In-kernel API shouldn't check it, exactly like other functions that
don't check device capabilities.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/verbs.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 635642a3ecbc..2090f3c9f689 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1219,16 +1219,6 @@ struct ib_qp *ib_create_named_qp(struct ib_pd *pd,
struct ib_qp *qp;
int ret;
- if (qp_init_attr->rwq_ind_tbl &&
- (qp_init_attr->recv_cq ||
- qp_init_attr->srq || qp_init_attr->cap.max_recv_wr ||
- qp_init_attr->cap.max_recv_sge))
- return ERR_PTR(-EINVAL);
-
- if ((qp_init_attr->create_flags & IB_QP_CREATE_INTEGRITY_EN) &&
- !(device->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER))
- return ERR_PTR(-EINVAL);
-
/*
* If the callers is using the RDMA API calculate the resources
* needed for the RDMA READ/WRITE operations.
--
2.31.1
From: Leon Romanovsky <[email protected]>
The QP usecnts were incremented through QP attributes structure while
decreased through QP itself. Rely on the ib_creat_qp_user() code that
initialized all QP parameters prior returning to the user and increment
exactly like destroy does.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/core_priv.h | 2 +
drivers/infiniband/core/uverbs_cmd.c | 13 +---
drivers/infiniband/core/uverbs_std_types_qp.c | 13 +---
drivers/infiniband/core/verbs.c | 60 ++++++++++---------
4 files changed, 39 insertions(+), 49 deletions(-)
diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index d28ced053222..d8f464b43dbc 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -320,6 +320,8 @@ struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
struct ib_qp_init_attr *attr,
struct ib_udata *udata, struct ib_uqp_object *uobj,
const char *caller);
+void ib_qp_usecnt_inc(struct ib_qp *qp);
+void ib_qp_usecnt_dec(struct ib_qp *qp);
struct rdma_dev_addr;
int rdma_resolve_ip_route(struct sockaddr *src_addr,
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index b5153200b8a8..62cafd768d89 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1445,18 +1445,9 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
ret = PTR_ERR(qp);
goto err_put;
}
+ ib_qp_usecnt_inc(qp);
- if (cmd->qp_type != IB_QPT_XRC_TGT) {
- atomic_inc(&pd->usecnt);
- if (attr.send_cq)
- atomic_inc(&attr.send_cq->usecnt);
- if (attr.recv_cq)
- atomic_inc(&attr.recv_cq->usecnt);
- if (attr.srq)
- atomic_inc(&attr.srq->usecnt);
- if (ind_tbl)
- atomic_inc(&ind_tbl->usecnt);
- } else {
+ if (cmd->qp_type == IB_QPT_XRC_TGT) {
/* It is done in _ib_create_qp for other QP types */
qp->uobject = obj;
}
diff --git a/drivers/infiniband/core/uverbs_std_types_qp.c b/drivers/infiniband/core/uverbs_std_types_qp.c
index 92812f6a21b0..a0e734735ba5 100644
--- a/drivers/infiniband/core/uverbs_std_types_qp.c
+++ b/drivers/infiniband/core/uverbs_std_types_qp.c
@@ -258,18 +258,9 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE)(
ret = PTR_ERR(qp);
goto err_put;
}
+ ib_qp_usecnt_inc(qp);
- if (attr.qp_type != IB_QPT_XRC_TGT) {
- atomic_inc(&pd->usecnt);
- if (attr.send_cq)
- atomic_inc(&attr.send_cq->usecnt);
- if (attr.recv_cq)
- atomic_inc(&attr.recv_cq->usecnt);
- if (attr.srq)
- atomic_inc(&attr.srq->usecnt);
- if (attr.rwq_ind_tbl)
- atomic_inc(&attr.rwq_ind_tbl->usecnt);
- } else {
+ if (attr.qp_type == IB_QPT_XRC_TGT) {
obj->uxrcd = container_of(xrcd_uobj, struct ib_uxrcd_object,
uobject);
atomic_inc(&obj->uxrcd->refcnt);
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 612c73861e0d..acf866038277 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1275,6 +1275,36 @@ struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
}
EXPORT_SYMBOL(_ib_create_qp);
+void ib_qp_usecnt_inc(struct ib_qp *qp)
+{
+ if (qp->pd)
+ atomic_inc(&qp->pd->usecnt);
+ if (qp->send_cq)
+ atomic_inc(&qp->send_cq->usecnt);
+ if (qp->recv_cq)
+ atomic_inc(&qp->recv_cq->usecnt);
+ if (qp->srq)
+ atomic_inc(&qp->srq->usecnt);
+ if (qp->rwq_ind_tbl)
+ atomic_inc(&qp->rwq_ind_tbl->usecnt);
+}
+EXPORT_SYMBOL(ib_qp_usecnt_inc);
+
+void ib_qp_usecnt_dec(struct ib_qp *qp)
+{
+ if (qp->rwq_ind_tbl)
+ atomic_dec(&qp->rwq_ind_tbl->usecnt);
+ if (qp->srq)
+ atomic_dec(&qp->srq->usecnt);
+ if (qp->recv_cq)
+ atomic_dec(&qp->recv_cq->usecnt);
+ if (qp->send_cq)
+ atomic_dec(&qp->send_cq->usecnt);
+ if (qp->pd)
+ atomic_dec(&qp->pd->usecnt);
+}
+EXPORT_SYMBOL(ib_qp_usecnt_dec);
+
/**
* ib_create_qp_kernel - Creates a kernel QP associated with the specified
* protection domain.
@@ -1316,14 +1346,7 @@ struct ib_qp *ib_create_qp_kernel(struct ib_pd *pd,
return xrc_qp;
}
- if (qp_init_attr->recv_cq)
- atomic_inc(&qp_init_attr->recv_cq->usecnt);
- if (qp->srq)
- atomic_inc(&qp_init_attr->srq->usecnt);
-
- atomic_inc(&pd->usecnt);
- if (qp_init_attr->send_cq)
- atomic_inc(&qp_init_attr->send_cq->usecnt);
+ ib_qp_usecnt_inc(qp);
if (qp_init_attr->cap.max_rdma_ctxs) {
ret = rdma_rw_init_mrs(qp, qp_init_attr);
@@ -1981,10 +2004,6 @@ int ib_destroy_qp_user(struct ib_qp *qp, struct ib_udata *udata)
{
const struct ib_gid_attr *alt_path_sgid_attr = qp->alt_path_sgid_attr;
const struct ib_gid_attr *av_sgid_attr = qp->av_sgid_attr;
- struct ib_pd *pd;
- struct ib_cq *scq, *rcq;
- struct ib_srq *srq;
- struct ib_rwq_ind_table *ind_tbl;
struct ib_qp_security *sec;
int ret;
@@ -1996,11 +2015,6 @@ int ib_destroy_qp_user(struct ib_qp *qp, struct ib_udata *udata)
if (qp->real_qp != qp)
return __ib_destroy_shared_qp(qp);
- pd = qp->pd;
- scq = qp->send_cq;
- rcq = qp->recv_cq;
- srq = qp->srq;
- ind_tbl = qp->rwq_ind_tbl;
sec = qp->qp_sec;
if (sec)
ib_destroy_qp_security_begin(sec);
@@ -2020,16 +2034,8 @@ int ib_destroy_qp_user(struct ib_qp *qp, struct ib_udata *udata)
rdma_put_gid_attr(alt_path_sgid_attr);
if (av_sgid_attr)
rdma_put_gid_attr(av_sgid_attr);
- if (pd)
- atomic_dec(&pd->usecnt);
- if (scq)
- atomic_dec(&scq->usecnt);
- if (rcq)
- atomic_dec(&rcq->usecnt);
- if (srq)
- atomic_dec(&srq->usecnt);
- if (ind_tbl)
- atomic_dec(&ind_tbl->usecnt);
+
+ ib_qp_usecnt_dec(qp);
if (sec)
ib_destroy_qp_security_end(sec);
--
2.31.1
From: Leon Romanovsky <[email protected]>
The low-level create QP function grew to be larger than any sensible
incline function should be. The inline attribute is not really needed
for that function and can be implemented as exported symbol.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/core_priv.h | 59 ++-------------------
drivers/infiniband/core/verbs.c | 82 ++++++++++++++++++++++++++---
include/rdma/ib_verbs.h | 8 +--
3 files changed, 82 insertions(+), 67 deletions(-)
diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index cc54d74930d6..d28ced053222 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -316,61 +316,10 @@ struct ib_device *ib_device_get_by_index(const struct net *net, u32 index);
void nldev_init(void);
void nldev_exit(void);
-static inline struct ib_qp *
-_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
- struct ib_qp_init_attr *attr, struct ib_udata *udata,
- struct ib_uqp_object *uobj, const char *caller)
-{
- struct ib_qp *qp;
- int ret;
-
- if (!dev->ops.create_qp)
- return ERR_PTR(-EOPNOTSUPP);
-
- qp = rdma_zalloc_drv_obj_numa(dev, ib_qp);
- if (!qp)
- return ERR_PTR(-ENOMEM);
-
- qp->device = dev;
- qp->pd = pd;
- qp->uobject = uobj;
- qp->real_qp = qp;
-
- qp->qp_type = attr->qp_type;
- qp->rwq_ind_tbl = attr->rwq_ind_tbl;
- qp->srq = attr->srq;
- qp->rwq_ind_tbl = attr->rwq_ind_tbl;
- qp->event_handler = attr->event_handler;
- qp->port = attr->port_num;
- qp->qp_context = attr->qp_context;
-
- spin_lock_init(&qp->mr_lock);
- INIT_LIST_HEAD(&qp->rdma_mrs);
- INIT_LIST_HEAD(&qp->sig_mrs);
-
- rdma_restrack_new(&qp->res, RDMA_RESTRACK_QP);
- WARN_ONCE(!udata && !caller, "Missing kernel QP owner");
- rdma_restrack_set_name(&qp->res, udata ? NULL : caller);
- ret = dev->ops.create_qp(qp, attr, udata);
- if (ret)
- goto err_create;
-
- /*
- * TODO: The mlx4 internally overwrites send_cq and recv_cq.
- * Unfortunately, it is not an easy task to fix that driver.
- */
- qp->send_cq = attr->send_cq;
- qp->recv_cq = attr->recv_cq;
-
- rdma_restrack_add(&qp->res);
- return qp;
-
-err_create:
- rdma_restrack_put(&qp->res);
- kfree(qp);
- return ERR_PTR(ret);
-
-}
+struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
+ struct ib_qp_init_attr *attr,
+ struct ib_udata *udata, struct ib_uqp_object *uobj,
+ const char *caller);
struct rdma_dev_addr;
int rdma_resolve_ip_route(struct sockaddr *src_addr,
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 2090f3c9f689..645962490fa4 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1201,19 +1201,85 @@ static struct ib_qp *create_xrc_qp_user(struct ib_qp *qp,
}
/**
- * ib_create_named_qp - Creates a kernel QP associated with the specified protection
- * domain.
+ * _ib_create_qp - Creates a QP associated with the specified protection domain
+ * @dev: IB device
+ * @pd: The protection domain associated with the QP.
+ * @attr: A list of initial attributes required to create the
+ * QP. If QP creation succeeds, then the attributes are updated to
+ * the actual capabilities of the created QP.
+ * @udata: User data
+ * @uobj: uverbs obect
+ * @caller: caller's build-time module name
+ */
+struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
+ struct ib_qp_init_attr *attr,
+ struct ib_udata *udata, struct ib_uqp_object *uobj,
+ const char *caller)
+{
+ struct ib_qp *qp;
+ int ret;
+
+ if (!dev->ops.create_qp)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ qp = rdma_zalloc_drv_obj_numa(dev, ib_qp);
+ if (!qp)
+ return ERR_PTR(-ENOMEM);
+
+ qp->device = dev;
+ qp->pd = pd;
+ qp->uobject = uobj;
+ qp->real_qp = qp;
+
+ qp->qp_type = attr->qp_type;
+ qp->rwq_ind_tbl = attr->rwq_ind_tbl;
+ qp->srq = attr->srq;
+ qp->rwq_ind_tbl = attr->rwq_ind_tbl;
+ qp->event_handler = attr->event_handler;
+ qp->port = attr->port_num;
+ qp->qp_context = attr->qp_context;
+
+ spin_lock_init(&qp->mr_lock);
+ INIT_LIST_HEAD(&qp->rdma_mrs);
+ INIT_LIST_HEAD(&qp->sig_mrs);
+
+ rdma_restrack_new(&qp->res, RDMA_RESTRACK_QP);
+ WARN_ONCE(!udata && !caller, "Missing kernel QP owner");
+ rdma_restrack_set_name(&qp->res, udata ? NULL : caller);
+ ret = dev->ops.create_qp(qp, attr, udata);
+ if (ret)
+ goto err_create;
+
+ /*
+ * TODO: The mlx4 internally overwrites send_cq and recv_cq.
+ * Unfortunately, it is not an easy task to fix that driver.
+ */
+ qp->send_cq = attr->send_cq;
+ qp->recv_cq = attr->recv_cq;
+
+ rdma_restrack_add(&qp->res);
+ return qp;
+
+err_create:
+ rdma_restrack_put(&qp->res);
+ kfree(qp);
+ return ERR_PTR(ret);
+
+}
+EXPORT_SYMBOL(_ib_create_qp);
+
+/**
+ * ib_create_qp_kernel - Creates a kernel QP associated with the specified
+ * protection domain.
* @pd: The protection domain associated with the QP.
* @qp_init_attr: A list of initial attributes required to create the
* QP. If QP creation succeeds, then the attributes are updated to
* the actual capabilities of the created QP.
* @caller: caller's build-time module name
- *
- * NOTE: for user qp use ib_create_qp_user with valid udata!
*/
-struct ib_qp *ib_create_named_qp(struct ib_pd *pd,
- struct ib_qp_init_attr *qp_init_attr,
- const char *caller)
+struct ib_qp *ib_create_qp_kernel(struct ib_pd *pd,
+ struct ib_qp_init_attr *qp_init_attr,
+ const char *caller)
{
struct ib_device *device = pd ? pd->device : qp_init_attr->xrcd->device;
struct ib_qp *qp;
@@ -1280,7 +1346,7 @@ struct ib_qp *ib_create_named_qp(struct ib_pd *pd,
return ERR_PTR(ret);
}
-EXPORT_SYMBOL(ib_create_named_qp);
+EXPORT_SYMBOL(ib_create_qp_kernel);
static const struct {
int valid;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 8cd7d1fc719f..6514110168c1 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -3688,13 +3688,13 @@ static inline int ib_post_srq_recv(struct ib_srq *srq,
bad_recv_wr ? : &dummy);
}
-struct ib_qp *ib_create_named_qp(struct ib_pd *pd,
- struct ib_qp_init_attr *qp_init_attr,
- const char *caller);
+struct ib_qp *ib_create_qp_kernel(struct ib_pd *pd,
+ struct ib_qp_init_attr *qp_init_attr,
+ const char *caller);
static inline struct ib_qp *ib_create_qp(struct ib_pd *pd,
struct ib_qp_init_attr *init_attr)
{
- return ib_create_named_qp(pd, init_attr, KBUILD_MODNAME);
+ return ib_create_qp_kernel(pd, init_attr, KBUILD_MODNAME);
}
/**
--
2.31.1
From: Leon Romanovsky <[email protected]>
All QP creation flows called ib_create_qp_security(), but differently.
This caused to the need to provide exclusion conditions for the XRC_TGT,
because such QP already had selinux configuration call.
In order to fix it, move ib_create_qp_security() to the general QP
creation routine.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/uverbs_cmd.c | 7 -------
drivers/infiniband/core/uverbs_std_types_qp.c | 6 ------
drivers/infiniband/core/verbs.c | 11 +++++++----
3 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 8c8ca7bce3ca..b5153200b8a8 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1447,10 +1447,6 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
}
if (cmd->qp_type != IB_QPT_XRC_TGT) {
- ret = ib_create_qp_security(qp, device);
- if (ret)
- goto err_cb;
-
atomic_inc(&pd->usecnt);
if (attr.send_cq)
atomic_inc(&attr.send_cq->usecnt);
@@ -1502,9 +1498,6 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
resp.response_length = uverbs_response_length(attrs, sizeof(resp));
return uverbs_response(attrs, &resp, sizeof(resp));
-err_cb:
- ib_destroy_qp_user(qp, uverbs_get_cleared_udata(attrs));
-
err_put:
if (!IS_ERR(xrcd_uobj))
uobj_put_read(xrcd_uobj);
diff --git a/drivers/infiniband/core/uverbs_std_types_qp.c b/drivers/infiniband/core/uverbs_std_types_qp.c
index c00cfb5ed387..92812f6a21b0 100644
--- a/drivers/infiniband/core/uverbs_std_types_qp.c
+++ b/drivers/infiniband/core/uverbs_std_types_qp.c
@@ -280,12 +280,6 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE)(
obj->uevent.uobject.object = qp;
uverbs_finalize_uobj_create(attrs, UVERBS_ATTR_CREATE_QP_HANDLE);
- if (attr.qp_type != IB_QPT_XRC_TGT) {
- ret = ib_create_qp_security(qp, device);
- if (ret)
- return ret;
- }
-
set_caps(&attr, &cap, false);
ret = uverbs_copy_to_struct_or_zero(attrs,
UVERBS_ATTR_CREATE_QP_RESP_CAP, &cap,
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 645962490fa4..612c73861e0d 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1216,6 +1216,7 @@ struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
struct ib_udata *udata, struct ib_uqp_object *uobj,
const char *caller)
{
+ struct ib_udata dummy = {};
struct ib_qp *qp;
int ret;
@@ -1257,9 +1258,15 @@ struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
qp->send_cq = attr->send_cq;
qp->recv_cq = attr->recv_cq;
+ ret = ib_create_qp_security(qp, dev);
+ if (ret)
+ goto err_security;
+
rdma_restrack_add(&qp->res);
return qp;
+err_security:
+ qp->device->ops.destroy_qp(qp, udata ? &dummy : NULL);
err_create:
rdma_restrack_put(&qp->res);
kfree(qp);
@@ -1298,10 +1305,6 @@ struct ib_qp *ib_create_qp_kernel(struct ib_pd *pd,
if (IS_ERR(qp))
return qp;
- ret = ib_create_qp_security(qp, device);
- if (ret)
- goto err;
-
if (qp_init_attr->qp_type == IB_QPT_XRC_TGT) {
struct ib_qp *xrc_qp =
create_xrc_qp_user(qp, qp_init_attr);
--
2.31.1
From: Leon Romanovsky <[email protected]>
XRC_TGT QPs are created through kernel verbs and don't have udata at all.
Fixes: 6eefa839c4dd ("RDMA/mlx5: Protect from kernel crash if XRC_TGT doesn't have udata")
Fixes: e383085c2425 ("RDMA/mlx5: Set ECE options during QP create")
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/hw/mlx5/qp.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 18b018f1db83..81e3170a1ae6 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -1908,7 +1908,6 @@ static int get_atomic_mode(struct mlx5_ib_dev *dev,
static int create_xrc_tgt_qp(struct mlx5_ib_dev *dev, struct mlx5_ib_qp *qp,
struct mlx5_create_qp_params *params)
{
- struct mlx5_ib_create_qp *ucmd = params->ucmd;
struct ib_qp_init_attr *attr = params->attr;
u32 uidx = params->uidx;
struct mlx5_ib_resources *devr = &dev->devr;
@@ -1928,8 +1927,6 @@ static int create_xrc_tgt_qp(struct mlx5_ib_dev *dev, struct mlx5_ib_qp *qp,
if (!in)
return -ENOMEM;
- if (MLX5_CAP_GEN(mdev, ece_support) && ucmd)
- MLX5_SET(create_qp_in, in, ece, ucmd->ece_options);
qpc = MLX5_ADDR_OF(create_qp_in, in, qpc);
MLX5_SET(qpc, qpc, st, MLX5_QP_ST_XRC);
--
2.31.1
From: Leon Romanovsky <[email protected]>
Unify create QP creation interface to make clean approach to create
XRC_TGT and regular QPs.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/infiniband/core/core_priv.h | 21 +++++-
drivers/infiniband/core/uverbs_cmd.c | 12 +--
drivers/infiniband/core/uverbs_std_types_qp.c | 9 +--
drivers/infiniband/core/verbs.c | 73 +++++++++++--------
4 files changed, 63 insertions(+), 52 deletions(-)
diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index d8f464b43dbc..dd4c2e560b59 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -316,10 +316,23 @@ struct ib_device *ib_device_get_by_index(const struct net *net, u32 index);
void nldev_init(void);
void nldev_exit(void);
-struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
- struct ib_qp_init_attr *attr,
- struct ib_udata *udata, struct ib_uqp_object *uobj,
- const char *caller);
+struct ib_qp *ib_create_qp_user(struct ib_device *dev, struct ib_pd *pd,
+ struct ib_qp_init_attr *attr,
+ struct ib_udata *udata,
+ struct ib_uqp_object *uobj, const char *caller);
+static inline struct ib_qp *ib_create_qp_uverbs(struct ib_device *dev,
+ struct ib_pd *pd,
+ struct ib_qp_init_attr *attr,
+ struct ib_udata *udata,
+ struct ib_uqp_object *uobj)
+{
+ if (attr->qp_type == IB_QPT_XRC_TGT)
+ return ib_create_qp_user(dev, pd, attr, NULL, uobj,
+ KBUILD_MODNAME);
+
+ return ib_create_qp_user(dev, pd, attr, udata, uobj, NULL);
+}
+
void ib_qp_usecnt_inc(struct ib_qp *qp);
void ib_qp_usecnt_dec(struct ib_qp *qp);
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 62cafd768d89..39206ecdeaad 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1435,23 +1435,13 @@ static int create_qp(struct uverbs_attr_bundle *attrs,
attr.source_qpn = cmd->source_qpn;
}
- if (cmd->qp_type == IB_QPT_XRC_TGT)
- qp = ib_create_qp(pd, &attr);
- else
- qp = _ib_create_qp(device, pd, &attr, &attrs->driver_udata, obj,
- NULL);
-
+ qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
if (IS_ERR(qp)) {
ret = PTR_ERR(qp);
goto err_put;
}
ib_qp_usecnt_inc(qp);
- if (cmd->qp_type == IB_QPT_XRC_TGT) {
- /* It is done in _ib_create_qp for other QP types */
- qp->uobject = obj;
- }
-
obj->uevent.uobject.object = qp;
obj->uevent.event_file = READ_ONCE(attrs->ufile->default_async_file);
if (obj->uevent.event_file)
diff --git a/drivers/infiniband/core/uverbs_std_types_qp.c b/drivers/infiniband/core/uverbs_std_types_qp.c
index a0e734735ba5..15ea00188363 100644
--- a/drivers/infiniband/core/uverbs_std_types_qp.c
+++ b/drivers/infiniband/core/uverbs_std_types_qp.c
@@ -248,12 +248,7 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE)(
set_caps(&attr, &cap, true);
mutex_init(&obj->mcast_lock);
- if (attr.qp_type == IB_QPT_XRC_TGT)
- qp = ib_create_qp(pd, &attr);
- else
- qp = _ib_create_qp(device, pd, &attr, &attrs->driver_udata, obj,
- NULL);
-
+ qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
if (IS_ERR(qp)) {
ret = PTR_ERR(qp);
goto err_put;
@@ -264,8 +259,6 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QP_CREATE)(
obj->uxrcd = container_of(xrcd_uobj, struct ib_uxrcd_object,
uobject);
atomic_inc(&obj->uxrcd->refcnt);
- /* It is done in _ib_create_qp for other QP types */
- qp->uobject = obj;
}
obj->uevent.uobject.object = qp;
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index acf866038277..9414fa8b54c4 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1200,21 +1200,10 @@ static struct ib_qp *create_xrc_qp_user(struct ib_qp *qp,
return qp;
}
-/**
- * _ib_create_qp - Creates a QP associated with the specified protection domain
- * @dev: IB device
- * @pd: The protection domain associated with the QP.
- * @attr: A list of initial attributes required to create the
- * QP. If QP creation succeeds, then the attributes are updated to
- * the actual capabilities of the created QP.
- * @udata: User data
- * @uobj: uverbs obect
- * @caller: caller's build-time module name
- */
-struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
- struct ib_qp_init_attr *attr,
- struct ib_udata *udata, struct ib_uqp_object *uobj,
- const char *caller)
+static struct ib_qp *create_qp(struct ib_device *dev, struct ib_pd *pd,
+ struct ib_qp_init_attr *attr,
+ struct ib_udata *udata,
+ struct ib_uqp_object *uobj, const char *caller)
{
struct ib_udata dummy = {};
struct ib_qp *qp;
@@ -1273,7 +1262,44 @@ struct ib_qp *_ib_create_qp(struct ib_device *dev, struct ib_pd *pd,
return ERR_PTR(ret);
}
-EXPORT_SYMBOL(_ib_create_qp);
+
+/**
+ * ib_create_qp_user - Creates a QP associated with the specified protection
+ * domain.
+ * @dev: IB device
+ * @pd: The protection domain associated with the QP.
+ * @attr: A list of initial attributes required to create the
+ * QP. If QP creation succeeds, then the attributes are updated to
+ * the actual capabilities of the created QP.
+ * @udata: User data
+ * @uobj: uverbs obect
+ * @caller: caller's build-time module name
+ */
+struct ib_qp *ib_create_qp_user(struct ib_device *dev, struct ib_pd *pd,
+ struct ib_qp_init_attr *attr,
+ struct ib_udata *udata,
+ struct ib_uqp_object *uobj, const char *caller)
+{
+ struct ib_uqp_object *obj = uobj;
+ struct ib_qp *qp, *xrc_qp;
+
+ if (attr->qp_type == IB_QPT_XRC_TGT)
+ obj = NULL;
+
+ qp = create_qp(dev, pd, attr, udata, obj, caller);
+ if (attr->qp_type != IB_QPT_XRC_TGT || IS_ERR(qp))
+ return qp;
+
+ xrc_qp = create_xrc_qp_user(qp, attr);
+ if (IS_ERR(xrc_qp)) {
+ ib_destroy_qp(qp);
+ return xrc_qp;
+ }
+
+ xrc_qp->uobject = uobj;
+ return xrc_qp;
+}
+EXPORT_SYMBOL(ib_create_qp_user);
void ib_qp_usecnt_inc(struct ib_qp *qp)
{
@@ -1318,7 +1344,7 @@ struct ib_qp *ib_create_qp_kernel(struct ib_pd *pd,
struct ib_qp_init_attr *qp_init_attr,
const char *caller)
{
- struct ib_device *device = pd ? pd->device : qp_init_attr->xrcd->device;
+ struct ib_device *device = pd->device;
struct ib_qp *qp;
int ret;
@@ -1331,21 +1357,10 @@ struct ib_qp *ib_create_qp_kernel(struct ib_pd *pd,
if (qp_init_attr->cap.max_rdma_ctxs)
rdma_rw_init_qp(device, qp_init_attr);
- qp = _ib_create_qp(device, pd, qp_init_attr, NULL, NULL, caller);
+ qp = create_qp(device, pd, qp_init_attr, NULL, NULL, caller);
if (IS_ERR(qp))
return qp;
- if (qp_init_attr->qp_type == IB_QPT_XRC_TGT) {
- struct ib_qp *xrc_qp =
- create_xrc_qp_user(qp, qp_init_attr);
-
- if (IS_ERR(xrc_qp)) {
- ret = PTR_ERR(xrc_qp);
- goto err;
- }
- return xrc_qp;
- }
-
ib_qp_usecnt_inc(qp);
if (qp_init_attr->cap.max_rdma_ctxs) {
--
2.31.1
On Wed, Jul 21, 2021 at 09:13:02AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <[email protected]>
>
> The ib_create_named_qp() is kernel verb that is not used for user
> supplied attributes. In such case, it is ULP responsibility to provide
> valid QP attributes.
>
> In-kernel API shouldn't check it, exactly like other functions that
> don't check device capabilities.
Hmm. These still looks like useful debugging checks.
> +/**
> + * ib_create_qp_kernel - Creates a kernel QP associated with the specified
Any reason this function is renamed? This seems rather unrelated to
the rest of th patch.
> + * protection domain.
> * @pd: The protection domain associated with the QP.
> * @qp_init_attr: A list of initial attributes required to create the
> * QP. If QP creation succeeds, then the attributes are updated to
> * the actual capabilities of the created QP.
> * @caller: caller's build-time module name
> - *
> - * NOTE: for user qp use ib_create_qp_user with valid udata!
> */
Also a kerneldoc comment for a function that is an implementation
detail is actively harmful. Please document ib_create_qp instead.
> +struct ib_qp *ib_create_qp_user(struct ib_device *dev, struct ib_pd *pd,
> + struct ib_qp_init_attr *attr,
> + struct ib_udata *udata,
> + struct ib_uqp_object *uobj, const char *caller);
> +static inline struct ib_qp *ib_create_qp_uverbs(struct ib_device *dev,
> + struct ib_pd *pd,
> + struct ib_qp_init_attr *attr,
> + struct ib_udata *udata,
> + struct ib_uqp_object *uobj)
> +{
> + if (attr->qp_type == IB_QPT_XRC_TGT)
> + return ib_create_qp_user(dev, pd, attr, NULL, uobj,
> + KBUILD_MODNAME);
> +
> + return ib_create_qp_user(dev, pd, attr, udata, uobj, NULL);
Why not always pass along the udata and caller and just not use them
in the low-level code?
On Wed, Jul 21, 2021 at 07:41:12AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 21, 2021 at 09:13:02AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <[email protected]>
> >
> > The ib_create_named_qp() is kernel verb that is not used for user
> > supplied attributes. In such case, it is ULP responsibility to provide
> > valid QP attributes.
> >
> > In-kernel API shouldn't check it, exactly like other functions that
> > don't check device capabilities.
>
> Hmm. These still looks like useful debugging checks.
All in-tree ULPs already set properly based on device capabilities.
Thanks
On Wed, Jul 21, 2021 at 07:45:48AM +0100, Christoph Hellwig wrote:
> > +/**
> > + * ib_create_qp_kernel - Creates a kernel QP associated with the specified
>
> Any reason this function is renamed? This seems rather unrelated to
> the rest of th patch.
I wanted to make two functions: ib_create_qp_kernel and ib_create_qp_user.
>
> > + * protection domain.
> > * @pd: The protection domain associated with the QP.
> > * @qp_init_attr: A list of initial attributes required to create the
> > * QP. If QP creation succeeds, then the attributes are updated to
> > * the actual capabilities of the created QP.
> > * @caller: caller's build-time module name
> > - *
> > - * NOTE: for user qp use ib_create_qp_user with valid udata!
> > */
>
> Also a kerneldoc comment for a function that is an implementation
> detail is actively harmful. Please document ib_create_qp instead.
Sure, will do.
On Wed, Jul 21, 2021 at 07:48:48AM +0100, Christoph Hellwig wrote:
> > +struct ib_qp *ib_create_qp_user(struct ib_device *dev, struct ib_pd *pd,
> > + struct ib_qp_init_attr *attr,
> > + struct ib_udata *udata,
> > + struct ib_uqp_object *uobj, const char *caller);
> > +static inline struct ib_qp *ib_create_qp_uverbs(struct ib_device *dev,
> > + struct ib_pd *pd,
> > + struct ib_qp_init_attr *attr,
> > + struct ib_udata *udata,
> > + struct ib_uqp_object *uobj)
> > +{
> > + if (attr->qp_type == IB_QPT_XRC_TGT)
> > + return ib_create_qp_user(dev, pd, attr, NULL, uobj,
> > + KBUILD_MODNAME);
> > +
> > + return ib_create_qp_user(dev, pd, attr, udata, uobj, NULL);
>
> Why not always pass along the udata and caller and just not use them
> in the low-level code?
You will need to add some sort of "if qp tpye" for ib_create_qp_uverbs() callers,
because they always provide udata != NULL.
After this series, the callers look like this:
1438 qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
^^^^^^^^^ not NULL
So instead of bothering callers, I implemented it here with one "if".
Thanks
On Wed, Jul 21, 2021 at 10:06:06AM +0300, Leon Romanovsky wrote:
> You will need to add some sort of "if qp tpye" for ib_create_qp_uverbs() callers,
> because they always provide udata != NULL.
>
> After this series, the callers look like this:
>
> 1438 qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
> ^^^^^^^^^ not NULL
>
> So instead of bothering callers, I implemented it here with one "if".
Sorry if my mail was confusing. I don't want it in the callers, I
want it as deep down in the stack as possible instead of having the
strange wrapper.
On Wed, Jul 21, 2021 at 08:12:35AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 21, 2021 at 10:06:06AM +0300, Leon Romanovsky wrote:
> > You will need to add some sort of "if qp tpye" for ib_create_qp_uverbs() callers,
> > because they always provide udata != NULL.
> >
> > After this series, the callers look like this:
> >
> > 1438 qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
> > ^^^^^^^^^ not NULL
> >
> > So instead of bothering callers, I implemented it here with one "if".
>
> Sorry if my mail was confusing. I don't want it in the callers, I
> want it as deep down in the stack as possible instead of having the
> strange wrapper.
In fact ib_create_qp_user already sets udata to NULL for IB_QPT_XRC_TGT,
and _ib_create_qp/create_qp ignores the caller if the udata is NULL.
So I think you can just remove the wrapper and we're already fine.
On Wed, Jul 21, 2021 at 08:12:35AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 21, 2021 at 10:06:06AM +0300, Leon Romanovsky wrote:
> > You will need to add some sort of "if qp tpye" for ib_create_qp_uverbs() callers,
> > because they always provide udata != NULL.
> >
> > After this series, the callers look like this:
> >
> > 1438 qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
> > ^^^^^^^^^ not NULL
> >
> > So instead of bothering callers, I implemented it here with one "if".
>
> Sorry if my mail was confusing. I don't want it in the callers, I
> want it as deep down in the stack as possible instead of having the
> strange wrapper.
I see, will change and resend.
Thanks
On Wed, Jul 21, 2021 at 08:20:49AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 21, 2021 at 08:12:35AM +0100, Christoph Hellwig wrote:
> > On Wed, Jul 21, 2021 at 10:06:06AM +0300, Leon Romanovsky wrote:
> > > You will need to add some sort of "if qp tpye" for ib_create_qp_uverbs() callers,
> > > because they always provide udata != NULL.
> > >
> > > After this series, the callers look like this:
> > >
> > > 1438 qp = ib_create_qp_uverbs(device, pd, &attr, &attrs->driver_udata, obj);
> > > ^^^^^^^^^ not NULL
> > >
> > > So instead of bothering callers, I implemented it here with one "if".
> >
> > Sorry if my mail was confusing. I don't want it in the callers, I
> > want it as deep down in the stack as possible instead of having the
> > strange wrapper.
>
> In fact ib_create_qp_user already sets udata to NULL for IB_QPT_XRC_TGT,
> and _ib_create_qp/create_qp ignores the caller if the udata is NULL.
> So I think you can just remove the wrapper and we're already fine.
Yeah, thanks
On 7/21/2021 2:13 PM, Leon Romanovsky wrote:
> External email: Use caution opening links or attachments
>
>
> From: Leon Romanovsky <[email protected]>
>
> The low-level create QP function grew to be larger than any sensible
> incline function should be. The inline attribute is not really needed
> for that function and can be implemented as exported symbol.
incline -> inline?
On Wed, Jul 21, 2021 at 03:47:34PM +0800, Mark Zhang wrote:
> On 7/21/2021 2:13 PM, Leon Romanovsky wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > From: Leon Romanovsky <[email protected]>
> >
> > The low-level create QP function grew to be larger than any sensible
> > incline function should be. The inline attribute is not really needed
> > for that function and can be implemented as exported symbol.
>
> incline -> inline?
Thanks
>
>