[PATCH 0/2] User-index fixes

[PATCH 0/2] User-index fixes

[Matan Barak]

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Hi Doug,

This series fixes two issues introduced in 4.5 after applying my
series "Raw Packet QP user-space support for mlx5".

SRQs don't use user-index, therefore the user shouldn't pass it and the
kernel shouldn't verify. The first patch fixes this by avoiding
user-index verification for non-xrc srqs.

The mlx5_ib driver fails libraries that support user-index but don't
pass it. It relies on the input length of the vendor specific part to
determine whether the user-space supports user-index.


In Legacy verbs, the given vendor specific part input length is given
including struct ib_uverbs_cmd_hdr. In contrast, extended commands
pass the same length exluding the header size.
The vendor driver doesn't know whether the command is a legacy
command or an extended command. Thus, we need to modify IB/core
in order to pass a consistent size. The second patch fixes the input
length in the IB/core and the wrong usage in the mlx5_ib.

We verified that other vendors don't use the inlen parameter and thus
won't break by this change.

Thanks,
Majd and Matan


Majd Dibbiny (2):
  IB/mlx5: Avoid using user-index for SRQs
  IB/{core, mlx5}: Fix input len in vendor specific part of
    create_qp/srq cmd

 drivers/infiniband/core/uverbs_cmd.c |  9 +++++---
 drivers/infiniband/hw/mlx5/srq.c     | 41 +++++++++++++++++++-----------------
 2 files changed, 28 insertions(+), 22 deletions(-)

-- 
1.8.3.1

--
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

[PATCH 1/2] IB/mlx5: Avoid using user-index for SRQs

[Matan Barak]

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Normal SRQs, unlike XRC SRQs, don't have user-index, therefore
avoid verifying it and using it.

Fixes: cfb5e088e26a ('IB/mlx5: Add CQE version 1 support to user QPs and SRQs')
Signed-off-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/srq.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index 4659256..a1b7122 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -75,7 +75,8 @@ static void mlx5_ib_srq_event(struct mlx5_core_srq *srq, enum mlx5_event type)
 
 static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 			   struct mlx5_create_srq_mbox_in **in,
-			   struct ib_udata *udata, int buf_size, int *inlen)
+			   struct ib_udata *udata, int buf_size, int *inlen,
+			   int is_xrc)
 {
 	struct mlx5_ib_dev *dev = to_mdev(pd->device);
 	struct mlx5_ib_create_srq ucmd = {};
@@ -108,10 +109,12 @@ static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 				 drv_data - sizeof(ucmd)))
 		return -EINVAL;
 
-	err = get_srq_user_index(to_mucontext(pd->uobject->context),
-				 &ucmd, udata->inlen, &uidx);
-	if (err)
-		return err;
+	if (is_xrc) {
+		err = get_srq_user_index(to_mucontext(pd->uobject->context),
+					 &ucmd, udata->inlen, &uidx);
+		if (err)
+			return err;
+	}
 
 	srq->wq_sig = !!(ucmd.flags & MLX5_SRQ_FLAG_SIGNATURE);
 
@@ -151,7 +154,8 @@ static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 	(*in)->ctx.log_pg_sz = page_shift - MLX5_ADAPTER_PAGE_SHIFT;
 	(*in)->ctx.pgoff_cqn = cpu_to_be32(offset << 26);
 
