Re: [PATCH V4 for-next 3/4] IB/core: Export ib_create/destroy_flow through uverbs

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Le mercredi 28 août 2013 à 15:47 +0300, Matan Barak a écrit :
> From: Hadar Hen Zion <hadarh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Implement ib_uverbs_create_flow and ib_uverbs_destroy_flow to
> support flow steering for user space applications.
> 
> Change-Id: I0bc6dbe26f4776d00f95b6200ff43ccb24000e31
> Signed-off-by: Hadar Hen Zion <hadarh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Seen this in linux-next as commit
436f2ad05a0b65b1467ddf51bc68171c381bf844

> ---
>  drivers/infiniband/core/uverbs.h      |    3 +
>  drivers/infiniband/core/uverbs_cmd.c  |  228 +++++++++++++++++++++++++++++++++
>  drivers/infiniband/core/uverbs_main.c |   13 ++-
>  include/rdma/ib_verbs.h               |    1 +
>  include/uapi/rdma/ib_user_verbs.h     |   89 +++++++++++++-
>  5 files changed, 332 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index b105140..2856696 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2597,6 +2599,232 @@ out_put:
>  	return ret ? ret : in_len;
>  }
>  
> +static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
> +				union ib_flow_spec *ib_spec)
> +{
> +	ib_spec->type = kern_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)
> +			return -EINVAL;
> +		memcpy(&ib_spec->eth.val, &kern_spec->eth.val,
> +		       sizeof(struct ib_flow_eth_filter));
> +		memcpy(&ib_spec->eth.mask, &kern_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)
> +			return -EINVAL;
> +		memcpy(&ib_spec->ipv4.val, &kern_spec->ipv4.val,
> +		       sizeof(struct ib_flow_ipv4_filter));
> +		memcpy(&ib_spec->ipv4.mask, &kern_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)
> +			return -EINVAL;
> +		memcpy(&ib_spec->tcp_udp.val, &kern_spec->tcp_udp.val,
> +		       sizeof(struct ib_flow_tcp_udp_filter));
> +		memcpy(&ib_spec->tcp_udp.mask, &kern_spec->tcp_udp.mask,
> +		       sizeof(struct ib_flow_tcp_udp_filter));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> +			      const char __user *buf, int in_len,
> +			      int out_len)
> +{
> +	struct ib_uverbs_create_flow	  cmd;
> +	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_flow_attr		  *flow_attr;
> +	struct ib_qp			  *qp;
> +	int err = 0;
> +	void *kern_spec;
> +	void *ib_spec;

why void * here, there's a type for them, and the function
kern_spec_to_ib_spec() has a prototype with those types.

> +	int i;
> +	int kern_attr_size;
> +
> +	if (out_len < sizeof(resp))
> +		return -ENOSPC;
> +
> +	if (copy_from_user(&cmd, buf, sizeof(cmd)))
> +		return -EFAULT;
> +
> +	if (cmd.comp_mask)
> +		return -EINVAL;
> +
> +	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
> +	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
> +		return -EPERM;
> +
> +	if (cmd.flow_attr.num_of_specs < 0 ||

num_of_specs is unsigned ...

> +	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
> +		return -EINVAL;
> +
> +	kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
> +			 sizeof(struct ib_uverbs_cmd_hdr_ex);
> +

Please, no under/overflow. Check that cmd.flow_attr.size is bigger than
sizeof(cmd) + sizeof(struct ib_uverbs_cmd_hdr_ex) before computing
kern_attr_size.

> +	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||

just like num_of_specs, it's unsigned !

cmd.flow_attr.size is certainly less then in_len, so you probably mean
cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
ib_uverbs_cmd_hdr_ex)).

> +	    kern_attr_size < 0 || kern_attr_size >
> +	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
> +		return -EINVAL;
> +

let's make kern_attr_size unsigned.

And be stricter: kern_attr_size != cmd.flow_attr.num_of_specs *
sizeof(struct ib_kern_spec) might be welcome.

> +	if (cmd.flow_attr.num_of_specs) {
> +		kern_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
> +		if (!kern_flow_attr)
> +			return -ENOMEM;
> +
> +		memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));

might be sizeof(struct ib_kern_spec) as used in the computation.

> +		if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
> +				   kern_attr_size)) {
> +			err = -EFAULT;
> +			goto err_free_attr;
> +		}

I don't feel comfortable with the bounds: previously it was 
sizeof(cmd) + sizeof(struct ib_uverbs_cmd_hdr_ex), and here
copy_from_user() is copying starting from buf + sizeof(cmd) ... so where
is the struct ib_uverbs_cmd_hdr_ex ?

And I dislike the + 1 throughout this portion of the code: why the first
element is being so different than the others. I didn't dig enough in
the code to understand, but from my point of view, it doesn't fit.

> +	kern_spec = kern_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);
> +		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;
> +		ib_spec += ((union ib_flow_spec *)ib_spec)->size;

Why use void * for ib_spec and kern_spec if there's a union type for
them ?

> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
> index 61535aa..0b233c5 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -86,7 +86,9 @@ enum {

> +struct ib_kern_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;
> +	};
> +};
> +

[Aside note: no IPv6 spec ?]


> +struct ib_kern_flow_attr {
> +	__u32 type;
> +	__u16 size;
> +	__u16 priority;
> +	__u8  num_of_specs;
> +	__u8  reserved[2];
> +	__u8  port;
> +	__u32 flags;
> +	/* Following are the optional layers according to user request
> +	 * struct ib_flow_spec_xxx
> +	 * struct ib_flow_spec_yyy
> +	 */
> +};
> +
> +struct ib_uverbs_create_flow  {
> +	__u32 comp_mask;

Alignment notice: There's a hole here on 64bits system with stricter
alignment rules.

> +	__u64 response;
> +	__u32 qp_handle;
> +	struct ib_kern_flow_attr flow_attr;
> +};
> +

Sorry I've missed that before it got included in infiniband git tree.

Regards.

-- 
Yann Droneaud
OPTEYA



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