[PATCH rdma-next 00/12] RDMA fixes 2018-06-24

[PATCH rdma-next 00/12] RDMA fixes 2018-06-24

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Hi,

This is bunch of patches trigged by running syzkaller internally.

I'm sending them based on rdma-next mainly for two reasons:
1, Most of the patches fix the old issues and it doesn't matter when
they will hit the Linus's tree: now or later in a couple of weeks
during merge window.
2. They interleave with code cleanup, mlx5-next patches and Michael's
feedback on flow counters series.

Thanks

Leon Romanovsky (12):
  RDMA/uverbs: Protect from attempts to create flows on unsupported QP
  RDMA/uverbs: Check existence of create_flow callback
  RDMA/verbs: Drop kernel variant of create_flow
  RDMA/verbs: Drop kernel variant of destroy_flow
  net/mlx5: Rate limit errors in command interface
  RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR
  RDMA/umem: Don't check for negative return value of dma_map_sg_attrs()
  overflow.h: Add arithmetic shift helper
  RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq
  RDMA/mlx5: Reuse existed shift_overlow helper
  RDMA/uverbs: Remove redundant check
  RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

 drivers/infiniband/core/umem.c                     |  2 +-
 drivers/infiniband/core/uverbs_cmd.c               | 49 ++++++++++++++--------
 drivers/infiniband/core/uverbs_std_types.c         |  9 ++--
 drivers/infiniband/core/verbs.c                    | 29 -------------
 drivers/infiniband/hw/mlx5/qp.c                    | 16 +++++--
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c      | 11 ++---
 .../net/ethernet/mellanox/mlx5/core/mlx5_core.h    |  6 +++
 include/linux/overflow.h                           | 23 ++++++++++
 include/rdma/ib_verbs.h                            |  4 --
 9 files changed, 83 insertions(+), 66 deletions(-)

[PATCH rdma-next 01/12] RDMA/uverbs: Protect from attempts to create flows on unsupported QP

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Flows can be created on UD and RAW_PACKET QP types. Attempts to provide
other QP types as an input causes to various unpredictable failures.

The reason to it that in order to support all various types (e.g. XRC),
we are supposed to use real_qp handle and not qp handle and give to
driver/FW to fail such (XRC) flows. Being valuable solution, the simpler
and safer variant is to ban all QP types except UD and RAW_PACKET,
instead of relying on driver/FW.

Cc: <stable@vger.kernel.org> # 3.11
Fixes: 436f2ad05a0b ("IB/core: Export ib_create/destroy_flow through uverbs")
Cc: syzkaller <syzkaller@googlegroups.com>
Reported-by: Noa Osherovich <noaos@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 779892b63729..c842a9423fbf 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3553,14 +3553,20 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 		goto err_free_attr;
 	}
 
-	qp = uobj_get_obj_read(qp, UVERBS_OBJECT_QP, cmd.qp_handle, file->ucontext);
+	qp = uobj_get_obj_read(qp, UVERBS_OBJECT_QP, cmd.qp_handle,
+			       file->ucontext);
 	if (!qp) {
 		err = -EINVAL;
 		goto err_uobj;
 	}
 
+	if (qp->qp_type != IB_QPT_UD && qp->qp_type != IB_QPT_RAW_PACKET) {
+		err = -EINVAL;
+		goto err_put;
+	}
+
 	flow_attr = kzalloc(struct_size(flow_attr, flows,
-				cmd.flow_attr.num_of_specs), GFP_KERNEL);
+					cmd.flow_attr.num_of_specs), GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;
-- 
2.14.4

[PATCH rdma-next 02/12] RDMA/uverbs: Check existence of create_flow callback

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

In the accepted series "Refactor ib_uverbs_write path", we presented the
roadmap to get rid of uverbs_cmd_mask and uverbs_ex_cmd_mask fields in
favor of simple check of function pointer. So let's put NULL check of
create_flow function callback despite the fact that uverbs_ex_cmd_mask
still exists.

Link: https://www.spinics.net/lists/linux-rdma/msg60753.html
Suggested-by: Michael J Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index c842a9423fbf..6251d80db732 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3565,6 +3565,11 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 		goto err_put;
 	}
 
+	if (!qp->device->create_flow) {
+		err = -EOPNOTSUPP;
+		goto err_put;
+	}
+
 	flow_attr = kzalloc(struct_size(flow_attr, flows,
 					cmd.flow_attr.num_of_specs), GFP_KERNEL);
 	if (!flow_attr) {
-- 
2.14.4

[PATCH rdma-next 03/12] RDMA/verbs: Drop kernel variant of create_flow

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

There are no kernel users of this interface so let's drop it.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/verbs.c | 17 -----------------
 include/rdma/ib_verbs.h         |  2 --
 2 files changed, 19 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 9a72b88fea80..5ada09f708f5 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2275,23 +2275,6 @@ int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *rwq_ind_table)
 }
 EXPORT_SYMBOL(ib_destroy_rwq_ind_table);
 
