From mboxrd@z Thu Jan 1 00:00:00 1970 From: Haggai Eran Subject: Re: [PATCH v2 for-next 1/7] IB/core: Extend ib_uverbs_create_qp Date: Wed, 21 Oct 2015 11:34:10 +0300 Message-ID: <56274E02.7010002@mellanox.com> References: <1444909482-17113-1-git-send-email-eranbe@mellanox.com> <1444909482-17113-2-git-send-email-eranbe@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1444909482-17113-2-git-send-email-eranbe-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Eran Ben Elisha , Doug Ledford Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Or Gerlitz , Christoph Lameter List-Id: linux-rdma@vger.kernel.org On 15/10/2015 14:44, Eran Ben Elisha wrote: > ib_uverbs_ex_create_qp follows the extension verbs > mechanism. New features (for example, QP creation flags > field which is added in a downstream patch) could used > via user-space libraries without breaking the ABI. > > Signed-off-by: Eran Ben Elisha > --- > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > index be4cb9f..e795d59 100644 > --- a/drivers/infiniband/core/uverbs_cmd.c > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -1741,66 +1741,65 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file, > return in_len; > } > > -ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, > - struct ib_device *ib_dev, > - const char __user *buf, int in_len, > - int out_len) > -{ > - struct ib_uverbs_create_qp cmd; > - struct ib_uverbs_create_qp_resp resp; > - struct ib_udata udata; > - struct ib_uqp_object *obj; > - struct ib_device *device; > - struct ib_pd *pd = NULL; > - struct ib_xrcd *xrcd = NULL; > - struct ib_uobject *uninitialized_var(xrcd_uobj); > - struct ib_cq *scq = NULL, *rcq = NULL; > - struct ib_srq *srq = NULL; > - struct ib_qp *qp; > - struct ib_qp_init_attr attr; > - int ret; > - > - if (out_len < sizeof resp) > - return -ENOSPC; > - > - if (copy_from_user(&cmd, buf, sizeof cmd)) > - return -EFAULT; > +static int create_qp(struct ib_uverbs_file *file, > + struct ib_udata *ucore, > + struct ib_udata *uhw, > + struct ib_uverbs_ex_create_qp *cmd, > + size_t cmd_sz, > + int (*cb)(struct ib_uverbs_file *file, > + struct ib_uverbs_ex_create_qp_resp *resp, > + struct ib_udata *udata), > + void *context) > +{ > + struct ib_uqp_object *obj; > + struct ib_device *device; > + struct ib_pd *pd = NULL; > + struct ib_xrcd *xrcd = NULL; > + struct ib_uobject *uninitialized_var(xrcd_uobj); > + struct ib_cq *scq = NULL, *rcq = NULL; > + struct ib_srq *srq = NULL; > + struct ib_qp *qp; > + char *buf; > + struct ib_qp_init_attr attr; > + struct ib_uverbs_ex_create_qp_resp resp; > + int ret; > > - if (cmd.qp_type == IB_QPT_RAW_PACKET && !capable(CAP_NET_RAW)) > + if (cmd->qp_type == IB_QPT_RAW_PACKET && !capable(CAP_NET_RAW)) > return -EPERM; > > - INIT_UDATA(&udata, buf + sizeof cmd, > - (unsigned long) cmd.response + sizeof resp, > - in_len - sizeof cmd, out_len - sizeof resp); > - > obj = kzalloc(sizeof *obj, GFP_KERNEL); > if (!obj) > return -ENOMEM; > > - init_uobj(&obj->uevent.uobject, cmd.user_handle, file->ucontext, &qp_lock_class); > + init_uobj(&obj->uevent.uobject, cmd->user_handle, file->ucontext, > + &qp_lock_class); > down_write(&obj->uevent.uobject.mutex); > > - if (cmd.qp_type == IB_QPT_XRC_TGT) { > - xrcd = idr_read_xrcd(cmd.pd_handle, file->ucontext, &xrcd_uobj); > + if (cmd->qp_type == IB_QPT_XRC_TGT) { > + xrcd = idr_read_xrcd(cmd->pd_handle, file->ucontext, > + &xrcd_uobj); > if (!xrcd) { > ret = -EINVAL; > goto err_put; > } > device = xrcd->device; > } else { > - if (cmd.qp_type == IB_QPT_XRC_INI) { > - cmd.max_recv_wr = cmd.max_recv_sge = 0; > + if (cmd->qp_type == IB_QPT_XRC_INI) { > + cmd->max_recv_wr = 0; > + cmd->max_recv_sge = 0; > } else { > - if (cmd.is_srq) { > - srq = idr_read_srq(cmd.srq_handle, file->ucontext); > + if (cmd->is_srq) { > + srq = idr_read_srq(cmd->srq_handle, > + file->ucontext); > if (!srq || srq->srq_type != IB_SRQT_BASIC) { > ret = -EINVAL; > goto err_put; > } > } > > - if (cmd.recv_cq_handle != cmd.send_cq_handle) { > - rcq = idr_read_cq(cmd.recv_cq_handle, file->ucontext, 0); > + if (cmd->recv_cq_handle != cmd->send_cq_handle) { > + rcq = idr_read_cq(cmd->recv_cq_handle, > + file->ucontext, 0); > if (!rcq) { > ret = -EINVAL; > goto err_put; > @@ -1808,9 +1807,9 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, > } > } > > - scq = idr_read_cq(cmd.send_cq_handle, file->ucontext, !!rcq); > + scq = idr_read_cq(cmd->send_cq_handle, file->ucontext, !!rcq); > rcq = rcq ?: scq; > - pd = idr_read_pd(cmd.pd_handle, file->ucontext); > + pd = idr_read_pd(cmd->pd_handle, file->ucontext); > if (!pd || !scq) { > ret = -EINVAL; > goto err_put; > @@ -1825,31 +1824,54 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, > attr.recv_cq = rcq; > attr.srq = srq; > attr.xrcd = xrcd; > - attr.sq_sig_type = cmd.sq_sig_all ? IB_SIGNAL_ALL_WR : IB_SIGNAL_REQ_WR; > - attr.qp_type = cmd.qp_type; > + attr.sq_sig_type = cmd->sq_sig_all ? IB_SIGNAL_ALL_WR : > + IB_SIGNAL_REQ_WR; > + attr.qp_type = cmd->qp_type; > attr.create_flags = 0; > > - attr.cap.max_send_wr = cmd.max_send_wr; > - attr.cap.max_recv_wr = cmd.max_recv_wr; > - attr.cap.max_send_sge = cmd.max_send_sge; > - attr.cap.max_recv_sge = cmd.max_recv_sge; > - attr.cap.max_inline_data = cmd.max_inline_data; > + attr.cap.max_send_wr = cmd->max_send_wr; > + attr.cap.max_recv_wr = cmd->max_recv_wr; > + attr.cap.max_send_sge = cmd->max_send_sge; > + attr.cap.max_recv_sge = cmd->max_recv_sge; > + attr.cap.max_inline_data = cmd->max_inline_data; > > obj->uevent.events_reported = 0; > INIT_LIST_HEAD(&obj->uevent.event_list); > INIT_LIST_HEAD(&obj->mcast_list); > > - if (cmd.qp_type == IB_QPT_XRC_TGT) > + if (cmd_sz >= offsetof(typeof(*cmd), create_flags) + > + sizeof(cmd->create_flags)) > + attr.create_flags = cmd->create_flags; > + > + if (attr.create_flags) { > + ret = -EINVAL; > + goto err_put; > + } > + > + if (cmd->comp_mask) { Do you need this check in addition to the check in ib_uverbs_ex_create_qp()? > + ret = -EINVAL; > + goto err_put; > + } > + > + buf = (void *)cmd + sizeof(*cmd); > + if (cmd_sz > sizeof(*cmd)) > + if (!(buf[0] == 0 && !memcmp(buf, buf + 1, > + cmd_sz - sizeof(*cmd) - 1))) { > + ret = -EINVAL; > + goto err_put; > + } > + > + if (cmd->qp_type == IB_QPT_XRC_TGT) > qp = ib_create_qp(pd, &attr); > else > - qp = device->create_qp(pd, &attr, &udata); > + qp = device->create_qp(pd, &attr, uhw); > > if (IS_ERR(qp)) { > ret = PTR_ERR(qp); > goto err_put; > } > > - if (cmd.qp_type != IB_QPT_XRC_TGT) { > + if (cmd->qp_type != IB_QPT_XRC_TGT) { > qp->real_qp = qp; > qp->device = device; > qp->pd = pd; > @@ -1875,19 +1897,20 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, > goto err_destroy; > > memset(&resp, 0, sizeof resp); > - resp.qpn = qp->qp_num; > - resp.qp_handle = obj->uevent.uobject.id; > - resp.max_recv_sge = attr.cap.max_recv_sge; > - resp.max_send_sge = attr.cap.max_send_sge; > - resp.max_recv_wr = attr.cap.max_recv_wr; > - resp.max_send_wr = attr.cap.max_send_wr; > - resp.max_inline_data = attr.cap.max_inline_data; > + resp.base.qpn = qp->qp_num; > + resp.base.qp_handle = obj->uevent.uobject.id; > + resp.base.max_recv_sge = attr.cap.max_recv_sge; > + resp.base.max_send_sge = attr.cap.max_send_sge; > + resp.base.max_recv_wr = attr.cap.max_recv_wr; > + resp.base.max_send_wr = attr.cap.max_send_wr; > + resp.base.max_inline_data = attr.cap.max_inline_data; > > - if (copy_to_user((void __user *) (unsigned long) cmd.response, > - &resp, sizeof resp)) { > - ret = -EFAULT; > - goto err_copy; > - } > + resp.response_length = offsetof(typeof(resp), response_length) + > + sizeof(resp.response_length); > + > + ret = cb(file, &resp, ucore); > + if (ret) > + goto err_cb; > > if (xrcd) { > obj->uxrcd = container_of(xrcd_uobj, struct ib_uxrcd_object, > @@ -1913,9 +1936,8 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, > > up_write(&obj->uevent.uobject.mutex); > > - return in_len; > - > -err_copy: > + return 0; > +err_cb: > idr_remove_uobj(&ib_uverbs_qp_idr, &obj->uevent.uobject); > > err_destroy: > @@ -1937,6 +1959,113 @@ err_put: > return ret; > } > > +static int ib_uverbs_create_qp_cb(struct ib_uverbs_file *file, > + struct ib_uverbs_ex_create_qp_resp *resp, > + struct ib_udata *ucore) > +{ > + if (ib_copy_to_udata(ucore, &resp->base, sizeof(resp->base))) > + return -EFAULT; > + > + return 0; > +} > + > +ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file, > + struct ib_device *ib_dev, > + const char __user *buf, int in_len, > + int out_len) > +{ > + struct ib_uverbs_create_qp cmd; > + struct ib_uverbs_ex_create_qp cmd_ex; > + struct ib_udata ucore; > + struct ib_udata uhw; > + ssize_t resp_size = sizeof(struct ib_uverbs_create_qp_resp); > + int err; I would expect a check here that in_len >= sizeof(cmd). But I see the previous code didn't have it either. Any reason adding the check would break user-space? > + > + if (out_len < resp_size) > + return -ENOSPC; > + > + if (copy_from_user(&cmd, buf, sizeof(cmd))) > + return -EFAULT; > + > + INIT_UDATA(&ucore, buf, (unsigned long)cmd.response, sizeof(cmd), > + resp_size); > + INIT_UDATA(&uhw, buf + sizeof(cmd), > + (unsigned long)cmd.response + resp_size, > + in_len - sizeof(cmd), out_len - resp_size); > + > + memset(&cmd_ex, 0, sizeof(cmd_ex)); > + cmd_ex.user_handle = cmd.user_handle; > + cmd_ex.pd_handle = cmd.pd_handle; > + cmd_ex.send_cq_handle = cmd.send_cq_handle; > + cmd_ex.recv_cq_handle = cmd.recv_cq_handle; > + cmd_ex.srq_handle = cmd.srq_handle; > + cmd_ex.max_send_wr = cmd.max_send_wr; > + cmd_ex.max_recv_wr = cmd.max_recv_wr; > + cmd_ex.max_send_sge = cmd.max_send_sge; > + cmd_ex.max_recv_sge = cmd.max_recv_sge; > + cmd_ex.max_inline_data = cmd.max_inline_data; > + cmd_ex.sq_sig_all = cmd.sq_sig_all; > + cmd_ex.qp_type = cmd.qp_type; > + cmd_ex.is_srq = cmd.is_srq; > + > + err = create_qp(file, &ucore, &uhw, &cmd_ex, > + offsetof(typeof(cmd_ex), is_srq) + > + sizeof(cmd.is_srq), ib_uverbs_create_qp_cb, > + NULL); > + > + if (err) > + return err; > + > + return in_len; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html