[PATCH libibverbs 0/9] Proper fields initializations

[PATCH libibverbs 0/9] Proper fields initializations

[Yann Droneaud]

Hi,

Following discussions in '[PATCH for-next 1/2] IB/uverbs: Add QP
creation flags, allow blocking UD multicast loopback' thread[1],
I've reviewed libibverbs and how it's handling "reserved" fields.

I have identified portion of data structures not initialized but
sent to the kernel.

That means the kernel doesn't check the value of those
non-initialized fields. In turn, this means such fields cannot
be used for future extension, or it cannot be used for extension
without an other mean to declare the presence of the extension.

Please have a look to this patchset where I tried to have proper
handling of reserved field, padding, etc. just like following
commits did it before:

  commit 00042559262a Add Valgrind annotations
  (same as commit 0e0604213ed7 Add Valgrind annotations)
  commit d5052fa0bf81 Initialize reserved attributes in modify QP command
  commit 9eb2125b2027 Fix several valgrind false positives

As I haven't tested, nor benchmarked the code with my patches,
think about it before applying them.

BTW, I think it's time to use valgrind again.

[1] http://marc.info/?i=1410799972.7830.25.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org

Yann Droneaud (9):
  ibv_cmd_create_ah(): initialize reserved fields in various structures
  ibv_cmd_post_recv(): initialize reserved field in struct
    ibv_kern_recv_wr
  ibv_cmd_post_send(): initialize reserved field in struct
    ibv_kern_send_wr
  ibv_cmd_create_srq_ex(): set reserved field in struct ibv_create_xsrq
  kern-abi: remove unused qp_type union in struct ibv_kern_send_wr
  ibv_cmd_open_qp(): initialize reserved field in struct ibv_open_qp
  ibv_cmd_create_flow(): explicitly clear fields
  ibv_cmd_create_flow(): initialize flow_id->comp_mask
  ibv_cmd_destroy_flow(): initialize comp_mask explicitly

 include/infiniband/kern-abi.h |  5 -----
 src/cmd.c                     | 24 ++++++++++++++++++++++--
 2 files changed, 22 insertions(+), 7 deletions(-)

-- 
1.9.3

--
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 libibverbs 1/9] ibv_cmd_create_ah(): initialize reserved fields in various structures

[Yann Droneaud]

Reserved fields in struct ibv_kern_global_route,
struct ibv_kern_ah_attr and struct ibv_create_ah must be
properly initialized when exchanged with kernel uverbs layer,
as:
- it may remove valgrind spurious warnings;
- it may allow to use the field later for future expansion.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: eb0663777c24 ('Add support for new datapath kernel commands')
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/cmd.c b/src/cmd.c
index 45ea06ff4705..b281082f2f4d 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1179,16 +1179,19 @@ int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
 	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
 	cmd.user_handle            = (uintptr_t) ah;
 	cmd.pd_handle              = pd->handle;
+	cmd.reserved               = 0;
 	cmd.attr.dlid              = attr->dlid;
 	cmd.attr.sl                = attr->sl;
 	cmd.attr.src_path_bits     = attr->src_path_bits;
 	cmd.attr.static_rate       = attr->static_rate;
 	cmd.attr.is_global         = attr->is_global;
 	cmd.attr.port_num          = attr->port_num;
+	cmd.attr.reserved          = 0;
 	cmd.attr.grh.flow_label    = attr->grh.flow_label;
 	cmd.attr.grh.sgid_index    = attr->grh.sgid_index;
 	cmd.attr.grh.hop_limit     = attr->grh.hop_limit;
 	cmd.attr.grh.traffic_class = attr->grh.traffic_class;