-	if (MLX5_CAP_GEN(dev->mdev, cqe_version) == MLX5_CQE_VERSION_V1) {
+	if ((MLX5_CAP_GEN(dev->mdev, cqe_version) == MLX5_CQE_VERSION_V1) &&
+	     is_xrc){
 		xsrqc = MLX5_ADDR_OF(create_xrc_srq_in, *in,
 				     xrc_srq_context_entry);
 		MLX5_SET(xrc_srqc, xsrqc, user_index, uidx);
@@ -170,7 +174,7 @@ err_umem:
 
 static int create_srq_kernel(struct mlx5_ib_dev *dev, struct mlx5_ib_srq *srq,
 			     struct mlx5_create_srq_mbox_in **in, int buf_size,
-			     int *inlen)
+			     int *inlen, int is_xrc)
 {
 	int err;
 	int i;
@@ -224,7 +228,8 @@ static int create_srq_kernel(struct mlx5_ib_dev *dev, struct mlx5_ib_srq *srq,
 
 	(*in)->ctx.log_pg_sz = page_shift - MLX5_ADAPTER_PAGE_SHIFT;
 
-	if (MLX5_CAP_GEN(dev->mdev, cqe_version) == MLX5_CQE_VERSION_V1) {
+	if ((MLX5_CAP_GEN(dev->mdev, cqe_version) == MLX5_CQE_VERSION_V1) &&
+	     is_xrc){
 		xsrqc = MLX5_ADDR_OF(create_xrc_srq_in, *in,
 				     xrc_srq_context_entry);
 		/* 0xffffff means we ask to work with cqe version 0 */
@@ -302,10 +307,14 @@ struct ib_srq *mlx5_ib_create_srq(struct ib_pd *pd,
 		    desc_size, init_attr->attr.max_wr, srq->msrq.max, srq->msrq.max_gs,
 		    srq->msrq.max_avail_gather);
 
+	is_xrc = (init_attr->srq_type == IB_SRQT_XRC);
+
 	if (pd->uobject)
-		err = create_srq_user(pd, srq, &in, udata, buf_size, &inlen);
+		err = create_srq_user(pd, srq, &in, udata, buf_size, &inlen,
+				      is_xrc);
 	else
-		err = create_srq_kernel(dev, srq, &in, buf_size, &inlen);
+		err = create_srq_kernel(dev, srq, &in, buf_size, &inlen,
+					is_xrc);
 
 	if (err) {
 		mlx5_ib_warn(dev, "create srq %s failed, err %d\n",
@@ -313,7 +322,6 @@ struct ib_srq *mlx5_ib_create_srq(struct ib_pd *pd,
 		goto err_srq;
 	}
 
-	is_xrc = (init_attr->srq_type == IB_SRQT_XRC);
 	in->ctx.state_log_sz = ilog2(srq->msrq.max);
 	flgs = ((srq->msrq.wqe_shift - 4) | (is_xrc << 5) | (srq->wq_sig << 7)) << 24;
 	xrcdn = 0;
-- 
1.8.3.1

--
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

[PATCH 2/2] IB/{core, mlx5}: Fix input len in vendor specific part of create_qp/srq cmd

[Matan Barak]

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Currently, the inlen field of the vendor's part of the command
doesn't match the command buffer. This happens because the inlen
accommodates ib_uverbs_cmd_hdr which is deducted from the in buffer.
This is problematic since the vendor function could be called either
from the legacy verb (where the input length mismatches the actual
length) or by the extended verb (where the length matches). The vendor
has no idea which function calls it and therefore has no way to know
how the length variable should be treated.

Fixing this by aligning the inlen to the correct length.

All vendor drivers either assumed that inlen >= sizeof(vendor_uhw_cmd)
or just failed wrongly (mlx5) and fixed in this patch.

Fixes: cfb5e088e26a ('IB/mlx5: Add CQE version 1 support to user QPs and SRQs')
Signed-off-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c |  9 ++++++---
 drivers/infiniband/hw/mlx5/srq.c     | 11 +++--------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 6ffc9c4..6c6fbff 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1970,7 +1970,8 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 		   resp_size);
 	INIT_UDATA(&uhw, buf + sizeof(cmd),
 		   (unsigned long)cmd.response + resp_size,
-		   in_len - sizeof(cmd), out_len - resp_size);
+		   in_len - sizeof(cmd) - sizeof(struct ib_uverbs_cmd_hdr),
+		   out_len - resp_size);
 
 	memset(&cmd_ex, 0, sizeof(cmd_ex));
 	cmd_ex.user_handle = cmd.user_handle;
@@ -3413,7 +3414,8 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   (unsigned long) cmd.response + sizeof resp,
-		   in_len - sizeof cmd, out_len - sizeof resp);
+		   in_len - sizeof cmd - sizeof(struct ib_uverbs_cmd_hdr),
+		   out_len - sizeof resp);
 
 	ret = __uverbs_create_xsrq(file, ib_dev, &xcmd, &udata);
 	if (ret)
@@ -3439,7 +3441,8 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
 		   (unsigned long) cmd.response + sizeof resp,
-		   in_len - sizeof cmd, out_len - sizeof resp);
+		   in_len - sizeof cmd - sizeof(struct ib_uverbs_cmd_hdr),
+		   out_len - sizeof resp);
 
 	ret = __uverbs_create_xsrq(file, ib_dev, &cmd, &udata);
 	if (ret)
diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index a1b7122..3b2ddd6 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -88,13 +88,8 @@ static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 	int ncont;
 	u32 offset;
 	u32 uidx = MLX5_IB_DEFAULT_UIDX;
-	int drv_data = udata->inlen - sizeof(struct ib_uverbs_cmd_hdr);
 
-	if (drv_data < 0)
-		return -EINVAL;
-
-	ucmdlen = (drv_data < sizeof(ucmd)) ?
-		  drv_data : sizeof(ucmd);
+	ucmdlen = min(udata->inlen, sizeof(ucmd));
 
 	if (ib_copy_from_udata(&ucmd, udata, ucmdlen)) {
 		mlx5_ib_dbg(dev, "failed copy udata\n");
@@ -104,9 +99,9 @@ static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 	if (ucmd.reserved0 || ucmd.reserved1)
 		return -EINVAL;
 
-	if (drv_data > sizeof(ucmd) &&
+	if (udata->inlen > sizeof(ucmd) &&
 	    !ib_is_udata_cleared(udata, sizeof(ucmd),
-				 drv_data - sizeof(ucmd)))
+				 udata->inlen - sizeof(ucmd)))
 		return -EINVAL;
 
 	if (is_xrc) {
-- 
1.8.3.1

--
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

Re: [PATCH 0/2] User-index fixes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

On 02/14/2016 11:35 AM, Matan Barak wrote:
> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Hi Doug,
> 
> This series fixes two issues introduced in 4.5 after applying my
> series "Raw Packet QP user-space support for mlx5".
> 
> SRQs don't use user-index, therefore the user shouldn't pass it and the
> kernel shouldn't verify. The first patch fixes this by avoiding
> user-index verification for non-xrc srqs.
> 
> The mlx5_ib driver fails libraries that support user-index but don't
> pass it. It relies on the input length of the vendor specific part to
> determine whether the user-space supports user-index.
> 
> 
> In Legacy verbs, the given vendor specific part input length is given
> including struct ib_uverbs_cmd_hdr. In contrast, extended commands
> pass the same length exluding the header size.
> The vendor driver doesn't know whether the command is a legacy
> command or an extended command. Thus, we need to modify IB/core
> in order to pass a consistent size. The second patch fixes the input
> length in the IB/core and the wrong usage in the mlx5_ib.
> 
> We verified that other vendors don't use the inlen parameter and thus
> won't break by this change.
> 
> Thanks,
> Majd and Matan
> 
> 
> Majd Dibbiny (2):
>   IB/mlx5: Avoid using user-index for SRQs
>   IB/{core, mlx5}: Fix input len in vendor specific part of
>     create_qp/srq cmd
> 
>  drivers/infiniband/core/uverbs_cmd.c |  9 +++++---
>  drivers/infiniband/hw/mlx5/srq.c     | 41 +++++++++++++++++++-----------------
>  2 files changed, 28 insertions(+), 22 deletions(-)
> 

These went in already in case you missed it.  Thanks.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH 0/2] User-index fixes

[Majd Dibbiny]

On 17/2/2016 6:19 PM, Doug Ledford wrote:
> On 02/14/2016 11:35 AM, Matan Barak wrote:
>> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> Hi Doug,
>>
>> This series fixes two issues introduced in 4.5 after applying my
>> series "Raw Packet QP user-space support for mlx5".
>>
>> SRQs don't use user-index, therefore the user shouldn't pass it and the
>> kernel shouldn't verify. The first patch fixes this by avoiding
>> user-index verification for non-xrc srqs.
>>
>> The mlx5_ib driver fails libraries that support user-index but don't
>> pass it. It relies on the input length of the vendor specific part to
>> determine whether the user-space supports user-index.
>>
>>
>> In Legacy verbs, the given vendor specific part input length is given
>> including struct ib_uverbs_cmd_hdr. In contrast, extended commands
>> pass the same length exluding the header size.
>> The vendor driver doesn't know whether the command is a legacy
>> command or an extended command. Thus, we need to modify IB/core
>> in order to pass a consistent size. The second patch fixes the input
>> length in the IB/core and the wrong usage in the mlx5_ib.
>>
>> We verified that other vendors don't use the inlen parameter and thus
>> won't break by this change.
>>
>> Thanks,
>> Majd and Matan
>>
>>
>> Majd Dibbiny (2):
>>   IB/mlx5: Avoid using user-index for SRQs
>>   IB/{core, mlx5}: Fix input len in vendor specific part of
>>     create_qp/srq cmd
>>
>>  drivers/infiniband/core/uverbs_cmd.c |  9 +++++---
>>  drivers/infiniband/hw/mlx5/srq.c     | 41 +++++++++++++++++++-----------------
>>  2 files changed, 28 insertions(+), 22 deletions(-)
>>
> These went in already in case you missed it.  Thanks.
Hi Doug,

I can't see those patches in your git repositories.
Where were they applied to?

Thanks
>

--
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

Re: [PATCH 0/2] User-index fixes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

On 03/03/2016 04:57 AM, Majd Dibbiny wrote:
> 
> On 17/2/2016 6:19 PM, Doug Ledford wrote:
>> On 02/14/2016 11:35 AM, Matan Barak wrote:
>>> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>
>>> Hi Doug,
>>>
>>> This series fixes two issues introduced in 4.5 after applying my
>>> series "Raw Packet QP user-space support for mlx5".
>>>
>>> SRQs don't use user-index, therefore the user shouldn't pass it and the
>>> kernel shouldn't verify. The first patch fixes this by avoiding
>>> user-index verification for non-xrc srqs.
>>>
>>> The mlx5_ib driver fails libraries that support user-index but don't
>>> pass it. It relies on the input length of the vendor specific part to
>>> determine whether the user-space supports user-index.
>>>
>>>
>>> In Legacy verbs, the given vendor specific part input length is given
>>> including struct ib_uverbs_cmd_hdr. In contrast, extended commands
>>> pass the same length exluding the header size.
>>> The vendor driver doesn't know whether the command is a legacy
>>> command or an extended command. Thus, we need to modify IB/core
>>> in order to pass a consistent size. The second patch fixes the input
>>> length in the IB/core and the wrong usage in the mlx5_ib.
>>>
>>> We verified that other vendors don't use the inlen parameter and thus
>>> won't break by this change.
>>>
>>> Thanks,
>>> Majd and Matan
>>>
>>>
>>> Majd Dibbiny (2):
>>>   IB/mlx5: Avoid using user-index for SRQs
>>>   IB/{core, mlx5}: Fix input len in vendor specific part of
>>>     create_qp/srq cmd
>>>
>>>  drivers/infiniband/core/uverbs_cmd.c |  9 +++++---
>>>  drivers/infiniband/hw/mlx5/srq.c     | 41 +++++++++++++++++++-----------------
>>>  2 files changed, 28 insertions(+), 22 deletions(-)
>>>
>> These went in already in case you missed it.  Thanks.
> Hi Doug,
> 
> I can't see those patches in your git repositories.
> Where were they applied to?

I *thought* they had went in via 4.5-rc already.  Upon double checking,
they weren't there, but I had marked them accepted in patchworks.  I
never mark them accepted until I download the patch files, and the patch
files for these two patches were no longer in my download directory.
That means either I applied them at one point and then they got removed
(probably by a git reset --hard HEAD~# to redo something before I pushed
it) or else simply never got applied when I thought they did.  In any
case, thanks for catching this.  I've added it to my k.o/for-4.5-rc
branch to go out later today to Linus.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]