IB/core: more fixes for create_flow uverbs (for v3.12)

PATCH In review Yann Droneaud 2 participants 15 messages Oct 9, 2013 – Oct 10, 2013 4 messages not in the archive InfiniBand

lore

Share & tools

lore

Yann Droneaud

author

2013-10-09 21:12 UTC #0 lore raw

cover IB/core: more fixes for create_flow uverbs (for v3.12)

Hi,

I've suggested reverting temporarily commit 22878db, 436f2ad, 400dbc9 for v3.12 in a previous message [1]. This proposition doesn't sound acceptable for Or.

So I continue to improve them to be in a better shape for v3.12.

Please find some important changes against create_flow uverbs implementation.

The most notable changes that should go to v3.12:
- structure renaming to match others uverbs public structs;
- changes usage of the flow_attr.size to not count the "extended command header" but to describe only the size of the flow specs following flow_attr;
- removed unneeded flow_spec structure that don't need to be exposed to userspace.
- ensure 64bits alignement (regarding patchset [2])

Others changes are mostly improving the uverbs function to protect from malformed flow_spec. Those could be added later (v3.13) but I haven't ordered the patches this way: all of this is a bit developped in a hurry to catch v3.12.

Please review and tell me which patchset should go first
to rebase one on the other: this one, or the "improved infrastructure" patchset[2].

Links:

[1]: http://marc.info/[email protected]

     http://mid.gmane.org/[email protected]

[2]: http://marc.info/?i=cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

     http://mid.gmane.org/cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Regards.

Yann Droneaud (10):
IB/core: Rename 'flow' structs to match other uverbs structs IB/core: Makes uverbs flow structure using names more similar to verbs
ones
IB/core: Don't include command size in uverbs create_flow IB/core: Allocate memory only for flow_spec in create_flow uverbs IB/core: Remove checks for negative values in create_flow uverbs IB/core: Check against uverbs structure size while parsing flow_spec IB/core: Uses a common header for uverbs flow_specs IB/core: Use flow_spec common header in uverbs flow_spec conversion
function
IB/core: Remove ib_uverbs_flow_spec structure from userspace IB/core: Add proper bound checking in flow_spec uverbs conversion to
verbs

 drivers/infiniband/core/uverbs.h     |  16 +++++
 drivers/infiniband/core/uverbs_cmd.c | 136 +++++++++++++++++++++--------------
 include/uapi/rdma/ib_user_verbs.h    |  69 ++++++++++--------
 3 files changed, 139 insertions(+), 82 deletions(-)

Signature · 6 lines

1.8.3.1

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

A message that is not in the archive (sent off-list or not yet received): cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud

author

2013-10-09 21:12 UTC #2 lore raw

PATCH 1/10 IB/core: Rename 'flow' structs to match other uverbs structs

Commit 436f2ad added public data structures to support receive flow steering. The new structs are not following the 'uverbs' pattern: they're lacking the common prefix 'ib_uverbs'.

This patch replaces ib_kern prefix by ib_uverbs.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 76 ++++++++++++++++++------------------
 include/uapi/rdma/ib_user_verbs.h    | 36 ++++++++---------
 2 files changed, 56 insertions(+), 56 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +38 ~~-38~~

index f2b81b9..e16e22e 100644
@@ -2599,38 +2599,38 @@ out_put:
 	return ret ? ret : in_len;
 }
 