+	cmd.attr.grh.reserved      = 0;
 	memcpy(cmd.attr.grh.dgid, attr->grh.dgid.raw, 16);
 
 	if (write(pd->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
-- 
1.9.3

--
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 libibverbs 2/9] ibv_cmd_post_recv(): initialize reserved field in struct ibv_kern_recv_wr

[Yann Droneaud]

Properly initialize data structure exchanged with kernel uverbs
layer:
- may remove valgrind spurious warnings;
- may allow to use the field later for future expansion.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: eb0663777c24 ('Add support for new datapath kernel commands')
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/cmd.c b/src/cmd.c
index b281082f2f4d..d057494260ee 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1082,6 +1082,7 @@ int ibv_cmd_post_recv(struct ibv_qp *ibqp, struct ibv_recv_wr *wr,
 	for (i = wr; i; i = i->next) {
 		tmp->wr_id   = i->wr_id;
 		tmp->num_sge = i->num_sge;
+		tmp->reserved = 0;
 
 		if (tmp->num_sge) {
 			memcpy(s, i->sg_list, tmp->num_sge * sizeof *s);
-- 
1.9.3

--
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 libibverbs 3/9] ibv_cmd_post_send(): initialize reserved field in struct ibv_kern_send_wr

[Yann Droneaud]

Properly initialize data structure exchanged with kernel uverbs
layer:
- may remove valgrind spurious warnings;
- may allow to use the field later for future expansion.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: eb0663777c24 ('Add support for new datapath kernel commands')
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/cmd.c b/src/cmd.c
index d057494260ee..8d8f7e8294e5 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -999,6 +999,9 @@ int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 			tmp->wr.ud.ah 	       = i->wr.ud.ah->handle;
 			tmp->wr.ud.remote_qpn  = i->wr.ud.remote_qpn;
 			tmp->wr.ud.remote_qkey = i->wr.ud.remote_qkey;
+			memset(&tmp->wr.ud.reserved, 0,
+			       sizeof(tmp->wr.ud.reserved)
+			       + (sizeof(tmp->wr) - sizeof(tmp->wr.ud)));
 		} else {
 			switch (i->opcode) {
 			case IBV_WR_RDMA_WRITE:
@@ -1007,6 +1010,9 @@ int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 				tmp->wr.rdma.remote_addr =
 					i->wr.rdma.remote_addr;
 				tmp->wr.rdma.rkey = i->wr.rdma.rkey;
+				memset(&tmp->wr.rdma.reserved, 0,
+				       sizeof(tmp->wr.rdma.reserved)
+				       + (sizeof(tmp->wr) - sizeof(tmp->wr.rdma)));
 				break;
 			case IBV_WR_ATOMIC_CMP_AND_SWP:
 			case IBV_WR_ATOMIC_FETCH_AND_ADD:
@@ -1016,8 +1022,12 @@ int ibv_cmd_post_send(struct ibv_qp *ibqp, struct ibv_send_wr *wr,
 					i->wr.atomic.compare_add;
 				tmp->wr.atomic.swap = i->wr.atomic.swap;
 				tmp->wr.atomic.rkey = i->wr.atomic.rkey;
+				memset(&tmp->wr.atomic.reserved, 0,
+				       sizeof(tmp->wr.atomic.reserved)
+				       + (sizeof(tmp->wr) - sizeof(tmp->wr.atomic)));
 				break;
 			default:
+				memset(&tmp->wr, 0, sizeof(tmp->wr));
 				break;
 			}
 		}
-- 
1.9.3

--
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 libibverbs 4/9] ibv_cmd_create_srq_ex(): set reserved field in struct ibv_create_xsrq

[Yann Droneaud]

Properly initialize data structure exchanged with kernel uverbs
layer:
- may remove valgrind spurious warnings;
- may allow to use the field later for future expansion.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: 40c1365b2198 ('Add support for XRC SRQs')
Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/cmd.c b/src/cmd.c
index 8d8f7e8294e5..f80ce4d30832 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -463,6 +463,7 @@ int ibv_cmd_create_srq_ex(struct ibv_context *context,
 	cmd->max_wr      = attr_ex->attr.max_wr;
 	cmd->max_sge     = attr_ex->attr.max_sge;
 	cmd->srq_limit   = attr_ex->attr.srq_limit;
+	cmd->reserved    = 0;
 
 	cmd->srq_type = (attr_ex->comp_mask & IBV_SRQ_INIT_ATTR_TYPE) ?
 			attr_ex->srq_type : IBV_SRQT_BASIC;
-- 
1.9.3

--
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 libibverbs 5/9] kern-abi: remove unused qp_type union in struct ibv_kern_send_wr

[Yann Droneaud]

Added by commit c7e3e61052dd ('Add support for XRC QPs') in
struct ibv_kern_send_wr, qp_type union is not initialized
in ibv_cmd_post_send().

Additionnaly, as reported by pahole, this field introduces
padding in the end of struct ibv_kern_send_wr:

    struct ibv_kern_send_wr {

            /* ... */

            union {
                    struct {
                            __u32      remote_srqn;          /*    56     4 */
                    } xrc;                                   /*           4 */
            } qp_type;                                       /*    56     4 */

            /* size: 64, cachelines: 1, members: 7 */
            /* padding: 4 */
    };

This padding is not initialized too.

Since this field is not used at all kernel side and not exposed
to drivers, qp_type union in struct ibv_kern_send_wr could be
removed from the structure.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: c7e3e61052dd ('Add support for XRC QPs')
Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 include/infiniband/kern-abi.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/infiniband/kern-abi.h b/include/infiniband/kern-abi.h
index 91b45d837239..7023a0294df2 100644
--- a/include/infiniband/kern-abi.h
+++ b/include/infiniband/kern-abi.h
@@ -676,11 +676,6 @@ struct ibv_kern_send_wr {
 			__u32 reserved;
 		} ud;
 	} wr;
-	union {
-		struct {
-			__u32 remote_srqn;
-		} xrc;
-	} qp_type;
 };
 
 struct ibv_kern_eth_filter {
-- 
1.9.3

--
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 libibverbs 6/9] ibv_cmd_open_qp(): initialize reserved field in struct ibv_open_qp