-struct ib_flow *ib_create_flow(struct ib_qp *qp,
-			       struct ib_flow_attr *flow_attr,
-			       int domain)
-{
-	struct ib_flow *flow_id;
-	if (!qp->device->create_flow)
-		return ERR_PTR(-EOPNOTSUPP);
-
-	flow_id = qp->device->create_flow(qp, flow_attr, domain, NULL);
-	if (!IS_ERR(flow_id)) {
-		atomic_inc(&qp->usecnt);
-		flow_id->qp = qp;
-	}
-	return flow_id;
-}
-EXPORT_SYMBOL(ib_create_flow);
-
 int ib_destroy_flow(struct ib_flow *flow_id)
 {
 	int err;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index d1e2f2d91766..a55e1aa808a7 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -3832,8 +3832,6 @@ struct ib_xrcd *__ib_alloc_xrcd(struct ib_device *device, const char *caller);
  */
 int ib_dealloc_xrcd(struct ib_xrcd *xrcd);
 
-struct ib_flow *ib_create_flow(struct ib_qp *qp,
-			       struct ib_flow_attr *flow_attr, int domain);
 int ib_destroy_flow(struct ib_flow *flow_id);
 
 static inline int ib_check_mr_access(int flags)
-- 
2.14.4

[PATCH rdma-next 04/12] RDMA/verbs: Drop kernel variant of destroy_flow

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Following the removal of ib_create_flow(), adjust the code to get rid of
ib_destroy_flow() too.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c       |  3 ++-
 drivers/infiniband/core/uverbs_std_types.c |  9 ++++++---
 drivers/infiniband/core/verbs.c            | 12 ------------
 include/rdma/ib_verbs.h                    |  2 --
 4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 6251d80db732..3aba63aa1779 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3642,7 +3642,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 		kfree(kern_flow_attr);
 	return 0;
 err_copy:
-	ib_destroy_flow(flow_id);
+	if (!qp->device->destroy_flow(flow_id))
+		atomic_dec(&qp->usecnt);
 err_free:
 	ib_uverbs_flow_resources_free(uflow_res);
 err_free_flow_attr:
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 6497263d13c8..9b4e1e53cd9c 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -48,14 +48,17 @@ static int uverbs_free_ah(struct ib_uobject *uobject,
 static int uverbs_free_flow(struct ib_uobject *uobject,
 			    enum rdma_remove_reason why)
 {
-	int ret;
 	struct ib_flow *flow = (struct ib_flow *)uobject->object;
 	struct ib_uflow_object *uflow =
 		container_of(uobject, struct ib_uflow_object, uobject);
+	struct ib_qp *qp = flow->qp;
+	int ret;
 
-	ret = ib_destroy_flow(flow);
-	if (!ret)
+	ret = qp->device->destroy_flow(flow);
+	if (!ret) {
+		atomic_dec(&qp->usecnt);
 		ib_uverbs_flow_resources_free(uflow->resources);
+	}
 
 	return ret;
 }
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 5ada09f708f5..128d94988dd8 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2275,18 +2275,6 @@ int ib_destroy_rwq_ind_table(struct ib_rwq_ind_table *rwq_ind_table)
 }
 EXPORT_SYMBOL(ib_destroy_rwq_ind_table);
 
-int ib_destroy_flow(struct ib_flow *flow_id)
-{
-	int err;
-	struct ib_qp *qp = flow_id->qp;
-
-	err = qp->device->destroy_flow(flow_id);
-	if (!err)
-		atomic_dec(&qp->usecnt);
-	return err;
-}
-EXPORT_SYMBOL(ib_destroy_flow);
-
 int ib_check_mr_status(struct ib_mr *mr, u32 check_mask,
 		       struct ib_mr_status *mr_status)
 {
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index a55e1aa808a7..6c51190ae7a1 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -3832,8 +3832,6 @@ struct ib_xrcd *__ib_alloc_xrcd(struct ib_device *device, const char *caller);
  */
 int ib_dealloc_xrcd(struct ib_xrcd *xrcd);
 
-int ib_destroy_flow(struct ib_flow *flow_id);
-
 static inline int ib_check_mr_access(int flags)
 {
 	/*
-- 
2.14.4

[PATCH mlx5-next 05/12] net/mlx5: Rate limit errors in command interface

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Any error status returned by FW will trigger similar
to the following error message in the dmesg.

[   55.884355] mlx5_core 0000:00:04.0: mlx5_cmd_check:712:(pid 555):
ALLOC_UAR(0x802) op_mod(0x0) failed, status limits exceeded(0x8),
syndrome (0x0)

Those prints are extremely valuable to diagnose issues with running
system and it is important to keep them. However, not-so-careful user
can trigger endless number of such prints by depleting HW resources
and will spam dmesg.

Rate limiting of such messages solves this issue.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c       | 11 ++++-------
 drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h |  6 ++++++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index 9d03a202abb1..7dd878b00196 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
@@ -717,13 +717,10 @@ static int mlx5_cmd_check(struct mlx5_core_dev *dev, void *in, void *out)
 	uid    = MLX5_GET(mbox_in, in, uid);
 
 	if (!uid && opcode != MLX5_CMD_OP_DESTROY_MKEY)
-		mlx5_core_err(dev,
-		      "%s(0x%x) op_mod(0x%x) failed, status %s(0x%x), syndrome (0x%x)\n",
-		      mlx5_command_str(opcode),
-		      opcode, op_mod,
-		      cmd_status_str(status),
-		      status,
-		      syndrome);
+		mlx5_core_err_rl(dev,
+			"%s(0x%x) op_mod(0x%x) failed, status %s(0x%x), syndrome (0x%x)\n",
+			mlx5_command_str(opcode), opcode, op_mod,
+			cmd_status_str(status), status, syndrome);
 	else
 		mlx5_core_dbg(dev,
 		      "%s(0x%x) op_mod(0x%x) failed, status %s(0x%x), syndrome (0x%x)\n",
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
index 023882d9a22e..49955117ae36 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
@@ -66,6 +66,12 @@ do {									\
 		__func__, __LINE__, current->pid,	\
 	       ##__VA_ARGS__)
 
+#define mlx5_core_err_rl(__dev, format, ...)				\
+	dev_err_ratelimited(&(__dev)->pdev->dev,			\
+			   "%s:%d:(pid %d): " format,			\
+			   __func__, __LINE__, current->pid,		\
+			   ##__VA_ARGS__)
+
 #define mlx5_core_warn(__dev, format, ...)				\
 	dev_warn(&(__dev)->pdev->dev, "%s:%d:(pid %d): " format,	\
 		 __func__, __LINE__, current->pid,			\
-- 
2.14.4

[PATCH rdma-next 06/12] RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Number of specs is provided by user and in valid case can be equal to zero.
Such argument causes to call to kcalloc() with zero-length request and in
return the ZERO_SIZE_PTR is assigned. This pointer is different from NULL
and makes various if (..) checks to success.

Fixes: b6ba4a9aa59f ("IB/uverbs: Add support for flow counters")
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 3aba63aa1779..8ed4b674416f 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2768,6 +2768,9 @@ static struct ib_uflow_resources *flow_resources_alloc(size_t num_specs)
 	if (!resources)
 		return NULL;
 
+	if (!num_specs)
+		goto out;
+
 	resources->counters =
 		kcalloc(num_specs, sizeof(*resources->counters), GFP_KERNEL);
 	resources->collection =
@@ -2776,8 +2779,8 @@ static struct ib_uflow_resources *flow_resources_alloc(size_t num_specs)
 	if (!resources->counters || !resources->collection)
 		goto err;
 
+out:
 	resources->max = num_specs;
-
 	return resources;
 
 err:
-- 
2.14.4

[PATCH rdma-next 07/12] RDMA/umem: Don't check for negative return value of dma_map_sg_attrs()

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

dma_map_sg_attrs() returns 0 on error and can't return negative number
(ensured by BUG_ON), so don't check for being negative value.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/umem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 54ab6335c48d..498f59bb4989 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -206,7 +206,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 				  DMA_BIDIRECTIONAL,
 				  dma_attrs);
 
-	if (umem->nmap <= 0) {
+	if (!umem->nmap) {
 		ret = -ENOMEM;
 		goto out;
 	}
-- 
2.14.4

[PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Add shift_overflow() helper to help driver authors to ensure that
shift operand doesn't cause to overflow, which is very common pattern
for RDMA drivers.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 include/linux/overflow.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 8712ff70995f..2a3395248e94 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -202,6 +202,29 @@

 #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */

+/**
+ * shift_overflow() - Peform shift operation with overflow check
+ * @a: value to be shifted
+ * @b: shift operand
+ *
+ * Checks if a << b will overflow
+ *
+ * Returns: result of shift for no overflow or SIZE_MAX for overflow
+ */
+static inline __must_check size_t shift_overflow(size_t a, size_t b)
+{
+	size_t c, res;
+
+	if (b >= sizeof(size_t) * BITS_PER_BYTE)
+		return SIZE_MAX;
+
+	c = (size_t)1 << b;
+	if (check_mul_overflow(a, c, &res))
+		return SIZE_MAX;
+
+	return res;
+}
+
 /**
  * array_size() - Calculate size of 2-dimensional array.
  *
--
2.14.4

[PATCH rdma-next 09/12] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

[   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
[   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
[   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
[   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
[   61.188315] Call Trace:
[   61.188661]  dump_stack+0xc7/0x13b
[   61.190427]  ubsan_epilogue+0x9/0x49
[   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
[   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
[   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
[   61.213892]  ib_uverbs_write+0x77e/0xae0
[   61.248018]  vfs_write+0x121/0x3b0
[   61.249831]  ksys_write+0xa1/0x120
[   61.254024]  do_syscall_64+0x7c/0x2a0
[   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   61.259211] RIP: 0033:0x7f54bab70e99
[   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
[   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
[   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
[   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
[   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
[   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000

Cc: <stable@vger.kernel.org> # 4.7
Fixes: 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")
Cc: syzkaller <syzkaller@googlegroups.com>
Reported-by: Noa Osherovich <noaos@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 6034a670859f..8e40263fd40e 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -5377,7 +5377,11 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
 
 	rwq->wqe_count = ucmd->rq_wqe_count;
 	rwq->wqe_shift = ucmd->rq_wqe_shift;
-	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
+	rwq->buf_size =
+		shift_overflow((size_t)rwq->wqe_count, (size_t)rwq->wqe_shift);
+	if (rwq->buf_size == SIZE_MAX)
+		return -EINVAL;
+
 	rwq->log_rq_stride = rwq->wqe_shift;
 	rwq->log_rq_size = ilog2(rwq->wqe_count);
 	return 0;
-- 
2.14.4

[PATCH rdma-next 10/12] RDMA/mlx5: Reuse existed shift_overlow helper

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Rewrite commit 002bf2282b2d ("RDMA/mlx5: Protect from shift operand
overflow") to reuse newly introduced shift_overflow() helper.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/hw/mlx5/qp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 8e40263fd40e..5471b57b873d 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -259,13 +259,17 @@ static int set_rq_size(struct mlx5_ib_dev *dev, struct ib_qp_cap *cap,
 		cap->max_recv_sge = 0;
 	} else {
 		if (ucmd) {
+			size_t s;
+
 			qp->rq.wqe_cnt = ucmd->rq_wqe_count;
-			if (ucmd->rq_wqe_shift > BITS_PER_BYTE * sizeof(ucmd->rq_wqe_shift))
+			s = shift_overflow(1, ucmd->rq_wqe_shift);
+			if (s == SIZE_MAX)
 				return -EINVAL;
 			qp->rq.wqe_shift = ucmd->rq_wqe_shift;
-			if ((1 << qp->rq.wqe_shift) / sizeof(struct mlx5_wqe_data_seg) < qp->wq_sig)
+			if (s / sizeof(struct mlx5_wqe_data_seg) < qp->wq_sig)
 				return -EINVAL;
-			qp->rq.max_gs = (1 << qp->rq.wqe_shift) / sizeof(struct mlx5_wqe_data_seg) - qp->wq_sig;
+			qp->rq.max_gs = s / sizeof(struct mlx5_wqe_data_seg) -
+					qp->wq_sig;
 			qp->rq.max_post = qp->rq.wqe_cnt;
 		} else {
 			wqe_size = qp->wq_sig ? sizeof(struct mlx5_wqe_signature_seg) : 0;
-- 
2.14.4

[PATCH rdma-next 11/12] RDMA/uverbs: Remove redundant check

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

kern_spec->reserved is checked prior to calling to
kern_spec_to_ib_spec_filter() and it makes this second check redundant.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 8ed4b674416f..3a0bc4c1b17b 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3045,9 +3045,6 @@ static int kern_spec_to_ib_spec_filter(struct ib_uverbs_flow_spec *kern_spec,
 	void *kern_spec_mask;
 	void *kern_spec_val;
 
-	if (kern_spec->reserved)
-		return -EINVAL;
-
 	kern_filter_sz = kern_spec_filter_sz(&kern_spec->hdr);
 
 	kern_spec_val = (void *)kern_spec +
-- 
2.14.4

[PATCH rdma-next 12/12] RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

The check of cmd.flow_attr.size should check into account the size of
reserved field (2 bytes), otherwise user can provide size whihc will
cause to slab-out-of-bounds warning below.

==================================================================
BUG: KASAN: slab-out-of-bounds in ib_uverbs_ex_create_flow+0x1740/0x1d00
Read of size 2 at addr ffff880068dff1a6 by task syz-executor775/269

CPU: 0 PID: 269 Comm: syz-executor775 Not tainted 4.18.0-rc1+ #245
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
Call Trace:
 dump_stack+0xef/0x17e
 print_address_description+0x83/0x3b0
 kasan_report+0x18d/0x4d0
 ib_uverbs_ex_create_flow+0x1740/0x1d00
 ib_uverbs_write+0x923/0x1010
 __vfs_write+0x10d/0x720
 vfs_write+0x1b0/0x550
 ksys_write+0xc6/0x1a0
 do_syscall_64+0xa7/0x590
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x433899
Code: fd ff 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f 00 48 89 f8 48 89
f7 48 89 d6 48 89 ca 4d 89 c2 4d
89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 3b 91 fd ff c3 66
2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffc2724db58 EFLAGS: 00000217 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000020006880 RCX: 0000000000433899
RDX: 00000000000000e0 RSI: 0000000020002480 RDI: 0000000000000003
RBP: 00000000006d7018 R08: 00000000004002f8 R09: 00000000004002f8
R10: 00000000004002f8 R11: 0000000000000217 R12: 0000000000000000

R13: 000000000040cd20 R14: 000000000040cdb0 R15: 0000000000000006

Allocated by task 269:
 kasan_kmalloc+0xa0/0xd0
 __kmalloc+0x1a9/0x510
 ib_uverbs_ex_create_flow+0x26c/0x1d00
 ib_uverbs_write+0x923/0x1010
 __vfs_write+0x10d/0x720
 vfs_write+0x1b0/0x550
 ksys_write+0xc6/0x1a0
 do_syscall_64+0xa7/0x590
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 0:
 __kasan_slab_free+0x12e/0x180
 kfree+0x159/0x630
 detach_buf+0x559/0x7a0
 virtqueue_get_buf_ctx+0x3cc/0xab0
 virtblk_done+0x1eb/0x3d0
 vring_interrupt+0x16d/0x2b0
 __handle_irq_event_percpu+0x10a/0x980
 handle_irq_event_percpu+0x77/0x190
 handle_irq_event+0xc6/0x1a0
 handle_edge_irq+0x211/0xd80
 handle_irq+0x3d/0x60
 do_IRQ+0x9b/0x220

The buggy address belongs to the object at ffff880068dff180
 which belongs to the cache kmalloc-64 of size 64
The buggy address is located 38 bytes inside of
 64-byte region [ffff880068dff180, ffff880068dff1c0)
The buggy address belongs to the page:
page:ffffea0001a37fc0 count:1 mapcount:0 mapping:ffff88006c401780
index:0x0
flags: 0x4000000000000100(slab)
raw: 4000000000000100 ffffea0001a31100 0000001100000011 ffff88006c401780
raw: 0000000000000000 00000000802a002a 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff880068dff080: fb fb fb fb fc fc fc fc fb fb fb fb fb fb fb fb
 ffff880068dff100: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
>ffff880068dff180: 00 00 00 00 07 fc fc fc fc fc fc fc fb fb fb fb
                               ^
 ffff880068dff200: fb fb fb fb fc fc fc fc 00 00 00 00 00 00 fc fc
 ffff880068dff280: fc fc fc fc 00 00 00 00 00 00 00 00 fc fc fc fc
==================================================================

Cc: <stable@vger.kernel.org> # 3.12
Fixes: f88482743872 ("IB/core: clarify overflow/underflow checks on ib_create/destroy_flow")
Cc: syzkaller <syzkaller@googlegroups.com>
Reported-by: Noa Osherovich <noaos@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 3a0bc4c1b17b..b6bca79fd48b 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3488,8 +3488,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	struct ib_flow_attr		  *flow_attr;
 	struct ib_qp			  *qp;
 	struct ib_uflow_resources	  *uflow_res;
+	struct ib_uverbs_flow_spec_hdr	  *kern_spec;
 	int err = 0;
-	void *kern_spec;
 	void *ib_spec;
 	int i;
 
@@ -3538,8 +3538,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 		if (!kern_flow_attr)
 			return -ENOMEM;
 
-		memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
-		err = ib_copy_from_udata(kern_flow_attr + 1, ucore,
+		*kern_flow_attr = cmd.flow_attr;
+		err = ib_copy_from_udata(&kern_flow_attr->flow_specs, ucore,
 					 cmd.flow_attr.size);
 		if (err)
 			goto err_free_attr;
@@ -3589,21 +3589,22 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	flow_attr->flags = kern_flow_attr->flags;
 	flow_attr->size = sizeof(*flow_attr);
 
-	kern_spec = kern_flow_attr + 1;
+	kern_spec = kern_flow_attr->flow_specs;
 	ib_spec = flow_attr + 1;
 	for (i = 0; i < flow_attr->num_of_specs &&
-	     cmd.flow_attr.size > offsetof(struct ib_uverbs_flow_spec, reserved) &&
-	     cmd.flow_attr.size >=
-	     ((struct ib_uverbs_flow_spec *)kern_spec)->size; i++) {
-		err = kern_spec_to_ib_spec(file->ucontext, kern_spec, ib_spec,
-					   uflow_res);
+			cmd.flow_attr.size > sizeof(*kern_spec) &&
+			cmd.flow_attr.size >= kern_spec->size;
+	     i++) {
+		err = kern_spec_to_ib_spec(
+				file->ucontext, (struct ib_uverbs_flow_spec *)kern_spec,
+				ib_spec, uflow_res);
 		if (err)
 			goto err_free;
 
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		cmd.flow_attr.size -= ((struct ib_uverbs_flow_spec *)kern_spec)->size;
-		kern_spec += ((struct ib_uverbs_flow_spec *) kern_spec)->size;
+		cmd.flow_attr.size -= kern_spec->size;
+		kern_spec = ((void *)kern_spec) + kern_spec->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
 	if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
-- 
2.14.4

Re: [PATCH rdma-next 09/12] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

[Jason Gunthorpe]

On Sun, Jun 24, 2018 at 11:23:50AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> [   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
> [   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
> [   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
> [   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> [   61.188315] Call Trace:
> [   61.188661]  dump_stack+0xc7/0x13b
> [   61.190427]  ubsan_epilogue+0x9/0x49
> [   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
> [   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
> [   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
> [   61.213892]  ib_uverbs_write+0x77e/0xae0
> [   61.248018]  vfs_write+0x121/0x3b0
> [   61.249831]  ksys_write+0xa1/0x120
> [   61.254024]  do_syscall_64+0x7c/0x2a0
> [   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   61.259211] RIP: 0033:0x7f54bab70e99
> [   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
> [   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
> [   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
> [   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
> [   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
> [   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000
> 
> Cc: <stable@vger.kernel.org> # 4.7
> Fixes: 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")
> Cc: syzkaller <syzkaller@googlegroups.com>
> Reported-by: Noa Osherovich <noaos@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> index 6034a670859f..8e40263fd40e 100644
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -5377,7 +5377,11 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
>  
>  	rwq->wqe_count = ucmd->rq_wqe_count;
>  	rwq->wqe_shift = ucmd->rq_wqe_shift;
> -	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
> +	rwq->buf_size =
> +		shift_overflow((size_t)rwq->wqe_count, (size_t)rwq->wqe_shift);

The casts are redundant, the function argument is already size_t so
implicit promotion is guaranteed.

Jason

Re: [PATCH rdma-next 06/12] RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR

[Jason Gunthorpe]

On Sun, Jun 24, 2018 at 11:23:47AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Number of specs is provided by user and in valid case can be equal to zero.
> Such argument causes to call to kcalloc() with zero-length request and in
> return the ZERO_SIZE_PTR is assigned. This pointer is different from NULL
> and makes various if (..) checks to success.

The one seems really weird. There is nothing wrong with ZERO_SIZE_PTR,
but this description and fix suggest that something did

ptr = kalloc(0);
ptr[0] = ...;

Which is not allowed of course. Doesn't this mean there is also a
missing range check someplace?

Jason

Re: [PATCH rdma-next 06/12] RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR

[Leon Romanovsky]

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

On Sun, Jun 24, 2018 at 01:57:51PM -0600, Jason Gunthorpe wrote:
> On Sun, Jun 24, 2018 at 11:23:47AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Number of specs is provided by user and in valid case can be equal to zero.
> > Such argument causes to call to kcalloc() with zero-length request and in
> > return the ZERO_SIZE_PTR is assigned. This pointer is different from NULL
> > and makes various if (..) checks to success.
>
> The one seems really weird. There is nothing wrong with ZERO_SIZE_PTR,
> but this description and fix suggest that something did
>
> ptr = kalloc(0);
> ptr[0] = ...;
>
> Which is not allowed of course. Doesn't this mean there is also a
> missing range check someplace?

I don't know, this issue was found during code review of
ib_uvrebs_ex_create_flow(), may or may not be real issue.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 09/12] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

[Leon Romanovsky]

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

On Sun, Jun 24, 2018 at 01:56:24PM -0600, Jason Gunthorpe wrote:
> On Sun, Jun 24, 2018 at 11:23:50AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > [   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
> > [   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
> > [   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
> > [   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> > [   61.188315] Call Trace:
> > [   61.188661]  dump_stack+0xc7/0x13b
> > [   61.190427]  ubsan_epilogue+0x9/0x49
> > [   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
> > [   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
> > [   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
> > [   61.213892]  ib_uverbs_write+0x77e/0xae0
> > [   61.248018]  vfs_write+0x121/0x3b0
> > [   61.249831]  ksys_write+0xa1/0x120
> > [   61.254024]  do_syscall_64+0x7c/0x2a0
> > [   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [   61.259211] RIP: 0033:0x7f54bab70e99
> > [   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
> > [   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
> > [   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
> > [   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
> > [   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
> > [   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000
> >
> > Cc: <stable@vger.kernel.org> # 4.7
> > Fixes: 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")
> > Cc: syzkaller <syzkaller@googlegroups.com>
> > Reported-by: Noa Osherovich <noaos@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > index 6034a670859f..8e40263fd40e 100644
> > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > @@ -5377,7 +5377,11 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
> >
> >  	rwq->wqe_count = ucmd->rq_wqe_count;
> >  	rwq->wqe_shift = ucmd->rq_wqe_shift;
> > -	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
> > +	rwq->buf_size =
> > +		shift_overflow((size_t)rwq->wqe_count, (size_t)rwq->wqe_shift);
>
> The casts are redundant, the function argument is already size_t so
> implicit promotion is guaranteed.

rwq->wqe_count and rwq->wqe_shift are declared as u32 and not as size_t.

https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/hw/mlx5/mlx5_ib.h#L296

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 09/12] RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq

[Jason Gunthorpe]

On Mon, Jun 25, 2018 at 11:10:41AM +0300, Leon Romanovsky wrote:
> On Sun, Jun 24, 2018 at 01:56:24PM -0600, Jason Gunthorpe wrote:
> > On Sun, Jun 24, 2018 at 11:23:50AM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@mellanox.com>
> > >
> > > [   61.182439] UBSAN: Undefined behaviour in drivers/infiniband/hw/mlx5/qp.c:5366:34
> > > [   61.183673] shift exponent 4294967288 is too large for 32-bit type 'unsigned int'
> > > [   61.185530] CPU: 0 PID: 639 Comm: qp Not tainted 4.18.0-rc1-00037-g4aa1d69a9c60-dirty #96
> > > [   61.186981] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> > > [   61.188315] Call Trace:
> > > [   61.188661]  dump_stack+0xc7/0x13b
> > > [   61.190427]  ubsan_epilogue+0x9/0x49
> > > [   61.190899]  __ubsan_handle_shift_out_of_bounds+0x1ea/0x22f
> > > [   61.197040]  mlx5_ib_create_wq+0x1c99/0x1d50
> > > [   61.206632]  ib_uverbs_ex_create_wq+0x499/0x820
> > > [   61.213892]  ib_uverbs_write+0x77e/0xae0
> > > [   61.248018]  vfs_write+0x121/0x3b0
> > > [   61.249831]  ksys_write+0xa1/0x120
> > > [   61.254024]  do_syscall_64+0x7c/0x2a0
> > > [   61.256178]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > [   61.259211] RIP: 0033:0x7f54bab70e99
> > > [   61.262125] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89
> > > [   61.268678] RSP: 002b:00007ffe1541c318 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > > [   61.271076] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f54bab70e99
> > > [   61.273795] RDX: 0000000000000070 RSI: 0000000020000240 RDI: 0000000000000003
> > > [   61.276982] RBP: 00007ffe1541c330 R08: 00000000200078e0 R09: 0000000000000002
> > > [   61.280035] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000004005c0
> > > [   61.283279] R13: 00007ffe1541c420 R14: 0000000000000000 R15: 0000000000000000
> > >
> > > Cc: <stable@vger.kernel.org> # 4.7
> > > Fixes: 79b20a6c3014 ("IB/mlx5: Add receive Work Queue verbs")
> > > Cc: syzkaller <syzkaller@googlegroups.com>
> > > Reported-by: Noa Osherovich <noaos@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > >  drivers/infiniband/hw/mlx5/qp.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > > index 6034a670859f..8e40263fd40e 100644
> > > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > > @@ -5377,7 +5377,11 @@ static int set_user_rq_size(struct mlx5_ib_dev *dev,
> > >
> > >  	rwq->wqe_count = ucmd->rq_wqe_count;
> > >  	rwq->wqe_shift = ucmd->rq_wqe_shift;
> > > -	rwq->buf_size = (rwq->wqe_count << rwq->wqe_shift);
> > > +	rwq->buf_size =
> > > +		shift_overflow((size_t)rwq->wqe_count, (size_t)rwq->wqe_shift);
> >
> > The casts are redundant, the function argument is already size_t so
> > implicit promotion is guaranteed.
> 
> rwq->wqe_count and rwq->wqe_shift are declared as u32 and not as size_t.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/hw/mlx5/mlx5_ib.h#L296

It doesn't matter, passing them to a function accepting size_t does
implicit promotion, the same as the explicit cast.

Jason

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Jason Gunthorpe]

On Mon, Jun 25, 2018 at 11:26:05AM +0200, Rasmus Villemoes wrote:

>    check_shift_overflow(a, s, d) {
>        unsigned _nbits = 8*sizeof(a);
>        typeof(a) _a = (a);
>        typeof(s) _s = (s);
>        typeof(d) _d = (d);
> 
>        *_d = ((u64)(_a) << (_s & (_nbits-1)));
>        _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
>    is_signed_type(a))) != 0);
>    }

Those types are not quite right.. What about this?

    check_shift_overflow(a, s, d) ({
        unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
        typeof(d) _a = a;  // Shift is always performed on type 'd'
        typeof(s) _s = s;
        typeof(d) _d = d;
 
        *_d = (_a << (_s & (_nbits-1)));

	(((*_d) >> (_s & (_nbits-1)) != _a);
    })

And can we use mathamatcial invertability to prove no overlow and
bound _a ? As above.

Jason

Re: [PATCH rdma-next 01/12] RDMA/uverbs: Protect from attempts to create flows on unsupported QP

[Jason Gunthorpe]

On Sun, Jun 24, 2018 at 11:23:42AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Flows can be created on UD and RAW_PACKET QP types. Attempts to provide
> other QP types as an input causes to various unpredictable failures.
> 
> The reason to it that in order to support all various types (e.g. XRC),
> we are supposed to use real_qp handle and not qp handle and give to
> driver/FW to fail such (XRC) flows. Being valuable solution, the simpler
> and safer variant is to ban all QP types except UD and RAW_PACKET,
> instead of relying on driver/FW.
> 
> Cc: <stable@vger.kernel.org> # 3.11
> Fixes: 436f2ad05a0b ("IB/core: Export ib_create/destroy_flow through uverbs")
> Cc: syzkaller <syzkaller@googlegroups.com>
> Reported-by: Noa Osherovich <noaos@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/uverbs_cmd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 779892b63729..c842a9423fbf 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -3553,14 +3553,20 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>  		goto err_free_attr;
>  	}
>  
> -	qp = uobj_get_obj_read(qp, UVERBS_OBJECT_QP, cmd.qp_handle, file->ucontext);
> +	qp = uobj_get_obj_read(qp, UVERBS_OBJECT_QP, cmd.qp_handle,
> +			       file->ucontext);

This hunk is just whitespace changing

>  	if (!qp) {
>  		err = -EINVAL;
>  		goto err_uobj;
>  	}
>  
> +	if (qp->qp_type != IB_QPT_UD && qp->qp_type != IB_QPT_RAW_PACKET) {
> +		err = -EINVAL;
> +		goto err_put;
> +	}
> +
>  	flow_attr = kzalloc(struct_size(flow_attr, flows,
> -				cmd.flow_attr.num_of_specs), GFP_KERNEL);
> +					cmd.flow_attr.num_of_specs), GFP_KERNEL);

Same here.

I dropped the two hunks and applied this to for-rc since it has
stable tags.

Jason

Re: [PATCH rdma-next 00/12] RDMA fixes 2018-06-24

[Jason Gunthorpe]

On Sun, Jun 24, 2018 at 11:23:41AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Hi,
> 
> This is bunch of patches trigged by running syzkaller internally.
> 
> I'm sending them based on rdma-next mainly for two reasons:
> 1, Most of the patches fix the old issues and it doesn't matter when
> they will hit the Linus's tree: now or later in a couple of weeks
> during merge window.
> 2. They interleave with code cleanup, mlx5-next patches and Michael's
> feedback on flow counters series.
> 
> Thanks
> 
> Leon Romanovsky (12):
>   RDMA/uverbs: Protect from attempts to create flows on unsupported QP
>   RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow

I applied these two to for-rc

>   RDMA/uverbs: Check existence of create_flow callback
>   RDMA/verbs: Drop kernel variant of create_flow
>   RDMA/verbs: Drop kernel variant of destroy_flow
>   net/mlx5: Rate limit errors in command interface
>   RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR
>   RDMA/umem: Don't check for negative return value of dma_map_sg_attrs()
>   RDMA/uverbs: Remove redundant check

These to for-next

>   overflow.h: Add arithmetic shift helper
>   RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq
>   RDMA/mlx5: Reuse existed shift_overlow helper

And these will have to be respun.

Thanks,
Jason

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Leon Romanovsky]

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

On Mon, Jun 25, 2018 at 11:11:57AM -0600, Jason Gunthorpe wrote:
> On Mon, Jun 25, 2018 at 11:26:05AM +0200, Rasmus Villemoes wrote:
>
> >    check_shift_overflow(a, s, d) {
> >        unsigned _nbits = 8*sizeof(a);
> >        typeof(a) _a = (a);
> >        typeof(s) _s = (s);
> >        typeof(d) _d = (d);
> >
> >        *_d = ((u64)(_a) << (_s & (_nbits-1)));
> >        _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
> >    is_signed_type(a))) != 0);
> >    }
>
> Those types are not quite right.. What about this?
>
>     check_shift_overflow(a, s, d) ({
>         unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
>         typeof(d) _a = a;  // Shift is always performed on type 'd'
>         typeof(s) _s = s;
>         typeof(d) _d = d;
>
>         *_d = (_a << (_s & (_nbits-1)));
>
> 	(((*_d) >> (_s & (_nbits-1)) != _a);
>     })
>
> And can we use mathamatcial invertability to prove no overlow and
> bound _a ? As above.

Rasmus and Jason,

Thanks for the feedback.
The reason why I introduced function, because wanted to reuse
check_mul_overflow macro, but for any reasons which I don't remember
now, I had hard time to fix compilation errors.

Anyway, I'll resubmit.

Thanks


>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 00/12] RDMA fixes 2018-06-24

[Leon Romanovsky]

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

On Mon, Jun 25, 2018 at 03:34:38PM -0600, Jason Gunthorpe wrote:
> On Sun, Jun 24, 2018 at 11:23:41AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Hi,
> >
> > This is bunch of patches trigged by running syzkaller internally.
> >
> > I'm sending them based on rdma-next mainly for two reasons:
> > 1, Most of the patches fix the old issues and it doesn't matter when
> > they will hit the Linus's tree: now or later in a couple of weeks
> > during merge window.
> > 2. They interleave with code cleanup, mlx5-next patches and Michael's
> > feedback on flow counters series.
> >
> > Thanks
> >
> > Leon Romanovsky (12):
> >   RDMA/uverbs: Protect from attempts to create flows on unsupported QP
> >   RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow
>
> I applied these two to for-rc
>
> >   RDMA/uverbs: Check existence of create_flow callback
> >   RDMA/verbs: Drop kernel variant of create_flow
> >   RDMA/verbs: Drop kernel variant of destroy_flow
> >   net/mlx5: Rate limit errors in command interface
> >   RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR
> >   RDMA/umem: Don't check for negative return value of dma_map_sg_attrs()
> >   RDMA/uverbs: Remove redundant check
>
> These to for-next

Jason,

We would like to see patch "[PATCH mlx5-next 05/12] net/mlx5:
Rate limit errors in command interface" in out mlx5-next. Is it possible
at this point to drop it from for-next, so I'll be able to take it into
mlx5-next?

Thanks

>
> >   overflow.h: Add arithmetic shift helper
> >   RDMA/mlx5: Fix shift overflow in mlx5_ib_create_wq
> >   RDMA/mlx5: Reuse existed shift_overlow helper
>
> And these will have to be respun.
>
> Thanks,
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Leon Romanovsky]

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

On Mon, Jun 25, 2018 at 11:26:05AM +0200, Rasmus Villemoes wrote:
> On 24 June 2018 at 10:23, Leon Romanovsky <leon@kernel.org> wrote:
>
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Add shift_overflow() helper to help driver authors to ensure that
> > shift operand doesn't cause to overflow, which is very common pattern
> > for RDMA drivers.
> >
>
> Not a huge fan. The other _overflow functions have a different behaviour
> (in how they return the result and the overflow status) and are
> type-generic, and I think someone at some point will use such a
> generically-named helper for stuff other than size_t. At least the
> array_size and struct_size helpers have size in their name and are
> specifically about computing the size of something, and are designed to be
> used directly as arguments to allocators, where SIZE_MAX is a suitable
> sentinel. I can't see the other patches in this series, so I don't know how
> you plan on using it, but it should also be usable outside rdma.
>
> Aside: why does b have type size_t?
>
> Does __must_check really make sense for a function without side effects? It
> doesn't tell gcc to warn if the result is not used in a conditional, it
> just warns if the result is not used at all, which wouldn't realistically
> happen for a pure function.
>
> I'd much rather see a type-generic check_shift_overflow (we can agree to
> leave "left" out of the name) with semantics similar to the other
> check_*_overflow functions. Then, if a size_t-eating, SIZE_MAX-returning
> helper is more convenient for rdma, that should be easy to implement on top
> of that. It shouldn't really be that hard to do. Something like
>
> check_shift_overflow(a, s, d) {
>     unsigned _nbits = 8*sizeof(a);
>     typeof(a) _a = (a);
>     typeof(s) _s = (s);
>     typeof(d) _d = (d);
>
>     *_d = ((u64)(_a) << (_s & (_nbits-1)));
>
>     _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s - is_signed_type(a))) !=
> 0);
> }
>
> which should also handle shifts of signed types (though it allows << 0 for
> negative values; that's easy to also disallow). But the exact semantics
> should be documented via a bunch of tests (hint hint) exercising corner
> cases.

I'll respin.

Thanks for the feedback.

>
> Rasmus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Leon Romanovsky]

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

On Tue, Jun 26, 2018 at 10:07:07AM +0200, Rasmus Villemoes wrote:
> On 25 June 2018 at 19:11, Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> > On Mon, Jun 25, 2018 at 11:26:05AM +0200, Rasmus Villemoes wrote:
> >
> > >    check_shift_overflow(a, s, d) {
> > >        unsigned _nbits = 8*sizeof(a);
> > >        typeof(a) _a = (a);
> > >        typeof(s) _s = (s);
> > >        typeof(d) _d = (d);
> > >
> > >        *_d = ((u64)(_a) << (_s & (_nbits-1)));
> > >        _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
> > >    is_signed_type(a))) != 0);
> > >    }
> >
> > Those types are not quite right.. What about this?
> >
> >     check_shift_overflow(a, s, d) ({
> >         unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
> >         typeof(d) _a = a;  // Shift is always performed on type 'd'
> >         typeof(s) _s = s;
> >         typeof(d) _d = d;
> >
> >         *_d = (_a << (_s & (_nbits-1)));
> >
> >         (((*_d) >> (_s & (_nbits-1)) != _a);
> >     })
> >
>
> No, because, the check_*_overflow (and the __builtin_*_overflow cousins)
> functions must do their job without causing undefined behaviour, regardless
> of what crazy input values and types they are given. Also, the output must
> be completely defined for all inputs [1]. I omitted it for brevity, but I
> also wanted a and *d to have the same type, so there should also be one of
> those (void)(&_a == _d); statements. See the other check_*_overflow and the
> commit adding them. Without the (u64) cast, any signed (and negative) a
> would cause UB in your suggestion. Also, having _nbits be 31 when a (and/or
> *d) has type int, and then and'ing the shift by 30 doesn't make any sense;
> I have no idea what you're trying to do. I haven't tested the above, but I
> know from when I wrote the other ones that gcc is smart enough not to
> actually do the arithmetic in 64 bits when only <= 32 bit types are
> involved (i.e., gcc sees that the result is anyway implicitly truncated to
> 32 bits, so only bothers to compute the lower 32 bits).
>
> [1] For this one, it would probably be most consistent to say that the
> result is a*2^s computed in infinite-precision, then truncated to fit in d.
> So for too large s, that would just yield 0. But that becomes a bit
> annoying when s is negative; we don't want to start handling a negative
> left shift as a right shift. That's also why I said that one should sit
> down and think about the semantics one really wants, then implement that,
> and write tests. For a first implementation, it might be completely
> reasonable to simply say BUILD_BUG_ON(is_signed_type(a)), but that still
> leaves open what to put in *d when s is negative. But maybe another
> BUILD_BUG_ON(is_signed_type(s)) could handle that, though that's a bit
> annoying for integer literals.
>
> And can we use mathamatcial invertability to prove no overlow and
> > bound _a ? As above.
> >
>
> It's quite possible that the expression determining whether overflow
> occured can be written differently, possibly in terms of shifting back, but
> it definitely needs to return true when s is greater than nbits;
> check_shift_overflow(1U, 32, &d) must be true. And that expression also
> must not involve UB.

Rasmus,

RDMA doesn't really need specific size_t variant, but wants to prevent
shift overflows from users commands, so any true/false function/macro
will work for us.

https://patchwork.kernel.org/patch/10484055/
https://patchwork.kernel.org/patch/10484053/

Thanks

>
> Rasmus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Jason Gunthorpe]

On Tue, Jun 26, 2018 at 10:07:07AM +0200, Rasmus Villemoes wrote:
>    On 25 June 2018 at 19:11, Jason Gunthorpe <[1]jgg@mellanox.com> wrote:
> 
>      On Mon, Jun 25, 2018 at 11:26:05AM +0200, Rasmus Villemoes wrote:
>      >    check_shift_overflow(a, s, d) {
>      >        unsigned _nbits = 8*sizeof(a);
>      >        typeof(a) _a = (a);
>      >        typeof(s) _s = (s);
>      >        typeof(d) _d = (d);
>      >
>      >        *_d = ((u64)(_a) << (_s & (_nbits-1)));
>      >        _s >= _nbits || (_s > 0 && (_a >> (_nbits - _s -
>      >    is_signed_type(a))) != 0);
>      >    }
>      Those types are not quite right.. What about this?
>          check_shift_overflow(a, s, d) ({
>              unsigned int _nbits = 8*sizeof(d) - is_signed_type(d);
>              typeof(d) _a = a;  // Shift is always performed on type 'd'
>              typeof(s) _s = s;
>              typeof(d) _d = d;
>              *_d = (_a << (_s & (_nbits-1)));
>              (((*_d) >> (_s & (_nbits-1)) != _a);
>          })
> 
>    No, because, the check_*_overflow (and the __builtin_*_overflow
>    cousins) functions must do their job without causing undefined
>    behaviour, regardless of what crazy input values and types they are
>    given.

Okay, I see you are concerned about a UB during shifting signed
values. I didn't consider that..

>    Also, the output must be completely defined for all inputs [1].
>    I omitted it for brevity, but I also wanted a and *d to have the same
>    type, so there should also be one of those (void)(&_a == _d);

Humm. No, that doesn't match the use case. Typically this would take
an ABI constant like a u32 and shift it into a size_t for use with an
allocator. So demanding a and d have equal types is not good, and
requiring user casting is not good as the casting could be truncating.

>    statements. See the other check_*_overflow and the commit adding them.
>    Without the (u64) cast, any signed (and negative) a would cause UB in
>    your suggestion.

When thinking about signed cases.. The explicit u64 cast, and
implict promotion to typeof(d), produce something counter intuitive,
eg:

  (u64)(s32)-1 == 0xffffffffffffffff

Which would result in a shift oucome that is not what anyone would
expect, IMHO... So the first version isn't what I'd expect either..

>    Also, having _nbits be 31 when a (and/or *d) has type
>    int, and then and'ing the shift by 30 doesn't make any sense; I have no
>    idea what you're trying to do.

Yes, it is not helpful to avoid UB when a is signed..

>    [1] For this one, it would probably be most consistent to say that the
>    result is a*2^s computed in infinite-precision, then truncated to fit
>    in d.

I think this does not match the usual use cases, this should strictly
be an unsigned shift. The output is guarenteed to always be positive
or overflow is signaled.

Signed types are alllowed, but negative values are not.

What about more like this?

          check_shift_overflow(a, s, d) ({
	      // Shift is always performed on the machine's largest unsigned
              u64 _a = a;
	      typeof(s) _s = s;
              typeof(d) _d = d;

	      // Make s safe against UB
	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;

              *_d = (_a << _to_shift);

	       // s is malformed
              (_to_shift != _s ||
	       // d is a signed type and became negative
	       *_d < 0 ||
	       // a is a signed type and was negative
	       _a < 0 ||
	       // Not invertable means a was truncated during shifting
	       (*_d >> _to_shift) != a))
          })

I'm not seeing a UB with this?

Jason

Re: [PATCH rdma-next 00/12] RDMA fixes 2018-06-24

[Jason Gunthorpe]

On Tue, Jun 26, 2018 at 07:21:26AM +0300, Leon Romanovsky wrote:
> On Mon, Jun 25, 2018 at 03:34:38PM -0600, Jason Gunthorpe wrote:
> > On Sun, Jun 24, 2018 at 11:23:41AM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@mellanox.com>
> > >
> > > Hi,
> > >
> > > This is bunch of patches trigged by running syzkaller internally.
> > >
> > > I'm sending them based on rdma-next mainly for two reasons:
> > > 1, Most of the patches fix the old issues and it doesn't matter when
> > > they will hit the Linus's tree: now or later in a couple of weeks
> > > during merge window.
> > > 2. They interleave with code cleanup, mlx5-next patches and Michael's
> > > feedback on flow counters series.
> > >
> > > Thanks
> > >
> > > Leon Romanovsky (12):
> > >   RDMA/uverbs: Protect from attempts to create flows on unsupported QP
> > >   RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow
> >
> > I applied these two to for-rc
> >
> > >   RDMA/uverbs: Check existence of create_flow callback
> > >   RDMA/verbs: Drop kernel variant of create_flow
> > >   RDMA/verbs: Drop kernel variant of destroy_flow
> > >   net/mlx5: Rate limit errors in command interface
> > >   RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR
> > >   RDMA/umem: Don't check for negative return value of dma_map_sg_attrs()
> > >   RDMA/uverbs: Remove redundant check
> >
> > These to for-next
> 
> Jason,
> 
> We would like to see patch "[PATCH mlx5-next 05/12] net/mlx5:
> Rate limit errors in command interface" in out mlx5-next. Is it possible
> at this point to drop it from for-next, so I'll be able to take it into
> mlx5-next?

Okay, you got to this while it was still 'wip', so it is dropped. Add
it to the mlx5-next branch and netdev or rdma can pull it next time
there is some reason to pull the branch..

Jason

Re: [PATCH rdma-next 00/12] RDMA fixes 2018-06-24

[Leon Romanovsky]

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

On Tue, Jun 26, 2018 at 02:39:21PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 26, 2018 at 07:21:26AM +0300, Leon Romanovsky wrote:
> > On Mon, Jun 25, 2018 at 03:34:38PM -0600, Jason Gunthorpe wrote:
> > > On Sun, Jun 24, 2018 at 11:23:41AM +0300, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@mellanox.com>
> > > >
> > > > Hi,
> > > >
> > > > This is bunch of patches trigged by running syzkaller internally.
> > > >
> > > > I'm sending them based on rdma-next mainly for two reasons:
> > > > 1, Most of the patches fix the old issues and it doesn't matter when
> > > > they will hit the Linus's tree: now or later in a couple of weeks
> > > > during merge window.
> > > > 2. They interleave with code cleanup, mlx5-next patches and Michael's
> > > > feedback on flow counters series.
> > > >
> > > > Thanks
> > > >
> > > > Leon Romanovsky (12):
> > > >   RDMA/uverbs: Protect from attempts to create flows on unsupported QP
> > > >   RDMA/uverbs: Fix slab-out-of-bounds in ib_uverbs_ex_create_flow
> > >
> > > I applied these two to for-rc
> > >
> > > >   RDMA/uverbs: Check existence of create_flow callback
> > > >   RDMA/verbs: Drop kernel variant of create_flow
> > > >   RDMA/verbs: Drop kernel variant of destroy_flow
> > > >   net/mlx5: Rate limit errors in command interface
> > > >   RDMA/uverbs: Don't overwrite NULL pointer with ZERO_SIZE_PTR
> > > >   RDMA/umem: Don't check for negative return value of dma_map_sg_attrs()
> > > >   RDMA/uverbs: Remove redundant check
> > >
> > > These to for-next
> >
> > Jason,
> >
> > We would like to see patch "[PATCH mlx5-next 05/12] net/mlx5:
> > Rate limit errors in command interface" in out mlx5-next. Is it possible
> > at this point to drop it from for-next, so I'll be able to take it into
> > mlx5-next?
>
> Okay, you got to this while it was still 'wip', so it is dropped. Add
> it to the mlx5-next branch and netdev or rdma can pull it next time
> there is some reason to pull the branch..

Thanks, I cherry-picked that patch from your wip branch, so it includes
your SOB too. Most probably, the RDMA will pull it in dump_fill_mkey
series.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH mlx5-next 05/12] net/mlx5: Rate limit errors in command interface

[Leon Romanovsky]

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

On Sun, Jun 24, 2018 at 11:23:46AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Any error status returned by FW will trigger similar
> to the following error message in the dmesg.
>
> [   55.884355] mlx5_core 0000:00:04.0: mlx5_cmd_check:712:(pid 555):
> ALLOC_UAR(0x802) op_mod(0x0) failed, status limits exceeded(0x8),
> syndrome (0x0)
>
> Those prints are extremely valuable to diagnose issues with running
> system and it is important to keep them. However, not-so-careful user
> can trigger endless number of such prints by depleting HW resources
> and will spam dmesg.
>
> Rate limiting of such messages solves this issue.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/cmd.c       | 11 ++++-------
>  drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h |  6 ++++++
>  2 files changed, 10 insertions(+), 7 deletions(-)
>

Thanks, applied to mlx5-next.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Leon Romanovsky]

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

On Wed, Jun 27, 2018 at 11:36:03AM +0200, Rasmus Villemoes wrote:
> On 26 June 2018 at 19:54, Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> > On Tue, Jun 26, 2018 at 10:07:07AM +0200, Rasmus Villemoes wrote:
> > >    On 25 June 2018 at 19:11, Jason Gunthorpe <[1]jgg@mellanox.com>
> > wrote:
> > >
> >
> > When thinking about signed cases.. The explicit u64 cast, and
> > implict promotion to typeof(d), produce something counter intuitive,
> > eg:
> >
> >   (u64)(s32)-1 == 0xffffffffffffffff
> >
> > Which would result in a shift oucome that is not what anyone would
> > expect, IMHO... So the first version isn't what I'd expect either..
> >
>
> Wouldn't check_shift_overflow(-1, 4, &someint) just put -16 in someint and
> report no overflow? That's what I'd expect, if negative values are to be
> supported at all.

Most if not all the times we don't do shifts on negative values, so I
don't think that we should support them.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Jason Gunthorpe]

On Wed, Jun 27, 2018 at 11:36:03AM +0200, Rasmus Villemoes wrote:
>    OK. The requirement of everything having the same type for the
>    check_*_overflow when gccs builtins are not available was mostly a
>    consequence of my inability to implement completely type-generic
>    versions (but also to enforce some sanity, so people don't do
>    check_add_overflow( s8, size_t, int*)). There's no gcc builtin for
>    shift, but if it's relatively simple to one allowing a and *d to have
>    different types, then why not. It's of course particularly convenient
>    to allow a bare "1" (i.e. int) as a while having *d have some random
>    type.

Yes

>    Wouldn't check_shift_overflow(-1, 4, &someint) just put -16 in someint
>    and report no overflow? That's what I'd expect, if negative values are
>    to be supported at all.

I would say that is not a desired outcome, bitshift is defined on
bits, if the caller wanted something defined as signed multiply they
should use multiply.

IMHO, nobody writes 'a << b' expecting sign preservation..

>    Well, the types you can check at compile-time, the values not, so you
>    still have to define the result, i.e. contents of *d, for negative
>    values (even if we decide that "overflow" should always be signalled in
>    that case).

Why do a need to define a 'result' beyond whatever the not-undefined
behavior shift expression produces?

>      What about more like this?
>                check_shift_overflow(a, s, d) ({
>                    // Shift is always performed on the machine's largest
>      unsigned
>                    u64 _a = a;
>                    typeof(s) _s = s;
>                    typeof(d) _d = d;
>                    // Make s safe against UB
>                    unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;
>                    *_d = (_a << _to_shift);
>                     // s is malformed
>                    (_to_shift != _s ||
>                     // d is a signed type and became negative
>                     *_d < 0 ||
>                     // a is a signed type and was negative
>                     _a < 0 ||
>                     // Not invertable means a was truncated during
>      shifting
>                     (*_d >> _to_shift) != a))
>                })
>      I'm not seeing a UB with this?
> 
>    Something like that might work, but you're not there yet. In
>    particular, your test for whether a is negative is thwarted by using
>    u64 for _a and testing _a < 0...

Oops, yes that was intended to be 'a', and of course we need to
capture it..

Leon? Seems like agreement, Can you work with this version?

#include <stdint.h>
#include <stdbool.h>
#include <assert.h>

#define u64 uint64_t

/*
 * Compute *d = (a << s)
 *
 * Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
 * - 'a << s' causes bits to be lost when stored in d
 * - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
 * - 'a' is negative
 * - 'a << s' sets the sign bit, if any, in '*d'
 * *d is not defined if false is returned.
 */
#define check_shift_overflow(a, s, d)                                          \
	({                                                                     \
		typeof(a) _a = a;                                              \
		typeof(s) _s = s;                                              \
		typeof(d) _d = d;                                              \
		u64 _a_full = _a;                                              \
		unsigned int _to_shift =                                       \
			_s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
                                                                               \
		*_d = (_a_full << _to_shift);                                  \
                                                                               \
		(_to_shift != _s || *_d < 0 || _a < 0 ||                       \
		 (*_d >> _to_shift) != a);                                     \
	})

int main(int argc, const char *argv[])
{
	int32_t s32;
	uint32_t u32;

	assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));
	assert(check_shift_overflow(1, 1, &s32) == false && s32 == (1 << 1));
	assert(check_shift_overflow(1, 30, &s32) == false && s32 == (1 << 30));
	assert(check_shift_overflow(1, 31, &s32) == true);
	assert(check_shift_overflow(1, 32, &s32) == true);
	assert(check_shift_overflow(-1, 1, &s32) == true);
	assert(check_shift_overflow(-1, 0, &s32) == true);

	assert(check_shift_overflow(1, 0, &u32) == false && u32 == (1 << 0));
	assert(check_shift_overflow(1, 1, &u32) == false && u32 == (1 << 1));
	assert(check_shift_overflow(1, 30, &u32) == false && u32 == (1 << 30));
	assert(check_shift_overflow(1, 31, &u32) == false && u32 == (1UL << 31));
	assert(check_shift_overflow(1, 32, &u32) == true);
	assert(check_shift_overflow(-1, 1, &u32) == true);
	assert(check_shift_overflow(-1, 0, &u32) == true);

	assert(check_shift_overflow(0xFFFFFFFF, 0, &u32) == false && u32 == (0xFFFFFFFFUL << 0));
	assert(check_shift_overflow(0xFFFFFFFF, 1, &u32) == true);
	assert(check_shift_overflow(0xFFFFFFFF, 0, &s32) == true);
	assert(check_shift_overflow(0xFFFFFFFF, 1, &s32) == true);
}

Thanks,
Jason

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Leon Romanovsky]