-static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
+static int uverbs_spec_to_ib_spec(struct ib_uverbs_spec *uverbs_spec,
 				union ib_flow_spec *ib_spec)
 {
-	ib_spec->type = kern_spec->type;
+	ib_spec->type = uverbs_spec->type;
 
 	switch (ib_spec->type) {
 	case IB_FLOW_SPEC_ETH:
 		ib_spec->eth.size = sizeof(struct ib_flow_spec_eth);
-		if (ib_spec->eth.size != kern_spec->eth.size)
+		if (ib_spec->eth.size != uverbs_spec->eth.size)
 			return -EINVAL;
-		memcpy(&ib_spec->eth.val, &kern_spec->eth.val,
+		memcpy(&ib_spec->eth.val, &uverbs_spec->eth.val,
 		       sizeof(struct ib_flow_eth_filter));
-		memcpy(&ib_spec->eth.mask, &kern_spec->eth.mask,
+		memcpy(&ib_spec->eth.mask, &uverbs_spec->eth.mask,
 		       sizeof(struct ib_flow_eth_filter));
 		break;
 	case IB_FLOW_SPEC_IPV4:
 		ib_spec->ipv4.size = sizeof(struct ib_flow_spec_ipv4);
-		if (ib_spec->ipv4.size != kern_spec->ipv4.size)
+		if (ib_spec->ipv4.size != uverbs_spec->ipv4.size)
 			return -EINVAL;
-		memcpy(&ib_spec->ipv4.val, &kern_spec->ipv4.val,
+		memcpy(&ib_spec->ipv4.val, &uverbs_spec->ipv4.val,
 		       sizeof(struct ib_flow_ipv4_filter));
-		memcpy(&ib_spec->ipv4.mask, &kern_spec->ipv4.mask,
+		memcpy(&ib_spec->ipv4.mask, &uverbs_spec->ipv4.mask,
 		       sizeof(struct ib_flow_ipv4_filter));
 		break;
 	case IB_FLOW_SPEC_TCP:
 	case IB_FLOW_SPEC_UDP:
 		ib_spec->tcp_udp.size = sizeof(struct ib_flow_spec_tcp_udp);
-		if (ib_spec->tcp_udp.size != kern_spec->tcp_udp.size)
+		if (ib_spec->tcp_udp.size != uverbs_spec->tcp_udp.size)
 			return -EINVAL;
-		memcpy(&ib_spec->tcp_udp.val, &kern_spec->tcp_udp.val,
+		memcpy(&ib_spec->tcp_udp.val, &uverbs_spec->tcp_udp.val,
 		       sizeof(struct ib_flow_tcp_udp_filter));
-		memcpy(&ib_spec->tcp_udp.mask, &kern_spec->tcp_udp.mask,
+		memcpy(&ib_spec->tcp_udp.mask, &uverbs_spec->tcp_udp.mask,
 		       sizeof(struct ib_flow_tcp_udp_filter));
 		break;
 	default:

@@ -2647,14 +2647,14 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	struct ib_uverbs_create_flow_resp resp;
 	struct ib_uobject		  *uobj;
 	struct ib_flow			  *flow_id;
-	struct ib_kern_flow_attr	  *kern_flow_attr;
+	struct ib_uverbs_flow_attr	  *uverbs_flow_attr;
 	struct ib_flow_attr		  *flow_attr;
 	struct ib_qp			  *qp;
 	int err = 0;
-	void *kern_spec;
+	void *uverbs_spec;
 	void *ib_spec;
 	int i;
-	int kern_attr_size;
+	int uverbs_attr_size;
 
 	if (out_len < sizeof(resp))
 		return -ENOSPC;

@@ -2673,28 +2673,28 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
 		return -EINVAL;
 
-	kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
+	uverbs_attr_size = cmd.flow_attr.size - sizeof(cmd) -
 			 sizeof(struct ib_uverbs_cmd_hdr_ex);
 
 	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
-	    kern_attr_size < 0 || kern_attr_size >
-	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
+	    uverbs_attr_size < 0 || uverbs_attr_size >
+	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_spec)))
 		return -EINVAL;
 
 	if (cmd.flow_attr.num_of_specs) {
-		kern_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
-		if (!kern_flow_attr)
+		uverbs_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+		if (!uverbs_flow_attr)
 			return -ENOMEM;
 
-		memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
-		if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
-				   kern_attr_size)) {
+		memcpy(uverbs_flow_attr, &cmd.flow_attr, sizeof(*uverbs_flow_attr));
+		if (copy_from_user(uverbs_flow_attr + 1, buf + sizeof(cmd),
+				   uverbs_attr_size)) {
 			err = -EFAULT;
 			goto err_free_attr;
 		}
 	} else {
-		kern_flow_attr = &cmd.flow_attr;
-		kern_attr_size = sizeof(cmd.flow_attr);
+		uverbs_flow_attr = &cmd.flow_attr;
+		uverbs_attr_size = sizeof(cmd.flow_attr);
 	}
 
 	uobj = kmalloc(sizeof(*uobj), GFP_KERNEL);

@@ -2717,28 +2717,28 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		goto err_put;
 	}
 
-	flow_attr->type = kern_flow_attr->type;
-	flow_attr->priority = kern_flow_attr->priority;
-	flow_attr->num_of_specs = kern_flow_attr->num_of_specs;
-	flow_attr->port = kern_flow_attr->port;
-	flow_attr->flags = kern_flow_attr->flags;
+	flow_attr->type = uverbs_flow_attr->type;
+	flow_attr->priority = uverbs_flow_attr->priority;
+	flow_attr->num_of_specs = uverbs_flow_attr->num_of_specs;
+	flow_attr->port = uverbs_flow_attr->port;
+	flow_attr->flags = uverbs_flow_attr->flags;
 	flow_attr->size = sizeof(*flow_attr);
 
-	kern_spec = kern_flow_attr + 1;
+	uverbs_spec = uverbs_flow_attr + 1;
 	ib_spec = flow_attr + 1;
-	for (i = 0; i < flow_attr->num_of_specs && kern_attr_size > 0; i++) {
-		err = kern_spec_to_ib_spec(kern_spec, ib_spec);
+	for (i = 0; i < flow_attr->num_of_specs && uverbs_attr_size > 0; i++) {
+		err = uverbs_spec_to_ib_spec(uverbs_spec, ib_spec);
 		if (err)
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		kern_attr_size -= ((struct ib_kern_spec *) kern_spec)->size;
-		kern_spec += ((struct ib_kern_spec *) kern_spec)->size;
+		uverbs_attr_size -= ((struct ib_uverbs_spec *) uverbs_spec)->size;
+		uverbs_spec += ((struct ib_uverbs_spec *) uverbs_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
-	if (kern_attr_size) {
+	if (uverbs_attr_size) {
 		pr_warn("create flow failed, %d bytes left from uverb cmd\n",
-			kern_attr_size);
+			uverbs_attr_size);
 		goto err_free;
 	}
 	flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);

@@ -2773,7 +2773,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	up_write(&uobj->mutex);
 	kfree(flow_attr);
 	if (cmd.flow_attr.num_of_specs)
-		kfree(kern_flow_attr);
+		kfree(uverbs_flow_attr);
 	return in_len;
 err_copy:
 	idr_remove_uobj(&ib_uverbs_rule_idr, uobj);

@@ -2787,7 +2787,7 @@ err_uobj:
 	put_uobj_write(uobj);
 err_free_attr:
 	if (cmd.flow_attr.num_of_specs)
-		kfree(kern_flow_attr);
+		kfree(uverbs_flow_attr);
 	return err;
 }

include/uapi/rdma/ib_user_verbs.h +18 ~~-18~~

index 0b233c5..f8fea78 100644
@@ -696,61 +696,61 @@ struct ib_uverbs_detach_mcast {
 	__u64 driver_data[0];
 };
 
-struct ib_kern_eth_filter {
+struct ib_uverbs_eth_filter {
 	__u8  dst_mac[6];
 	__u8  src_mac[6];
 	__be16 ether_type;
 	__be16 vlan_tag;
 };
 
-struct ib_kern_spec_eth {
+struct ib_uverbs_spec_eth {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_kern_eth_filter val;
-	struct ib_kern_eth_filter mask;
+	struct ib_uverbs_eth_filter val;
+	struct ib_uverbs_eth_filter mask;
 };
 
-struct ib_kern_ipv4_filter {
+struct ib_uverbs_ipv4_filter {
 	__be32 src_ip;
 	__be32 dst_ip;
 };
 
-struct ib_kern_spec_ipv4 {
+struct ib_uverbs_spec_ipv4 {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_kern_ipv4_filter val;
-	struct ib_kern_ipv4_filter mask;
+	struct ib_uverbs_ipv4_filter val;
+	struct ib_uverbs_ipv4_filter mask;
 };
 
-struct ib_kern_tcp_udp_filter {
+struct ib_uverbs_tcp_udp_filter {
 	__be16 dst_port;
 	__be16 src_port;
 };
 
-struct ib_kern_spec_tcp_udp {
+struct ib_uverbs_spec_tcp_udp {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_kern_tcp_udp_filter val;
-	struct ib_kern_tcp_udp_filter mask;
+	struct ib_uverbs_tcp_udp_filter val;
+	struct ib_uverbs_tcp_udp_filter mask;
 };
 
-struct ib_kern_spec {
+struct ib_uverbs_spec {
 	union {
 		struct {
 			__u32 type;
 			__u16 size;
 			__u16 reserved;
 		};
-		struct ib_kern_spec_eth	    eth;
-		struct ib_kern_spec_ipv4    ipv4;
-		struct ib_kern_spec_tcp_udp tcp_udp;
+		struct ib_uverbs_spec_eth	    eth;
+		struct ib_uverbs_spec_ipv4    ipv4;
+		struct ib_uverbs_spec_tcp_udp tcp_udp;
 	};
 };
 
-struct ib_kern_flow_attr {
+struct ib_uverbs_flow_attr {
 	__u32 type;
 	__u16 size;
 	__u16 priority;

@@ -768,7 +768,7 @@ struct ib_uverbs_create_flow  {
 	__u32 comp_mask;
 	__u64 response;
 	__u32 qp_handle;
-	struct ib_kern_flow_attr flow_attr;
+	struct ib_uverbs_flow_attr flow_attr;
 };
 
 struct ib_uverbs_create_flow_resp {

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #3 lore raw

PATCH 2/10 IB/core: Makes uverbs flow structure using names more similar to verbs ones

In reply to a message not in the archive

This patch add "flow" prefix to most of data structure added as part of commit 436f2ad to keep those names in sync with the data structures added in commit 319a441.

It's just a matter of translating 'ib_flow' to 'ib_uverbs_flow'.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c |  8 ++++----
 include/uapi/rdma/ib_user_verbs.h    | 32 ++++++++++++++++----------------
 2 files changed, 20 insertions(+), 20 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +4 -4

index e16e22e..a4e387d 100644
@@ -2599,7 +2599,7 @@ out_put:
 	return ret ? ret : in_len;
 }
 
-static int uverbs_spec_to_ib_spec(struct ib_uverbs_spec *uverbs_spec,
+static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec *uverbs_spec,
 				union ib_flow_spec *ib_spec)
 {
 	ib_spec->type = uverbs_spec->type;

@@ -2678,7 +2678,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 
 	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
 	    uverbs_attr_size < 0 || uverbs_attr_size >
-	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_spec)))
+	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
 
 	if (cmd.flow_attr.num_of_specs) {

@@ -2732,8 +2732,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		uverbs_attr_size -= ((struct ib_uverbs_spec *) uverbs_spec)->size;
-		uverbs_spec += ((struct ib_uverbs_spec *) uverbs_spec)->size;
+		uverbs_attr_size -= ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
+		uverbs_spec += ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
 	if (uverbs_attr_size) {

include/uapi/rdma/ib_user_verbs.h +16 ~~-16~~

index f8fea78..61ecd59 100644
@@ -696,57 +696,57 @@ struct ib_uverbs_detach_mcast {
 	__u64 driver_data[0];
 };
 
-struct ib_uverbs_eth_filter {
+struct ib_uverbs_flow_eth_filter {
 	__u8  dst_mac[6];
 	__u8  src_mac[6];
 	__be16 ether_type;
 	__be16 vlan_tag;
 };
 
-struct ib_uverbs_spec_eth {
+struct ib_uverbs_flow_spec_eth {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_uverbs_eth_filter val;
-	struct ib_uverbs_eth_filter mask;
+	struct ib_uverbs_flow_eth_filter val;
+	struct ib_uverbs_flow_eth_filter mask;
 };
 
-struct ib_uverbs_ipv4_filter {
+struct ib_uverbs_flow_ipv4_filter {
 	__be32 src_ip;
 	__be32 dst_ip;
 };
 
-struct ib_uverbs_spec_ipv4 {
+struct ib_uverbs_flow_spec_ipv4 {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_uverbs_ipv4_filter val;
-	struct ib_uverbs_ipv4_filter mask;
+	struct ib_uverbs_flow_ipv4_filter val;
+	struct ib_uverbs_flow_ipv4_filter mask;
 };
 
-struct ib_uverbs_tcp_udp_filter {
+struct ib_uverbs_flow_tcp_udp_filter {
 	__be16 dst_port;
 	__be16 src_port;
 };
 
-struct ib_uverbs_spec_tcp_udp {
+struct ib_uverbs_flow_spec_tcp_udp {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_uverbs_tcp_udp_filter val;
-	struct ib_uverbs_tcp_udp_filter mask;
+	struct ib_uverbs_flow_tcp_udp_filter val;
+	struct ib_uverbs_flow_tcp_udp_filter mask;
 };
 
-struct ib_uverbs_spec {
+struct ib_uverbs_flow_spec {
 	union {
 		struct {
 			__u32 type;
 			__u16 size;
 			__u16 reserved;
 		};
-		struct ib_uverbs_spec_eth	    eth;
-		struct ib_uverbs_spec_ipv4    ipv4;
-		struct ib_uverbs_spec_tcp_udp tcp_udp;
+		struct ib_uverbs_flow_spec_eth	    eth;
+		struct ib_uverbs_flow_spec_ipv4    ipv4;
+		struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
 	};
 };

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #4 lore raw

PATCH 3/10 IB/core: Don't include command size in uverbs create_flow

In reply to a message not in the archive

This patch changes the semantic of the size field of create_flow command to be the size of the flow_spec's list instead of the total size of the command, counting command header, command and flow_spec's.

Flow attributes must be independent of command header:
they are part of different layers.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +3 -4

index a4e387d..54ee607 100644
@@ -2673,10 +2673,9 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
 		return -EINVAL;
 
-	uverbs_attr_size = cmd.flow_attr.size - sizeof(cmd) -
-			 sizeof(struct ib_uverbs_cmd_hdr_ex);
+	uverbs_attr_size = cmd.flow_attr.size;
 
-	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
+	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
 	    uverbs_attr_size < 0 || uverbs_attr_size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;

@@ -2711,7 +2710,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		goto err_uobj;
 	}
 
-	flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+	flow_attr = kmalloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #5 lore raw

PATCH 4/10 IB/core: Allocate memory only for flow_spec in create_flow uverbs

In reply to a message not in the archive

Instead of allocating memory for the flow attribute structure and all the flow specs, this patch makes use of the flow attribute structure already read as part of the command, allocating memory only for the flow_specs.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 52 +++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 28 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +24 ~~-28~~

index 54ee607..29c89a3 100644
@@ -2647,14 +2647,14 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	struct ib_uverbs_create_flow_resp resp;
 	struct ib_uobject		  *uobj;
 	struct ib_flow			  *flow_id;
-	struct ib_uverbs_flow_attr	  *uverbs_flow_attr;
+	struct ib_uverbs_flow_spec	  *uverbs_flow_spec = NULL;
 	struct ib_flow_attr		  *flow_attr;
 	struct ib_qp			  *qp;
 	int err = 0;
 	void *uverbs_spec;
 	void *ib_spec;
 	int i;
-	int uverbs_attr_size;
+	int uverbs_spec_size;
 
 	if (out_len < sizeof(resp))
 		return -ENOSPC;

@@ -2673,33 +2673,29 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
 		return -EINVAL;
 
-	uverbs_attr_size = cmd.flow_attr.size;
+	uverbs_spec_size = cmd.flow_attr.size;
 
 	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
-	    uverbs_attr_size < 0 || uverbs_attr_size >
+	    uverbs_spec_size < 0 || uverbs_spec_size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
 
 	if (cmd.flow_attr.num_of_specs) {
-		uverbs_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
-		if (!uverbs_flow_attr)
+		uverbs_flow_spec = kmalloc(uverbs_spec_size, GFP_KERNEL);
+		if (!uverbs_flow_spec)
 			return -ENOMEM;
 
-		memcpy(uverbs_flow_attr, &cmd.flow_attr, sizeof(*uverbs_flow_attr));
-		if (copy_from_user(uverbs_flow_attr + 1, buf + sizeof(cmd),
-				   uverbs_attr_size)) {
+		if (copy_from_user(uverbs_flow_spec, buf + sizeof(cmd),
+				   uverbs_spec_size)) {
 			err = -EFAULT;
-			goto err_free_attr;
+			goto err_free_spec;
 		}
-	} else {
-		uverbs_flow_attr = &cmd.flow_attr;
-		uverbs_attr_size = sizeof(cmd.flow_attr);
 	}
 
 	uobj = kmalloc(sizeof(*uobj), GFP_KERNEL);
 	if (!uobj) {
 		err = -ENOMEM;
-		goto err_free_attr;
+		goto err_free_spec;
 	}
 	init_uobj(uobj, 0, file->ucontext, &rule_lock_class);
 	down_write(&uobj->mutex);

@@ -2710,34 +2706,34 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		goto err_uobj;
 	}
 
-	flow_attr = kmalloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
+	flow_attr = kmalloc(sizeof(*flow_attr) + uverbs_spec_size, GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;
 	}
 
-	flow_attr->type = uverbs_flow_attr->type;
-	flow_attr->priority = uverbs_flow_attr->priority;
-	flow_attr->num_of_specs = uverbs_flow_attr->num_of_specs;
-	flow_attr->port = uverbs_flow_attr->port;
-	flow_attr->flags = uverbs_flow_attr->flags;
+	flow_attr->type = cmd.flow_attr.type;
+	flow_attr->priority = cmd.flow_attr.priority;
+	flow_attr->num_of_specs = cmd.flow_attr.num_of_specs;
+	flow_attr->port = cmd.flow_attr.port;
+	flow_attr->flags = cmd.flow_attr.flags;
 	flow_attr->size = sizeof(*flow_attr);
 
-	uverbs_spec = uverbs_flow_attr + 1;
+	uverbs_spec = uverbs_flow_spec;
 	ib_spec = flow_attr + 1;
-	for (i = 0; i < flow_attr->num_of_specs && uverbs_attr_size > 0; i++) {
+	for (i = 0; i < flow_attr->num_of_specs && uverbs_spec_size > 0; i++) {
 		err = uverbs_spec_to_ib_spec(uverbs_spec, ib_spec);
 		if (err)
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		uverbs_attr_size -= ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
+		uverbs_spec_size -= ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
 		uverbs_spec += ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
-	if (uverbs_attr_size) {
+	if (uverbs_spec_size) {
 		pr_warn("create flow failed, %d bytes left from uverb cmd\n",
-			uverbs_attr_size);
+			uverbs_spec_size);
 		goto err_free;
 	}
 	flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);

@@ -2772,7 +2768,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	up_write(&uobj->mutex);
 	kfree(flow_attr);
 	if (cmd.flow_attr.num_of_specs)
-		kfree(uverbs_flow_attr);
+		kfree(uverbs_flow_spec);
 	return in_len;
 err_copy:
 	idr_remove_uobj(&ib_uverbs_rule_idr, uobj);

@@ -2784,9 +2780,9 @@ err_put:
 	put_qp_read(qp);
 err_uobj:
 	put_uobj_write(uobj);
-err_free_attr:
+err_free_spec:
 	if (cmd.flow_attr.num_of_specs)
-		kfree(uverbs_flow_attr);
+		kfree(uverbs_flow_spec);
 	return err;
 }

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #6 lore raw

PATCH 5/10 IB/core: Remove checks for negative values in create_flow uverbs

In reply to a message not in the archive

Fields in flow_attr structure are unsigned, so it's not necessary to check for negative values.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +6 -5

index 29c89a3..0ea5529 100644
@@ -2669,17 +2669,18 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
 		return -EPERM;
 
-	if (cmd.flow_attr.num_of_specs < 0 ||
-	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
+	if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
 		return -EINVAL;
 
-	uverbs_spec_size = cmd.flow_attr.size;
+	if (cmd.flow_attr.size > (in_len - sizeof(cmd)))
+		return -EINVAL;
 
-	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
-	    uverbs_spec_size < 0 || uverbs_spec_size >
+	if (cmd.flow_attr.size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
 
+	uverbs_spec_size = cmd.flow_attr.size;
+
 	if (cmd.flow_attr.num_of_specs) {
 		uverbs_flow_spec = kmalloc(uverbs_spec_size, GFP_KERNEL);
 		if (!uverbs_flow_spec)

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #7 lore raw

PATCH 6/10 IB/core: Check against uverbs structure size while parsing flow_spec

In reply to a message not in the archive

Instead of checking input size against the kernel verbs data structure, it's safer to check input size against uverbs data structure.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +6 -6

index 0ea5529..106e997 100644
@@ -2606,18 +2606,18 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec *uverbs_spec,
 
 	switch (ib_spec->type) {
 	case IB_FLOW_SPEC_ETH:
-		ib_spec->eth.size = sizeof(struct ib_flow_spec_eth);
-		if (ib_spec->eth.size != uverbs_spec->eth.size)
+		if (uverbs_spec->eth.size != sizeof(struct ib_uverbs_flow_spec_eth))
 			return -EINVAL;
+		ib_spec->eth.size = sizeof(struct ib_flow_spec_eth);
 		memcpy(&ib_spec->eth.val, &uverbs_spec->eth.val,
 		       sizeof(struct ib_flow_eth_filter));
 		memcpy(&ib_spec->eth.mask, &uverbs_spec->eth.mask,
 		       sizeof(struct ib_flow_eth_filter));
 		break;
 	case IB_FLOW_SPEC_IPV4:
-		ib_spec->ipv4.size = sizeof(struct ib_flow_spec_ipv4);
-		if (ib_spec->ipv4.size != uverbs_spec->ipv4.size)
+		if (uverbs_spec->ipv4.size != sizeof(struct ib_uverbs_flow_spec_ipv4))
 			return -EINVAL;
+		ib_spec->ipv4.size = sizeof(struct ib_flow_spec_ipv4);
 		memcpy(&ib_spec->ipv4.val, &uverbs_spec->ipv4.val,
 		       sizeof(struct ib_flow_ipv4_filter));
 		memcpy(&ib_spec->ipv4.mask, &uverbs_spec->ipv4.mask,

@@ -2625,9 +2625,9 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec *uverbs_spec,
 		break;
 	case IB_FLOW_SPEC_TCP:
 	case IB_FLOW_SPEC_UDP:
-		ib_spec->tcp_udp.size = sizeof(struct ib_flow_spec_tcp_udp);
-		if (ib_spec->tcp_udp.size != uverbs_spec->tcp_udp.size)
+		if (uverbs_spec->tcp_udp.size != sizeof(struct ib_uverbs_flow_spec_tcp_udp))
 			return -EINVAL;
+		ib_spec->tcp_udp.size = sizeof(struct ib_flow_spec_tcp_udp);
 		memcpy(&ib_spec->tcp_udp.val, &uverbs_spec->tcp_udp.val,
 		       sizeof(struct ib_flow_tcp_udp_filter));
 		memcpy(&ib_spec->tcp_udp.mask, &uverbs_spec->tcp_udp.mask,

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #8 lore raw

PATCH 7/10 IB/core: Uses a common header for uverbs flow_specs

In reply to a message not in the archive

A common header allows better checking of flow specs size, while ensuring strict alignment to 64bits.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 10 +++++--
 include/uapi/rdma/ib_user_verbs.h    | 53 +++++++++++++++++++++++++++---------
 2 files changed, 47 insertions(+), 16 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +7 -3

index 106e997..6b066a3 100644
@@ -2647,7 +2647,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	struct ib_uverbs_create_flow_resp resp;
 	struct ib_uobject		  *uobj;
 	struct ib_flow			  *flow_id;
-	struct ib_uverbs_flow_spec	  *uverbs_flow_spec = NULL;
+	struct ib_uverbs_flow_spec_hdr	  *uverbs_flow_spec = NULL;
 	struct ib_flow_attr		  *flow_attr;
 	struct ib_qp			  *qp;
 	int err = 0;

@@ -2675,6 +2675,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	if (cmd.flow_attr.size > (in_len - sizeof(cmd)))
 		return -EINVAL;
 
+	if (cmd.flow_attr.size <
+	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec_hdr)))
+		return -EINVAL;
+
 	if (cmd.flow_attr.size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;

@@ -2728,8 +2732,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		uverbs_spec_size -= ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
-		uverbs_spec += ((struct ib_uverbs_flow_spec *) uverbs_spec)->size;
+		uverbs_spec_size -= ((struct ib_uverbs_flow_spec_hdr *) uverbs_spec)->size;
+		uverbs_spec += ((struct ib_uverbs_flow_spec_hdr *) uverbs_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
 	if (uverbs_spec_size) {

include/uapi/rdma/ib_user_verbs.h +40 ~~-13~~

89 lines not on this page · raw message

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #9 lore raw

PATCH 8/10 IB/core: Use flow_spec common header in uverbs flow_spec conversion function

In reply to a message not in the archive

While the validity (and type) of the flow_spec is not known before calling the conversion function, it's make more sense to use a pointer to the header structure.

Once the header parameters are validated, the flow_spec can be processed.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +10 -5

index 6b066a3..8079383 100644
@@ -2599,15 +2599,18 @@ out_put:
 	return ret ? ret : in_len;
 }
 
-static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec *uverbs_spec,
+static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hdr,
 				union ib_flow_spec *ib_spec)
 {
-	ib_spec->type = uverbs_spec->type;
+	struct ib_uverbs_flow_spec *uverbs_spec;
+
+	ib_spec->type = uverbs_spec_hdr->type;
 
 	switch (ib_spec->type) {
 	case IB_FLOW_SPEC_ETH:
-		if (uverbs_spec->eth.size != sizeof(struct ib_uverbs_flow_spec_eth))
+		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_eth))
 			return -EINVAL;
+		uverbs_spec = (struct ib_uverbs_flow_spec *)uverbs_spec_hdr;
 		ib_spec->eth.size = sizeof(struct ib_flow_spec_eth);
 		memcpy(&ib_spec->eth.val, &uverbs_spec->eth.val,
 		       sizeof(struct ib_flow_eth_filter));

@@ -2615,8 +2618,9 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec *uverbs_spec,
 		       sizeof(struct ib_flow_eth_filter));
 		break;
 	case IB_FLOW_SPEC_IPV4:
-		if (uverbs_spec->ipv4.size != sizeof(struct ib_uverbs_flow_spec_ipv4))
+		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_ipv4))
 			return -EINVAL;
+		uverbs_spec = (struct ib_uverbs_flow_spec *)uverbs_spec_hdr;
 		ib_spec->ipv4.size = sizeof(struct ib_flow_spec_ipv4);
 		memcpy(&ib_spec->ipv4.val, &uverbs_spec->ipv4.val,
 		       sizeof(struct ib_flow_ipv4_filter));

@@ -2625,8 +2629,9 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec *uverbs_spec,
 		break;
 	case IB_FLOW_SPEC_TCP:
 	case IB_FLOW_SPEC_UDP:
-		if (uverbs_spec->tcp_udp.size != sizeof(struct ib_uverbs_flow_spec_tcp_udp))
+		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_tcp_udp))
 			return -EINVAL;
+		uverbs_spec = (struct ib_uverbs_flow_spec *)uverbs_spec_hdr;
 		ib_spec->tcp_udp.size = sizeof(struct ib_flow_spec_tcp_udp);
 		memcpy(&ib_spec->tcp_udp.val, &uverbs_spec->tcp_udp.val,
 		       sizeof(struct ib_flow_tcp_udp_filter));

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #10 lore raw

PATCH 9/10 IB/core: Remove ib_uverbs_flow_spec structure from userspace

In reply to a message not in the archive

The structure holding any types of flow_spec is of no use to userspace. It would be wrong for userspace to do:

struct ib_uverbs_flow_spec flow_spec;

  flow_spec.type = IB_FLOW_SPEC_TCP;
  flow_spec.size = sizeof(flow_spec);

Instead, userspace should use the dedicated flow_spec structure for
- Ethernet : struct ib_uverbs_flow_spec_eth,
- IPv4 : struct ib_uverbs_flow_spec_ipv4,
- TCP/UDP : struct ib_uverbs_flow_spec_tcp_udp.

In other words, struct ib_uverbs_flow_spec is a "virtual" data structure that can only be use by the kernel as an alias to the other.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs.h     | 16 ++++++++++++++++
 drivers/infiniband/core/uverbs_cmd.c | 10 +++++-----
 include/uapi/rdma/ib_user_verbs.h    | 16 ----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

drivers/infiniband/core/uverbs.h +16 -0

index d040b87..b16e9fb 100644
@@ -178,6 +178,22 @@ void ib_uverbs_event_handler(struct ib_event_handler *handler,
 			     struct ib_event *event);
 void ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd);
 
+struct ib_uverbs_flow_spec_storage {
+	union {
+		union {
+			struct ib_uverbs_flow_spec_hdr hdr;
+			struct {
+				__u32 type;
+				__u16 size;
+				__u16 reserved;
+			};
+		};
+		struct ib_uverbs_flow_spec_eth     eth;
+		struct ib_uverbs_flow_spec_ipv4    ipv4;
+		struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
+	};
+};
+
 #define IB_UVERBS_DECLARE_CMD(name)					\
 	ssize_t ib_uverbs_##name(struct ib_uverbs_file *file,		\
 				 const char __user *buf, int in_len,	\

drivers/infiniband/core/uverbs_cmd.c +5 -5

index 8079383..902dfd3 100644
@@ -2602,7 +2602,7 @@ out_put:
 static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hdr,
 				union ib_flow_spec *ib_spec)
 {
-	struct ib_uverbs_flow_spec *uverbs_spec;
+	struct ib_uverbs_flow_spec_storage *uverbs_spec;
 
 	ib_spec->type = uverbs_spec_hdr->type;

@@ -2610,7 +2610,7 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hd
 	case IB_FLOW_SPEC_ETH:
 		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_eth))
 			return -EINVAL;
-		uverbs_spec = (struct ib_uverbs_flow_spec *)uverbs_spec_hdr;
+		uverbs_spec = (struct ib_uverbs_flow_spec_storage *)uverbs_spec_hdr;
 		ib_spec->eth.size = sizeof(struct ib_flow_spec_eth);
 		memcpy(&ib_spec->eth.val, &uverbs_spec->eth.val,
 		       sizeof(struct ib_flow_eth_filter));

@@ -2620,7 +2620,7 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hd
 	case IB_FLOW_SPEC_IPV4:
 		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_ipv4))
 			return -EINVAL;
-		uverbs_spec = (struct ib_uverbs_flow_spec *)uverbs_spec_hdr;
+		uverbs_spec = (struct ib_uverbs_flow_spec_storage *)uverbs_spec_hdr;
 		ib_spec->ipv4.size = sizeof(struct ib_flow_spec_ipv4);
 		memcpy(&ib_spec->ipv4.val, &uverbs_spec->ipv4.val,
 		       sizeof(struct ib_flow_ipv4_filter));

@@ -2631,7 +2631,7 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hd
 	case IB_FLOW_SPEC_UDP:
 		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_tcp_udp))
 			return -EINVAL;
-		uverbs_spec = (struct ib_uverbs_flow_spec *)uverbs_spec_hdr;
+		uverbs_spec = (struct ib_uverbs_flow_spec_storage *)uverbs_spec_hdr;
 		ib_spec->tcp_udp.size = sizeof(struct ib_flow_spec_tcp_udp);
 		memcpy(&ib_spec->tcp_udp.val, &uverbs_spec->tcp_udp.val,
 		       sizeof(struct ib_flow_tcp_udp_filter));

@@ -2685,7 +2685,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		return -EINVAL;
 
 	if (cmd.flow_attr.size >
-	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
+	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec_storage)))
 		return -EINVAL;
 
 	uverbs_spec_size = cmd.flow_attr.size;

include/uapi/rdma/ib_user_verbs.h +0 ~~-16~~

index 8be941e..cd12dd7 100644
@@ -760,22 +760,6 @@ struct ib_uverbs_flow_spec_tcp_udp {
 	struct ib_uverbs_flow_tcp_udp_filter mask;
 };
 
-struct ib_uverbs_flow_spec {
-	union {
-		union {
-			struct ib_uverbs_flow_spec_hdr hdr;
-			struct {
-				__u32 type;
-				__u16 size;
-				__u16 reserved;
-			};
-		};
-		struct ib_uverbs_flow_spec_eth	    eth;
-		struct ib_uverbs_flow_spec_ipv4    ipv4;
-		struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
-	};
-};
-
 struct ib_uverbs_flow_attr {
 	__u32 type;
 	__u16 size;

Signature · 6 lines

1.8.3.1

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

Yann Droneaud

author

2013-10-09 21:12 UTC #11 lore raw

PATCH 10/10 IB/core: Add proper bound checking in flow_spec uverbs conversion to verbs

In reply to a message not in the archive

Relying on the size sent by userspace as the size of kernel internal data structure is not safe.

This patch add some level of protections to
- not read past uverbs command in case of truncated uverbs flow spec
- not write past flow_attr in case of changes in kernel flow spec data structure format: at some point the format of internal flow_spec may change while the userspace format is kept the same.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
 drivers/infiniband/core/uverbs_cmd.c | 41 +++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 8 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +33 -8

index 902dfd3..1887b8a 100644
@@ -2600,17 +2600,25 @@ out_put:
 }
 
 static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hdr,
-				union ib_flow_spec *ib_spec)
+				  size_t uverbs_spec_size,
+				  union ib_flow_spec *ib_spec,
+				  size_t ib_spec_size)
 {
 	struct ib_uverbs_flow_spec_storage *uverbs_spec;
 
-	ib_spec->type = uverbs_spec_hdr->type;
+	if (uverbs_spec_size < sizeof(struct ib_uverbs_flow_spec_hdr))
+		return -EINVAL;
 
-	switch (ib_spec->type) {
+	switch (uverbs_spec_hdr->type) {
 	case IB_FLOW_SPEC_ETH:
+		if (uverbs_spec_size < sizeof(struct ib_uverbs_flow_spec_eth))
+			return -EINVAL;
 		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_eth))
 			return -EINVAL;
 		uverbs_spec = (struct ib_uverbs_flow_spec_storage *)uverbs_spec_hdr;
+		if (ib_spec_size < sizeof(struct ib_flow_spec_eth))
+			return -EINVAL;
+		ib_spec->type = IB_FLOW_SPEC_ETH;
 		ib_spec->eth.size = sizeof(struct ib_flow_spec_eth);
 		memcpy(&ib_spec->eth.val, &uverbs_spec->eth.val,
 		       sizeof(struct ib_flow_eth_filter));

@@ -2618,9 +2626,14 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hd
 		       sizeof(struct ib_flow_eth_filter));
 		break;
 	case IB_FLOW_SPEC_IPV4:
+		if (uverbs_spec_size < sizeof(struct ib_uverbs_flow_spec_ipv4))
+			return -EINVAL;
 		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_ipv4))
 			return -EINVAL;
 		uverbs_spec = (struct ib_uverbs_flow_spec_storage *)uverbs_spec_hdr;
+		if (ib_spec_size < sizeof(struct ib_flow_spec_ipv4))
+			return -EINVAL;
+		ib_spec->type = IB_FLOW_SPEC_IPV4;
 		ib_spec->ipv4.size = sizeof(struct ib_flow_spec_ipv4);
 		memcpy(&ib_spec->ipv4.val, &uverbs_spec->ipv4.val,
 		       sizeof(struct ib_flow_ipv4_filter));

@@ -2629,9 +2642,14 @@ static int uverbs_spec_to_ib_spec(struct ib_uverbs_flow_spec_hdr *uverbs_spec_hd
 		break;
 	case IB_FLOW_SPEC_TCP:
 	case IB_FLOW_SPEC_UDP:
+		if (uverbs_spec_size < sizeof(struct ib_uverbs_flow_spec_tcp_udp))
+			return -EINVAL;
 		if (uverbs_spec_hdr->size != sizeof(struct ib_uverbs_flow_spec_tcp_udp))
 			return -EINVAL;
 		uverbs_spec = (struct ib_uverbs_flow_spec_storage *)uverbs_spec_hdr;
+		if (ib_spec_size < sizeof(struct ib_flow_spec_tcp_udp))
+			return -EINVAL;
+		ib_spec->type = uverbs_spec_hdr->type;
 		ib_spec->tcp_udp.size = sizeof(struct ib_flow_spec_tcp_udp);
 		memcpy(&ib_spec->tcp_udp.val, &uverbs_spec->tcp_udp.val,
 		       sizeof(struct ib_flow_tcp_udp_filter));

@@ -2659,7 +2677,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	void *uverbs_spec;
 	void *ib_spec;
 	int i;
-	int uverbs_spec_size;
+	size_t uverbs_spec_size;
+	size_t ib_spec_size;
 
 	if (out_len < sizeof(resp))
 		return -ENOSPC;

@@ -2716,7 +2735,9 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		goto err_uobj;
 	}
 
-	flow_attr = kmalloc(sizeof(*flow_attr) + uverbs_spec_size, GFP_KERNEL);
+	ib_spec_size = min(uverbs_spec_size,
+			   cmd.flow_attr.num_of_specs * sizeof(union ib_flow_spec));
+	flow_attr = kmalloc(sizeof(*flow_attr) + ib_spec_size, GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;

@@ -2731,18 +2752,22 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 
 	uverbs_spec = uverbs_flow_spec;
 	ib_spec = flow_attr + 1;
-	for (i = 0; i < flow_attr->num_of_specs && uverbs_spec_size > 0; i++) {
-		err = uverbs_spec_to_ib_spec(uverbs_spec, ib_spec);
+	for (i = 0;
+	     i < flow_attr->num_of_specs && ib_spec_size > 0 &&	uverbs_spec_size > 0;
+	     i++) {
+		err = uverbs_spec_to_ib_spec(uverbs_spec, uverbs_spec_size,
+					     ib_spec, ib_spec_size);
 		if (err)
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
 		uverbs_spec_size -= ((struct ib_uverbs_flow_spec_hdr *) uverbs_spec)->size;
 		uverbs_spec += ((struct ib_uverbs_flow_spec_hdr *) uverbs_spec)->size;
+		ib_spec_size -= ((union ib_flow_spec *) ib_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
 	if (uverbs_spec_size) {
-		pr_warn("create flow failed, %d bytes left from uverb cmd\n",
+		pr_warn("create flow failed, %zu bytes left from uverb cmd\n",
 			uverbs_spec_size);
 		goto err_free;
 	}

Signature · 6 lines

1.8.3.1

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

Or Gerlitz

2013-10-10 08:02 UTC #12 lore raw

In reply to a message not in the archive

Quoted 5 lines · Yann Droneaud

I've suggested reverting temporarily commit 22878db, 436f2ad, 400dbc9 for v3.12 in a previous message [1]. This proposition doesn't sound acceptable for Or.
So I continue to improve them to be in a better shape for v3.12.

Yan, we are 3-4 days before rc5 and as such renaming/beautifying things or removing checks for negative values against unsigned variables et al cleanups are not acceptable. You need to re-arrange the series such that ONLY the fixes to the user/kernel API are targeted to 3.12 and appear as the first 2-3 patches, the rest will go to 3.13 subject to review.

Also - does git am removes the "Link: yyy" line you added in your patches while applying them? if not, please place below the --- line so we will not carry them in the kernel log.

Or.

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

A message that is not in the archive (sent off-list or not yet received): 52565F28.8010008-VPRAkNaXOzVWk0Htik3J/[email protected]

Yann Droneaud

author

2013-10-10 08:32 UTC #14 lore raw

Le 10.10.2013 10:02, Or Gerlitz a écrit :

Quoted 5 lines · Yann Droneaud

Quoted 4 lines
I've suggested reverting temporarily commit 22878db, 436f2ad, 400dbc9 for v3.12 in a previous message [1]. This proposition doesn't sound acceptable for Or.

Quoted 9 lines

Quoted 1 line
So I continue to improve them to be in a better shape for v3.12.
Yan, we are 3-4 days before rc5 and as such renaming/beautifying things or removing checks for negative values
against unsigned variables et al cleanups are not acceptable. You need to re-arrange the series such that ONLY the fixes
to the user/kernel API are targeted to 3.12 and appear as the first 2-3 patches, the rest will go to 3.13 subject to review.

It does not only "beautifying" the code: it actually fix it.
In its current form, the code is not acceptable and should not go in v3.12.

I've made a review of this code but no patch actually fix all the size computation issues.

http://marc.info/?i=1378908269.2258.31.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
http://marc.info/[email protected]

Even if I move the "renaming" patches later, the data structure name must be updated throughout the code, this will make the patchset still a bit large for v3.12-rc5.

I really understand it's too big for v3.12, that's why I've proposed to revert the current code for v3.12 and push all of this in v3.13.

Quoted 4 lines

Also - does git am removes the "Link: yyy" line you added in your patches while applying them? if not, please place
below the --- line so we will not carry them in the kernel log.

I've added them on purpose so they are kept in the kernel log. I'm following a good advice found in comments[1] of "What's missing from our changelogs" article[2]

[1] http://lwn.net/Articles/560392/
[2] http://lwn.net/Articles/560617/

Regards.

Signature · 7 lines

Yann Droneaud
OPTEYA

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Or Gerlitz

2013-10-10 09:01 UTC #16 lore raw

Quoted 16 lines · Yann Droneaud

Quoted 5 lines · Or Gerlitz
Quoted 4 lines · Yann Droneaud
I've suggested reverting temporarily commit 22878db, 436f2ad, 400dbc9 for v3.12 in a previous message [1]. This proposition doesn't sound acceptable for Or.
Quoted 9 lines
Quoted 1 line
So I continue to improve them to be in a better shape for v3.12.
Yan, we are 3-4 days before rc5 and as such renaming/beautifying things or removing checks for negative values
against unsigned variables et al cleanups are not acceptable. You need to re-arrange the series such that ONLY the fixes
to the user/kernel API are targeted to 3.12 and appear as the first 2-3 patches, the rest will go to 3.13 subject to review.

Quoted 1 line

It does not only "beautifying" the code: it actually fix it.

IB/core: Rename 'flow' structs to match other uverbs structs IB/core: Makes uverbs flow structure using names more similar to verbs ones IB/core: Remove checks for negative values in create_flow uverbs

You must agree that these three patches can go to 3.13 -- also, if there are other small fixes which do not touch the user/kernel API they can go to 3.13 too and subject to review into -stable

What you need to prepare for 3.12 are fixes that relate to user/kernel API
and are not fixed in the patch Matan sent on Sep 22nd https://patchwork.kernel.org/patch/2924341
and relative to that patch

Or.

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

A message that is not in the archive (sent off-list or not yet received): 52566CFC.3010802-VPRAkNaXOzVWk0Htik3J/[email protected]

Yann Droneaud

author

2013-10-10 09:18 UTC #18 lore raw

Le 10.10.2013 11:01, Or Gerlitz a écrit :

Quoted 17 lines · Yann Droneaud

Quoted 16 lines · Or Gerlitz
Quoted 14 lines · Yann Droneaud
Quoted 5 lines
I've suggested reverting temporarily commit 22878db, 436f2ad, 400dbc9 for v3.12 in a previous message [1]. This proposition doesn't sound acceptable for Or.
So I continue to improve them to be in a better shape for v3.12.
Yan, we are 3-4 days before rc5 and as such renaming/beautifying things or removing checks for negative values
against unsigned variables et al cleanups are not acceptable. You need to re-arrange the series such that ONLY the fixes
to the user/kernel API are targeted to 3.12 and appear as the first 2-3 patches, the rest will go to 3.13 subject to review.

Quoted 3 lines

Quoted 1 line
It does not only "beautifying" the code: it actually fix it.
IB/core: Rename 'flow' structs to match other uverbs structs

Public API, exposed to userspace

Quoted 1 line

IB/core: Makes uverbs flow structure using names more similar to verbs ones

Public API, exposed to userspace

Quoted 2 lines

IB/core: Remove checks for negative values in create_flow uverbs

This one could go for v3.13. (But probably to discard if Matan's one is applied first, see below).

Quoted 9 lines

You must agree that these three patches can go to 3.13 -- also, if there are other small fixes which do not touch the user/kernel API they can go to 3.13 too and subject to review into -stable
What you need to prepare for 3.12 are fixes that relate to user/kernel API and are not fixed in the patch Matan sent on Sep 22nd
https://patchwork.kernel.org/patch/2924341
and relative to that patch

This patch could go for 3.13 too: it's not altering (fixing) the public API.

In the meantime, I've sent a patch that will lower the urge to fix for v3.12, and reduce the size of the merge request to be proposed to Linus.

Regards.

Signature · 7 lines

Yann Droneaud
OPTEYA

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