On Wed, Oct 21, 2015 at 11:34 AM, Haggai Eran <haggaie@xxxxxxxxxxxx> wrote:
> 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 <eranbe@xxxxxxxxxxxx>
>> ---
>
>> 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()?
You are right, I will remove this extra check and resend.
>
>> + 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?
This patch just refactor in ib_uverbs_create_qp and doesn't change any
of it's logic or fix any bug. we can consider such a fix for the
future.
>
>> +
>> + 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