On Wed, Jun 27, 2018 at 12:10:12PM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 27, 2018 at 11:36:03AM +0200, Rasmus Villemoes wrote:
> >    OK. The requirement of everything having the same type for the
> >    check_*_overflow when gccs builtins are not available was mostly a
> >    consequence of my inability to implement completely type-generic
> >    versions (but also to enforce some sanity, so people don't do
> >    check_add_overflow( s8, size_t, int*)). There's no gcc builtin for
> >    shift, but if it's relatively simple to one allowing a and *d to have
> >    different types, then why not. It's of course particularly convenient
> >    to allow a bare "1" (i.e. int) as a while having *d have some random
> >    type.
>
> Yes
>
> >    Wouldn't check_shift_overflow(-1, 4, &someint) just put -16 in someint
> >    and report no overflow? That's what I'd expect, if negative values are
> >    to be supported at all.
>
> I would say that is not a desired outcome, bitshift is defined on
> bits, if the caller wanted something defined as signed multiply they
> should use multiply.
>
> IMHO, nobody writes 'a << b' expecting sign preservation..
>
> >    Well, the types you can check at compile-time, the values not, so you
> >    still have to define the result, i.e. contents of *d, for negative
> >    values (even if we decide that "overflow" should always be signalled in
> >    that case).
>
> Why do a need to define a 'result' beyond whatever the not-undefined
> behavior shift expression produces?
>
> >      What about more like this?
> >                check_shift_overflow(a, s, d) ({
> >                    // Shift is always performed on the machine's largest
> >      unsigned
> >                    u64 _a = a;
> >                    typeof(s) _s = s;
> >                    typeof(d) _d = d;
> >                    // Make s safe against UB
> >                    unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;
> >                    *_d = (_a << _to_shift);
> >                     // s is malformed
> >                    (_to_shift != _s ||
> >                     // d is a signed type and became negative
> >                     *_d < 0 ||
> >                     // a is a signed type and was negative
> >                     _a < 0 ||
> >                     // Not invertable means a was truncated during
> >      shifting
> >                     (*_d >> _to_shift) != a))
> >                })
> >      I'm not seeing a UB with this?
> >
> >    Something like that might work, but you're not there yet. In
> >    particular, your test for whether a is negative is thwarted by using
> >    u64 for _a and testing _a < 0...
>
> Oops, yes that was intended to be 'a', and of course we need to
> capture it..
>
> Leon? Seems like agreement, Can you work with this version?