[Yann Droneaud]

Properly initialize data structure exchanged with kernel uverbs
layer:
- may remove valgrind spurious warnings;
- may allow to use the field later for future expansion.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: 5f5292bbc51fb ('Add ibv_open_qp() for XRC receive QPs')
Cc: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/cmd.c b/src/cmd.c
index f80ce4d30832..f9e4a03fd84d 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -785,6 +785,7 @@ int ibv_cmd_open_qp(struct ibv_context *context, struct verbs_qp *qp,
 	cmd->pd_handle   = xrcd->handle;
 	cmd->qpn         = attr->qp_num;
 	cmd->qp_type     = attr->qp_type;
+	memset(cmd->reserved, 0, sizeof(cmd->reserved));
 
 	if (write(context->cmd_fd, cmd, cmd_size) != cmd_size)
 		return errno;
-- 
1.9.3

--
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 libibverbs 7/9] ibv_cmd_create_flow(): explicitly clear fields

[Yann Droneaud]

There's no need to clear the whole struct ibv_create_flow
+ struct ibv_kern_spec with memset().

This patch removes call to memset(), adds explicit
initialization for reserved fields, set comp_mask
explicitly

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: 389de6a6ef4e ('Add receive flow steering support')
Cc: Hadar Hen Zion <hadarh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/cmd.c b/src/cmd.c
index f9e4a03fd84d..dc155d6bf4c7 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1289,6 +1289,7 @@ static int ib_spec_to_kern_spec(struct ibv_flow_spec *ib_spec,
 				struct ibv_kern_spec *kern_spec)
 {
 	kern_spec->hdr.type = ib_spec->hdr.type;
+	kern_spec->hdr.reserved = 0;
 
 	switch (ib_spec->hdr.type) {
 	case IBV_FLOW_SPEC_ETH:
@@ -1337,15 +1338,17 @@ struct ibv_flow *ibv_cmd_create_flow(struct ibv_qp *qp,
 	flow_id = malloc(sizeof(*flow_id));
 	if (!flow_id)
 		return NULL;
-	memset(cmd, 0, cmd_size);
 
+	cmd->comp_mask = 0;
 	cmd->qp_handle = qp->handle;
 
+	cmd->flow_attr.size = 0;
 	cmd->flow_attr.type = flow_attr->type;
 	cmd->flow_attr.priority = flow_attr->priority;
 	cmd->flow_attr.num_of_specs = flow_attr->num_of_specs;
 	cmd->flow_attr.port = flow_attr->port;
 	cmd->flow_attr.flags = flow_attr->flags;
+	cmd->flow_attr.reserved[0] = cmd->flow_attr.reserved[1] = 1;
 
 	kern_spec = cmd + 1;
 	ib_spec = flow_attr + 1;
-- 
1.9.3

--
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 libibverbs 8/9] ibv_cmd_create_flow(): initialize flow_id->comp_mask

[Yann Droneaud]

ibv_cmd_create_flow() allocates and returns a struct ibv_flow,
but doesn't initialize its comp_mask field.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: 389de6a6ef4 ('Add receive flow steering support')
Cc: Hadar Hen Zion <hadarh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/cmd.c b/src/cmd.c
index dc155d6bf4c7..2f039f6852bb 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1372,6 +1372,7 @@ struct ibv_flow *ibv_cmd_create_flow(struct ibv_qp *qp,
 
 	flow_id->context = qp->context;
 	flow_id->handle = resp.flow_handle;
+	flow_id->comp_mask = 0;
 	return flow_id;
 err:
 	free(flow_id);
-- 
1.9.3

--
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 libibverbs 9/9] ibv_cmd_destroy_flow(): initialize comp_mask explicitly

[Yann Droneaud]

There's no need to clear the whole struct ibv_destroy_flow
with memset(): only comp_mask field is not explicitly
initialized in the function.

This patch removes call to memset() and initialize
comp_mask to 0.

Link: http://marc.info/?i=cover.1412163687.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Fixes: 389de6a6ef4e ('Add receive flow steering support')
Cc: Hadar Hen Zion <hadarh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cmd.c b/src/cmd.c
index 2f039f6852bb..d849e6dac214 100644
--- a/src/cmd.c
+++ b/src/cmd.c
@@ -1384,8 +1384,8 @@ int ibv_cmd_destroy_flow(struct ibv_flow *flow_id)
 	struct ibv_destroy_flow cmd;
 	int ret = 0;
 
-	memset(&cmd, 0, sizeof(cmd));
 	IBV_INIT_CMD_EX(&cmd, sizeof(cmd), DESTROY_FLOW);
+	cmd.comp_mask = 0;
 	cmd.flow_handle = flow_id->handle;
 
 	if (write(flow_id->context->cmd_fd, &cmd, sizeof(cmd)) != sizeof(cmd))
-- 
1.9.3

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