Yes, sure, I waited for an agreement.

>
> #include <stdint.h>
> #include <stdbool.h>
> #include <assert.h>
>
> #define u64 uint64_t
>
> /*
>  * Compute *d = (a << s)
>  *
>  * Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
>  * - 'a << s' causes bits to be lost when stored in d
>  * - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
>  * - 'a' is negative
>  * - 'a << s' sets the sign bit, if any, in '*d'
>  * *d is not defined if false is returned.
>  */
> #define check_shift_overflow(a, s, d)                                          \
> 	({                                                                     \
> 		typeof(a) _a = a;                                              \
> 		typeof(s) _s = s;                                              \
> 		typeof(d) _d = d;                                              \
> 		u64 _a_full = _a;                                              \
> 		unsigned int _to_shift =                                       \
> 			_s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
>                                                                                \
> 		*_d = (_a_full << _to_shift);                                  \
>                                                                                \
> 		(_to_shift != _s || *_d < 0 || _a < 0 ||                       \
> 		 (*_d >> _to_shift) != a);                                     \
> 	})
>
> int main(int argc, const char *argv[])
> {
> 	int32_t s32;
> 	uint32_t u32;
>
> 	assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));
> 	assert(check_shift_overflow(1, 1, &s32) == false && s32 == (1 << 1));
> 	assert(check_shift_overflow(1, 30, &s32) == false && s32 == (1 << 30));
> 	assert(check_shift_overflow(1, 31, &s32) == true);
> 	assert(check_shift_overflow(1, 32, &s32) == true);
> 	assert(check_shift_overflow(-1, 1, &s32) == true);
> 	assert(check_shift_overflow(-1, 0, &s32) == true);
>
> 	assert(check_shift_overflow(1, 0, &u32) == false && u32 == (1 << 0));
> 	assert(check_shift_overflow(1, 1, &u32) == false && u32 == (1 << 1));
> 	assert(check_shift_overflow(1, 30, &u32) == false && u32 == (1 << 30));
> 	assert(check_shift_overflow(1, 31, &u32) == false && u32 == (1UL << 31));
> 	assert(check_shift_overflow(1, 32, &u32) == true);
> 	assert(check_shift_overflow(-1, 1, &u32) == true);
> 	assert(check_shift_overflow(-1, 0, &u32) == true);
>
> 	assert(check_shift_overflow(0xFFFFFFFF, 0, &u32) == false && u32 == (0xFFFFFFFFUL << 0));
> 	assert(check_shift_overflow(0xFFFFFFFF, 1, &u32) == true);
> 	assert(check_shift_overflow(0xFFFFFFFF, 0, &s32) == true);
> 	assert(check_shift_overflow(0xFFFFFFFF, 1, &s32) == true);
> }
>
> Thanks,
> Jason

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Kees Cook]

On Wed, Jun 27, 2018 at 11:10 AM, Jason Gunthorpe <jgg@mellanox.com> wrote:
> Leon? Seems like agreement, Can you work with this version?
>
> #include <stdint.h>
> #include <stdbool.h>
> #include <assert.h>
>
> #define u64 uint64_t
>
> /*
>  * Compute *d = (a << s)
>  *
>  * Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
>  * - 'a << s' causes bits to be lost when stored in d
>  * - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
>  * - 'a' is negative
>  * - 'a << s' sets the sign bit, if any, in '*d'
>  * *d is not defined if false is returned.
>  */
> #define check_shift_overflow(a, s, d)                                          \
>         ({                                                                     \
>                 typeof(a) _a = a;                                              \
>                 typeof(s) _s = s;                                              \
>                 typeof(d) _d = d;                                              \
>                 u64 _a_full = _a;                                              \
>                 unsigned int _to_shift =                                       \
>                         _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
>                                                                                \
>                 *_d = (_a_full << _to_shift);                                  \
>                                                                                \
>                 (_to_shift != _s || *_d < 0 || _a < 0 ||                       \
>                  (*_d >> _to_shift) != a);                                     \
>         })
>
> int main(int argc, const char *argv[])
> {
>         int32_t s32;
>         uint32_t u32;
>
>         assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));
>         assert(check_shift_overflow(1, 1, &s32) == false && s32 == (1 << 1));
>         assert(check_shift_overflow(1, 30, &s32) == false && s32 == (1 << 30));
>         assert(check_shift_overflow(1, 31, &s32) == true);
>         assert(check_shift_overflow(1, 32, &s32) == true);
>         assert(check_shift_overflow(-1, 1, &s32) == true);
>         assert(check_shift_overflow(-1, 0, &s32) == true);
>
>         assert(check_shift_overflow(1, 0, &u32) == false && u32 == (1 << 0));
>         assert(check_shift_overflow(1, 1, &u32) == false && u32 == (1 << 1));
>         assert(check_shift_overflow(1, 30, &u32) == false && u32 == (1 << 30));
>         assert(check_shift_overflow(1, 31, &u32) == false && u32 == (1UL << 31));
>         assert(check_shift_overflow(1, 32, &u32) == true);
>         assert(check_shift_overflow(-1, 1, &u32) == true);
>         assert(check_shift_overflow(-1, 0, &u32) == true);
>
>         assert(check_shift_overflow(0xFFFFFFFF, 0, &u32) == false && u32 == (0xFFFFFFFFUL << 0));
>         assert(check_shift_overflow(0xFFFFFFFF, 1, &u32) == true);
>         assert(check_shift_overflow(0xFFFFFFFF, 0, &s32) == true);
>         assert(check_shift_overflow(0xFFFFFFFF, 1, &s32) == true);
> }

Oh yes, please include these tests in lib/test_overflow.c too! Nice. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Rasmus Villemoes]

On 2018-06-27 20:22, Leon Romanovsky wrote:
> On Wed, Jun 27, 2018 at 12:10:12PM -0600, Jason Gunthorpe wrote:
>> On Wed, Jun 27, 2018 at 11:36:03AM +0200, Rasmus Villemoes wrote:
>>>    Well, the types you can check at compile-time, the values not, so you
>>>    still have to define the result, i.e. contents of *d, for negative
>>>    values (even if we decide that "overflow" should always be signalled in
>>>    that case).
>>
>> Why do a need to define a 'result' beyond whatever the not-undefined
>> behavior shift expression produces?

Well, perhaps you don't, it's just that the other check_*_overflow have
that behaviour (which they inherit from gcc's builtins), and it's a
nice-to-have. But I see that it's hard to come up with something
sensible in all the "doesn't make sense" cases. When writing the tests
for test_overflow.c, you can of course just omit the comparison to
the/an expected result in the overflow case.

>> /*
>>  * Compute *d = (a << s)
>>  *
>>  * Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
>>  * - 'a << s' causes bits to be lost when stored in d
>>  * - 's' is garbage (eg negative) or so large that a << s is guarenteed to be 0
>>  * - 'a' is negative
>>  * - 'a << s' sets the sign bit, if any, in '*d'
>>  * *d is not defined if false is returned.
>>  */
>> #define check_shift_overflow(a, s, d)                                          \
>> 	({                                                                     \
>> 		typeof(a) _a = a;                                              \
>> 		typeof(s) _s = s;                                              \
>> 		typeof(d) _d = d;                                              \
>> 		u64 _a_full = _a;                                              \
>> 		unsigned int _to_shift =                                       \
>> 			_s >= 0 && _s < 8 * sizeof(*d) ? _s : 0;               \
>>                                                                                \
>> 		*_d = (_a_full << _to_shift);                                  \
>>                                                                                \
>> 		(_to_shift != _s || *_d < 0 || _a < 0 ||                       \
>> 		 (*_d >> _to_shift) != a);                                     \
>> 	})

That last a still needs to be _a. Other than that, I don't see anything
wrong with this version.

>> int main(int argc, const char *argv[])
>> {
>> 	int32_t s32;
>> 	uint32_t u32;
>>
>> 	assert(check_shift_overflow(1, 0, &s32) == false && s32 == (1 << 0));
[...]>> 	assert(check_shift_overflow(0xFFFFFFFF, 1, &s32) == true);

Please add these in some form to test_overflow.c, but do also include
cases where a and *d have different width, e.g. check_shift_overflow(1,
32, &s64) should be ok, while check_shift_overflow(65432, 0, &s16)
should not.

Rasmus

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Peter Zijlstra]

On Tue, Jun 26, 2018 at 11:54:35AM -0600, Jason Gunthorpe wrote:

> What about more like this?
> 
>           check_shift_overflow(a, s, d) ({

Should that not be: check_shl_overflow() ? Just 'shift' lacks a
direction.

> 	      // Shift is always performed on the machine's largest unsigned
>               u64 _a = a;
> 	      typeof(s) _s = s;
>               typeof(d) _d = d;
> 
> 	      // Make s safe against UB
> 	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;

Should we not do a gcc-plugin or something that fixes that particular
UB? Shift acting all retarded like that is just annoying. I feel we
should eliminate UBs from the language where possible, like
-fno-strict-overflow mandates 2s complement.

>               *_d = (_a << _to_shift);
> 
> 	       // s is malformed
>               (_to_shift != _s ||

Not strictly an overflow though, just not expected behaviour.

> 	       // d is a signed type and became negative
> 	       *_d < 0 ||

Only a problem if it wasn't negative to start out with.

> 	       // a is a signed type and was negative
> 	       _a < 0 ||

Why would that be a problem? You can shift left negative values just
fine. The only problem is when you run out of sign bits.

> 	       // Not invertable means a was truncated during shifting
> 	       (*_d >> _to_shift) != a))
>           })

And I'm not exactly seeing the use case for this macro. What's the point
of a shift-left if you cannot truncate bits. I suppose it's in the name
_overflow, but still.

Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper

[Jason Gunthorpe]

On Wed, Aug 01, 2018 at 11:36:03AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 26, 2018 at 11:54:35AM -0600, Jason Gunthorpe wrote:
> 
> > What about more like this?
> > 
> >           check_shift_overflow(a, s, d) ({
> 
> Should that not be: check_shl_overflow() ? Just 'shift' lacks a
> direction.

Yes, that makes sense.

> > 	      // Shift is always performed on the machine's largest unsigned
> >               u64 _a = a;
> > 	      typeof(s) _s = s;
> >               typeof(d) _d = d;
> > 
> > 	      // Make s safe against UB
> > 	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;
> 
> Should we not do a gcc-plugin or something that fixes that particular
> UB? Shift acting all retarded like that is just annoying. I feel we
> should eliminate UBs from the language where possible, like
> -fno-strict-overflow mandates 2s complement.

No idea, if someone does this they can remove the above overhead..

> >               *_d = (_a << _to_shift);
> > 
> > 	       // s is malformed
> >               (_to_shift != _s ||
> 
> Not strictly an overflow though, just not expected behaviour.

'overflow' here means the math didn't work, ie
   C = A << B
has a C that does not match A << B done on infinite precision. It is
not limited to checking overflow.

> > 	       // d is a signed type and became negative
> > 	       *_d < 0 ||
> 
> Only a problem if it wasn't negative to start out with.

> > 	       // a is a signed type and was negative
> > 	       _a < 0 ||
>
> Why would that be a problem? You can shift left negative values just
> fine. The only problem is when you run out of sign bits.

These are both a problem because of how the macro is setup, nobody had
an idea how to make this work with different types and allow for
negatives to work properly.

You could define this, but since there is no usecase..

> > 	       // Not invertable means a was truncated during shifting
> > 	       (*_d >> _to_shift) != a))
> >           })
> 
> And I'm not exactly seeing the use case for this macro. What's the point
> of a shift-left if you cannot truncate bits. I suppose it's in the name
> _overflow, but still.

It is basically a specialized case of check_mul_overflow where the
multiply is known to be a power of 2.

Jason